diff --git a/includes/commerce.controller.inc b/includes/commerce.controller.inc index ac8165b..cb2ce20 100644 --- a/includes/commerce.controller.inc +++ b/includes/commerce.controller.inc @@ -7,15 +7,18 @@ * A full fork of Entity API's controller, with support for revisions. */ +/** + * Interface for the default Drupal Commerce entity controller. + */ interface DrupalCommerceEntityControllerInterface extends EntityAPIControllerInterface { /** * Determines whether the provided entity is locked. * - * @param $entity + * @param object $entity * The entity to check. * - * @return + * @return bool * True if the entity is locked, false otherwise. */ public function isLocked($entity); @@ -23,30 +26,36 @@ interface DrupalCommerceEntityControllerInterface extends EntityAPIControllerInt /** * Determines whether the provided entity is cached. * - * @param $entity + * @param object $entity * The entity to check. * - * @return + * @return bool * True if the entity is cached, false otherwise. */ public function isCached($entity); /** - * Request that locking be skipped, - * actually skipping locking may or may not be possible. + * Request that locking be skipped. * - * @param $skip_locking + * 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 + * 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 { /** @@ -55,21 +64,24 @@ 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 it is specifically set to false + * + * 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 + * + * If locking has been requested as well it will take preference and the + * entity load will default to locking. */ protected $requestSkipLocking = FALSE; @@ -81,7 +93,7 @@ class DrupalCommerceEntityController extends DrupalDefaultEntityController imple } /** - * Implements DrupalCommerceEntityControllerInterface::isLocked(). + * Implements DrupalCommerceEntityControllerInterface::isCached(). */ public function isCached($entity) { return isset($this->entityCache[$entity->{$this->idKey}]); @@ -97,9 +109,9 @@ class DrupalCommerceEntityController extends DrupalDefaultEntityController imple } /** - * Determines whether the current entity type requires or requested pessimistic locking. + * Determines whether the current entity type requires locking. * - * @return + * @return bool * True if locking is required, false otherwise. */ protected function requireLocking() { @@ -242,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); } @@ -329,9 +347,9 @@ class DrupalCommerceEntityController extends DrupalDefaultEntityController imple if (!empty($update_base_table)) { // Go back to the base table and update the pointer to the revision ID. db_update($this->entityInfo['base table']) - ->fields(array($this->revisionKey => $entity->{$this->revisionKey})) - ->condition($this->idKey, $entity->{$this->idKey}) - ->execute(); + ->fields(array($this->revisionKey => $entity->{$this->revisionKey})) + ->condition($this->idKey, $entity->{$this->idKey}) + ->execute(); } // Update the static cache so that the next entity_load() will return this diff --git a/modules/cart/tests/commerce_cart.test b/modules/cart/tests/commerce_cart.test index 0a63177..cc3572e 100644 --- a/modules/cart/tests/commerce_cart.test +++ b/modules/cart/tests/commerce_cart.test @@ -509,9 +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); - entity_get_controller('commerce_order')->releaseLocking(); // Access to the cart and check if the product is in it. $this->drupalGet($this->getCommerceUrl('cart')); @@ -529,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')->releaseLocking(); // 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 fa81eb1..7fc3797 100644 --- a/modules/checkout/tests/commerce_checkout.test +++ b/modules/checkout/tests/commerce_checkout.test @@ -76,9 +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); - entity_get_controller('commerce_order')->releaseLocking(); } /** @@ -161,8 +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); - entity_get_controller('commerce_order')->releaseLocking(); + $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.')); @@ -322,8 +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); - entity_get_controller('commerce_order')->releaseLocking(); + $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 5f76e4a..5bff771 100644 --- a/modules/order/commerce_order.module +++ b/modules/order/commerce_order.module @@ -1508,34 +1508,6 @@ function commerce_order_entity_query_alter($query) { } /** - * Implements hook_views_pre_execute(). - * - * Ensures that order views don't lock the loaded orders if not necessary. - */ -function commerce_order_views_pre_execute(&$view) { - if ($view->base_table != 'commerce_order') { - return; - } - - $previous_result = &drupal_static(__FUNCTION__); - if ($previous_result === FALSE) { - // The previous order view needed locking, which means that locking can't - // be skipped in this request. No need to evaluate other views. - return; - } - - // Locking can't be skipped if the view has form elements, except VBO. - // VBO modifies orders in a multi-step process, so locking is only needed - // after that process is initiated by the user submitting the VBO selection. - $has_form_elements = views_view_has_form_elements($view); - $has_vbo_field = isset($view->field['views_bulk_operations']); - $submitted = isset($_POST['operation']); - $skip_locking = (!$has_form_elements || ($has_vbo_field && !$submitted)); - entity_get_controller('commerce_order')->requestSkipLocking($skip_locking); - $previous_result = $skip_locking; -} - -/** * Implements hook_preprocess_views_view(). * * When the line item summary and order total area handlers are present on Views diff --git a/modules/order/commerce_order_ui.module b/modules/order/commerce_order_ui.module index be40d8a..8dc2333 100644 --- a/modules/order/commerce_order_ui.module +++ b/modules/order/commerce_order_ui.module @@ -35,8 +35,6 @@ function commerce_order_ui_menu() { ); $items['admin/commerce/orders/%commerce_order'] = array( - // Load the order unlocked, the view page won't modify it. - 'load arguments' => array(FALSE), 'title callback' => 'commerce_order_ui_order_title', 'title arguments' => array(3), 'page callback' => 'commerce_order_ui_order_view', @@ -45,14 +43,12 @@ function commerce_order_ui_menu() { 'access arguments' => array(3), ); $items['admin/commerce/orders/%commerce_order/view'] = array( - 'load arguments' => array(FALSE), 'title' => 'View', 'type' => MENU_DEFAULT_LOCAL_TASK, 'weight' => -10, 'context' => MENU_CONTEXT_PAGE | MENU_CONTEXT_INLINE, ); $items['admin/commerce/orders/%commerce_order/edit'] = array( - 'load arguments' => array(TRUE), 'title' => 'Edit', 'page callback' => 'commerce_order_ui_order_form_wrapper', 'page arguments' => array(3), @@ -64,7 +60,6 @@ function commerce_order_ui_menu() { 'file' => 'includes/commerce_order_ui.orders.inc', ); $items['admin/commerce/orders/%commerce_order/delete'] = array( - 'load arguments' => array(TRUE), 'title' => 'Delete', 'page callback' => 'commerce_order_ui_order_delete_form_wrapper', 'page arguments' => array(3), @@ -91,8 +86,6 @@ function commerce_order_ui_menu() { ); $items['user/%user/orders/%commerce_order'] = array( - // Load the order unlocked, since it won't be modified. - '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 5d46587..cb43f69 100644 --- a/modules/order/tests/commerce_order.test +++ b/modules/order/tests/commerce_order.test @@ -97,54 +97,6 @@ class CommerceOrderCRUDTestCase extends CommerceBaseTestCase { } /** - * Test order locking. - */ - function testCommerceOrderLocking() { - $created_order = $this->createDummyOrder(); - entity_get_controller('commerce_order')->releaseLocking(); - entity_get_controller('commerce_order')->resetCache(); - - // 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.'); - $this->assertTrue(commerce_order_is_cached($unlocked_order), 'commerce_order_load() cached the 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.'); - $this->assertTrue(commerce_order_is_cached($locked_order), 'commerce_order_load() cached the order.'); - entity_get_controller('commerce_order')->releaseLocking(); - $this->assertTrue(commerce_order_is_cached($locked_order), 'commerce_order_load() cached the order.'); - - // Ensure that skipLocking() works. - entity_get_controller('commerce_order')->releaseLocking(); - entity_get_controller('commerce_order')->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.'); - - // Simulate an order view that has a non-VBO form field. - $fake_view = new stdClass(); - $fake_view->base_table = 'commerce_order'; - $fake_view->field = array(); - $fake_view->field['test'] = new StdClass(); - $fake_view->field['test']->views_form_callback = 'test_callback'; - commerce_order_views_pre_execute($fake_view); - $result = drupal_static('commerce_order_views_pre_execute', NULL); - $this->assertFalse($result, 'commerce_order_views_pre_execute() did not skip locking.'); - - // Simulate a VBO order view. Since the previous view required locking, - // locking should still not be skipped. - $fake_view->field['views_bulk_operations'] = $fake_view->field['test']; - commerce_order_views_pre_execute($fake_view); - $result = drupal_static('commerce_order_views_pre_execute', NULL); - $this->assertFalse($result, 'commerce_order_views_pre_execute() did not skip locking.'); - - // Start from scratch, test just the VBO order view. - drupal_static_reset('commerce_order_views_pre_execute'); - commerce_order_views_pre_execute($fake_view); - $result = drupal_static('commerce_order_views_pre_execute', NULL); - $this->assertTrue($result, 'commerce_order_views_pre_execute() skipped locking.'); - } - - /** * Test order Token replacement. */ function testCommerceOrderTokens() { @@ -169,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 6ac1783..d5892d6 100644 --- a/modules/order/tests/commerce_order_ui.test +++ b/modules/order/tests/commerce_order_ui.test @@ -60,11 +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); - entity_get_controller('commerce_order')->releaseLocking(); - // Enable an additional currency. $this->enableCurrencies(array('EUR')); } @@ -149,9 +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); - - entity_get_controller('commerce_order')->releaseLocking(); + $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) { @@ -184,12 +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); - entity_get_controller('commerce_order')->releaseLocking(); - // Also wrap the product to access easier to its price. $product_wrapper = entity_metadata_wrapper('commerce_product', $this->product); @@ -261,7 +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)); - entity_get_controller('commerce_order')->releaseLocking(); $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/commerce_payment_ui.module b/modules/payment/commerce_payment_ui.module index db45ef8..bd728ae 100644 --- a/modules/payment/commerce_payment_ui.module +++ b/modules/payment/commerce_payment_ui.module @@ -13,7 +13,6 @@ function commerce_payment_ui_menu() { // Payment tab on orders. $items['admin/commerce/orders/%commerce_order/payment'] = array( - 'load arguments' => array(TRUE), 'title' => 'Payment', 'page callback' => 'commerce_payment_ui_order_tab', 'page arguments' => array(3), diff --git a/modules/payment/tests/commerce_payment_ui.test b/modules/payment/tests/commerce_payment_ui.test index 7fd6448..f62134c 100644 --- a/modules/payment/tests/commerce_payment_ui.test +++ b/modules/payment/tests/commerce_payment_ui.test @@ -143,8 +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); - entity_get_controller('commerce_order')->releaseLocking(); + $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)); diff --git a/tests/commerce_base.test b/tests/commerce_base.test index 28b7a8e..43585f6 100644 --- a/tests/commerce_base.test +++ b/tests/commerce_base.test @@ -11,6 +11,8 @@ */ abstract class CommerceBaseTestCase extends DrupalWebTestCase { + protected $profile = 'minimal'; + /** * Helper function to determine which modules should be enabled. Should be * used in place of standard parent::setUp('moduleA', 'moduleB') call.