I came across this issue since on our shop some product types are limited to one item per cart.
This means if a customer chooses as second product of the same type I've to delete the existing one.
Unfortunately this doesn't work for some reason.
I drilled into the code and I guess I found the base issue(s).

I've no solution for now and any advice / suggestion is welcome :) (the only workaround I know is to reuse the existing line item for the new product instead delete / add).

Scenario:
If you delete a line item the controller triggers different hooks e.g. commerce_line_item_delete().
One of these hook implementations is commerce_line_item_commerce_line_item_delete().
This function takes care of deleting all kinds of references to the deleted line item - related issue: #1030140: Do not throw exceptions when wrapping an entity with a stale reference .
This includes the references of commerce order - thus on line item delete a order save is triggered automatically - what again means that the hook commerce_order_presave() is fired.
commerce_cart implements this hook to keep track of changes in the line items.
This implementation tries to compare the current line items with the original ones.
Thus it tries to load the unchanged order from the db by reseting the cache when calling commerce_order_load_multiple().
This again triggers commerce_cart_commerce_order_load where we iterate over the line items.
And there are a lot of other calls - some when we end up in commerce_cart_commerce_line_item_presave (still in the event for loading the unchanged order.).
There the method tries to evaluate if the order has to be stored (again) by comparing the quantity of the affected line item.
To do so, it loads the unchanged line item from the db, by using commerce_line_item_load_multiple() with enabled cache reset.
This ends up in a FALSE, because the line item is already deleted at that point.
The FALSE is interpreted as a change of the line item what means it tries to store the unchanged order to force the order status to "cart".
Now that means the code tries to save the unchanged order, we just wanted to load for comparison. The fun thing about that is that we now store a order with a reference to a line item that doesn't exist not anymore.
That finally causes the error: EntityMetadataWrapperException: Unable to load the commerce_line_item with the id 643. in EntityDrupalWrapper->value().

But even if I jump over the code in commerce_cart_commerce_line_item_presave I end up getting the error because there are plenty of other locations where we deal with the unchanged order in a similar way.

Besides the fact that this issue is really annoying I'm concerned about the pattern I see in the stacktrace. It looks to me like, even if we can solve the "caching" issue, we will run into an endless loop because of the hook nesting. And during my attempts to fix the issue I ran several times in a "Nesting level exception" which probably means I had the solution for the issue related to the entity metadata wrapper already :|

StackTrace:

form_submit_handler 
  commerce_line_item.module.commerce_line_item_delete 
    commerce_line_item.module.commerce_line_item_delete_multiple 
      commerce_line_item.controller.inc.CommerceLineItemEntityController->delete 
        commerce_cart.module.commerce_cart_commerce_line_item_delete 
          commerce_order.module.commerce_order_save 
            commerce_order.controller.inc.CommerceOrderEntityController->save 
              commerce_cart.module.commerce_cart_commerce_order_presave 
                commerce_order.module.commerce_order_load_multiple
                  commerce_order.controller.inc.CommerceOrderEntityController->attachLoad 
                    commerce_cart.module.commerce_cart_commerce_order_load 
                      commerce_line_item.module.commerce_line_item_save
                        commerce_cart.module.commerce_cart_commerce_line_item_presave
                          commerce_order.module.commerce_order_save 
                            entity.wrapper.inc.CommerceOrderEntityController->save 
                              entity.wrapper.inc.EntityDrupalWrapper->value

^^Imagine what is the next call after a successfull commerce_order_save()? I bet it's commerce_cart_commerce_order_presave() starting over the whole process.

To make it easier to work on this issue I've created a small module which works similar to the function I use in our system.
If you enable the module you'll get a new admin menu item where you can find a simple form and checkout the issue.

I set the priority to critical since this issue introduces the need of some very special knowledge to use some of the basic functionality.
Any feedback is highly welcome.

Comments

rszrama’s picture

Hmm, I wonder if I came across this same problem in my debugging over the weekend while I converted price amount fields to integers. I found I needed to do two things to fix my problems, and I'm wondering if they help you at all:

  1. Switch the order of presave hooks in our controllers so the event / entity hook was firing first and the hook_field_presave() second. This got me out of an infinite loop. (See https://github.com/rszrama/drupalcommerce/commit/8b237180afe51abf03eaaae...)
  2. In a couple commits, I had to manually reset the line item cache to make sure Rules wasn't getting stale data from the entity_load() cache. You might try this as well, using entity_get_controller('commerce_line_item')->resetCache(). And if you pass resetCache() an array of line item IDs, you can just reset those specifically.

However, I also made a note to clean up the mess in those various hooks in the Cart module. My gut tells me they can be vastly simplified now that the line item has an order_id property on it.

das-peter’s picture

StatusFileSize
new704 bytes

Woohoo, just found for sure one nasty issue.
commerce_cart_order_load() maintained it's own static order cache to prevent db lookups for the order with a specific uid.
But instead just mapping the order_id and leave caching up to the commerce_order module it stored the whole (decoupled) order object.
I guess this could explain some odd behaviour.
Attached patch changes the static cache to just keep the order id. We still prevent db lookups but the caching is done on the correct place.

das-peter’s picture

Status: Active » Needs review
StatusFileSize
new2.66 KB

Patch had its own issues :| Rewritten now :)

das-peter’s picture

Man, I really should commit all my changes before make a diff....

das-peter’s picture

And another small fix of the patch.

rszrama’s picture

Status: Needs review » Fixed

Committed with some small tweaks to the variable names. All seems to work great! Fixing unless there's some remaining issue here... I do believe the infinite loop has been broken apart, though.

Status: Fixed » Closed (fixed)

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