Problem

While trying to take advantage of hook_commerce_cart_order_is_cart() I discovered (after a lot of wasted time) that the hook actually doesn't appear to be invoked in a way that allows it to be useful unless you want to return false. Setting $is_cart when passing it by reference doesn't actually do anything.

I ultimately had to resort to using the info_alter hooks for pages and statuses to override default cart detection instead.

For clarity, 

  // Allow other modules to make the judgment based on some other criteria.
  if (in_array(FALSE, module_invoke_all('commerce_cart_order_is_cart', $order, $is_cart))) {
    $is_cart = FALSE;
  }

does not work but as a test, 

  // Allow other modules to make the judgment based on some other criteria.
  if (in_array(FALSE, module_invoke_all('commerce_cart_order_is_cart', $order, $is_cart))) {
    $is_cart = FALSE;
  }
  mymodule_commerce_cart_order_is_cart($order, $is_cart);

actually did what the hook is supposed to do.

This could be a D7 bug in module_invoke_all but I am unfamiliar with the code there and if it has changed at all. It isn't obvious to me how variable references would work in that function, either.

CommentFileSizeAuthor
#4 1322854-order-is-cart-alter-4.patch2.09 KBamateescu
#1 1322854-order-is-cart-alter.patch2.21 KBdamien tournoud

Comments

damien tournoud’s picture

damien tournoud commented
Status: Active » Needs review
StatusFileSize
new 1322854-order-is-cart-alter.patch2.21 KB

Confirmed.

We should have been using a proper alter hook here, so I suggest we fix hook_commerce_cart_order_is_cart() but deprecate it. Deprecating here means that it will be removed in the n+2 version (in that case Commerce 1.3).

Status: Needs review » Needs work

The last submitted patch, 1322854-order-is-cart-alter.patch, failed testing.

rszrama’s picture

rszrama commented
Version: 7.x-1.x-dev » 7.x-1.0
Status: Needs review » Needs work

Hmm, Damien, I'm not sure your change to fix the hook as is does what you expect. It seems to preserve the existing behavior of only allowing hook_commerce_cart_order_is_cart() to falsify an order from being a cart. Could it just be:

foreach (module_implements('commerce_cart_order_is_cart') as $module) {
  $function = $module . '_commerce_cart_order_is_cart';
  $function($order, $is_cart);
}
amateescu’s picture

amateescu commented
Version: 7.x-1.0 » 7.x-1.x-dev
Status: Needs work » Needs review
StatusFileSize
new 1322854-order-is-cart-alter-4.patch2.09 KB

@rszrama, no, that code introduces the ability to set $is_cart = TRUE in the hook, but loses the ability to return FALSE. Tested all possible combinations :)

I think DamZ's approach is the right one because it fixes the current behavior and also cleans-up this mess. Re-rolled for current HEAD.

rszrama’s picture

rszrama commented
Version: 7.x-1.0 » 7.x-1.x-dev
Status: Needs work » Fixed

Ahh, I see my mistake in reading. All clear! fwiw, I was thrown off by the if statement around the return value, wondering why it was even necessary... then I realized that's to preserve backwards compatibility. Duh. I added a comment block above the if statement so I don't forget this in the future.

Committed! : )

Status: Fixed » Closed (fixed)

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