Combine does not work with different line-item types and custom comparison properties [#1925646]

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.

Comment	File	Size	Author
#4	1925646.cart_line_item_comparison.patch	2.63 KB	rszrama
#4

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

rszrama CreditAttribution: rszrama commented 25 February 2013 at 16:57

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

Comment #2

haggins CreditAttribution: haggins commented 25 February 2013 at 18:24

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.

Comment #3

rszrama CreditAttribution: rszrama commented 3 July 2013 at 21:52

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

Ok, I improved our combining logic like so:

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.
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.
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'.

Comment #4

rszrama CreditAttribution: rszrama commented 3 July 2013 at 21:53

File	Size
1925646.cart_line_item_comparison.patch	2.63 KB

Ugh, patch did not attach. :-/

Comment #5

haggins CreditAttribution: haggins commented 3 July 2013 at 23:19

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 :)