core/lib/Drupal/Core/Access/AccessInterface.php | 38 -------- core/lib/Drupal/Core/Access/AccessManager.php | 5 +- .../Drupal/Core/Access/AccessManagerInterface.php | 13 ++- core/lib/Drupal/Core/Access/AccessResult.php | 86 +++++++++++------- .../Drupal/Core/Access/AccessResultInterface.php | 22 ++--- .../lib/Drupal/Core/Access/AccessibleInterface.php | 2 +- core/lib/Drupal/Core/Cache/CacheableInterface.php | 8 +- .../Core/Entity/EntityAccessControlHandler.php | 4 +- core/lib/Drupal/Core/Menu/MenuLinkDefault.php | 1 + .../Drupal/Core/Routing/Access/AccessInterface.php | 4 +- core/modules/block/block.api.php | 15 +--- core/modules/node/node.api.php | 16 ++-- core/modules/node/node.module | 13 +-- .../node/src/NodeGrantDatabaseStorageInterface.php | 8 +- .../src/Access/EditEntityFieldAccessCheckTest.php | 10 +-- core/modules/system/entity.api.php | 12 +-- .../toolbar/src/Controller/ToolbarController.php | 5 +- .../Drupal/Tests/Core/Access/AccessManagerTest.php | 100 ++++++++------------- .../Drupal/Tests/Core/Access/AccessResultTest.php | 26 ++++-- .../Tests/Core/Access/CustomAccessCheckTest.php | 18 ++-- 20 files changed, 176 insertions(+), 230 deletions(-) diff --git a/core/lib/Drupal/Core/Access/AccessInterface.php b/core/lib/Drupal/Core/Access/AccessInterface.php deleted file mode 100644 index faa798c..0000000 --- a/core/lib/Drupal/Core/Access/AccessInterface.php +++ /dev/null @@ -1,38 +0,0 @@ -andIf($other); + $result->andIf($other); } return $result; } @@ -328,7 +329,7 @@ protected function performCheck($service_id, $route, $request, $account) { $service_access = call_user_func_array($callable, $arguments); if (!$service_access instanceof AccessResultInterface) { - throw new AccessException("Access error in $service_id. Access services can only return AccessResultInterface objects."); + throw new AccessException("Access error in $service_id. Access services must return an object that implements AccessResultInterface."); } return $service_access; diff --git a/core/lib/Drupal/Core/Access/AccessManagerInterface.php b/core/lib/Drupal/Core/Access/AccessManagerInterface.php index d9dc0ac..b90acf8 100644 --- a/core/lib/Drupal/Core/Access/AccessManagerInterface.php +++ b/core/lib/Drupal/Core/Access/AccessManagerInterface.php @@ -18,20 +18,17 @@ interface AccessManagerInterface { /** - * All access checkers have to return AccessInterface::ALLOW. + * All access checkers must return an AccessResultInterface object where + * ::isAllowed() is TRUE. * * self::ACCESS_MODE_ALL is the default behavior. - * - * @see \Drupal\Core\Access\AccessInterface::ALLOW */ const ACCESS_MODE_ALL = 'ALL'; /** - * At least one access checker has to return AccessInterface::ALLOW - * and none should return AccessInterface::KILL. - * - * @see \Drupal\Core\Access\AccessInterface::ALLOW - * @see \Drupal\Core\Access\AccessInterface::KILL + * At least one access checker must return an AccessResultInterface object + * where ::isAllowed() is TRUE and none may return one where ::isForbidden() + * is TRUE. */ const ACCESS_MODE_ANY = 'ANY'; diff --git a/core/lib/Drupal/Core/Access/AccessResult.php b/core/lib/Drupal/Core/Access/AccessResult.php index a325906..7a7b5cb 100644 --- a/core/lib/Drupal/Core/Access/AccessResult.php +++ b/core/lib/Drupal/Core/Access/AccessResult.php @@ -19,9 +19,24 @@ class AccessResult implements AccessResultInterface, CacheableInterface { /** + * The value that explicitly allows access. + */ + const ALLOW = 'ALLOW'; + + /** + * The value that neither explicitly allows nor explicitly forbids access. + */ + const DENY = 'DENY'; + + /** + * The value that explicitly forbids access. + */ + const KILL = 'KILL'; + + /** * The access result value. * - * A \Drupal\Core\Access\AccessInterface constant value. + * A \Drupal\Core\Access\AccessResultInterface constant value. * * @var string */ @@ -62,7 +77,7 @@ class AccessResult implements AccessResultInterface, CacheableInterface { * Constructs a new AccessResult object. */ public function __construct() { - $this->reset(); + $this->resetAccess(); $this->setCacheable(TRUE) ->resetCacheContexts() ->resetCacheTags() @@ -77,10 +92,13 @@ public function __construct() { * This factory method exists to improve DX; it allows developers to fluently * create access results. * + * Defaults to a cacheable access result that neither explicitly allows nor + * explicitly forbids access. + * * @return \Drupal\Core\Access\AccessResult */ public static function create() { - return new AccessResult(); + return new static(); } /** @@ -89,7 +107,7 @@ public static function create() { * @return \Drupal\Core\Access\AccessResult */ public static function allowed() { - return AccessResult::create()->allow(); + return static::create()->allow(); } /** @@ -98,111 +116,111 @@ public static function allowed() { * @return \Drupal\Core\Access\AccessResult */ public static function forbidden() { - return AccessResult::create()->forbid(); + return static::create()->forbid(); } /** * Convenience method, creates an AccessResult object and calls allowIf(). * * @param bool $condition - * The condition to evaluate. If TRUE, AccessInterface::ALLOW will be set, - * otherwise AccessInterface::DENY. + * The condition to evaluate. If TRUE, ::allow() will be called. * * @return \Drupal\Core\Access\AccessResult */ public static function allowedIf($condition) { - return AccessResult::create()->allowIf($condition); + return static::create()->allowIf($condition); } /** * Convenience method, creates an AccessResult object and calls forbiddenIf(). * * @param bool $condition - * The condition to evaluate. If TRUE, AccessInterface::KILL will be set, - * otherwise AccessInterface::DENY. + * The condition to evaluate. If TRUE, ::forbid() will be called. * * @return \Drupal\Core\Access\AccessResult */ public static function forbiddenIf($condition) { - return AccessResult::create()->forbidIf($condition); + return static::create()->forbidIf($condition); } /** * {@inheritdoc} */ public function isAllowed() { - return $this->value === AccessInterface::ALLOW; + return $this->value === static::ALLOW; } /** * {@inheritdoc} */ public function isForbidden() { - return $this->value === AccessInterface::KILL; + return $this->value === static::KILL; } /** - * Sets AccessInterface::ALLOW. + * Explicitly allows access. * * @return $this */ public function allow() { - $this->value = AccessInterface::ALLOW; + $this->value = static::ALLOW; return $this; } /** - * Sets AccessInterface::KILL. + * Explicitly forbids access. * * @return $this */ public function forbid() { - $this->value = AccessInterface::KILL; + $this->value = static::KILL; return $this; } /** - * Sets AccessInterface::DENY. + * Neither explicitly allows nor explicitly forbids access. * * @return $this */ - public function reset() { - $this->value = AccessInterface::DENY; + public function resetAccess() { + $this->value = static::DENY; return $this; } /** - * Sets AccessInterface::ALLOW or AccessInterface::DENY. + * Conditionally calls ::allow(). * * @param bool $condition - * The condition to evaluate. If TRUE, AccessInterface::ALLOW will be set, - * otherwise AccessInterface::DENY. + * The condition to evaluate. If TRUE, ::allow() will be called. * * @return $this */ public function allowIf($condition) { - $this->value = $condition ? AccessInterface::ALLOW : AccessInterface::DENY; + if ($condition) { + $this->allow(); + } return $this; } /** - * Sets AccessInterface::KILL or AccessInterface::DENY. + * Conditionally calls ::forbid(). * * @param bool $condition - * The condition to evaluate. If TRUE, AccessInterface::KILL will be set, - * otherwise AccessInterface::DENY. + * The condition to evaluate. If TRUE, ::forbid() will be called. * * @return $this */ public function forbidIf($condition) { - $this->value = $condition ? AccessInterface::KILL: AccessInterface::DENY; + if ($condition) { + $this->forbid(); + } return $this; } /** * {@inheritdoc} * - * AccessResult objects solely return cache context IDs, no 'regular' strings. + * AccessResult objects solely return cache context tokens, no static strings. */ public function getCacheKeys() { sort($this->contexts); @@ -219,7 +237,7 @@ public function getCacheTags() { /** * {@inheritdoc} * - * It's not very useful to cache individual cache results, but the interface + * It's not very useful to cache individual access results, but the interface * forces us to implement this method, so just use the default cache bin. */ public function getCacheBin() { @@ -326,7 +344,7 @@ public function setCacheMaxAge($max_age) { } /** - * Convenience method, to simply setting the "per role" cache context. + * Convenience method, adds the "cache_context.user.roles" cache context. * * @return $this */ @@ -336,7 +354,7 @@ public function cachePerRole() { } /** - * Convenience method, to simply setting the "per user" cache context. + * Convenience method, adds the "cache_context.user" cache context. * * @return $this */ @@ -346,7 +364,7 @@ public function cachePerUser() { } /** - * Convenience method, to simplify setting an entity's cache tag. + * Convenience method, adds the entity's cache tag. * * @param \Drupal\Core\Entity\EntityInterface $entity * The entity whose cache tag to set on the access result. @@ -400,7 +418,7 @@ public function andIf(AccessResultInterface $other) { $this->forbid(); } else if (!$other->isAllowed()) { - $this->reset(); + $this->resetAccess(); } $this->mergeCacheabilityMetadata($other); return $this; diff --git a/core/lib/Drupal/Core/Access/AccessResultInterface.php b/core/lib/Drupal/Core/Access/AccessResultInterface.php index 21afba7..5aed081 100644 --- a/core/lib/Drupal/Core/Access/AccessResultInterface.php +++ b/core/lib/Drupal/Core/Access/AccessResultInterface.php @@ -11,20 +11,17 @@ * Interface for checking access. * * Note: you can check whether access is neither explicitly allowed nor - * explicitly forbidden (i.e. AccessInterface::DENY, which means 'no opinion'): + * explicitly forbidden: * * @code * $no_opinion = !$access->isAllowed() && !$access->isForbidden(); * @endcode */ -interface AccessResultInterface extends AccessInterface { +interface AccessResultInterface { /** * Checks whether this access result indicates access is explicitly allowed. * - * When the value stored in this access result equals AccessInterface::ALLOW, - * access is allowed. - * * @return bool */ public function isAllowed(); @@ -32,9 +29,6 @@ public function isAllowed(); /** * Checks whether this access result indicates access is explicitly forbidden. * - * When the value stored in this access result equals AccessInterface::KILL, - * access is forbidden. - * * @return bool */ public function isForbidden(); @@ -43,9 +37,9 @@ public function isForbidden(); * Combine this access result with another using OR. * * When OR-ing two access results, the result is: - * - AccessInterface::KILL (isForbidden()) in either ⇒ AccessInterface::KILL - * - AccessInterface::ALLOW (isAllowed()) in either ⇒ AccessInterface::ALLOW - * - AccessInterface::DENY (neither isForbidden() nor isAllowed()) + * - isForbidden() in either ⇒ isForbidden() + * - isAllowed() in either ⇒ isAllowed() + * - neither isForbidden() nor isAllowed() => !isAllowed() && !isForbidden() * * @param \Drupal\Core\Access\AccessResultInterface $other * The other access result to OR this one with. @@ -58,9 +52,9 @@ public function orIf(AccessResultInterface $other); * Combine this access result with another using OR. * * When OR-ing two access results, the result is: - * - AccessInterface::KILL (isForbidden()) in either ⇒ AccessInterface::KILL - * - AccessInterface::ALLOW (isAllowed()) in both ⇒ AccessInterface::ALLOW - * - AccessInterface::DENY (neither isForbidden() nor isAllowed()) + * - isForbidden() in either ⇒ isForbidden() + * - isAllowed() in both ⇒ isAllowed() + * - neither isForbidden() nor isAllowed() => !isAllowed() && !isForbidden() * * @param \Drupal\Core\Access\AccessResultInterface $other * The other access result to AND this one with. diff --git a/core/lib/Drupal/Core/Access/AccessibleInterface.php b/core/lib/Drupal/Core/Access/AccessibleInterface.php index 557e0da..63beb8a 100644 --- a/core/lib/Drupal/Core/Access/AccessibleInterface.php +++ b/core/lib/Drupal/Core/Access/AccessibleInterface.php @@ -14,7 +14,7 @@ * * @ingroup entity_api */ -interface AccessibleInterface extends AccessInterface { +interface AccessibleInterface { /** * Checks data value access. diff --git a/core/lib/Drupal/Core/Cache/CacheableInterface.php b/core/lib/Drupal/Core/Cache/CacheableInterface.php index 85950ff..67afc2e 100644 --- a/core/lib/Drupal/Core/Cache/CacheableInterface.php +++ b/core/lib/Drupal/Core/Cache/CacheableInterface.php @@ -16,8 +16,14 @@ /** * The cache keys associated with this potentially cacheable object. * + * Cache keys may either be static (just strings) or tokens (placeholders + * that are converted to static keys by the @cache_contexts service, depending + * depending on the request). + * * @return array - * An array of strings or cache context IDs, used to generate a cache ID. + * An array of strings or cache context tokens, used to generate a cache ID. + * + * @see \Drupal\Core\Cache\CacheContexts::convertTokensToKeys() */ public function getCacheKeys(); diff --git a/core/lib/Drupal/Core/Entity/EntityAccessControlHandler.php b/core/lib/Drupal/Core/Entity/EntityAccessControlHandler.php index d7c0742..4902108 100644 --- a/core/lib/Drupal/Core/Entity/EntityAccessControlHandler.php +++ b/core/lib/Drupal/Core/Entity/EntityAccessControlHandler.php @@ -79,7 +79,7 @@ public function access(EntityInterface $entity, $operation, $langcode = Language if (!$return->isAllowed() && !$return->isForbidden()) { // No module had an opinion about the access, so let's the access // handler check access. - $return = $this->checkAccess($entity, $operation, $langcode, $account); + $return->orIf($this->checkAccess($entity, $operation, $langcode, $account)); } return $this->setCache($return, $entity->uuid(), $operation, $langcode, $account); } @@ -233,7 +233,7 @@ public function createAccess($entity_bundle = NULL, AccountInterface $account = if (!$return->isAllowed() && !$return->isForbidden()) { // No module had an opinion about the access, so let's the access // handler check create access. - $return = $this->checkCreateAccess($account, $context, $entity_bundle); + $return->orIf($this->checkCreateAccess($account, $context, $entity_bundle)); } return $this->setCache($return, $cid, 'create', $context['langcode'], $account); } diff --git a/core/lib/Drupal/Core/Menu/MenuLinkDefault.php b/core/lib/Drupal/Core/Menu/MenuLinkDefault.php index b822aba..e118824 100644 --- a/core/lib/Drupal/Core/Menu/MenuLinkDefault.php +++ b/core/lib/Drupal/Core/Menu/MenuLinkDefault.php @@ -95,6 +95,7 @@ public function getDescription() { */ public function isResettable() { // The link can be reset if it has an override. + // @todo This will be cacheable after https://www.drupal.org/node/2040135. return AccessResult::allowedIf($this->staticOverride->loadOverride($this->getPluginId()))->setCacheable(FALSE); } diff --git a/core/lib/Drupal/Core/Routing/Access/AccessInterface.php b/core/lib/Drupal/Core/Routing/Access/AccessInterface.php index 24c943d..2a2f4a9 100644 --- a/core/lib/Drupal/Core/Routing/Access/AccessInterface.php +++ b/core/lib/Drupal/Core/Routing/Access/AccessInterface.php @@ -7,12 +7,10 @@ namespace Drupal\Core\Routing\Access; -use Drupal\Core\Access\AccessInterface as GenericAccessInterface; - /** * An access check service determines access rules for particular routes. */ -interface AccessInterface extends GenericAccessInterface { +interface AccessInterface { // @todo Remove this interface since it no longer defines any methods? // @see https://drupal.org/node/2266817. diff --git a/core/modules/block/block.api.php b/core/modules/block/block.api.php index 8ecfcd6..c8edea5 100644 --- a/core/modules/block/block.api.php +++ b/core/modules/block/block.api.php @@ -142,17 +142,10 @@ function hook_block_view_BASE_BLOCK_ID_alter(array &$build, \Drupal\Core\Block\B * The language code to perform the access check operation on. * * @return \Drupal\Core\Access\AccessResultInterface - * The access result. Its value can be set - * to: - * - AccessInterface::ALLOW: the block may be accessed by the user - * - AccessInterface::KILL: the block may not be accessed by the user - * - AccessInterface::DENY: the hook implementation has no opinion on - * whether this block may be accessed by the user; if all implementations - * of this hook return this, then default access rules from - * \Drupal\block\BlockAccessControlHandler::checkAccess() are used. - * The cacheability metadata should also be set, to reflect whether this - * access check result applies for example to all users of this role, or only - * to the current user, or… + * The access result. If all implementations of this hook return + * AccessResultInterface objects whose value is !isAllowed() and + * !isForbidden(), then default access rules from + * \Drupal\block\BlockAccessControlHandler::checkAccess() are used. * * @see \Drupal\Core\Entity\EntityAccessControlHandler::access() * @see \Drupal\block\BlockAccessControlHandler::checkAccess() diff --git a/core/modules/node/node.api.php b/core/modules/node/node.api.php index 1a75128..8355012 100644 --- a/core/modules/node/node.api.php +++ b/core/modules/node/node.api.php @@ -295,9 +295,10 @@ function hook_node_grants_alter(&$grants, \Drupal\Core\Session\AccountInterface * interface. * * Note that not all modules will want to influence access on all node types. If - * your module does not want to actively grant or block access, return an access - * check result with AccessInterface::DENY. Blindly returning - * AccessInterface::KILL will break other node access modules. + * your module does not want to explicitly allow or forbid access, return an + * AccessResultInterface object with neither isAllowed() nor isForbidden() + * equaling TRUE. Blindly returning an object with isForbidden() equaling TRUE + * will break other node access modules. * * Also note that this function isn't called for node listings (e.g., RSS feeds, * the default home page at path 'node', a recent content block, etc.) See @@ -318,14 +319,7 @@ function hook_node_grants_alter(&$grants, \Drupal\Core\Session\AccountInterface * The language code to perform the access check operation on. * * @return \Drupal\Core\Access\AccessResultInterface - * The access result. Its value can be set - * to: - * - AccessInterface::ALLOW: if the operation is to be allowed - * - AccessInterface::KILL: if the operation is to be forbidden - * - AccessInterface::DENY: to not affect this operation at all. - * The cacheability metadata should also be set, to reflect whether this - * access check result applies for example to all users of this role, or only - * to the current user, or… + * The access result. * * @ingroup node_access */ diff --git a/core/modules/node/node.module b/core/modules/node/node.module index 7eb88f2..d9bed73 100644 --- a/core/modules/node/node.module +++ b/core/modules/node/node.module @@ -1136,12 +1136,13 @@ function node_form_system_themes_admin_form_submit($form, FormStateInterface $fo * @link entity_api Entity API topic @endlink for more information on entity * queries. * - * Note: Even a single module returning AccessInterface::KILL from - * hook_node_access() will block access to the node. Therefore, implementers - * should take care to not deny access unless they really intend to. Unless a - * module wishes to actively forbid access it should return - * AccessInterface::DENY to allow other modules or the node_access table to - * control access. + * Note: Even a single module returning an AccessResultInterface object from + * hook_node_access() whose isForbidden() method equals TRUE will block access + * to the node. Therefore, implementers should take care to not deny access + * unless they really intend to. Unless a module wishes to actively forbid + * access it should return an AccessResultInterface object whose isAllowed() nor + * isForbidden() methods return TRUE, to allow other modules or the node_access + * table to control access. * * To see how to write a node access module of your own, see * node_access_example.module. diff --git a/core/modules/node/src/NodeGrantDatabaseStorageInterface.php b/core/modules/node/src/NodeGrantDatabaseStorageInterface.php index fce8bfc..e865387 100644 --- a/core/modules/node/src/NodeGrantDatabaseStorageInterface.php +++ b/core/modules/node/src/NodeGrantDatabaseStorageInterface.php @@ -105,12 +105,8 @@ public function writeDefault(); * @param \Drupal\Core\Session\AccountInterface $account * The user for which to check access. * - * @return \Drupal\Core\Access\AccessResultInterface|null - * The access result. The access check - * result's value is AccessInterface::ALLOW if access was granted, - * AccessInterface::KILL if access was denied. NULL is returned when no - * module implements hook_node_grants(), the node does not (yet) have an id - * or none of the implementing modules explicitly granted or denied access. + * @return \Drupal\Core\Access\AccessResultInterface + * The access result. */ public function access(NodeInterface $node, $operation, $langcode, AccountInterface $account); diff --git a/core/modules/quickedit/tests/src/Access/EditEntityFieldAccessCheckTest.php b/core/modules/quickedit/tests/src/Access/EditEntityFieldAccessCheckTest.php index cc37cf2..7130a32 100644 --- a/core/modules/quickedit/tests/src/Access/EditEntityFieldAccessCheckTest.php +++ b/core/modules/quickedit/tests/src/Access/EditEntityFieldAccessCheckTest.php @@ -103,20 +103,20 @@ public function testAccess(EntityInterface $entity, FieldStorageConfigInterface } /** - * Tests checking access to routes that result in AccessCheckInterface::KILL. + * Tests checking access to routes that result in AccessResult::isForbidden(). * - * @dataProvider providerTestAccessKill + * @dataProvider providerTestAccessForbidden */ - public function testAccessKill($field_name, $langcode) { + public function testAccessForbidden($field_name, $langcode) { $account = $this->getMock('Drupal\Core\Session\AccountInterface'); $entity = $this->createMockEntity(); $this->assertEquals(AccessResult::forbidden(), $this->editAccessCheck->access($entity, $field_name, $langcode, $account)); } /** - * Provides test data for testAccessKill. + * Provides test data for testAccessForbidden. */ - public function providerTestAccessKill() { + public function providerTestAccessForbidden() { $data = array(); // Tests the access method without a field_name. $data[] = array(NULL, LanguageInterface::LANGCODE_NOT_SPECIFIED); diff --git a/core/modules/system/entity.api.php b/core/modules/system/entity.api.php index 7a1d06f..cf51ae9 100644 --- a/core/modules/system/entity.api.php +++ b/core/modules/system/entity.api.php @@ -1861,12 +1861,12 @@ function hook_entity_field_access_alter(array &$grants, array $context) { /** @var \Drupal\Core\Field\FieldDefinitionInterface $field_definition */ $field_definition = $context['field_definition']; if ($field_definition->getName() == 'field_of_interest' && $grants['node']->isForbidden()) { - // Override node module's restriction to no opinion. We don't want to - // provide our own access hook, we only want to take out node module's part - // in the access handling of this field. We also don't want to switch node - // module's grant to AccessInterface::ALLOW, because the grants of other - // modules should still decide on their own if this field is accessible or - // not. + // Override node module's restriction to no opinion (neither allowed nor + // forbidden). We don't want to provide our own access hook, we only want to + // take out node module's part in the access handling of this field. We also + // don't want to switch node module's grant to + // AccessResultInterface::isAllowed() , because the grants of other modules + // should still decide on their own if this field is accessible or not $grants['node']->reset(); } } diff --git a/core/modules/toolbar/src/Controller/ToolbarController.php b/core/modules/toolbar/src/Controller/ToolbarController.php index 2fe3ee1..b1de82d 100644 --- a/core/modules/toolbar/src/Controller/ToolbarController.php +++ b/core/modules/toolbar/src/Controller/ToolbarController.php @@ -38,9 +38,8 @@ public function subtreesJsonp() { * @param string $langcode * The langcode of the requested site, NULL if none given. * - * @return string - * Returns AccessInterface::ALLOW when access was granted, otherwise - * AccessInterface::DENY. + * @return \Drupal\Core\Access\AccessResultInterface + * The access result. */ public function checkSubTreeAccess(Request $request, $langcode) { $hash = $request->get('hash'); diff --git a/core/tests/Drupal/Tests/Core/Access/AccessManagerTest.php b/core/tests/Drupal/Tests/Core/Access/AccessManagerTest.php index 0c16d98..bc1ec97 100644 --- a/core/tests/Drupal/Tests/Core/Access/AccessManagerTest.php +++ b/core/tests/Drupal/Tests/Core/Access/AccessManagerTest.php @@ -238,127 +238,127 @@ public function providerTestCheckConjunctions() { $access_configurations[] = array( 'conjunction' => AccessManagerInterface::ACCESS_MODE_ALL, 'name' => 'test_route_4', - 'condition_one' => AccessCheckInterface::ALLOW, - 'condition_two' => AccessCheckInterface::KILL, + 'condition_one' => 'TRUE', + 'condition_two' => 'FALSE', 'expected' => $access_kill, ); $access_configurations[] = array( 'conjunction' => NULL, 'name' => 'test_route_4', - 'condition_one' => AccessCheckInterface::ALLOW, - 'condition_two' => AccessCheckInterface::KILL, + 'condition_one' => 'TRUE', + 'condition_two' => 'FALSE', 'expected' => $access_kill, ); $access_configurations[] = array( 'conjunction' => AccessManagerInterface::ACCESS_MODE_ALL, 'name' => 'test_route_5', - 'condition_one' => AccessCheckInterface::ALLOW, - 'condition_two' => AccessCheckInterface::DENY, + 'condition_one' => 'TRUE', + 'condition_two' => 'NULL', 'expected' => $access_deny, ); $access_configurations[] = array( 'conjunction' => NULL, 'name' => 'test_route_5', - 'condition_one' => AccessCheckInterface::ALLOW, - 'condition_two' => AccessCheckInterface::DENY, + 'condition_one' => 'TRUE', + 'condition_two' => 'NULL', 'expected' => $access_deny, ); $access_configurations[] = array( 'conjunction' => AccessManagerInterface::ACCESS_MODE_ALL, 'name' => 'test_route_6', - 'condition_one' => AccessCheckInterface::KILL, - 'condition_two' => AccessCheckInterface::DENY, + 'condition_one' => 'FALSE', + 'condition_two' => 'NULL', 'expected' => $access_kill, ); $access_configurations[] = array( 'conjunction' => NULL, 'name' => 'test_route_6', - 'condition_one' => AccessCheckInterface::KILL, - 'condition_two' => AccessCheckInterface::DENY, + 'condition_one' => 'FALSE', + 'condition_two' => 'NULL', 'expected' => $access_kill, ); $access_configurations[] = array( 'conjunction' => AccessManagerInterface::ACCESS_MODE_ALL, 'name' => 'test_route_7', - 'condition_one' => AccessCheckInterface::ALLOW, - 'condition_two' => AccessCheckInterface::ALLOW, + 'condition_one' => 'TRUE', + 'condition_two' => 'TRUE', 'expected' => $access_allow, ); $access_configurations[] = array( 'conjunction' => NULL, 'name' => 'test_route_7', - 'condition_one' => AccessCheckInterface::ALLOW, - 'condition_two' => AccessCheckInterface::ALLOW, + 'condition_one' => 'TRUE', + 'condition_two' => 'TRUE', 'expected' => $access_allow, ); $access_configurations[] = array( 'conjunction' => AccessManagerInterface::ACCESS_MODE_ALL, 'name' => 'test_route_8', - 'condition_one' => AccessCheckInterface::KILL, - 'condition_two' => AccessCheckInterface::KILL, + 'condition_one' => 'FALSE', + 'condition_two' => 'FALSE', 'expected' => $access_kill, ); $access_configurations[] = array( 'conjunction' => NULL, 'name' => 'test_route_8', - 'condition_one' => AccessCheckInterface::KILL, - 'condition_two' => AccessCheckInterface::KILL, + 'condition_one' => 'FALSE', + 'condition_two' => 'FALSE', 'expected' => $access_kill, ); $access_configurations[] = array( 'conjunction' => AccessManagerInterface::ACCESS_MODE_ALL, 'name' => 'test_route_9', - 'condition_one' => AccessCheckInterface::DENY, - 'condition_two' => AccessCheckInterface::DENY, + 'condition_one' => 'NULL', + 'condition_two' => 'NULL', 'expected' => $access_deny, ); $access_configurations[] = array( 'conjunction' => NULL, 'name' => 'test_route_9', - 'condition_one' => AccessCheckInterface::DENY, - 'condition_two' => AccessCheckInterface::DENY, + 'condition_one' => 'NULL', + 'condition_two' => 'NULL', 'expected' => $access_deny, ); $access_configurations[] = array( 'conjunction' => AccessManagerInterface::ACCESS_MODE_ANY, 'name' => 'test_route_10', - 'condition_one' => AccessCheckInterface::ALLOW, - 'condition_two' => AccessCheckInterface::KILL, + 'condition_one' => 'TRUE', + 'condition_two' => 'FALSE', 'expected' => $access_kill, ); $access_configurations[] = array( 'conjunction' => AccessManagerInterface::ACCESS_MODE_ANY, 'name' => 'test_route_11', - 'condition_one' => AccessCheckInterface::ALLOW, - 'condition_two' => AccessCheckInterface::DENY, + 'condition_one' => 'TRUE', + 'condition_two' => 'NULL', 'expected' => $access_allow, ); $access_configurations[] = array( 'conjunction' => AccessManagerInterface::ACCESS_MODE_ANY, 'name' => 'test_route_12', - 'condition_one' => AccessCheckInterface::KILL, - 'condition_two' => AccessCheckInterface::DENY, + 'condition_one' => 'FALSE', + 'condition_two' => 'NULL', 'expected' => $access_kill, ); $access_configurations[] = array( 'conjunction' => AccessManagerInterface::ACCESS_MODE_ANY, 'name' => 'test_route_13', - 'condition_one' => AccessCheckInterface::ALLOW, - 'condition_two' => AccessCheckInterface::ALLOW, + 'condition_one' => 'TRUE', + 'condition_two' => 'TRUE', 'expected' => $access_allow, ); $access_configurations[] = array( 'conjunction' => AccessManagerInterface::ACCESS_MODE_ANY, 'name' => 'test_route_14', - 'condition_one' => AccessCheckInterface::KILL, - 'condition_two' => AccessCheckInterface::KILL, + 'condition_one' => 'FALSE', + 'condition_two' => 'FALSE', 'expected' => $access_kill, ); $access_configurations[] = array( 'conjunction' => AccessManagerInterface::ACCESS_MODE_ANY, 'name' => 'test_route_15', - 'condition_one' => AccessCheckInterface::DENY, - 'condition_two' => AccessCheckInterface::DENY, + 'condition_one' => 'NULL', + 'condition_two' => 'NULL', 'expected' => $access_deny, ); @@ -381,8 +381,8 @@ public function testCheckConjunctions($conjunction, $name, $condition_one, $cond $route_collection = new RouteCollection(); // Setup a test route for each access configuration. $requirements = array( - '_access' => static::convertAccessCheckInterfaceToString($condition_one), - '_test_access' => static::convertAccessCheckInterfaceToString($condition_two), + '_access' => $condition_one, + '_test_access' => $condition_two, ); $options = $conjunction ? array('_access_mode' => $conjunction) : array(); $route = new Route($name, array(), $requirements, $options); @@ -644,30 +644,6 @@ public function providerCheckException() { } /** - * Converts AccessCheckInterface constants to a string. - * - * @param mixed $constant - * The access constant which is tested, so either - * AccessCheckInterface::ALLOW, AccessCheckInterface::DENY OR - * AccessCheckInterface::KILL. - * - * @return string - * The corresponding string used in route requirements, so 'TRUE', 'FALSE' - * or 'NULL'. - */ - protected static function convertAccessCheckInterfaceToString($constant) { - if ($constant === AccessCheckInterface::ALLOW) { - return 'TRUE'; - } - if ($constant === AccessCheckInterface::DENY) { - return 'NULL'; - } - if ($constant === AccessCheckInterface::KILL) { - return 'FALSE'; - } - } - - /** * Adds a default access check service to the container and the access manager. */ protected function setupAccessChecker() { diff --git a/core/tests/Drupal/Tests/Core/Access/AccessResultTest.php b/core/tests/Drupal/Tests/Core/Access/AccessResultTest.php index 2103167..7e24bee 100644 --- a/core/tests/Drupal/Tests/Core/Access/AccessResultTest.php +++ b/core/tests/Drupal/Tests/Core/Access/AccessResultTest.php @@ -111,10 +111,10 @@ public function testAccessReset() { $this->assertDefaultCacheability($access); }; - $a = AccessResult::allowed()->reset(); + $a = AccessResult::allowed()->resetAccess(); $verify($a); - $b = AccessResult::forbidden()->reset(); + $b = AccessResult::forbidden()->resetAccess(); $verify($b); $this->assertEquals($a, $b); @@ -127,9 +127,9 @@ public function testAccessReset() { * @covers ::isForbidden */ public function testAccessConditionallyAllowed() { - $verify = function (AccessResult $access, $allowed) { + $verify = function (AccessResult $access, $allowed, $forbidden = FALSE) { $this->assertSame($allowed, $access->isAllowed()); - $this->assertFalse($access->isForbidden()); + $this->assertSame($forbidden, $access->isForbidden()); $this->assertDefaultCacheability($access); }; @@ -146,6 +146,13 @@ public function testAccessConditionallyAllowed() { $this->assertEquals($a1, $b1); $this->assertEquals($a2, $b2); + + // Verify that ::allowIf() does not overwrite an existing value when the + // condition does not evaluate to TRUE. + $a1 = AccessResult::forbidden()->allowIf(TRUE); + $verify($a1, TRUE); + $a2 = AccessResult::forbidden()->allowIf(FALSE); + $verify($a2, FALSE, TRUE); } /** @@ -155,8 +162,8 @@ public function testAccessConditionallyAllowed() { * @covers ::isForbidden */ public function testAccessConditionallyForbidden() { - $verify = function (AccessResult $access, $forbidden) { - $this->assertFalse($access->isAllowed()); + $verify = function (AccessResult $access, $forbidden, $allowed = FALSE) { + $this->assertSame($allowed, $access->isAllowed()); $this->assertSame($forbidden, $access->isForbidden()); $this->assertDefaultCacheability($access); }; @@ -174,6 +181,13 @@ public function testAccessConditionallyForbidden() { $this->assertEquals($a1, $b1); $this->assertEquals($a2, $b2); + + // Verify that ::forbidIf() does not overwrite an existing value when the + // condition does not evaluate to TRUE. + $a1 = AccessResult::allowed()->forbidIf(TRUE); + $verify($a1, TRUE); + $a2 = AccessResult::allowed()->forbidIf(FALSE); + $verify($a2, FALSE, TRUE); } /** diff --git a/core/tests/Drupal/Tests/Core/Access/CustomAccessCheckTest.php b/core/tests/Drupal/Tests/Core/Access/CustomAccessCheckTest.php index 23f500d..c270d90 100644 --- a/core/tests/Drupal/Tests/Core/Access/CustomAccessCheckTest.php +++ b/core/tests/Drupal/Tests/Core/Access/CustomAccessCheckTest.php @@ -8,6 +8,7 @@ namespace Drupal\Tests\Core\Access; use Drupal\Core\Access\AccessInterface; +use Drupal\Core\Access\AccessResult; use Drupal\Core\Access\CustomAccessCheck; use Drupal\Tests\UnitTestCase; use Symfony\Component\HttpFoundation\Request; @@ -86,13 +87,13 @@ public function testAccess() { $route = new Route('/test-route', array(), array('_custom_access' => '\Drupal\Tests\Core\Access\TestController::accessDeny')); $account = $this->getMock('Drupal\Core\Session\AccountInterface'); - $this->assertSame(AccessInterface::DENY, $this->accessChecker->access($route, $request, $account)); + $this->assertEquals(AccessResult::create(), $this->accessChecker->access($route, $request, $account)); $route = new Route('/test-route', array(), array('_custom_access' => '\Drupal\Tests\Core\Access\TestController::accessAllow')); - $this->assertSame(AccessInterface::ALLOW, $this->accessChecker->access($route, $request, $account)); + $this->assertEquals(AccessResult::allowed(), $this->accessChecker->access($route, $request, $account)); $route = new Route('/test-route', array('parameter' => 'TRUE'), array('_custom_access' => '\Drupal\Tests\Core\Access\TestController::accessParameter')); - $this->assertSame(AccessInterface::ALLOW, $this->accessChecker->access($route, $request, $account)); + $this->assertEquals(AccessResult::allowed(), $this->accessChecker->access($route, $request, $account)); } } @@ -100,20 +101,15 @@ public function testAccess() { class TestController { public function accessAllow() { - return AccessInterface::ALLOW; + return AccessResult::allowed(); } public function accessDeny() { - return AccessInterface::DENY; + return AccessResult::create(); } public function accessParameter($parameter) { - if ($parameter == 'TRUE') { - return AccessInterface::ALLOW; - } - else { - return AccessInterface::DENY; - } + return AccessResult::allowedIf($parameter == 'TRUE'); } }