diff --git a/includes/commerce.controller.inc b/includes/commerce.controller.inc index 1cdd605..cb2ce20 100644 --- a/includes/commerce.controller.inc +++ b/includes/commerce.controller.inc @@ -7,7 +7,56 @@ * A full fork of Entity API's controller, with support for revisions. */ -class DrupalCommerceEntityController extends DrupalDefaultEntityController implements EntityAPIControllerInterface { +/** + * Interface for the default Drupal Commerce entity controller. + */ +interface DrupalCommerceEntityControllerInterface extends EntityAPIControllerInterface { + + /** + * Determines whether the provided entity is locked. + * + * @param object $entity + * The entity to check. + * + * @return bool + * True if the entity is locked, false otherwise. + */ + public function isLocked($entity); + + /** + * Determines whether the provided entity is cached. + * + * @param object $entity + * The entity to check. + * + * @return bool + * True if the entity is cached, false otherwise. + */ + public function isCached($entity); + + /** + * Request that locking be skipped. + * + * Actual skipping of locking may or may not be possible. + * + * @param bool $skip_locking + * The boolean indicating whether locking should be skipped. + */ + public function requestSkipLocking($skip_locking); + + /** + * Releases locking on all entities, use with caution. + */ + public function releaseLocking(); + +} + +/** + * Default implementation of DrupalCommerceEntityControllerInterface. + * + * Provides base entity controller for Drupal Commerce entities. + */ +class DrupalCommerceEntityController extends DrupalDefaultEntityController implements DrupalCommerceEntityControllerInterface { /** * Stores our transaction object, necessary for pessimistic locking to work. @@ -15,12 +64,111 @@ class DrupalCommerceEntityController extends DrupalDefaultEntityController imple protected $controllerTransaction = NULL; /** - * Stores the ids of locked entities, necessary for knowing when to release a - * lock by committing the transaction. + * Stores the ids of locked entities. + * + * Necessary for knowing when to release lock by committing the transaction. */ protected $lockedEntities = array(); /** + * Stores the flag for if a condition has been passed for requesting locking. + * + * By default, locking is always requested unless specifically set to false. + */ + protected $requestLocking = TRUE; + + /** + * Stores whether a request for skipping locking has been set. + * + * If locking has been requested as well it will take preference and the + * entity load will default to locking. + */ + protected $requestSkipLocking = FALSE; + + /** + * Implements DrupalCommerceEntityControllerInterface::isLocked(). + */ + public function isLocked($entity) { + return isset($this->lockedEntities[$entity->{$this->idKey}]); + } + + /** + * Implements DrupalCommerceEntityControllerInterface::isCached(). + */ + public function isCached($entity) { + return isset($this->entityCache[$entity->{$this->idKey}]); + } + + /** + * Implements DrupalCommerceEntityControllerInterface::requestSkipLocking(). + */ + public function requestSkipLocking($skip_locking = TRUE) { + $this->requestSkipLocking = $skip_locking; + + return $this; + } + + /** + * Determines whether the current entity type requires locking. + * + * @return bool + * True if locking is required, false otherwise. + */ + protected function requireLocking() { + $enabled = isset($this->entityInfo['locking mode']) && $this->entityInfo['locking mode'] == 'pessimistic'; + $not_skipped = empty($this->requestSkipLocking); + + return $enabled && $not_skipped && $this->requestLocking; + } + + /** + * Checks the list of tracked locked entities, and if it's empty, commits + * the transaction in order to remove the acquired locks. + * + * The transaction is not necessarily committed immediately. Drupal will + * commit it as soon as possible given the state of the transaction stack. + */ + protected function releaseLock() { + if (empty($this->lockedEntities)) { + unset($this->controllerTransaction); + } + } + + /** + * Removes all locking for the controller, mostly used for testing + * if you want to stop locking and run something else, or you want to cancel locking + * on all entities at once, although be cautious with this. + */ + public function releaseLocking() { + unset($this->controllerTransaction); + $this->lockedEntities = array(); + } + + /** + * Overrides DrupalDefaultEntityController::load(). + * + * Accepts a condition of locking request, may not necessarily take effect + * as other options can override locking. If anything conflicts, locking always + * take precedence over not locking. + */ + public function load($ids = array(), $conditions = array()) { + // Lock by default if the caller didn't indicate a preference. + $conditions += array('_lock' => TRUE); + $this->requestLocking = $conditions["_lock"]; + unset($conditions['_lock']); + + // If locking has been required, then bypass the internal cache for any + // entities that are not already locked. + if ($this->requireLocking()) { + foreach (array_diff_key(array_flip($ids), $this->lockedEntities) as $id => $value) { + unset($this->entityCache[$id]); + } + } + + return parent::load($ids, $conditions); + } + + /** * Override of DrupalDefaultEntityController::buildQuery(). * * Handle pessimistic locking. @@ -28,7 +176,7 @@ class DrupalCommerceEntityController extends DrupalDefaultEntityController imple protected function buildQuery($ids, $conditions = array(), $revision_id = FALSE) { $query = parent::buildQuery($ids, $conditions, $revision_id); - if (isset($this->entityInfo['locking mode']) && $this->entityInfo['locking mode'] == 'pessimistic') { + if ($this->requireLocking()) { // In pessimistic locking mode, we issue the load query with a FOR UPDATE // clause. This will block all other load queries to the loaded objects // but requires us to start a transaction. @@ -48,41 +196,6 @@ class DrupalCommerceEntityController extends DrupalDefaultEntityController imple return $query; } - public function resetCache(array $ids = NULL) { - parent::resetCache($ids); - - // Maintain the list of locked entities, so that the releaseLock() method - // can know when it's time to commit the transaction. - if (!empty($this->lockedEntities)) { - if (isset($ids)) { - foreach ($ids as $id) { - unset($this->lockedEntities[$id]); - } - } - else { - $this->lockedEntities = array(); - } - } - - // Try to release the lock, if possible. - $this->releaseLock(); - } - - /** - * Checks the list of tracked locked entities, and if it's empty, commits - * the transaction in order to remove the acquired locks. - * - * The transaction is not necessarily committed immediately. Drupal will - * commit it as soon as possible given the state of the transaction stack. - */ - protected function releaseLock() { - if (isset($this->entityInfo['locking mode']) && $this->entityInfo['locking mode'] == 'pessimistic') { - if (empty($this->lockedEntities)) { - unset($this->controllerTransaction); - } - } - } - /** * (Internal use) Invokes a hook on behalf of the entity. * @@ -141,6 +254,12 @@ class DrupalCommerceEntityController extends DrupalDefaultEntityController imple // Reset the cache as soon as the changes have been applied. $this->resetCache($ids); + // Maintain the list of locked entities and release the lock if possible. + foreach ($ids as $id) { + unset($this->lockedEntities[$id]); + } + $this->releaseLock(); + foreach ($entities as $id => $entity) { $this->invoke('delete', $entity); } diff --git a/modules/cart/tests/commerce_cart.test b/modules/cart/tests/commerce_cart.test index da87246..cc3572e 100644 --- a/modules/cart/tests/commerce_cart.test +++ b/modules/cart/tests/commerce_cart.test @@ -509,10 +509,8 @@ class CommerceCartTestCaseAnonymousToAuthenticated extends CommerceCartTestCase $this->drupalPost('node/' . $this->product_node->nid, array(), t('Add to cart')); // Get the order just created. - $orders = commerce_order_load_multiple(array(), array('uid' => $user->uid, 'status' => 'cart'), TRUE); + $orders = commerce_order_load_multiple(array(), array('uid' => $user->uid, 'status' => 'cart', '_lock' => FALSE), TRUE); $order_anonymous = reset($orders); - // Reset the cache as we don't want to keep the lock. - entity_get_controller('commerce_order')->resetCache(); // Access to the cart and check if the product is in it. $this->drupalGet($this->getCommerceUrl('cart')); @@ -530,10 +528,9 @@ class CommerceCartTestCaseAnonymousToAuthenticated extends CommerceCartTestCase $this->drupalPost('user', array('name' => $this->store_customer->name, 'pass' => $this->store_customer->pass_raw), t('Log in')); // Get the order for user just logged in. - $orders = commerce_order_load_multiple(array(), array('uid' => $this->store_customer->uid, 'status' => 'cart'), TRUE); + $orders = commerce_order_load_multiple(array(), array('uid' => $this->store_customer->uid, 'status' => 'cart', '_lock' => FALSE), TRUE); $order_authenticated = reset($orders); // Reset the cache as we don't want to keep the lock. - entity_get_controller('commerce_order')->resetCache(); // Access to the cart and check if the product is in it. $this->drupalGet($this->getCommerceUrl('cart')); diff --git a/modules/checkout/tests/commerce_checkout.test b/modules/checkout/tests/commerce_checkout.test index 56351cc..7fc3797 100644 --- a/modules/checkout/tests/commerce_checkout.test +++ b/modules/checkout/tests/commerce_checkout.test @@ -76,10 +76,8 @@ class CommerceCheckoutTestProcess extends CommerceBaseTestCase { $this->drupalPost('node/' . $this->product_node->nid, array(), t('Add to cart')); // Get the order for the anonymous user. - $orders = commerce_order_load_multiple(array(), array('uid' => $user->uid, 'status' => 'cart'), TRUE); + $orders = commerce_order_load_multiple(array(), array('uid' => $user->uid, 'status' => 'cart', '_lock' => FALSE), TRUE); $this->order = reset($orders); - // Reset the cache as we don't want to keep the lock. - entity_get_controller('commerce_order')->resetCache(); } /** @@ -162,9 +160,7 @@ class CommerceCheckoutTestProcess extends CommerceBaseTestCase { $this->assertText('Example payment', t('Example payment method pane is present')); // Load the order to check the status. - $order = commerce_order_load_multiple(array($this->order->order_id), array(), TRUE); - // Reset the cache as we don't want to keep the lock. - entity_get_controller('commerce_order')->resetCache(); + $order = commerce_order_load_multiple(array($this->order->order_id), array('_lock' => FALSE), TRUE); // At this point we should be in Checkout Review. $this->assertEqual(reset($order)->status, 'checkout_review', t('Order status is \'Checkout Review\' in the review phase.')); @@ -324,9 +320,7 @@ class CommerceCheckoutTestProcess extends CommerceBaseTestCase { $this->assertText($user_mail, t('Account information is correct')); // Load the order to check the status. - $order = commerce_order_load_multiple(array($this->order->order_id), array(), TRUE); - // Reset the cache as we don't want to keep the lock. - entity_get_controller('commerce_order')->resetCache(); + $order = commerce_order_load_multiple(array($this->order->order_id), array('_lock' => FALSE), TRUE); // At this point we should be in Checkout Review. $this->assertEqual(reset($order)->status, 'checkout_review', t('Order status is \'Checkout Review\' in the review phase.')); diff --git a/modules/order/commerce_order.module b/modules/order/commerce_order.module index 786e013..5bff771 100644 --- a/modules/order/commerce_order.module +++ b/modules/order/commerce_order.module @@ -746,17 +746,27 @@ function commerce_order_save($order) { /** * Loads an order by ID. + * + * @param $order_id + * The order id. + * @param $lock + * Whether to lock the loaded order. */ -function commerce_order_load($order_id) { - $orders = commerce_order_load_multiple(array($order_id), array()); +function commerce_order_load($order_id, $lock = TRUE) { + $orders = commerce_order_load_multiple(array($order_id), array('_lock' => $lock), FALSE); return $orders ? reset($orders) : FALSE; } /** * Loads an order by number. + * + * @param $order_number + * The order number. + * @param $lock + * Whether to lock the loaded order. */ -function commerce_order_load_by_number($order_number) { - $orders = commerce_order_load_multiple(array(), array('order_number' => $order_number)); +function commerce_order_load_by_number($order_number, $lock = TRUE) { + $orders = commerce_order_load_multiple(array(), array('order_number' => $order_number, '_lock' => $lock), FALSE); return $orders ? reset($orders) : FALSE; } @@ -782,6 +792,32 @@ function commerce_order_load_multiple($order_ids = array(), $conditions = array( return entity_load('commerce_order', $order_ids, $conditions, $reset); } + /** + * Determines whether or not the given order object is locked. + * + * @param $order + * A fully loaded order object. + * + * @return + * Boolean indicating whether or not the order object is locked. + */ +function commerce_order_is_locked($order) { + return entity_get_controller('commerce_order')->isLocked($order); +} + +/** + * Determines whether or not the given order object is cached. + * + * @param $order + * A fully loaded order object. + * + * @return + * Boolean indicating whether or not the order object is cached. + */ +function commerce_order_is_cached($order) { + return entity_get_controller('commerce_order')->isCached($order); +} + /** * Determines whether or not the given order object represents the latest * revision of the order. diff --git a/modules/order/commerce_order_ui.module b/modules/order/commerce_order_ui.module index 8dc2333..e155725 100644 --- a/modules/order/commerce_order_ui.module +++ b/modules/order/commerce_order_ui.module @@ -35,6 +35,7 @@ function commerce_order_ui_menu() { ); $items['admin/commerce/orders/%commerce_order'] = array( + 'load arguments' => array(FALSE), 'title callback' => 'commerce_order_ui_order_title', 'title arguments' => array(3), 'page callback' => 'commerce_order_ui_order_view', @@ -86,6 +87,7 @@ function commerce_order_ui_menu() { ); $items['user/%user/orders/%commerce_order'] = array( + 'load arguments' => array(FALSE), 'title callback' => 'commerce_order_ui_order_title', 'title arguments' => array(3), 'page callback' => 'commerce_order_ui_order_view', diff --git a/modules/order/tests/commerce_order.test b/modules/order/tests/commerce_order.test index 1164634..cb43f69 100644 --- a/modules/order/tests/commerce_order.test +++ b/modules/order/tests/commerce_order.test @@ -121,3 +121,86 @@ class CommerceOrderCRUDTestCase extends CommerceBaseTestCase { $this->assertEqual(token_replace('[commerce-order:changed]', array('commerce-order' => $order)), format_date($order->changed, 'medium'), '[commerce-order:changed] was replaced with the changed date.'); } } + +/** + * Test order locking. + */ +class CommerceOrderLockingTestCase extends CommerceBaseTestCase { + + public static function getInfo() { + return array( + 'name' => 'Order locking', + 'description' => 'Test the order locking.', + 'group' => 'Drupal Commerce', + ); + } + + protected function setUp() { + $modules = parent::setUpHelper('api'); + $modules[] = 'commerce_cart'; + parent::setUp($modules); + } + + /** + * Test releasing order locks. + */ + public function testCommerceOrderReleaseLocking() { + /** @var CommerceOrderEntityController $order_controller */ + $order_controller = entity_get_controller('commerce_order'); + $created_order = $this->createDummyOrder(); + + // The order is locked when loaded from commerce_cart_order_load(), however + // it should not be locked because it was then saved. + $this->assertFalse(commerce_order_is_locked($created_order), 'Generated and saved order is not locked.'); + + // Ensure that loading locked and unlocked orders works. + $unlocked_order = commerce_order_load($created_order->order_id, FALSE); + $this->assertFalse(commerce_order_is_locked($unlocked_order), 'commerce_order_load() returned an unlocked order.'); + + $locked_order = commerce_order_load($created_order->order_id); + $this->assertTrue(commerce_order_is_locked($locked_order), 'commerce_order_load() returned a locked order.'); + + $order_controller->releaseLocking(); + $this->assertFalse(commerce_order_is_locked($locked_order), 'Order is not locked once all locks released.'); + } + + /** + * Test skip locking. + */ + public function testCommerceOrderSkipLocking() { + /** @var CommerceOrderEntityController $order_controller */ + $order_controller = entity_get_controller('commerce_order'); + $created_order = $this->createDummyOrder(); + + // Ensure that skipLocking() works. + $order_controller->requestSkipLocking(); + $unlocked_order = commerce_order_load($created_order->order_id); + $this->assertFalse(commerce_order_is_locked($unlocked_order), 'commerce_order_load() returned an unlocked order.'); + + // Turn off skipLocking(). + $order_controller->requestSkipLocking(FALSE); + // Our previous loaded order is not considered locked. + $this->assertFalse(commerce_order_is_locked($unlocked_order), 'commerce_order_load() returned a locked order.'); + // Re-loading the order will lock it. + $unlocked_order = commerce_order_load($created_order->order_id); + $this->assertTrue(commerce_order_is_locked($unlocked_order), 'commerce_order_load() returned a locked order.'); + } + + /** + * Test controller entity caching. + */ + public function testCommerceOrderControllerCaching() { + /** @var CommerceOrderEntityController $order_controller */ + $order_controller = entity_get_controller('commerce_order'); + $created_order = $this->createDummyOrder(); + + $this->assertTrue(commerce_order_is_cached($created_order)); + $created_order->data['test_cached_data_attribute'] = TRUE; + + $unlocked_cached_order = commerce_order_load($created_order->order_id, FALSE); + $this->assertTrue($unlocked_cached_order->data['test_cached_data_attribute'], 'Requesting read-only order loaded from controller entity cache has same data attribute.'); + + $locked_order = commerce_order_load($created_order->order_id); + $this->assertFalse(isset($locked_order->data['test_cached_data_attribute']), 'Loading a locked order reset controller entity cache for specific item.'); + } +} diff --git a/modules/order/tests/commerce_order_ui.test b/modules/order/tests/commerce_order_ui.test index 9a023f7..d5892d6 100644 --- a/modules/order/tests/commerce_order_ui.test +++ b/modules/order/tests/commerce_order_ui.test @@ -60,12 +60,9 @@ class CommerceOrderUIAdminTest extends CommerceBaseTestCase { $this->drupalPost(NULL, array('name' => $this->store_customer->name), t('Save order', array(), array('context' => 'a drupal commerce order'))); // Load the order from database for later use. - $orders = commerce_order_load_multiple(array(), array('uid' => $this->store_customer->uid)); + $orders = commerce_order_load_multiple(array(), array('uid' => $this->store_customer->uid, '_lock' => FALSE)); $this->order = reset($orders); - // Reset the cache as we don't want to keep the lock. - entity_get_controller('commerce_order')->resetCache(); - // Enable an additional currency. $this->enableCurrencies(array('EUR')); } @@ -150,10 +147,7 @@ class CommerceOrderUIAdminTest extends CommerceBaseTestCase { $this->drupalPost(NULL, array(), t('Save order', array(), array('context' => 'a drupal commerce order'))); // Reload the order directly from db. - $order = commerce_order_load_multiple(array($this->order->order_id), array(), TRUE); - - // Reset the cache as we don't want to keep the lock. - entity_get_controller('commerce_order')->resetCache(); + $order = commerce_order_load_multiple(array($this->order->order_id), array('_lock' => FALSE), TRUE); // Check if the product has been added to the order. foreach (entity_metadata_wrapper('commerce_order', reset($order))->commerce_line_items as $delta => $line_item_wrapper) { @@ -186,13 +180,10 @@ class CommerceOrderUIAdminTest extends CommerceBaseTestCase { $this->drupalPost(NULL, array(), t('Save order', array(), array('context' => 'a drupal commerce order'))); // Reload the order directly from db and wrap it to get the line item ids. - $orders = commerce_order_load_multiple(array($this->order->order_id), array(), TRUE); + $orders = commerce_order_load_multiple(array($this->order->order_id), array('_lock' => FALSE), TRUE); $order = reset($orders); $order_wrapper = entity_metadata_wrapper('commerce_order', $order); - // Reset the cache as we don't want to keep the lock. - entity_get_controller('commerce_order')->resetCache(); - // Also wrap the product to access easier to its price. $product_wrapper = entity_metadata_wrapper('commerce_product', $this->product); @@ -264,8 +255,6 @@ class CommerceOrderUIAdminTest extends CommerceBaseTestCase { // Check if the links for editing the order are present. $links = menu_contextual_links('commerce-order', 'admin/commerce/orders', array($this->order->order_id)); - // Reset the cache as we don't want to keep the lock. - entity_get_controller('commerce_order')->resetCache(); $this->assertRaw((theme('links', array('links' => $links, 'attributes' => array('class' => array('links', 'inline', 'operations'))))), t('Links for orders are present')); $this->drupalGet('admin/commerce/orders/'. $this->order->order_id . '/view'); diff --git a/modules/payment/tests/commerce_payment_ui.test b/modules/payment/tests/commerce_payment_ui.test index b59e657..f62134c 100644 --- a/modules/payment/tests/commerce_payment_ui.test +++ b/modules/payment/tests/commerce_payment_ui.test @@ -143,9 +143,7 @@ class CommercePaymentUITest extends CommerceBaseTestCase { $this->drupalPost(NULL, $post_data, t('Save')); // Reload the order. - $order = commerce_order_load_multiple(array($this->order->order_id), array(), TRUE); - // Reset the cache as we don't want to keep the lock. - entity_get_controller('commerce_order')->resetCache(); + $order = commerce_order_load_multiple(array($this->order->order_id), array('_lock' => FALSE), TRUE); // Check order balance, it should be half of total now. $new_balance = commerce_payment_order_balance(reset($order));