The function commerce_cart_order_product_line_item_delete() provides a skip_save argument which would allow you to not save the order when invoking that function:

function commerce_cart_order_product_line_item_delete($order, $line_item_id, $skip_save = FALSE) {
  ...
  // Delete the actual line item.
  commerce_line_item_delete($line_item->line_item_id);

  if (!$skip_save) {
    commerce_order_save($order);
  }

But when invoking commerce_line_item_delete() the order is saved anyway by

commerce_line_item_delete()
>> commerce_line_item_delete_multiple()
>> CommerceLineItemEntityController->delete( )
>> commerce_line_item_delete_references()
>> entity_save()

So this causes an infinite loop when you have logic in a hook_commerce_order_presave() that removes a line item.

So unless I'm using the wrong hook or function to remove a line item I suppose this is a bug.

Use case:
There are administrative costs that need to be added when the order is below a certain amount. So I check on commerce_order_presave() what the value of the cart is and I either add/remove/leave the admin-costs product.

CommentFileSizeAuthor
#11 2316709-skip_save-11.patch2.87 KBjcisio
#10 interdiff-8-9.txt838 bytesczigor
#10 commerce-2316709-9-skip-save.patch8.25 KBczigor
#8 commerce-2316709-8-skip-save.patch8.21 KBczigor
#7 interdiff-6-7.txt7.46 KBczigor
#6 commerce-2316709-6-skip-save.patch3.17 KBczigor
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Jax’s picture

Jax CreditAttribution: Jax commented

commerce_shipping_delete_shipping_line_items() has its own way of deleting line items. Not sure what is best.

Jax’s picture

Jax CreditAttribution: Jax commented

And that also triggers an order save. As far as I understand it's not possible to remove a line item from an order and delaying the order save since it always passes through commerce_line_item_delete_references() which saves the order.

If that's the case please close this ticket. I will need to find another place to do the calculation. Any suggestions for where are welcome.

rszrama’s picture

rszrama CreditAttribution: rszrama commented
Issue tags: +sprint

Tagging for http://contribkanban.com/#/board/commerce/7.x-1.x.

mglaman’s picture

mglaman
Primary language English
Location WI, USA
CreditAttribution: mglaman at Centarro commented
Issue summary: View changes

Fix formatting

mglaman’s picture

mglaman
Primary language English
Location WI, USA
CreditAttribution: mglaman at Centarro commented

Sounds like $skip_save is unable to provide functionality due how commerce line item's are saved. Perhaps we should just remove it to reduce confusion?

czigor’s picture

czigor CreditAttribution: czigor commented
Status: Active » Needs review
FileSize
commerce-2316709-6-skip-save.patch3.17 KB

Just bumped into this when I needed to remove line items on order change. Looking at the line item entity controller it seems to be impossible to remove line items without saving the order.

Since it seems to be easy to pass down $skip_save to the controller, let's pass down $skip_save to the controller.

Our use case is removing products, that are for sale only in one currency, from the cart when the order currency changes.

czigor’s picture

czigor CreditAttribution: czigor commented
FileSize
interdiff-6-7.txt7.46 KB

This is more complicated than this. We need to pass down the parent entity as well, otherwise we throw out the baby with the bath water.

czigor’s picture

czigor CreditAttribution: czigor commented
FileSize
commerce-2316709-8-skip-save.patch8.21 KB

Status: Needs review » Needs work

The last submitted patch, 8: commerce-2316709-8-skip-save.patch, failed testing.

czigor’s picture

czigor CreditAttribution: czigor commented
Status: Needs work » Needs review
FileSize
commerce-2316709-9-skip-save.patch8.25 KB
interdiff-8-9.txt838 bytes

5.3. does not support the 

myfunction()['arraykey'];

syntax.

jcisio’s picture

jcisio CreditAttribution: jcisio at Axess Open Web Services commented
FileSize
2316709-skip_save-11.patch2.87 KB

New patch, only pass $skip_save.

  • We know that parent entity type is always commerce_order (used in commerce_cart_order_product_line_item_delete, and in contrib commerce_shipping_delete_shipping_line_items, nothing else). But we keep saving on other entity types.
  • Keep the logic simple: a commerce_line_item entity can't be present on multiple order. We don't have to pass parent entity, there is only one.
  • Fix bug in the previous patch: when $parent_entity is passed, it updates this entity. But other entity types can also reference this line item.
czigor’s picture

czigor CreditAttribution: czigor at Centarro for FREITAG lab. AG commented
Status: Needs review » Reviewed & tested by the community

The changes in #11 make sense and also work for us.

czigor’s picture

czigor CreditAttribution: czigor at Centarro for FREITAG lab. AG commented
Issue tags: -sprint +release-1.14

rszrama’s picture

rszrama CreditAttribution: rszrama at Centarro commented
Status: Reviewed & tested by the community » Fixed

Great, thanks for the patches and reviews! Committing #11 with a little extra documentation in the controller to ensure callers know they are responsible for saving the order if they opt to skip the save in the controller's delete method.

Status: Fixed » Closed (fixed)

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

ezeedub’s picture

ezeedub CreditAttribution: ezeedub commented

Looks like #11 removed the actual fix to commerce_cart_order_product_line_item_delete(). Since this has already been released, I created a new issue #3006593 to add that back.