Orders need a strict guarantee that they won't be accidentally overwritten in case two users (admin/customer, or customer/script) collide.
There are two ways to accomplish this: pessimistic and optimistic locking.

Pessimistic locking ensures that only one side can load the order for editing, all other loads are blocked.
Optimistic locking allows all sides to load, but will only allow one to save, the others will fail.

This conflict is not common, and is usually one of two scenarios:
1) Admin editing a customer order (which is still a cart or in checkout)
2) A script (cron, queue) modifying an order being edited by either the admin or the customer.

Commerce 1.x

Commerce 1.x implemented pessimistic locking. This proved to be too strict, because it's hard to identify an edit context in Drupal. To be safe, the code would lock all orders viewed in an admin view, on the order view page, etc. This problem was identified and is being addressed in #2240427: Improve commerce entities locking.

Drupal 8 core

Drupal 8 implements optimistic locking (kinda) for content entities. Any content entity that implements EntityChangedInterface has the EntityChanged constraint added.
The EntityChangedConstraintValidator ensures that validation fails if the entity changed timestamp changed in the meantime.
The message shown is "The content has either been modified by another user, or you have already submitted modifications. As a result, your changes cannot be saved."

There are two problems with this:
- Created is not granular enough, since it's in seconds. There can be multiple entity saves in the same second.
- The constraint is not enforced on entity save, only on the entity form.

Commerce 2.x

We want to go with optimistic locking, since it will provide better performance. We'll go with an explicit version field, to ensure ideal granularity.

Plan:
- Unset the EntityChanged constraint.
- Create a version integer field that gets incremented on each save (in preSave())
- Create an OrderVersionConstraintValidator that does the same thing as EntityChangedConstraintValidator (load original order, compare versions), for order forms.
- Before incrementing the version field in preSave() repeat the logic from OrderVersionConstraintValidator, and throw an exception (OrderVersionMismatchException).
This ensures that the overwrite is caught even if the validator isn't invoked.
- From commerce_cart add an order form alter that shows a warning for all cart orders active in the last hour. "This order has been modified by the customer in the last hour. Modifying it here could cause their future changes to be lost. Proceed with caution.".

We also need explicit $order->lock() and $order->unlock() methods, for cases where optimistic locking isn't strict enough (payment page at checkout, for example). Trying to save a locked order throws an exception.

CommentFileSizeAuthor
#66 interdiff_62-66.txt4.49 KBjsacksick
#66 2656818-66.patch21.44 KBjsacksick
#65 order-settings-01.png102.35 KBrszrama
#64 order-settings.png103.73 KBrszrama
#62 interdiff_59-62.txt550 bytesjsacksick
#62 2656818-62.patch22.11 KBjsacksick
#59 2656818-59.patch22.11 KBjsacksick
#56 2656818-56.patch25.22 KBmglaman
#54 Screen Shot 2020-11-12 at 10.32.49 AM.png63.51 KBmglaman
#54 Screen Shot 2020-11-12 at 10.32.42 AM.png62.38 KBmglaman
#54 2656818-53.patch25.31 KBmglaman
#50 Screen Shot 2020-11-11 at 4.39.41 PM.png51.23 KBmglaman
#50 Screen Shot 2020-11-11 at 4.39.37 PM.png45.66 KBmglaman
#50 2656818-50.patch24.41 KBmglaman
#48 2656818-48.patch17.8 KBmglaman
#48 interdiff-2656818-45-48.txt3.77 KBmglaman
#45 2656818-45.patch16.94 KBmglaman
#43 2656818-43.patch16.19 KBmglaman
#35 2656818-35.patch16.26 KBmglaman
#33 2656818-33.patch16.6 KBmglaman
#31 2656818-31.patch23.57 KBmglaman
#29 2656818-29.patch23.13 KBmglaman
#25 2656818-25.patch20.27 KBmglaman
#23 2656818-23.patch20.2 KBmglaman
#20 2656818-20.patch16.63 KBmglaman
#9 order_version_locking-2656818-9.patch17 KBsmccabe
#7 order_version_locking-2656818-7.patch16.88 KBvasike
#6 order_version_locking-2656818-6.patch12.4 KBvasike
#5 order_version_locking-2656818-5.patch11.53 KBvasike
#3 order_version_locking-2656818-4.patch7.89 KBvasike
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

