Issue #2521956 by Berdir, Fabianx, effulgentsia, andypost, lauriii, seanB, Wim...

Issue #2521956 by Berdir, Fabianx, effulgentsia, andypost, lauriii, seanB, Wim Leers, Upchuk: Missing contexts prevent caching of block access

core/lib/Drupal/Component/Plugin/Exception/MissingValueContextException.php

+<?php
+
+namespace Drupal\Component\Plugin\Exception;
+
+/**
+ * An exception class thrown when contexts exist but are missing a value.
+ */
+class MissingValueContextException extends ContextException {
+
+  /**
+   * MissingValueContextException constructor.
+   *
+   * @param string[] $contexts_without_value
+   *   List of contexts with missing value.
+   */
+  public function __construct(array $contexts_without_value = []) {
+    $message = 'Required contexts without a value: ' . implode(', ', $contexts_without_value);
+    parent::__construct($message);
+  }
+
+}

core/lib/Drupal/Core/Plugin/Context/ContextHandler.php

@@ -3,6 +3,7 @@
 namespace Drupal\Core\Plugin\Context;
 
 use Drupal\Component\Plugin\Exception\ContextException;
+use Drupal\Component\Plugin\Exception\MissingValueContextException;
 use Drupal\Core\Cache\CacheableDependencyInterface;
 use Drupal\Core\Plugin\ContextAwarePluginInterface;
 
@@ -93,15 +94,17 @@ public function applyContextMapping(ContextAwarePluginInterface $plugin, $contex
       }
     }
 
-    // If there are any required contexts without a value, throw an exception.
-    if ($missing_value) {
-      throw new ContextException(sprintf('Required contexts without a value: %s.', implode(', ', $missing_value)));
-    }
-
     // If there are any mappings that were not satisfied, throw an exception.
+    // This is a more severe problem than missing values, so check and throw
+    // this first.
     if (!empty($mappings)) {
       throw new ContextException('Assigned contexts were not satisfied: ' . implode(',', array_keys($mappings)));
     }
+
+    // If there are any required contexts without a value, throw an exception.
+    if ($missing_value) {
+      throw new MissingValueContextException($missing_value);
+    }
   }
 
 }

core/lib/Drupal/Core/Plugin/Context/ContextHandlerInterface.php

