diff --git a/modules/order/src/Exception/OrderLockedSaveException.php b/modules/order/src/Exception/OrderLockedSaveException.php new file mode 100644 index 0000000000000000000000000000000000000000..767aa48e20a48824f9f343b525460f3e9ab9ffb0 --- /dev/null +++ b/modules/order/src/Exception/OrderLockedSaveException.php @@ -0,0 +1,8 @@ +<?php + +namespace Drupal\commerce_order\Exception; + +/** + * Thrown when attempting to save an order that is locked for updating. + */ +class OrderLockedSaveException extends \RuntimeException {} diff --git a/modules/order/src/OrderStorage.php b/modules/order/src/OrderStorage.php index 1ef9c80cf6dbde537153715d5c73c83972c1f324..533846862d4f7920868799cbf45843c07ec45491 100644 --- a/modules/order/src/OrderStorage.php +++ b/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; + } + } diff --git a/modules/order/src/OrderStorageInterface.php b/modules/order/src/OrderStorageInterface.php new file mode 100644 index 0000000000000000000000000000000000000000..34fabb7eebcdec2d54459f2a6e9c0a7429f10e78 --- /dev/null +++ b/modules/order/src/OrderStorageInterface.php @@ -0,0 +1,34 @@ +<?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; + +} diff --git a/modules/order/tests/modules/commerce_order_test/commerce_order_test.routing.yml b/modules/order/tests/modules/commerce_order_test/commerce_order_test.routing.yml index 3ae6b495c53cb3c8b0f15617dae5866404d66f72..43ce3202524efc615b0ce755deb3c87d4a62d24a 100644 --- a/modules/order/tests/modules/commerce_order_test/commerce_order_test.routing.yml +++ b/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' diff --git a/modules/order/tests/modules/commerce_order_test/src/Controller/CommerceOrderTestController.php b/modules/order/tests/modules/commerce_order_test/src/Controller/CommerceOrderTestController.php new file mode 100644 index 0000000000000000000000000000000000000000..c619ccd034c959af1fcd5cc196324db48f731fed --- /dev/null +++ b/modules/order/tests/modules/commerce_order_test/src/Controller/CommerceOrderTestController.php @@ -0,0 +1,50 @@ +<?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()]; + } + } + +} diff --git a/modules/order/tests/src/Functional/OrderTest.php b/modules/order/tests/src/Functional/OrderTest.php index 3ebdc69ba99e12fe3a9d5612a44c5d2f9054fc7b..e1a6f19d811a82df93b19f462cfd436fd145f5a2 100644 --- a/modules/order/tests/src/Functional/OrderTest.php +++ b/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')); + } + } diff --git a/modules/payment/src/Controller/PaymentCheckoutController.php b/modules/payment/src/Controller/PaymentCheckoutController.php index 84159e11e7e2d70f33a061b0754f2c4714df1a05..73a67153c11f819307acb1c49372e724d32d67f2 100644 --- a/modules/payment/src/Controller/PaymentCheckoutController.php +++ b/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(); diff --git a/modules/payment/src/PaymentOrderUpdater.php b/modules/payment/src/PaymentOrderUpdater.php index 2ad2135091600b8bfc1ba8af5180332960f9b646..678f35f26e0e66c37db9587503d374da78efb564 100644 --- a/modules/payment/src/PaymentOrderUpdater.php +++ b/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); } }