Issue #2340507 by chx, Wim Leers: Make the new AccessResult API and implementation even better.

core/lib/Drupal/Core/Access/AccessManager.php

@@ -229,7 +229,7 @@ public function check(RouteMatchInterface $route_match, AccountInterface $accoun
       $checks = array_diff($checks, $this->checkNeedsRequest);
     }
 
-    $result = AccessResult::create();
+    $result = AccessResult::neutral();
     if (!empty($checks)) {
       $arguments_resolver = $this->argumentsResolverFactory->getArgumentsResolver($route_match, $account, $request);
       if ($conjunction == static::ACCESS_MODE_ALL) {
@@ -256,34 +256,18 @@ public function check(RouteMatchInterface $route_match, AccountInterface $accoun
    * @see \Drupal\Core\Access\AccessResultInterface::andIf()
    */
   protected function checkAll(array $checks, ArgumentsResolverInterface $arguments_resolver) {
-    $results = array();
-
+    // Without checks no opinion can be formed.
+    if (!$checks) {
+      return AccessResult::neutral();
+    }
+    $result = AccessResult::allowed();
     foreach ($checks as $service_id) {
       if (empty($this->checks[$service_id])) {
         $this->loadCheck($service_id);
       }
-
-      $result = $this->performCheck($service_id, $arguments_resolver);
-      $results[] = $result;
-
-      // Stop as soon as the first non-allowed check is encountered.
-      if (!$result->isAllowed()) {
-        break;
-      }
-    }
-
-    if (empty($results)) {
-      // No opinion.
-      return AccessResult::create();
-    }
-    else {
-      /** @var \Drupal\Core\Access\AccessResultInterface $result */
-      $result = array_shift($results);
-      foreach ($results as $other) {
-        $result->andIf($other);
-      }
-      return $result;
+      $result = $result->andIf($this->performCheck($service_id, $arguments_resolver));
     }
+    return $result;
   }
 
   /**
@@ -301,7 +285,7 @@ protected function checkAll(array $checks, ArgumentsResolverInterface $arguments
    */
   protected function checkAny(array $checks, ArgumentsResolverInterface $arguments_resolver) {
     // No opinion by default.
-    $result = AccessResult::create();
+    $result = AccessResult::neutral();
 
     foreach ($checks as $service_id) {
       if (empty($this->checks[$service_id])) {

core/lib/Drupal/Core/Access/AccessResultAllowed.php

+<?php
+
+/**
+ * @file
+ * Contains \Drupal\Core\Access\AccessResultAllowed.
+ */
+
+namespace Drupal\Core\Access;
+
+/**
+ * Value object indicating an allowed access result, with cacheability metadata.
+ */
+class AccessResultAllowed extends AccessResult {
+
+  /**
+   * {@inheritdoc}
+   */
+  public function isAllowed() {
+    return TRUE;
+  }
+
+}

core/lib/Drupal/Core/Access/AccessResultForbidden.php

+<?php
+
+/**
+ * @file
+ * Contains \Drupal\Core\Access\AccessResultForbidden.
+ */
+
+namespace Drupal\Core\Access;
+
+/**
+ * Value object indicating a forbidden access result, with cacheability metadata.
+ */
+class AccessResultForbidden extends AccessResult {
+
+  /**
+   * {@inheritdoc}
+   */
+  public function isForbidden() {
+    return TRUE;
+  }
+
+}

core/lib/Drupal/Core/Access/AccessResultInterface.php

@@ -23,12 +23,10 @@
  * would never enter the else-statement and hence introduce a critical security
  * issue.
  *
- * Note: you can check whether access is neither explicitly allowed nor
- * explicitly forbidden:
- *
- * @code
- * $no_opinion = !$access->isAllowed() && !$access->isForbidden();
- * @endcode
+ * Objects implementing this interface are using Kleene's weak three-valued
+ * logic with the isAllowed() state being TRUE, the isForbidden() state being
+ * the intermediate truth value and isNeutral() being FALSE. See
+ * http://en.wikipedia.org/wiki/Many-valued_logic for more.
  */
 interface AccessResultInterface {
 
@@ -36,43 +34,56 @@ interface AccessResultInterface {
    * Checks whether this access result indicates access is explicitly allowed.
    *
    * @return bool
+   *   When TRUE then isForbidden() and isNeutral() are FALSE.
    */
   public function isAllowed();
 
   /**
    * Checks whether this access result indicates access is explicitly forbidden.
    *
+   * This is a kill switch — both orIf() and andIf() will result in
+   * isForbidden() if either results are isForbidden().
+   *
    * @return bool
+   *   When TRUE then isAllowed() and isNeutral() are FALSE.
    */
   public function isForbidden();
 
+  /**
+   * Checks whether this access result indicates access is not yet determined.
+   *
+   * @return bool
+   *   When TRUE then isAllowed() and isForbidden() are FALSE.
+   */
+  public function isNeutral();
+
   /**
    * Combine this access result with another using OR.
    *
    * When OR-ing two access results, the result is:
    * - isForbidden() in either ⇒ isForbidden()
-   * - isAllowed() in either ⇒ isAllowed()
-   * - neither isForbidden() nor isAllowed() => !isAllowed() && !isForbidden()
+   * - otherwise if isAllowed() in either ⇒ isAllowed()
+   * - otherwise both must be isNeutral() ⇒ isNeutral()
    *
    * @param \Drupal\Core\Access\AccessResultInterface $other
    *   The other access result to OR this one with.
    *
-   * @return $this
+   * @return static
    */
   public function orIf(AccessResultInterface $other);
 
   /**
    * Combine this access result with another using AND.
    *
-   * When OR-ing two access results, the result is:
+   * When AND-ing two access results, the result is:
    * - isForbidden() in either ⇒ isForbidden()
-   * - isAllowed() in both ⇒ isAllowed()
-   * - neither isForbidden() nor isAllowed() => !isAllowed() && !isForbidden()
+   * - otherwise, if isAllowed() in both ⇒ isAllowed()
+   * - otherwise, one of them is isNeutral()  ⇒ isNeutral()
    *
    * @param \Drupal\Core\Access\AccessResultInterface $other
    *   The other access result to AND this one with.
    *
-   * @return $this
+   * @return static
    */
   public function andIf(AccessResultInterface $other);
 

core/lib/Drupal/Core/Access/AccessResultNeutral.php

+<?php
+
+/**
+ * @file
+ * Contains \Drupal\Core\Access\AccessResultNeutral.
+ */
+
+namespace Drupal\Core\Access;
+
+/**
+ * Value object indicating a neutral access result, with cacheability metadata.
+ */
+class AccessResultNeutral extends AccessResult {
+
+  /**
+   * {@inheritdoc}
+   */
+  public function isNeutral() {
+    return TRUE;
+  }
+
+}

core/lib/Drupal/Core/Access/CsrfAccessCheck.php

@@ -49,17 +49,16 @@ public function __construct(CsrfTokenGenerator $csrf_token) {
    *   The access result.
    */
   public function access(Route $route, Request $request) {
-    // Not cacheable because the CSRF token is highly dynamic.
-    $access = AccessResult::create()->setCacheable(FALSE);
     // @todo Remove dependency on the internal _system_path attribute:
     //   https://www.drupal.org/node/2293501.
     if ($this->csrfToken->validate($request->query->get('token'), $request->attributes->get('_system_path'))) {
-      $access->allow();
+      $result = AccessResult::allowed();
     }
     else {
-      $access->forbid();
+      $result = AccessResult::forbidden();
     }
-    return $access;
+    // Not cacheable because the CSRF token is highly dynamic.
+    return $result->setCacheable(FALSE);
   }
 
 }

core/lib/Drupal/Core/Access/DefaultAccessCheck.php

@@ -32,7 +32,7 @@ public function access(Route $route) {
       return AccessResult::forbidden();
     }
     else {
-      return AccessResult::create();
+      return AccessResult::neutral();
     }
   }
 

core/lib/Drupal/Core/Block/BlockBase.php

@@ -170,18 +170,13 @@ public function access(AccountInterface $account) {
     // in order to fix that, we need condition plugins to return cache contexts,
     // otherwise it will be impossible to determine by which cache contexts the
     // result should be varied.
-    $access = AccessResult::create()->setCacheable(FALSE);
-    if ($this->resolveConditions($conditions, 'and', $contexts, $mappings) === FALSE) {
-      $access->forbid();
-      return $access;
-    }
-    if ($this->blockAccess($account)) {
-      $access->allow();
+    if ($this->resolveConditions($conditions, 'and', $contexts, $mappings) !== FALSE && $this->blockAccess($account)) {
+      $access = AccessResult::allowed();
     }
     else {
-      $access->forbid();
+      $access = AccessResult::forbidden();
     }
-    return $access;
+    return $access->setCacheable(FALSE);
   }
 
   /**

core/lib/Drupal/Core/Entity/EntityAccessCheck.php

@@ -55,7 +55,7 @@ public function access(Route $route, RouteMatchInterface $route_match, AccountIn
     }
     // No opinion, so other access checks should decide if access should be
     // allowed or not.
-    return AccessResult::create();
+    return AccessResult::neutral();
   }
 
 }

core/lib/Drupal/Core/Entity/EntityAccessControlHandler.php

@@ -76,10 +76,10 @@ public function access(EntityInterface $entity, $operation, $langcode = Language
     );
 
     $return = $this->processAccessHookResults($access);
-    if (!$return->isAllowed() && !$return->isForbidden()) {
+    if ($return->isNeutral()) {
       // No module had an opinion about the access, so let's the access
       // handler check access.
-      $return->orIf($this->checkAccess($entity, $operation, $langcode, $account));
+      $return = $return->orIf($this->checkAccess($entity, $operation, $langcode, $account));
     }
     $result = $this->setCache($return, $entity->uuid(), $operation, $langcode, $account);
     return $return_as_object ? $result : $result->isAllowed();
@@ -102,7 +102,7 @@ public function access(EntityInterface $entity, $operation, $langcode = Language
   protected function processAccessHookResults(array $access) {
     // No results means no opinion.
     if (empty($access)) {
-      return AccessResult::create();
+      return AccessResult::neutral();
     }
 
     /** @var \Drupal\Core\Access\AccessResultInterface $result */
@@ -141,7 +141,7 @@ protected function checkAccess(EntityInterface $entity, $operation, $langcode, A
     }
     else {
       // No opinion.
-      return AccessResult::create();
+      return AccessResult::neutral();
     }
   }
 
@@ -231,10 +231,10 @@ public function createAccess($entity_bundle = NULL, AccountInterface $account =
     );
 
     $return = $this->processAccessHookResults($access);
-    if (!$return->isAllowed() && !$return->isForbidden()) {
+    if ($return->isNeutral()) {
       // No module had an opinion about the access, so let's the access
       // handler check create access.
-      $return->orIf($this->checkCreateAccess($account, $context, $entity_bundle));
+      $return = $return->orIf($this->checkCreateAccess($account, $context, $entity_bundle));
     }
     $result = $this->setCache($return, $cid, 'create', $context['langcode'], $account);
     return $return_as_object ? $result : $result->isAllowed();
@@ -263,7 +263,7 @@ protected function checkCreateAccess(AccountInterface $account, array $context,
     }
     else {
       // No opinion.
-      return AccessResult::create();
+      return AccessResult::neutral();
     }
   }
 

core/lib/Drupal/Core/Entity/EntityCreateAccessCheck.php

@@ -67,7 +67,7 @@ public function access(Route $route, RouteMatchInterface $route_match, AccountIn
       }
       // If we were unable to replace all placeholders, deny access.
       if (strpos($bundle, '{') !== FALSE) {
-        return AccessResult::create();
+        return AccessResult::neutral();
       }
     }
     return $this->entityManager->getAccessControlHandler($entity_type)->createAccess($bundle, $account, [], TRUE);

core/modules/block/block.api.php

@@ -159,7 +159,7 @@ function hook_block_access(\Drupal\block\Entity\Block $block, $operation, \Drupa
   }
 
   // No opinion.
-  return AccessResult::create();
+  return AccessResult::neutral();
 }
 
 /**

core/modules/comment/src/CommentAccessControlHandler.php

@@ -42,7 +42,7 @@ protected function checkAccess(EntityInterface $entity, $operation, $langcode, A
 
       default:
         // No opinion.
-        return AccessResult::create()->cachePerRole();
+        return AccessResult::neutral()->cachePerRole();
     }
   }
 

core/modules/contact/src/Access/ContactPageAccess.php

@@ -71,9 +71,10 @@ public function access(UserInterface $user, AccountInterface $account) {
     }
 
     // User administrators should always have access to personal contact forms.
-    $access = AccessResult::create()->cachePerRole();
-    if ($account->hasPermission('administer users')) {
-      return $access->allow();
+    $access = AccessResult::neutral()->cachePerRole();
+    $permission_access = AccessResult::allowedIfHasPermission($account, 'administer users');
+    if ($permission_access->isAllowed()) {
+      return $access->orIf($permission_access);
     }
 
     // If requested user has been blocked, do not allow users to contact them.
@@ -94,7 +95,7 @@ public function access(UserInterface $user, AccountInterface $account) {
       return $access;
     }
 
-    return $access->allowIfHasPermission($account, 'access user contact forms');
+    return $access->orIf(AccessResult::allowedIfHasPermission($account, 'access user contact forms'));
   }
 
 }

core/modules/content_translation/src/Access/ContentTranslationManageAccessCheck.php

@@ -106,7 +106,7 @@ public function access(Route $route, RouteMatchInterface $route_match, AccountIn
     }
 
     // No opinion.
-    return AccessResult::create();
+    return AccessResult::neutral();
   }
 
 }

core/modules/content_translation/src/Access/ContentTranslationOverviewAccess.php

@@ -66,7 +66,7 @@ public function access(RouteMatchInterface $route_match, AccountInterface $accou
 
       // Check "translate any entity" permission.
       if ($account->hasPermission('translate any entity')) {
-        return $access->allow()->cachePerRole();
+        return AccessResult::allowed()->cachePerRole()->inheritCacheability($access);
       }
 
       // Check per entity permission.
@@ -74,10 +74,10 @@ public function access(RouteMatchInterface $route_match, AccountInterface $accou
       if ($definition->getPermissionGranularity() == 'bundle') {
         $permission = "translate {$bundle} {$entity_type_id}";
       }
-      return $access->allowIfHasPermission($account, $permission);
+      return AccessResult::allowedIfHasPermission($account, $permission)->inheritCacheability($access);
     }
 
     // No opinion.
-    return AccessResult::create();
+    return AccessResult::neutral();
   }
 }

core/modules/field_ui/src/Access/FormModeAccessCheck.php

@@ -62,6 +62,7 @@ public function __construct(EntityManagerInterface $entity_manager) {
    *   The access result.
    */
   public function access(Route $route, RouteMatchInterface $route_match, AccountInterface $account, $form_mode_name = 'default', $bundle = NULL) {
+    $access = AccessResult::neutral();
     if ($entity_type_id = $route->getDefault('entity_type_id')) {
       if (!isset($bundle)) {
         $entity_type = $this->entityManager->getDefinition($entity_type_id);
@@ -76,21 +77,16 @@ public function access(Route $route, RouteMatchInterface $route_match, AccountIn
         $visibility = $entity_display->status();
       }
 
-      $access = AccessResult::create();
       if ($form_mode_name != 'default' && $entity_display) {
         $access->cacheUntilEntityChanges($entity_display);
       }
 
       if ($visibility) {
         $permission = $route->getRequirement('_field_ui_form_mode_access');
-        $access->allowIfHasPermission($account, $permission);
+        $access = $access->orIf(AccessResult::allowedIfHasPermission($account, $permission));
       }
-      return $access;
-    }
-    else {
-      // No opinion.
-      return AccessResult::create();
     }
+    return $access;
   }
 
 }

core/modules/field_ui/src/Access/ViewModeAccessCheck.php

@@ -62,6 +62,7 @@ public function __construct(EntityManagerInterface $entity_manager) {
    *   The access result.
    */
   public function access(Route $route, RouteMatchInterface $route_match, AccountInterface $account, $view_mode_name = 'default', $bundle = NULL) {
+    $access = AccessResult::neutral();
     if ($entity_type_id = $route->getDefault('entity_type_id')) {
       if (!isset($bundle)) {
         $entity_type = $this->entityManager->getDefinition($entity_type_id);
@@ -76,21 +77,16 @@ public function access(Route $route, RouteMatchInterface $route_match, AccountIn
         $visibility = $entity_display->status();
       }
 
-      $access = AccessResult::create();
       if ($view_mode_name != 'default' && $entity_display) {
         $access->cacheUntilEntityChanges($entity_display);
       }
 
       if ($visibility) {
         $permission = $route->getRequirement('_field_ui_view_mode_access');
-        $access->allowIfHasPermission($account, $permission);
+        $access = $access->orIf(AccessResult::allowedIfHasPermission($account, $permission));
       }
-      return $access;
-    }
-    else {
-      // No opinion.
-      return AccessResult::create();
     }
+    return $access;
   }
 
 }

core/modules/file/src/FileAccessControlHandler.php

@@ -38,7 +38,7 @@ protected function checkAccess(EntityInterface $entity, $operation, $langcode, A
     }
 
     // No opinion.
-    return AccessResult::create();
+    return AccessResult::neutral();
   }
 
   /**

core/modules/filter/src/FilterFormatAccessControlHandler.php

@@ -51,7 +51,7 @@ protected function checkAccess(EntityInterface $filter_format, $operation, $lang
     }
 
     // No opinion.
-    return AccessResult::create();
+    return AccessResult::neutral();
   }
 
 }

core/modules/filter/src/Tests/FilterFormatAccessTest.php

@@ -125,7 +125,7 @@ function testFormatPermissions() {
     $this->assertTrue($this->allowed_format->access('use', $this->web_user), 'A regular user has access to use a text format they were granted access to.');
     $this->assertEqual(AccessResult::allowed()->cachePerRole(), $this->allowed_format->access('use', $this->web_user, TRUE), 'A regular user has access to use a text format they were granted access to.');
     $this->assertFalse($this->disallowed_format->access('use', $this->web_user), 'A regular user does not have access to use a text format they were not granted access to.');
-    $this->assertEqual(AccessResult::create()->cachePerRole(), $this->disallowed_format->access('use', $this->web_user, TRUE), 'A regular user does not have access to use a text format they were not granted access to.');
+    $this->assertEqual(AccessResult::neutral(), $this->disallowed_format->access('use', $this->web_user, TRUE)); //, 'A regular user does not have access to use a text format they were not granted access to.');
     $this->assertTrue($fallback_format->access('use', $this->web_user), 'A regular user has access to use the fallback format.');
     $this->assertEqual(AccessResult::allowed(), $fallback_format->access('use', $this->web_user, TRUE), 'A regular user has access to use the fallback format.');
 

core/modules/language/src/LanguageAccessControlHandler.php

@@ -31,7 +31,7 @@ public function checkAccess(EntityInterface $entity, $operation, $langcode, Acco
 
       default:
         // No opinion.
-        return AccessResult::create();
+        return AccessResult::neutral();
     }
   }
 

core/modules/menu_link_content/src/MenuLinkContentAccessControlHandler.php

@@ -55,23 +55,21 @@ protected function checkAccess(EntityInterface $entity, $operation, $langcode, A
     switch ($operation) {
       case 'view':
         // There is no direct view.
-        return AccessResult::create();
+        return AccessResult::neutral();
 
       case 'update':
         if (!$account->hasPermission('administer menu')) {
-          return AccessResult::create()->cachePerRole();
+          return AccessResult::neutral()->cachePerRole();
         }
         else {
-          $access = AccessResult::create()->cachePerRole()->cacheUntilEntityChanges($entity);
           // If there is a URL, this is an external link so always accessible.
-          if ($entity->getUrl()) {
-            return $access->allow();
-          }
-          else {
+          $access = AccessResult::allowed()->cachePerRole()->cacheUntilEntityChanges($entity);
+          if (!$entity->getUrl()) {
             // We allow access, but only if the link is accessible as well.
             $link_access = $this->accessManager->checkNamedRoute($entity->getRouteName(), $entity->getRouteParameters(), $account, TRUE);
-            return $access->allow()->andIf($link_access);
+            return $access->andIf($link_access);
           }
+          return $access;
         }
 
       case 'delete':

core/modules/node/node.api.php

@@ -348,7 +348,7 @@ function hook_node_access(\Drupal\node\NodeInterface $node, $op, \Drupal\Core\Se
 
     default:
       // No opinion.
-      return AccessResult::create();
+      return AccessResult::neutral();
   }
 }
 

core/modules/node/node.module

@@ -1042,7 +1042,7 @@ function node_node_access(NodeInterface $node, $op, $account) {
 
     default:
       // No opinion.
-      return AccessResult::create();
+      return AccessResult::neutral();
   }
 }
 

core/modules/node/src/Access/NodeAddAccessCheck.php

@@ -61,7 +61,7 @@ public function access(AccountInterface $account, NodeTypeInterface $node_type =
     }
 
     // No opinion.
-    return AccessResult::create();
+    return AccessResult::neutral();
   }
 
 }

core/modules/node/src/NodeAccessControlHandler.php

@@ -123,7 +123,7 @@ protected function checkAccess(EntityInterface $node, $operation, $langcode, Acc
     }
 
     // No opinion.
-    return AccessResult::create();
+    return AccessResult::neutral();
   }
 
   /**

core/modules/node/src/NodeGrantDatabaseStorage.php

@@ -59,7 +59,7 @@ public function access(NodeInterface $node, $operation, $langcode, AccountInterf
     // no point in querying the database for access grants.
     if (!$this->moduleHandler->getImplementations('node_grants') || !$node->id()) {
       // No opinion.
-      return AccessResult::create();
+      return AccessResult::neutral();
     }
 
     // Check the database for potential access grants.

core/modules/node/tests/modules/node_access_test/node_access_test.module

@@ -151,5 +151,5 @@ function node_access_test_node_access(\Drupal\node\NodeInterface $node, $op, \Dr
     return AccessResult::forbidden()->setCacheable(FALSE);
   }
   // No opinion.
-  return AccessResult::create()->setCacheable(FALSE);
+  return AccessResult::neutral()->setCacheable(FALSE);
 }

core/modules/quickedit/tests/src/Unit/Access/EditEntityFieldAccessCheckTest.php

@@ -49,7 +49,7 @@ public function providerTestAccess() {
     $non_editable_entity = $this->createMockEntity();
     $non_editable_entity->expects($this->any())
       ->method('access')
-      ->will($this->returnValue(AccessResult::create()->cachePerRole()));
+      ->will($this->returnValue(AccessResult::neutral()->cachePerRole()));
 
     $field_storage_with_access = $this->getMockBuilder('Dr