@@ -74,6 +74,9 @@ public function getMatchingContexts(array $contexts, ContextDefinitionInterface
    *
    * @throws \Drupal\Component\Plugin\Exception\ContextException
    *   Thrown when a context assignment was not satisfied.
+   * @throws \Drupal\Component\Plugin\Exception\MissingValueContextException
+   *   Thrown when a context is provided but has no value. Only thrown if
+   *   no contexts are missing.
    */
   public function applyContextMapping(ContextAwarePluginInterface $plugin, $contexts, $mappings = []);
 

core/modules/block/src/BlockAccessControlHandler.php

@@ -3,6 +3,7 @@
 namespace Drupal\block;
 
 use Drupal\Component\Plugin\Exception\ContextException;
+use Drupal\Component\Plugin\Exception\MissingValueContextException;
 use Drupal\Core\Access\AccessResult;
 use Drupal\Core\Cache\Cache;
 use Drupal\Core\Cache\CacheableDependencyInterface;
@@ -83,12 +84,16 @@ protected function checkAccess(EntityInterface $entity, $operation, AccountInter
     else {
       $conditions = [];
       $missing_context = FALSE;
+      $missing_value = FALSE;
       foreach ($entity->getVisibilityConditions() as $condition_id => $condition) {
         if ($condition instanceof ContextAwarePluginInterface) {
           try {
             $contexts = $this->contextRepository->getRuntimeContexts(array_values($condition->getContextMapping()));
             $this->contextHandler->applyContextMapping($condition, $contexts);
           }
+          catch (MissingValueContextException $e) {
+            $missing_value = TRUE;
+          }
           catch (ContextException $e) {
             $missing_context = TRUE;
           }
@@ -99,14 +104,15 @@ protected function checkAccess(EntityInterface $entity, $operation, AccountInter
       if ($missing_context) {
         // If any context is missing then we might be missing cacheable
         // metadata, and don't know based on what conditions the block is
-        // accessible or not. For example, blocks that have a node type
-        // condition will have a missing context on any non-node route like the
-        // frontpage.
-        // @todo Avoid setting max-age 0 for some or all cases, for example by
-        //   treating available contexts without value differently in
-        //   https://www.drupal.org/node/2521956.
+        // accessible or not. Make sure the result cannot be cached.
         $access = AccessResult::forbidden()->setCacheMaxAge(0);
       }
+      elseif ($missing_value) {
+        // The contexts exist but have no value. Deny access without
+        // disabling caching. For example the node type condition will have a
+        // missing context on any non-node route like the frontpage.
+        $access = AccessResult::forbidden();
+      }
       elseif ($this->resolveConditions($conditions, 'and') !== FALSE) {
         // Delegate to the plugin.
         $block_plugin = $entity->getPlugin();
@@ -117,12 +123,15 @@ protected function checkAccess(EntityInterface $entity, $operation, AccountInter
           }
           $access = $block_plugin->access($account, TRUE);
         }
+        catch (MissingValueContextException $e) {
+          // The contexts exist but have no value. Deny access without
+          // disabling caching.
+          $access = AccessResult::forbidden();
+        }
         catch (ContextException $e) {
-          // Setting access to forbidden if any context is missing for the same
-          // reasons as with conditions (described in the comment above).
-          // @todo Avoid setting max-age 0 for some or all cases, for example by
-          //   treating available contexts without value differently in
-          //   https://www.drupal.org/node/2521956.
+          // If any context is missing then we might be missing cacheable
+          // metadata, and don't know based on what conditions the block is
+          // accessible or not. Make sure the result cannot be cached.
           $access = AccessResult::forbidden()->setCacheMaxAge(0);
         }
       }

core/modules/node/tests/src/Functional/NodeBlockFunctionalTest.php

@@ -141,15 +141,32 @@ public function testRecentNodeBlock() {
     $label = $block->label();
     $this->assertNoText($label, 'Block was not displayed on the front page.');
     $this->assertCacheContexts(['languages:language_content', 'languages:language_interface', 'theme', 'url.query_args:' . MainContentViewSubscriber::WRAPPER_FORMAT, 'user', 'route']);
+
+    // Ensure that a page that does not have a node context can still be cached,
+    // the front page is the user page which is already cached from the login
+    // request above.
+    $this->assertSame('HIT', $this->getSession()->getResponseHeader('X-Drupal-Dynamic-Cache'));
+
     $this->drupalGet('node/add/article');
     $this->assertText($label, 'Block was displayed on the node/add/article page.');
     $this->assertCacheContexts(['languages:language_content', 'languages:language_interface', 'session', 'theme', 'url.path', 'url.query_args', 'user', 'route']);
+
+    // The node/add/article page is an admin path and currently uncacheable.
+    $this->assertSame('UNCACHEABLE', $this->getSession()->getResponseHeader('X-Drupal-Dynamic-Cache'));
+
     $this->drupalGet('node/' . $node1->id());
     $this->assertText($label, 'Block was displayed on the node/N when node is of type article.');
     $this->assertCacheContexts(['languages:language_content', 'languages:language_interface', 'theme', 'url.query_args:' . MainContentViewSubscriber::WRAPPER_FORMAT, 'user', 'route', 'timezone']);
+    $this->assertSame('MISS', $this->getSession()->getResponseHeader('X-Drupal-Dynamic-Cache'));
+    $this->drupalGet('node/' . $node1->id());
+    $this->assertSame('HIT', $this->getSession()->getResponseHeader('X-Drupal-Dynamic-Cache'));
+
     $this->drupalGet('node/' . $node5->id());
     $this->assertNoText($label, 'Block was not displayed on nodes of type page.');
     $this->assertCacheContexts(['languages:language_content', 'languages:language_interface', 'theme', 'url.query_args:' . MainContentViewSubscriber::WRAPPER_FORMAT, 'user', 'route', 'timezone']);
+    $this->assertSame('MISS', $this->getSession()->getResponseHeader('X-Drupal-Dynamic-Cache'));
+    $this->drupalGet('node/' . $node5->id());
+    $this->assertSame('HIT', $this->getSession()->getResponseHeader('X-Drupal-Dynamic-Cache'));
 
     $this->drupalLogin($this->adminUser);
     $this->drupalGet('admin/structure/block');

core/tests/Drupal/Tests/Core/Plugin/ContextHandlerTest.php

@@ -9,6 +9,7 @@
 
 use Drupal\Component\Plugin\ConfigurablePluginInterface;
 use Drupal\Component\Plugin\Exception\ContextException;
+use Drupal\Component\Plugin\Exception\MissingValueContextException;
 use Drupal\Core\Cache\NullBackend;
 use Drupal\Core\DependencyInjection\ClassResolverInterface;
 use Drupal\Core\DependencyInjection\ContainerBuilder;
@@ -336,7 +337,7 @@ public function testApplyContextMappingMissingRequired() {
     $plugin->expects($this->never())
       ->method('getContext');
 
-    $this->setExpectedException(ContextException::class, 'Required contexts without a value: hit.');
+    $this->setExpectedException(MissingValueContextException::class, 'Required contexts without a value: hit');
     $this->contextHandler->applyContextMapping($plugin, $contexts);
   }
 
@@ -404,7 +405,7 @@ public function testApplyContextMappingNoValueRequired() {
     $plugin->expects($this->never())
       ->method('setContextValue');
 
-    $this->setExpectedException(ContextException::class, 'Required contexts without a value: hit.');
+    $this->setExpectedException(MissingValueContextException::class, 'Required contexts without a value: hit');
     $this->contextHandler->applyContextMapping($plugin, $contexts);
   }