bojanz created an issue. See original summary.

bojanz’s picture

Issue summary: View changes

Add a node that we need to unset the EntityChanged constraint.

vasike’s picture

Status: Active » Needs review
FileSize
7.89 KB

here is patch for this that tries to implement what's requested in the issue summary.

However i wasn't able to test it, maybe there's something missing.

bojanz’s picture

Status: Needs review » Needs work

+ * "version" = "version",
version is not an entity key
entity keys are a fixed list (id, label, uuid, uid, bundle)
nothing else
+ $this->setVersion(1); -> this should be a default value set in baseFieldDefinitions

The next step will be to write a test.

We are also missing "- From commerce_cart add an order form alter that shows a warning for all cart orders active in the last hour. "This order has been modified by the customer in the last hour. Modifying it here could cause their future changes to be lost. Proceed with caution."

vasike’s picture

Status: Needs work » Needs review
FileSize
11.53 KB

here is patch with bojanz (#4) corrections and suggestions:
implement OrderConstraintsTest

it seems the order validation requires also a value for store_id, which i added to this test.
Should we update for other Order tests?

todo: commerce_cart add an order form alter

vasike’s picture

new update for the order edit message for the orders that are carts and modified in the last hour.

vasike’s picture

New patch update:
- Fix for order edit message: "<" instead of ">".
- Implements hook_form_alter() for Views commerce cart forms
- - Check for a Views commerce cart forms, check is a Views form and then check if it's the cart form
- - Replace the submit button label, moved from Edit quantity Views field handler
- - Add (& build) validation for submit buttons, also for remove buttons if present.

For checking "Views commerce cart forms" - there is a new helper function, also used for "commerce_order_type_form" alter.
It checks if View has 'commerce_cart_form' like tag, has commerce_order as base table and the order_id as the first argument.

vasike’s picture

also created a PR with the latest patch
https://github.com/drupalcommerce/commerce/pull/317

smccabe’s picture

smccabe’s picture

Status: Needs review » Needs work

Few notes, although it is a really big patch to review so probably needs more than 1 pass.

  1. +++ b/modules/cart/commerce_cart.module
    @@ -123,6 +173,10 @@ function commerce_cart_form_commerce_order_form_alter(array &$form, FormStateInt
    +      // Warning it the order is cart and it was modified in the last hour.
    

    small change, should be "if"

  2. +++ b/modules/cart/commerce_cart.module
    @@ -123,6 +173,10 @@ function commerce_cart_form_commerce_order_form_alter(array &$form, FormStateInt
    +      if (!empty($order->get('cart')->value) && (REQUEST_TIME - $order->getChangedTime()) < 3600) {
    +        drupal_set_message(t('This order has been modified by the customer in the last hour. Modifying it here could cause their future changes to be lost. Proceed with caution.'), 'warning');
    +      }
    

    Isn't this a message that would be seen by the customer? Should it say something like "Your order has been modified" and not "by the customer"? Also why do we only care about the last hour?

  3. +++ b/modules/cart/src/Plugin/views/field/EditQuantity.php
    @@ -107,8 +107,6 @@ class EditQuantity extends FieldPluginBase {
    -    // Replace the form submit button label.
    -    $form['actions']['submit']['#value'] = $this->t('Update cart');
    

    Why do we remove this button in the context of this patch? I don't follow.

  4. +++ b/modules/order/src/Entity/Order.php
    @@ -128,6 +141,21 @@ class Order extends ContentEntityBase implements OrderInterface {
    +  public function setVersion($version) {
    +    $this->set('version', $version);
    +    return $this;
    +  }
    

    Could this cause an issue where it has been updated but the version hasn't been saved yet?

bojanz’s picture

Issue summary: View changes

Expanded architecture with explicit lock/unlock methods.

vasike’s picture

Status: Needs work » Needs review

new PR available
https://github.com/drupalcommerce/commerce/pull/649

Updates from previous:
- Added $order->lock() and $order->unlock() methods (not sure about them yet).
- post_update hook to add the version field.
- Update Helper function to check if a view is a 'commerce_cart' view - it takes the tag as argument.
- Update the test - check for Entity validation only

$violations = $order->validate()->getEntityViolations();

Maybe here we should build other Test for all kind of validation for Orders - plan needed???.

Closed the previous pr.

vasike’s picture

new commits for the latest PR to make it green
https://github.com/drupalcommerce/commerce/pull/649

vasike’s picture

PR updated with requested changes there.

vasike’s picture

new updates for the latest PR to make it green

- There were some issues with 8.x-2.x updates.
- (bonus) Extend Order Constraints tests to Order validations tests.
As i noticed the Order has other thing to validate - Order (fields) data validation.

vasike’s picture

PR "reroll"

bojanz’s picture

Note that we committed a related fix in #2906563: Provide a way to explicitly lock the order.

vasike’s picture

Pr updated - according with module latest updates on locking.

mglaman’s picture

mglaman’s picture

Here is a re-roll of the PR against HEAD. It is missing the logic from commerce_cart.module to check versions when updating the cart.

Status: Needs review » Needs work

The last submitted patch, 20: 2656818-20.patch, failed testing. View results

mglaman’s picture

mglaman’s picture

Status: Needs work » Needs review
FileSize
20.2 KB

This provides a test that uncovers a fun problem in \Drupal\commerce_order\Entity\OrderItem::postSave

  /**
   * {@inheritdoc}
   */
  public function postSave(EntityStorageInterface $storage, $update = TRUE) {
    parent::postSave($storage, $update);

    $order = $this->getOrder();
    if ($order && !$order->hasItem($this)) {
      $order->addItem($this);
      $order->save();
    }
  }

With the current CartManager implementation, this causes a save that is out of sequence and breaks our version numbers. I wrote a test to demonstrate and commented out $order_item->set('order_id', $cart->id()); in CartManager to test.

      // Providing the order ID now causes the order item to attach itself to
      // the order before we explicitly do, causing a background save.
      // @see \Drupal\commerce_order\Entity\OrderItem::postSave
      // $order_item->set('order_id', $cart->id());

Status: Needs review » Needs work

The last submitted patch, 23: 2656818-23.patch, failed testing. View results

mglaman’s picture

Status: Needs work » Needs review
FileSize
20.27 KB

Forgot test group.

Status: Needs review » Needs work

The last submitted patch, 25: 2656818-25.patch, failed testing. View results

mglaman’s picture

Again, another post* in an entity's life cycle burns us. This time it is order items trying to clean up their references

  /**
   * {@inheritdoc}
   */
  public static function postDelete(EntityStorageInterface $storage, array $entities) {
    parent::postDelete($storage, $entities);

    /** @var \Drupal\commerce_order\Entity\OrderItemInterface[] $entities */
    foreach ($entities as $order_item) {
      // Remove the reference from the order.
      $order = $order_item->getOrder();
      if ($order && $order->hasItem($order_item)) {
        $order->removeItem($order_item);
        $order->save();
      }
    }
  }
mglaman’s picture

If we undo the work in #3005070: [Reverted] Make the order-order_item relationship more robust, most of our problems are solved.

mglaman’s picture

Status: Needs work » Needs review
FileSize
23.13 KB

This undoes the order item postSave and postDelete. In reality, we should have the order saving or deleting items at least since the order controls the relationship.

Status: Needs review » Needs work

The last submitted patch, 29: 2656818-29.patch, failed testing. View results

mglaman’s picture

Status: Needs work » Needs review
FileSize
23.57 KB

Fixes test assert which throws version mismatch.

bojanz’s picture

Status: Needs review » Needs work

#3005070: [Reverted] Make the order-order_item relationship more robust has now been reverted, this patch can be rerolled.

mglaman’s picture

Status: Needs work » Needs review
FileSize
16.6 KB

Here is a reroll.

Status: Needs review » Needs work

The last submitted patch, 33: 2656818-33.patch, failed testing. View results

mglaman’s picture

Status: Needs work » Needs review
FileSize
16.26 KB

Remove php7ness

Status: Needs review » Needs work

The last submitted patch, 35: 2656818-35.patch, failed testing. View results

skyredwang’s picture

amateescu’s picture

If the patch committed in #3130011: Clear broken order items reference on presave is not reverted in that issue, I think the patch here should revert it.

jsacksick’s picture

Priority: Normal » Major

We'll have to come back to this as I'm currently experiencing several issues due to the order not being locked, See #3119157: CartManager bug in asynchronous multi-threads and #3130011: Clear broken order items reference on presave.

IMO this is a critical feature that needs to be implemented, especially in a headless context where you have async requests potentially saving the same order.

Not having locking opens the door to data integrity issues.
That said, my main concern is that it requires to update any code that's saving the order and I suggest adding a setting (disabled by default) that toggles whether we throw an exception vs just logging.

When the setting is off, we should add a big warning that links to a documentation page explaining why locking is essential.

Bumping the priority.

jsacksick’s picture

I think the logic should be moved to OrderStorage::doOrderPresave() since I believe the version check should be happening before refreshing the order.

Also, the order now implements the EntityChangedInterface + Trait so the patch definetly needs a reroll.

mglaman’s picture

Discussing with @agoradesign, we'd definitely need to make this opt-in and not a default behavior. Just because most projects have code that don't support this.

Something like $settings['enable_order_locking']. We should make it enabled by default in tests to help improve future code. Kind of like the config schema checker in tests.

bojanz’s picture

Maybe put it on the same page as the checkbox for enabling product revisions? And default both to TRUE on new installs?

mglaman’s picture

Status: Needs work » Needs review
FileSize
16.19 KB

Here's a reroll. I need to add the settings as proposed in #42.

Status: Needs review » Needs work

The last submitted patch, 43: 2656818-43.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

mglaman’s picture

Status: Needs work » Needs review
FileSize
16.94 KB
+++ b/modules/order/tests/src/Functional/OrderAdminTest.php
+++ b/modules/order/tests/src/Functional/OrderAdminTest.php
@@ -283,6 +283,9 @@ class OrderAdminTest extends OrderBrowserTestBase {

@@ -283,6 +283,9 @@ class OrderAdminTest extends OrderBrowserTestBase {
     $this->assertSession()->buttonNotExists('Cancel order');
...
+    // The order was modified and needs to be reloaded.
+    $order = $this->reloadEntity($order);
+

Failure is due to forgetting to bring this over 🤦‍♂️

mglaman’s picture

#40:

      // Order::preSave() has completed, now run the storage-level pre-save
      // tasks. These tasks can modify the order, so they need to run
      // before the entity/field hooks are invoked.

The order preSave runs before the storage methods – so before refresh

    // Allow code to run before saving.
    $entity->preSave($this);
    $this->invokeHook('presave', $entity);
mglaman’s picture

Assigned: mglaman » Unassigned

#42

And default both to TRUE on new installs?

I keep thinking about this and I'm afraid it will cause so many bug reports, the same reason we are afraid of turning it on by default for existing sites.

mglaman’s picture

Okay, here's an attempt at making it a setting that is stored in the entity type's definition. It is set to log by default and tests turn it out by default. The next step is a setting form to control this.

mglaman’s picture

+++ b/modules/order/commerce_order.install
@@ -224,3 +224,20 @@ function commerce_order_update_8210() {
+    ->setDefaultValue(1);

This should be

    ->setDefaultValue(0)
    // Ensure any existing orders get set to version 1.
    ->setInitialValue(1);

Next is to provide a settings form at /admin/commerce/config/orders/settings that will live alongside the order types, order item types, and number patterns pages.

mglaman’s picture

This adds a setting form to disable logging and allow the exception to be thrown when saving an order out of sync.

Screenshots of the settings form. Words to be bikeshed.

Default:

Disable logging:

I'm beginning to think it needs to be a radio, not a checkbox:

  • Allow saving the order and logging version mismatch
  • Do not allow saving orders with a version mismatch
jsacksick’s picture

Why this? - $order_item->set('order_id', $cart->id());.

I'm beginning to think it needs to be a radio, not a checkbox:

Allow saving the order and logging version mismatch
Do not allow saving orders with a version mismatch

I also think this makes more sense.

mglaman’s picture

@jsacksick see #23

-      $order_item->set('order_id', $cart->id());
+      // Providing the order ID now causes the order item to attach itself to
+      // the order before we explicitly do, causing a background save.
+      // @see \Drupal\commerce_order\Entity\OrderItem::postSave
+      // $order_item->set('order_id', $cart->id());

mglaman’s picture

New screenshots. I also added the comment back per #52, so that we know why this change was made.

Crediting lisastreeter for words help :)


mglaman’s picture

Status: Needs review » Needs work

The post-update in commerce_order_post_update_14 fails... apparently the config importer doesn't work for regular config objects (even though we have code to.) It doesn't detect the config type for commerce_order.settings and tries to load a nil config entity type.

>  [notice] Update started: commerce_order_update_8211
>  [notice] The order version number field was created.
>  [notice] Update completed: commerce_order_update_8211
>  [notice] Update started: commerce_order_post_update_14
>  [error]  The "" entity type does not exist. 
>  [error]  Update failed: commerce_order_post_update_14 
 [error]  Update aborted by: commerce_order_post_update_14 
 [error]  Finished performing updates. 
mglaman’s picture

Status: Needs work » Needs review
FileSize
25.22 KB

This fixes the config import by using the config factory directly. I'll open a follow up issue for the config importer.

mglaman’s picture

jsacksick’s picture

Status: Needs review » Needs work

We need to put back$order_item->set('order_id', $cart->id());, this was causing an issue when OrderItem::postSave() was trying to set the reference from the order to the order item, but this is no longer the case.

I've been testing #3179312: Add an option to Buy X Get Y to auto-add the "Get" product variation where I also advised to remove $order_item->set('order_id', $cart->id()); and this is causing a fatal error when the tax module is trying to resolve the customer profile in TaxTypeBase::resolveCustomerProfile()

jsacksick’s picture

I put back $order_item->set('order_id', $cart->id()); which was removed because of #3005070: [Reverted] Make the order-order_item relationship more robust that was reverted since.

It's probably unnecessary though... As since #3143382: AddToCartForm should reference the destination cart when building the order item the order item references the order when the Add to cart form is built.

I fixed the post update / update numbers (to account for the updates that occurred since).

Additionally, I believe we stopped implementing the EntityChangedInterface in order to skip the EntityChanged constraint which is automatically added from the EntityType constructor:

    if ($this->entityClassImplements(EntityChangedInterface::class)) {
      $this->addConstraint('EntityChanged');
    }

However, I believe we could simply remove the constraint from commerce_order_entity_type_alter().

I'm tempted to commit the patch... Though what's making me hesitant is that we don't actually have any try catch blocks in our code expecting that exception... (Shouldn't we expect it from several places like AddToCartForm::submitForm() for example which saves the order?
PaymentCheckoutController::returnPage() would be a good candidate as well...

The main reason why we could go ahead and commit this is that we don't throw the exception by default and we simply log it... That is what's making this feature "safe" IMO.

Perhaps it's fine to commit the patch as is and wait for actual bug reports... Thoughts?
I have some tests failing locally for some reason... Let's see what d.org says.

Also... Does it actually make sense to expose a setting in the UI for this vs just setting it from the settings.php? (I'm afraid the form/settings are too technical for non developers...

mglaman’s picture

Status: Needs review » Reviewed & tested by the community

LETS DO THIS

+++ b/modules/order/src/Plugin/Validation/Constraint/OrderVersionConstraintValidator.php
@@ -0,0 +1,29 @@
+    if (isset($entity) && !$entity->isNew()) {
+      /** @var \Drupal\commerce_order\Entity\OrderInterface $saved_entity */
+      $saved_entity = \Drupal::entityTypeManager()
+        ->getStorage($entity->getEntityTypeId())
+        ->loadUnchanged($entity->id());

The only downside is that this can be such a performance killer (load unchanged.) But I cannot fathom a better alternative.

agoradesign’s picture

yeah, as long as we only log by default, I'm also for committing this! Hopefully some brave boys and girls will test the exception configuration in the wild then :D imho any step towards solving that problem is very important, as this is the most annoying Commerce bug I know so far!

@mglaman: well yes, but I don't think that the impact is that big, and a write operation (especially on an order) should be allowed to do some non-trivial stuff

jsacksick’s picture

@mglaman: The EntityChangedConstraint is also doing this... So it's not like we're introducing a performance regression...

As pointed out by Ryan, the path for the settings form is "wrong" and should contain /orders (so that the breadcrumb allows you to go back to the orders config, and not the general commerce config.

jsacksick’s picture

Reviewing the exception message, and I think it should be changed, since finding that exception in logs isn't going to be helpful...

What about...

Attempted to save order @order_id with version @version. Current version is @current_version.

Instead...

So if the exception is found in logs, we at least know which order ID it relates to... Thoughts?

rszrama’s picture

FileSize
103.73 KB

Good from a functionality standpoint. From a UX standpoint, proposed a simpler order settings form:

rszrama’s picture

FileSize
102.35 KB

Sorry, wrong image.

jsacksick’s picture

FileSize
21.44 KB
4.49 KB

Attached patch addresses comment #63 + the settings form improvements from @rzsrama.

Hopefully this is good to go now!

  • jsacksick committed f6427d7 on 8.x-2.x authored by mglaman
    Issue #2656818 by mglaman, vasike, jsacksick, smccabe, rszrama, bojanz,...
jsacksick’s picture

Status: Reviewed & tested by the community » Fixed

Committed 🎉 🎉 !!! Thanks everyone!

Status: Fixed » Closed (fixed)

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

newaytech’s picture

Hi folks,

Thanks for the awesome work on Commerce - it's a great product. I've noticed an error in my log files following the latest Optimistic locking patch - so I flicked the switch to fatal error to get a full stack trace. Nothing I can see in my custom code here - so thought I'd share here as an example of issues in the wild:

This is thrown on "/checkout/7051/payment" - right after adding credit card details and submitting the form.

The website encountered an unexpected error. Please try again later.

Drupal\Core\Entity\EntityStorageException: Attempted to save order 7051 with version 7. Current version is 8. in Drupal\Core\Entity\Sql\SqlContentEntityStorage->save() (line 846 of core\lib\Drupal\Core\Entity\Sql\SqlContentEntityStorage.php).
Drupal\Core\Entity\EntityStorageBase->doPreSave(Object) (Line: 700)
Drupal\Core\Entity\ContentEntityStorageBase->doPreSave(Object) (Line: 454)
Drupal\Core\Entity\EntityStorageBase->save(Object) (Line: 837)
Drupal\Core\Entity\Sql\SqlContentEntityStorage->save(Object) (Line: 395)
Drupal\Core\Entity\EntityBase->save() (Line: 198)
Drupal\commerce_checkout\Plugin\Commerce\CheckoutFlow\CheckoutFlowBase->redirectToStep('complete') (Line: 180)
Drupal\commerce_payment\Plugin\Commerce\CheckoutPane\PaymentProcess->buildPaneForm(Array, Object, Array) (Line: 546)
Drupal\commerce_checkout\Plugin\Commerce\CheckoutFlow\CheckoutFlowWithPanesBase->buildForm(Array, Object, 'payment')
call_user_func_array(Array, Array) (Line: 532)
Drupal\Core\Form\FormBuilder->retrieveForm('commerce_checkout_flow_multistep_default', Object) (Line: 278)
Drupal\Core\Form\FormBuilder->buildForm(Object, Object) (Line: 219)
Drupal\Core\Form\FormBuilder->getForm(Object, 'payment') (Line: 143)
Drupal\commerce_checkout\Controller\CheckoutController->formPage(Object)
call_user_func_array(Array, Array) (Line: 123)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() (Line: 573)
Drupal\Core\Render\Renderer->executeInRenderContext(Object, Object) (Line: 124)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->wrapControllerExecutionInRenderContext(Array, Array) (Line: 97)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() (Line: 151)
Symfony\Component\HttpKernel\HttpKernel->handleRaw(Object, 1) (Line: 68)
Symfony\Component\HttpKernel\HttpKernel->handle(Object, 1, 1) (Line: 57)
Drupal\Core\StackMiddleware\Session->handle(Object, 1, 1) (Line: 47)
Drupal\Core\StackMiddleware\KernelPreHandle->handle(Object, 1, 1) (Line: 106)
Drupal\page_cache\StackMiddleware\PageCache->pass(Object, 1, 1) (Line: 85)
Drupal\page_cache\StackMiddleware\PageCache->handle(Object, 1, 1) (Line: 47)
Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle(Object, 1, 1) (Line: 52)
Drupal\Core\StackMiddleware\NegotiationMiddleware->handle(Object, 1, 1) (Line: 23)
Stack\StackedHttpKernel->handle(Object, 1, 1) (Line: 708)
Drupal\Core\DrupalKernel->handle(Object) (Line: 19)
agoradesign’s picture

@newaytech which payment gateway plugin do you use?

newaytech’s picture

Hi @agoradesign - Stripe - which for every error was in use (we have manual payment as option too)

John Pitcairn’s picture

I'm also seeing this exception thrown and logged when commerce recurring processes a queued draft order from a cron run. We are also using Stripe.

Ambient.Impact’s picture

I'm also getting the same thing as #70 when I switch it to throw an exception, also using Stripe (though not recurring). The order seems to complete fine and our order paid event subscriber is correctly invoked if set to the default to just log it, so it doesn't seem to be causing issues but thought I'd confirm it.

The website encountered an unexpected error. Please try again later.

Drupal\Core\Entity\EntityStorageException: Attempted to save order 370 with version 6. Current version is 7. in Drupal\Core\Entity\Sql\SqlContentEntityStorage->save() (line 846 of core\lib\Drupal\Core\Entity\Sql\SqlContentEntityStorage.php).

Drupal\Core\Entity\EntityStorageBase->doPreSave(Object) (Line: 700)
Drupal\Core\Entity\ContentEntityStorageBase->doPreSave(Object) (Line: 454)
Drupal\Core\Entity\EntityStorageBase->save(Object) (Line: 837)
Drupal\Core\Entity\Sql\SqlContentEntityStorage->save(Object) (Line: 395)
Drupal\Core\Entity\EntityBase->save() (Line: 198)
Drupal\commerce_checkout\Plugin\Commerce\CheckoutFlow\CheckoutFlowBase->redirectToStep('complete') (Line: 180)
Drupal\commerce_payment\Plugin\Commerce\CheckoutPane\PaymentProcess->buildPaneForm(Array, Object, Array) (Line: 546)
Drupal\commerce_checkout\Plugin\Commerce\CheckoutFlow\CheckoutFlowWithPanesBase->buildForm(Array, Object, 'payment')
call_user_func_array(Array, Array) (Line: 532)
Drupal\Core\Form\FormBuilder->retrieveForm('commerce_checkout_flow_multistep_default', Object) (Line: 278)
Drupal\Core\Form\FormBuilder->buildForm(Object, Object) (Line: 219)
Drupal\Core\Form\FormBuilder->getForm(Object, 'payment') (Line: 143)
Drupal\commerce_checkout\Controller\CheckoutController->formPage(Object)
call_user_func_array(Array, Array) (Line: 123)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() (Line: 573)
Drupal\Core\Render\Renderer->executeInRenderContext(Object, Object) (Line: 124)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->wrapControllerExecutionInRenderContext(Array, Array) (Line: 97)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() (Line: 151)
Symfony\Component\HttpKernel\HttpKernel->handleRaw(Object, 1) (Line: 68)
Symfony\Component\HttpKernel\HttpKernel->handle(Object, 1, 1) (Line: 57)
Drupal\Core\StackMiddleware\Session->handle(Object, 1, 1) (Line: 47)
Drupal\Core\StackMiddleware\KernelPreHandle->handle(Object, 1, 1) (Line: 106)
Drupal\page_cache\StackMiddleware\PageCache->pass(Object, 1, 1) (Line: 85)
Drupal\page_cache\StackMiddleware\PageCache->handle(Object, 1, 1) (Line: 50)
Drupal\ban\BanMiddleware->handle(Object, 1, 1) (Line: 47)
Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle(Object, 1, 1) (Line: 52)
Drupal\Core\StackMiddleware\NegotiationMiddleware->handle(Object, 1, 1) (Line: 49)
Drupal\remove_http_headers\StackMiddleware\RemoveHttpHeadersMiddleware->handle(Object, 1, 1) (Line: 23)
Stack\StackedHttpKernel->handle(Object, 1, 1) (Line: 708)
Drupal\Core\DrupalKernel->handle(Object) (Line: 19)
John Pitcairn’s picture

For code that is saving orders somewhere and triggering this exception, is there something the code should now be doing to avoid that? Is that documented with an example? There's no change record.

Berdir’s picture

Make sure you are loading the latest version of an order before saving it. There are issues with race conditions, for example between the notify and return routes/callbacks of a payment gateway. See #3043180: The changes made to the order on the onNotify method are not applied on the onReturn method for my attempt at avoiding that, but it will require changes to the affected gateways. Feedback very welcome there.