gjh42:
I haven't studied the code there so I can't say how it does work, but it is clearly intended to work as I described
Oh, no doubt. But good intentions don't necessarily imply correct code! :wink:
The code as currently written only works (or rather, appears to work) because, for PHP version < 5.3.1, $cart_qty is always 0, which happens because a bad argument is being passed to in_cart_mixed() on the right-hand-side of the assignment. As long as $cart_qty is 0, the test:
($new_qty + $cart_qty > $add_max)
is equivalent to the test:
($new_qty > $add_max)
which turns out to be right for products without attributes, or for products that have attributes and for which "mixed" is false.
But once we start passing a proper product ID to in_cart_mixed(), and getting back a meaningful value for $cart_qty, the test that actually appears in the code is always wrong. Always. It would be right (again, for non-"mixed" products), if $new_qty represented the number of units to be added to the cart, but that's not what it represents. It represents the numeric value that appears in the quantity box at the moment the user clicks the "update" button.
Consider the case in which there's one product (ID) in the cart, and the user makes no change whatsoever to the number in the quantity box. It was n when the shopping cart page was initially rendered, and it's n when the user clicks the button. But the test will end up comparing 2**n* (i.e., n+n) to $add_max. And if n > $add_max/2, the user gets an error message even though the quantity wasn't altered!
The upshot of all this is that the correct calculation really does depend on knowing the number of units being added to the cart, and not just the value of the quantity box at the time the button was clicked. And to determine that value, for a "mixed" product having a particular combination of attribute values, we need to know how many units having that particular combination are currently in the cart, as well as the total number of units of that product (irrespective of attribute combinations).
So here's my latest whack at addressing these issues (new/changed code in red):
$add_max = zen_get_products_quantity_order_max($_POST['products_id'][$i]);
$cart_qty = $this->in_cart_mixed($_POST['products_id'][$i]); // How many of all flavors of this product already in the cart
$item_qty = $this->get_quantity($_POST['products_id'][$i]); // How many of just this flavor of this product already in the cart
$new_qty = $_POST['cart_quantity'][$i]; // Number in quantity box at click time
$add_qty = $new_qty - $item_qty; // Number of units of the current flavor to add
// ...
if (($add_max == 1 and $cart_qty == 1)) {
// do not add
$adjust_max= 'true';
} else {
// adjust quantity if needed
if ($add_qty > 0 and ($cart_qty + $add_qty > $add_max) and $add_max != 0) {
$adjust_max= 'true';
$new_qty = $new_qty - ($cart_qty + $add_qty - $add_max);
}
// ...
The main test has now been rewritten to explicitly use $add_qty in place of $new_qty. The additional conjunct, $add_qty > 0, is in there to keep the code from delivering surprises to the user by trying to adjust quantities that the user didn't change. There's another outstanding, related issue that these modifications don't address: because the overall logic of actionUpdateCart() works by iterating through the cart's line items, it gets fooled by quantity modifications that transiently violate the quantity constraints, even if the net change would satisfy the constraints.
Sorry. Don't try to parse that. Here's an example:
Let's say there's a "mixed" product with a quantity max of 10. In the first line-item of the cart we have 4 units of that product with attribute-combination "A," and in the second line-item we have 6 units with attribute-combination "B." Now, say we switch the values in the quantity boxes to indicate that we want 6 of flavor "A" and 4 of flavor "B." We want to still end up with a total of 10, which would satisfy the constraint. But because the function processes the line-items in order, it rejects the request to set the quantity of flavor "A" to 6, because that would lead to a (transient) total of 12 units, in violation of the constraint. In the second line-item, dialing the quantity back from 6 to 4 is okay, so we end up with 4 of "A" and 4 of "B."
I will probably end up fixing this outstanding issue at some point as well, but not likely very soon.
Cheers,
Michael
