As far as I can work out, the phenomenon is as in title. I've traced the entire entity loading process from both commerce_order_load() and commerce_cart_order_load() and all I've managed to conclude is that the line item prices are processed somewhere else and are merely loaded.

Basically, when the shopping cart block is shown first, the price is correct any place it's shown. However, if something else that shows a calculated price, particularly a formatted price field, is displayed first, only that price will be correct, and the shopping cart will skip the rules.

Files: 
CommentFileSizeAuthor
#12 calculate_sell_price-before.png132.84 KBjoelpittet
#12 calculate_sell_price-after.png130.44 KBjoelpittet
#12 recursion_in_price-1268472-12.patch872 bytesjoelpittet
PASSED: [[SimpleTest]]: [MySQL] 3,529 pass(es).
[ View ]
#1 1268472-1-price-calculation-recursion.patch588 byteszhangtaihao
FAILED: [[SimpleTest]]: [MySQL] 3,589 pass(es), 0 fail(s), and 91 exception(s).
[ View ]

Comments

zhangtaihao’s picture

Title:Price calculation prior to cart block causes cart to skip calculation rules» Recursion in price calculation causes cart to skip calculation rules
Priority:Normal» Major
StatusFileSize
new588 bytes
FAILED: [[SimpleTest]]: [MySQL] 3,589 pass(es), 0 fail(s), and 91 exception(s).
[ View ]

I've managed to trace it down to a recursion problem. It gets blocked by Rules, but I think it causes subsequent attempts at calculating the price to fail.

The scenario is this: I am trying to calculate GST (a type of VAT) based on country. Naturally, the price calculation rule includes order address component comparison. The home page contains both a Views listing (including price, calculated for the user) and the cart block. When the Views listing fires first, it attempts to compute the price through commerce_product_calculate_sell_price(). A dummy line item is created, and commerce_cart sets its order ID in hook_commerce_product_calculate_sell_price_line_item_alter().

As the event "commerce_product_calculate_sell_price" is invoked, the address component comparison causes the order to load. This then triggers the hook implementation commerce_cart_commerce_order_load(), which calls commerce_cart_order_refresh(), which invokes the "commerce_product_calculate_sell_price" event again.

I don't know why the load triggered by address component comparison isn't the same as commerce_cart_order_load(). Frankly, I've debugged every custom functionality before commerce_cart_order_load() ends up in entity_load(). My first guess is that the intentional load causes the static $refreshed array in commerce_cart_commerce_order_load() to set properly.

To demonstrate the point (and incidentally the problem), I've attached a patch that should fix the problem temporarily. However, I'm not sure if this solution is proper, given that commerce_cart_block_view() does a proper job simply with commerce_cart_order_load(). My feeling is still that some other part is incomplete, making a Rules entity load recurse into "commerce_product_calculate_sell_price".

zhangtaihao’s picture

Status:Active» Needs review
rszrama’s picture

Priority:Major» Normal
Status:Needs review» Needs work

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.

zhangtaihao’s picture

Thanks. I'll check it out.

timodwhit’s picture

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?

ParisLiakos’s picture

Status:Needs work» Needs review

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"
        }
      }
    ]
  }
}

Status:Needs review» Needs work

The last submitted patch, 1268472-1-price-calculation-recursion.patch, failed testing.

rszrama’s picture

Status:Needs work» Fixed

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

Status:Fixed» Closed (fixed)

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

joelpittet’s picture

Issue summary:View changes
Status:Closed (fixed)» Active

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:

<?php
 
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?

<?php
 
// 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.

joelpittet’s picture

Issue tags:+Performance, +mysql deadlock

Tagging with reasons.

joelpittet’s picture

Issue summary:View changes
Status:Active» Needs review
StatusFileSize
new872 bytes
PASSED: [[SimpleTest]]: [MySQL] 3,529 pass(es).
[ View ]
new130.44 KB
new132.84 KB

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.

torgosPizza’s picture

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?

joelpittet’s picture

I can't think of any potential side effects.

rszrama’s picture

Issue tags:+sprint

Tagging.

mglaman’s picture

Status:Needs review» Reviewed & tested by the community

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.