structure of foreach loop with key and if statement

Question

What am I doing wrong here? I think i've been looking at it too long because I cant spot it.

I'm trying to run a loop which basically says, if the cart is empty, add the product, if the cart is not empty, check if the product exists first. If it does, increment the quantity, if it doesn't add it as a new product in the cart.

public function addProductToCart($product_id){
  if(!empty($_SESSION['cart_products'])){
    foreach($_SESSION['cart_products'] as $i => $item) {
      if($_SESSION['cart_products'][$i]['id'] == $product_id){
        $_SESSION['cart_products'][$i]['quantity'] += 1;
      }
      if($_SESSION['cart_products'][$i]['id'] != $product_id){
        $newProduct = array("id" => $product_id, "quantity" => 1);
        array_push($_SESSION['cart_products'],$newProduct);
      }
    }
  } else {
    $newProduct = array("id" => $product_id, "quantity" => 1);
    array_push($_SESSION['cart_products'],$newProduct);
  }
}

After clicking a few products a couple of times to test if it works, my array looks like this

array(1) {
 ["cart_products"]=>
  array(6) {
    [0]=>
     array(2) {
       ["id"]=>
       string(1) "1"
       ["quantity"]=>
       int(5)
       }
      [1]=>
      array(2) {
       ["id"]=>
       string(1) "2"
       ["quantity"]=>
       int(1)
    }
     [2]=>
     array(2) {
     ["id"]=>
     string(1) "3"
     ["quantity"]=>
     int(2)
    }
    [3]=>
    array(2) {
      ["id"]=>
      string(1) "3"
      ["quantity"]=>
      int(2)
    }
    [4]=>
    array(2) {
      ["id"]=>
      string(1) "3"
      ["quantity"]=>
      int(1)
    }
    [5]=>
    array(2) {
      ["id"]=>
      string(1) "3"
      ["quantity"]=>
      int(1)
    }
  }
}

The $product_id argument being passed in is just an integer.

Answer 1

Each of your product has a unique ID (or at least, I hope they have). So you should take advantage of that, by using those IDs as keys in your cart.

public function addProductToCart($product_id)
{
    $products = $_SESSION['cart_products'];

    if (array_key_exists($product_id, $product) === false) {
        $products[$product_id] = 0;
    }

    $products[$product_id] += 1;

    $_SESSION['cart_products'] = $products;
}

This way the cart_products value in session will contain productID => quantity pairs.

Notes:

The "wrong" part is actually the majority of the code there.

Your shopping cart entity should not be directly interacting with the persistence (which, in this case is the session). The writing to persistence should probably be done by separate class (data mapper), because that way you would be able to at least add some unit tests.

Answer 2

Change code inside if-block:

// special flag
$product_found = false;
foreach($_SESSION['cart_products'] as $i => $item) {
  // here you found the required ID
  if($_SESSION['cart_products'][$i]['id'] == $product_id){
    $_SESSION['cart_products'][$i]['quantity'] += 1;
    // set flag as found
    $product_found = true;
    // break loop as you already found and incremented a required value
    break;
  }
}

// if you didn't find required value - add a new one
if (!$product_found) {
  $newProduct = array("id" => $product_id, "quantity" => 1);
  array_push($_SESSION['cart_products'],$newProduct);
}