diff --git a/core/lib/Drupal/Component/Plugin/Exception/MissingValueContextException.php b/core/lib/Drupal/Component/Plugin/Exception/MissingValueContextException.php new file mode 100644 index 0000000000000000000000000000000000000000..67b5d92b251501cb5ab72bd9f3cc005e98b4a411 --- /dev/null +++ b/core/lib/Drupal/Component/Plugin/Exception/MissingValueContextException.php @@ -0,0 +1,21 @@ +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); } } diff --git a/core/modules/node/tests/src/Functional/NodeBlockFunctionalTest.php b/core/modules/node/tests/src/Functional/NodeBlockFunctionalTest.php index c203c5017a1c1802d0b1ceb53c61238bfa7d5d16..c6f786df8e34f53bcb77858dfc90ee64ed136678 100644 --- a/core/modules/node/tests/src/Functional/NodeBlockFunctionalTest.php +++ b/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'); diff --git a/core/tests/Drupal/Tests/Core/Plugin/ContextHandlerTest.php b/core/tests/Drupal/Tests/Core/Plugin/ContextHandlerTest.php index f1ca26ecf7fd8e515b839e7fd499e37b54f2168e..81a1ee9b775989a1403ec9bc3e6a2c45b5fb5881 100644 --- a/core/tests/Drupal/Tests/Core/Plugin/ContextHandlerTest.php +++ b/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); }