Recursion in price calculation causes cart to skip calculation rules

Wow, that's quite a mess. I'll wade through it if need be, but I'm curious to know if the patch I just committed in #1281664: Price conversion in views for several displays linked to the same commerce product. didn't fix your problem. It also addressed scenarios where prices were properly calculated once but were ignored on subsequent attempts. That's not to say there can't be some funny business pertaining specifically to the rendering of price fields within Views, but we may as well give the other patch a shot first.

Since we're not sure the patch as posted provides a solution, let's leave it in needs work. It does sound like there may be something worth investigating with the re-load from the database happening. I'd assume it would load from the static cache instead of the database a second time.

Not sure if I am having the same issue but I am running to a similar issue where the rules fire correctly on /cart page but the cart block pricing is causing recursion in one of my rules when adding a product/line item of a certain type. I looked into my rules and can't figure out what is causing the recursion on my end.

Did this ever get worked out?

i confirm this behavior and that this patch works..

I could reproduce this in a clean commerce install..

Just import the following rule..
Expected behavior: when you add 6th product to cart, everything gets a discount..it happens in /cart page but not in the cart block

Need to check if patch applies to latest dev, i applied it manually..

Rule:

{ "rules_discount" : {
    "LABEL" : "Discount",
    "PLUGIN" : "reaction rule",
    "REQUIRES" : [
      "rules",
      "commerce_order",
      "commerce_line_item",
      "commerce_product_reference"
    ],
    "ON" : [ "commerce_product_calculate_sell_price" ],
    "IF" : [
      { "entity_has_field" : { "entity" : [ "commerce-line-item" ], "field" : "commerce_product" } },
      { "data_is" : {
          "data" : [ "commerce-line-item:commerce-product:type" ],
          "value" : "product"
        }
      },
      { "commerce_order_compare_total_product_quantity" : {
          "commerce_order" : [ "commerce-line-item:order" ],
          "operator" : "\u003E=",
          "value" : "5"
        }
      }
    ],
    "DO" : [
      { "commerce_line_item_unit_price_multiply" : {
          "commerce_line_item" : [ "commerce_line_item" ],
          "amount" : "0.85",
          "component_name" : "discount",
          "round_mode" : "1"
        }
      }
    ]
  }
}

As it turns out, the patch I referenced in comment #3 didn't fix this issue, and I actually ended up reporting a duplicate with pcambra during DrupalCon Munich. The problem manifested itself in the same way - using a Rules condition that had to load the order during Rules evaluation resulted in recursion if the block appeared on the same page as rendered field formatters that first loaded the cart order. I honestly haven't been able to find out what is wrong; I've backtraced just about every aspect of cart order loading / price calculation, and all I can say is that this patch fixes it. I can't even actually recreate duplicate calls to commerce_cart_order_refresh().

My only hunch is that it also has something to do with the Rules wrapper, but I'm at a loss to explain how it's creating recursion if I actually can't duplicate calls to commerce_cart_order_refresh(). I don't know how that static variable could be getting unset unless somewhere some module is doing a brute force static cache clear (not even sure if that's possible).

What does seem apparent is that this patch, which forces the cart order load prior to Rules, does the trick. It seems it changes the logic from this:

1. Prepare to render a field for which price calculation is necessary (price field, Add to Cart form, etc.).
2. Create the pseudo line item and point it to the cart order but don't load the cart order.
3. Invoke Rules which results in the evaluation of a condition requiring the line item's order to be loaded from the database.
4. Load the order, attempt to refresh it, and somehow get into a recursive state.

To this:

1. Prepare to render a field for which price calculation is necessary.
2. Create the pseudo line item and load the cart order to point the line item to it.
3. Attempt to refresh it, which results in the evaluation of a condition requiring the item's order to be loaded from the cache.
4. No problem - all prices calculate as expected!

Why is it different? Haven't a clue. Maybe the issue really is tucked away deeper inside Rules / the Entity API, but for the time being, I'm just going to commit this and trust that treating the symptom in this case is good enough.

Commit: http://drupalcode.org/project/commerce.git/commitdiff/8fc64da

Obligatory bug-fix followup: http://drupalcode.org/project/commerce.git/commitdiff/0580df8

Sorry for re-opening this but it has the context, feel free to re-close and I can open up a follow-up if you agree.

Could this:

  if (empty($line_item->order_id)) {
    $order = commerce_cart_order_load($user->uid);

    if ($order) {
      $line_item->order_id = $order->order_id;
    }
  }

be just commerce_cart_order_id?

  // Reference the current shopping cart order in the line item if it isn't set.
  if (empty($line_item->order_id) && $order_id = commerce_cart_order_id($user->uid)) {
    $line_item->order_id = $order_id;
  }

That way you aren't priming the order on every line item price calculation?

And if we still need to prime the order, just do it once? In commerce_product_calculate_sell_price or commerce_cart_add_to_cart_form_submit. Which ever one it's needed in?

Reason I'm asking is because there are a bunch of order loads seem unnecessary.

Tagging with reasons.

Here's the revert patch. While performance testing these are my results:

Before: 4.0 sec wall time

After: 2.4 sec wall time

Scenario: Product listing page with 9 product teasers and a block cart preview via dc_ajax_add_cart.

Keep in mind these are on warmed cache via XHProf and xdebug turned off.

Nicely done, and all the tests pass. Can you think of any potential side effects with this? I agree that the price calculation / order refreshing systems always seemed a bit over-engineered. Hopefully this (seemingly massive) performance win will be all pros and no cons. I'll give your patch a try soon. Thanks @joelpittet!

EDIT to add, I remember a similar issue which Joel also addressed #1819200: Reduce price calculation attempts on a single product page and which looks like still needs some eyes on it?

I can't think of any potential side effects.

Tagging.

Logic makes sense and the currently implementation is a bit circular compared to proposed patch in #12.

commerce_cart_order_load() literally calls commerce_cart_order_id() which the calls commerce_order_load() without resetting anything. So we'd be cutting down a few steps while ensuring proper order_id.

Hello,

I have a similar problem, but not very sure of it.
I am using EU VAT module. Each time, I add a product with VAT into cart, the cart block is shown with error : VAT rate is applied twice. If I request a refresh , cart block is OK.
If I ask for cart page, VAT and prices are OK.
Is it a known issue ?

Thx

Marking as needs work because @torgosPizza found a bug with commerce_coupon where the coupon/discount will remove if you go to a PDP page with this applied. We are trying to work it out. Doesn't happen with just discount just coupon so something is not loading correctly or relying on something that is loaded one more time by this.

Any update on this?
We have the same issue with a coupon/discount being applied and showing in the cart total View. After you visit a Product Display page (product details) and then return to the cart page, the cart total view now doesn't show the discount/coupon and the order total no longer includes the discount.

Oddly, if you remove an item from the cart, the discount/coupon reappears in the cart total view.