Issue #3043180 by Berdir, facine, matthiasm11, tBKoT, jsacksick: The changes...

Issue #3043180 by Berdir, facine, matthiasm11, tBKoT, jsacksick: The changes made to the order on the onNotify method are not applied on the onReturn method.

modules/order/src/Exception/OrderLockedSaveException.php

+<?php
+
+namespace Drupal\commerce_order\Exception;
+
+/**
+ * Thrown when attempting to save an order that is locked for updating.
+ */
+class OrderLockedSaveException extends \RuntimeException {}

modules/order/src/OrderStorage.php

@@ -6,14 +6,16 @@ use Drupal\commerce\CommerceContentEntityStorage;
 use Drupal\commerce_order\Entity\OrderInterface;
 use Drupal\commerce_order\Event\OrderEvent;
 use Drupal\commerce_order\Event\OrderEvents;
+use Drupal\commerce_order\Exception\OrderLockedSaveException;
 use Drupal\Core\Entity\EntityInterface;
+use Drupal\Core\Entity\EntityStorageException;
 use Drupal\Core\Entity\EntityTypeInterface;
 use Symfony\Component\DependencyInjection\ContainerInterface;
 
 /**
  * Defines the order storage.
  */
-class OrderStorage extends CommerceContentEntityStorage {
+class OrderStorage extends CommerceContentEntityStorage implements OrderStorageInterface {
 
   /**
    * The order refresh.
@@ -29,12 +31,27 @@ class OrderStorage extends CommerceContentEntityStorage {
    */
   protected $skipRefresh = FALSE;
 
+  /**
+   * List of successfully locked orders.
+   *
+   * @var int[]
+   */
+  protected $updateLocks = [];
+
+  /***
+   * The lock backend.
+   *
+   * @var \Drupal\Core\Lock\LockBackendInterface
+   */
+  protected $lockBackend;
+
   /**
    * {@inheritdoc}
    */
   public static function createInstance(ContainerInterface $container, EntityTypeInterface $entity_type) {
     $instance = parent::createInstance($container, $entity_type);
     $instance->orderRefresh = $container->get('commerce_order.order_refresh');
+    $instance->lockBackend = $container->get('lock');
     return $instance;
   }
 
@@ -76,6 +93,18 @@ class OrderStorage extends CommerceContentEntityStorage {
    *   The order.
    */
   protected function doOrderPreSave(OrderInterface $order) {
+    if (!$order->isNew() && !isset($this->updateLocks[$order->id()]) && !$this->lockBackend->lockMayBeAvailable($this->getLockId($order->id()))) {
+      // This is updating an order that someone else has locked.
+      $mismatch_exception = new OrderLockedSaveException('Attempted to save order ' . $order->id() . ' that is locked for updating. Use OrderStorage::loadForUpdate().');
+      $log_only = $this->getEntityType()->get('log_version_mismatch');
+      if ($log_only) {
+        watchdog_exception('commerce_order', $mismatch_exception);
+      }
+      else {
+        throw $mismatch_exception;
+      }
+    }
+
     // Ensure the order doesn't reference any removed order item by resetting
     // the "order_items" field with order items that were successfully loaded
     // from the database.
@@ -122,4 +151,52 @@ class OrderStorage extends CommerceContentEntityStorage {
     return parent::postLoad($entities);
   }
 
+  /**
+   * {@inheritdoc}
+   */
+  public function save(EntityInterface $entity) {
+    try {
+      return parent::save($entity);
+    }
+    finally {
+      // Release the update lock if it was acquired for this entity.
+      if (isset($this->updateLocks[$entity->id()])) {
+        $this->lockBackend->release($this->getLockId($entity->id()));
+        unset($this->updateLocks[$entity->id()]);
+      }
+    }
+  }
+
+  /**
+   * {@inheritdoc}
+   */
+  public function loadForUpdate(int $order_id): ?OrderInterface {
+    $lock_id = $this->getLockId($order_id);
+    if ($this->lockBackend->acquire($lock_id)) {
+      $this->updateLocks[$order_id] = TRUE;
+      return $this->loadUnchanged($order_id);
+    }
+    else {
+      // Failed to acquire initial lock, wait for it to free up.
+      if (!$this->lockBackend->wait($lock_id) && $this->lockBackend->acquire($lock_id)) {
+        $this->updateLocks[$order_id] = TRUE;
+        return $this->loadUnchanged($order_id);
+      }
+      throw new EntityStorageException('Failed to acquire lock');
+    }
+  }
+
+  /**
+   * Gets the lock ID for the given order ID.
+   *
+   * @param int $order_id
+   *   The order ID.
+   *
+   * @return string
+   *   The lock ID.
+   */
+  protected function getLockId(int $order_id): string {
+    return 'commerce_order_update:' . $order_id;
+  }
+
 }

modules/order/src/OrderStorageInterface.php

+<?php
+
+namespace Drupal\commerce_order;
+
+use Drupal\commerce_order\Entity\OrderInterface;
+use Drupal\Core\Entity\ContentEntityStorageInterface;
+
+/**
+ * Defines the interface for order storage.
+ */
+interface OrderStorageInterface extends ContentEntityStorageInterface {
+
+  /**
+   * Loads the unchanged entity, bypassing the static cache, and locks it.
+   *
+   * This implements explicit, pessimistic locking as opposed to the optimistic
+   * locking that will log or prevent a conflicting save. Use this method for
+   * use cases that load an order with the explicit purpose of immediately
+   * changing and saving it again. Especially if these cases may run in parallel
+   * to others, for example notification/return callbacks and termination
+   * events.
+   *
+   * @param int $order_id
+   *   The order ID.
+   *
+   * @return \Drupal\commerce_order\Entity\OrderInterface|null
+   *   The loaded order or NULL if the entity cannot be loaded.
+   *
+   * @throws \Drupal\Core\Entity\EntityStorageException
+   *   Thrown if the lock could not be acquired.
+   */
+  public function loadForUpdate(int $order_id): ?OrderInterface;
+
+}

modules/order/tests/modules/commerce_order_test/commerce_order_test.routing.yml

@@ -11,3 +11,26 @@ commerce_order_test.customer_profile_test_form:
         type: 'entity:profile'
   requirements:
     _access: 'TRUE'
+
+commerce_order_test.save_no_lock:
+  path: '/commerce/test/resave-no-lock/{commerce_order}'
+  defaults:
+    _controller: '\Drupal\commerce_order_test\Controller\CommerceOrderTestController::testSaveNoLock'
+    _title: 'Order save test'
+  requirements:
+    _access: 'TRUE'
+  options:
+    parameters:
+      profile:
+        type: 'entity:commerce_order'
+commerce_order_test.save_lock:
+  path: '/commerce/test/resave-lock/{commerce_order}'
+  defaults:
+    _controller: '\Drupal\commerce_order_test\Controller\CommerceOrderTestController::testSaveLock'
+    _title: 'Order save test'
+  requirements:
+    _access: 'TRUE'
+  options:
+    parameters:
+      profile:
+        type: 'entity:commerce_order'

modules/order/tests/modules/commerce_order_test/src/Controller/CommerceOrderTestController.php

+<?php
+
+namespace Drupal\commerce_order_test\Controller;
+
+use Drupal\commerce_order\Entity\OrderInterface;
+use Drupal\Core\Controller\ControllerBase;
+
+/**
+ * Test controller.
+ */
+class CommerceOrderTestController extends ControllerBase {
+
+  /**
+   * Attempts to save the commerce order without a lock.
+   *
+   * @param \Drupal\commerce_order\Entity\OrderInterface $commerce_order
+   *   The order entity.
+   */
+  public function testSaveNoLock(OrderInterface $commerce_order) {
+    try {
+      $commerce_order->setData('conflicting_update', 'successful');
+      $commerce_order->save();
+
+      return ['#markup' => $this->t('Saved the order successfully')];
+    }
+    catch (\Exception $e) {
+      return ['#markup' => $e->getMessage()];
+    }
+  }
+
+  /**
+   * Attempts to save the commerce order with a lock.
+   *
+   * @param \Drupal\commerce_order\Entity\OrderInterface $commerce_order
+   *   The order entity.
+   */
+  public function testSaveLock(OrderInterface $commerce_order) {
+    try {
+      $commerce_order = $this->entityTypeManager()->getStorage('commerce_order')->loadForUpdate($commerce_order->id());
+      $commerce_order->setData('second_update', 'successful');
+      $commerce_order->save();
+
+      return ['#markup' => $this->t('Saved the order successfully')];
+    }
+    catch (\Exception $e) {
+      return ['#markup' => $e->getMessage()];
+    }
+  }
+
+}

modules/order/tests/src/Functional/OrderTest.php

@@ -2,8 +2,11 @@
 
 namespace Drupal\Tests\commerce_order\Functional;
 
+use Behat\Mink\Driver\BrowserKitDriver;
 use Drupal\commerce_order\Entity\Order;
 use Drupal\commerce_order\Entity\OrderItem;
+use Drupal\Tests\DrupalTestBrowser;
+use GuzzleHttp\Exception\ConnectException;
 
 /**
  * Tests the commerce_order entity forms.
@@ -61,4 +64,66 @@ class OrderTest extends OrderBrowserTestBase {
     $this->assertEmpty($order_item_exists, 'The matching order item has been deleted from the database.');
   }
 
+  /**
+   * Tests load for update locking.
+   */
+  public function testLoadForUpdate() {
+    $order_item = $this->createEntity('commerce_order_item', [
+      'type' => 'default',
+      'unit_price' => [
+        'number' => '999',
+        'currency_code' => 'USD',
+      ],
+    ]);
+    $order = $this->createEntity('commerce_order', [
+      'type' => 'default',
+      'mail' => $this->loggedInUser->getEmail(),
+      'order_items' => [$order_item],
+      'uid' => $this->loggedInUser,
+      'store_id' => $this->store,
+    ]);
+
+    /** @var \Drupal\commerce_order\OrderStorageInterface $storage */
+    $storage = \Drupal::entityTypeManager()->getStorage('commerce_order');
+    $order = $storage->loadForUpdate($order->id());
+
+    $this->drupalGet('commerce/test/resave-no-lock/' . $order->id());
+    // Request a page that is also attempting to update the order.
+    $this->assertSession()->pageTextContains('Attempted to save order ' . $order->id() . ' that is locked for updating');
+
+    // Set a new client with a short timeout as the following request will
+    // wait for the lock.
+    $driver = $this->getSession()->getDriver();
+    if ($driver instanceof BrowserKitDriver) {
+      $client = $driver->getClient();
+      if ($client instanceof DrupalTestBrowser) {
+        $guzzle_client = $this->container->get('http_client_factory')->fromOptions([
+          'timeout' => 1,
+          'verify' => FALSE,
+        ]);
+        $client->setClient($guzzle_client);
+      }
+    }
+
+    try {
+      $this->drupalGet('commerce/test/resave-lock/' . $order->id());
+      $this->fail('Request is expected to wait for the lock and time out.');
+    }
+    catch (ConnectException $e) {
+
+    }
+
+    $order->setData('first_update', 'successful');
+    $order->save();
+
+    // Sleep for a second, afterwards the lock is expected to be freed and the
+    // test controller can make its change.
+    sleep(1);
+
+    $order = $storage->loadUnchanged($order->id());
+    $this->assertEquals('successful', $order->getData('first_update'));
+    $this->assertEquals('successful', $order->getData('second_update'));
+    $this->assertNotEquals('successful', $order->getData('conflicting_update'));
+  }
+
 }

modules/payment/src/Controller/PaymentCheckoutController.php

@@ -9,6 +9,7 @@ use Drupal\commerce_payment\Exception\PaymentGatewayException;
 use Drupal\commerce_payment\Plugin\Commerce\PaymentGateway\OffsitePaymentGatewayInterface;
 use Drupal\Core\Access\AccessException;
 use Drupal\Core\DependencyInjection\ContainerInjectionInterface;
+use Drupal\Core\Entity\EntityTypeManagerInterface;
 use Drupal\Core\Messenger\MessengerInterface;
 use Drupal\Core\Routing\RouteMatchInterface;
 use Drupal\Core\Url;
@@ -42,6 +43,13 @@ class PaymentCheckoutController implements ContainerInjectionInterface {
    */
   protected $logger;
 
+  /**
+   * The entity type manager.
+   *
+   * @var \Drupal\Core\Entity\EntityTypeManagerInterface
+   */
+  protected EntityTypeManagerInterface $entityTypeManager;
+
   /**
    * Constructs a new PaymentCheckoutController object.
    *
@@ -51,11 +59,14 @@ class PaymentCheckoutController implements ContainerInjectionInterface {
    *   The messenger.
    * @param \Psr\Log\LoggerInterface $logger
    *   The logger.
+   * @param \Drupal\Core\Entity\EntityTypeManagerInterface $entity_type_manager
+   *   The entity type manager.
    */
-  public function __construct(CheckoutOrderManagerInterface $checkout_order_manager, MessengerInterface $messenger, LoggerInterface $logger) {
+  public function __construct(CheckoutOrderManagerInterface $checkout_order_manager, MessengerInterface $messenger, LoggerInterface $logger, EntityTypeManagerInterface $entity_type_manager) {
     $this->checkoutOrderManager = $checkout_order_manager;
     $this->messenger = $messenger;
     $this->logger = $logger;
+    $this->entityTypeManager = $entity_type_manager;
   }
 
   /**
@@ -65,7 +76,8 @@ class PaymentCheckoutController implements ContainerInjectionInterface {
     return new static(
       $container->get('commerce_checkout.checkout_order_manager'),
       $container->get('messenger'),
-      $container->get('logger.channel.commerce_payment')
+      $container->get('logger.channel.commerce_payment'),
+      $container->get('entity_type.manager')
     );
   }
 
@@ -84,6 +96,18 @@ class PaymentCheckoutController implements ContainerInjectionInterface {
     $order = $route_match->getParameter('commerce_order');
     $step_id = $route_match->getParameter('step');
     $this->validateStepId($step_id, $order);
+
+    // Reload the order and mark it for updating, redirecting to step below
+    // will save it and free the lock. This must be done before the checkout
+    // flow plugin is initiated to make sure that it has the reloaded order
+    // object. Additionally, the checkout flow plugin gets the order from
+    // the route match object, so update the order there as well with. The
+    // passed in route match object is created on-demand in
+    // \Drupal\Core\Controller\ArgumentResolver\RouteMatchValueResolver and is
+    // not the same object as the current route match service.
+    $order = $this->entityTypeManager->getStorage('commerce_order')->loadForUpdate($order->id());
+    \Drupal::routeMatch()->getParameters()->set('commerce_order', $order);
+
     /** @var \Drupal\commerce_payment\Entity\PaymentGatewayInterface $payment_gateway */
     $payment_gateway = $order->get('payment_gateway')->entity;
     $payment_gateway_plugin = $payment_gateway->getPlugin();

modules/payment/src/PaymentOrderUpdater.php

@@ -52,10 +52,10 @@ class PaymentOrderUpdater implements PaymentOrderUpdaterInterface, DestructableI
    */
   public function updateOrders() {
     if (!empty($this->updateList)) {
+      /** @var \Drupal\commerce_order\OrderStorage $order_storage */
       $order_storage = $this->entityTypeManager->getStorage('commerce_order');
-      /** @var \Drupal\commerce_order\Entity\OrderInterface[] $orders */
-      $orders = $order_storage->loadMultiple($this->updateList);
-      foreach ($orders as $order) {
+      foreach ($this->updateList as $order_id) {
+        $order = $order_storage->loadForUpdate($order_id);
         $this->updateOrder($order, TRUE);
       }
     }