If you add some comparison_properties via hook_commerce_cart_product_comparison_properties_alter() which are available on one line item type but not on another you will get an "Undefined property"-error from the commerce_cart_product_add() function.

There should be either checked whether a property exists or not before accessing it. Or you pass not only current $comparison_properties but also the line-item type to the hook.

I prefer the second solution as it would give devs the possibility to declare different comparison properties for different line-item types.

CommentFileSizeAuthor
#4 1925646.cart_line_item_comparison.patch2.63 KBrszrama

Comments

rszrama’s picture

rszrama commented

Hmm, honestly, it shouldn't be trying to combine line items of different types. That sounds like the real issue to me.

haggins’s picture

haggins commented

That's not the point. It does not try to combine different types. But if combination is enabled at different types it will check all properties you added in hook_commerce_cart_product_comparison_properties_alter() independent from whether the property is available at the current line-item type or not.

Example:

1st type: product
no additional fields (=default installation)

2nd type: shirt
additional field/property: field_size

As I would like to combine shirts of the same size I implement above hook as follows:

function mymodule_commerce_cart_product_comparison_properties_alter(&$comparison_properties) {
  $comparison_properties = array_merge($comparison_properties, array('field_size'));
}

If you now add shirts everything is fine. But if you add products then commerce_add_to_cart() fails as it trys to compare the field_size property of products which is not available for this line-item type.

Hope it's explained clearer now :)

edit: this does not happen if you add product to an empty order as then there is nothing to compare. There must already be at least one line-item in the order/cart - regardless of its type.

rszrama’s picture

rszrama commented
Version: 7.x-1.5 » 7.x-1.x-dev
Status: Active » Needs review

Ok, I improved our combining logic like so:

  1. I added a check to skip the comparison of a property that exists on neither the line item in the cart or on the one we're attempting to combine into it.
  2. I then added two checks to the comparison if statement that produces a non-match when a property exists on one of the line items but not the other.
  3. I added a clone of the $line_item we're attempting to add to the cart as a parameter to hook_commerce_cart_product_comparison_properties_alter() so we can be a little more judicious about what we add to $comparison_properties if we want.

I'd also add that you don't really need that array_merge(). You could just do $comparison_properties[] = 'field_size'.

rszrama’s picture

rszrama commented
StatusFileSize
new 1925646.cart_line_item_comparison.patch2.63 KB

Ugh, patch did not attach. :-/

haggins’s picture

haggins commented

Thank you Ryan!
I wasn't able to apply the patch yet. But with your description and by a (quick) code review it looks as this solves the issue perfectly :)

rszrama’s picture

rszrama commented
Status: Needs review » Fixed

Test bot liked it, too. Committed.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

Anonymous’s picture

(not verified) commented
Issue summary: View changes

Updated issue summary.