summaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authorNathaniel Catchpole2018-06-08 11:12:00 (GMT)
committerNathaniel Catchpole2018-06-08 11:30:33 (GMT)
commit81a13c6436614c013317a12834c934cabc2d7345 (patch)
treeaedef4dbf9b9766064d7153511eed16e372a4a62
parent007415045b90af5c9b0e30cfbabee7f26e5df713 (diff)
Issue #2521956 by Berdir, Fabianx, effulgentsia, andypost, lauriii, seanB, Wim Leers, Upchuk: Missing contexts prevent caching of block access
-rw-r--r--core/lib/Drupal/Component/Plugin/Exception/MissingValueContextException.php21
-rw-r--r--core/lib/Drupal/Core/Plugin/Context/ContextHandler.php13
-rw-r--r--core/lib/Drupal/Core/Plugin/Context/ContextHandlerInterface.php3
-rw-r--r--core/modules/block/src/BlockAccessControlHandler.php31
-rw-r--r--core/modules/node/tests/src/Functional/NodeBlockFunctionalTest.php17
-rw-r--r--core/tests/Drupal/Tests/Core/Plugin/ContextHandlerTest.php5
6 files changed, 72 insertions, 18 deletions
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 0000000..67b5d92
--- /dev/null
+++ b/core/lib/Drupal/Component/Plugin/Exception/MissingValueContextException.php
@@ -0,0 +1,21 @@
+<?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);
+ }
+
+}
diff --git a/core/lib/Drupal/Core/Plugin/Context/ContextHandler.php b/core/lib/Drupal/Core/Plugin/Context/ContextHandler.php
index d276a55..f174d46 100644
--- a/core/lib/Drupal/Core/Plugin/Context/ContextHandler.php
+++ b/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 @@ class ContextHandler implements ContextHandlerInterface {
}
}
- // 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);
+ }
}
}
diff --git a/core/lib/Drupal/Core/Plugin/Context/ContextHandlerInterface.php b/core/lib/Drupal/Core/Plugin/Context/ContextHandlerInterface.php
index a0d5703..d524dcf 100644
--- a/core/lib/Drupal/Core/Plugin/Context/ContextHandlerInterface.php
+++ b/core/lib/Drupal/Core/Plugin/Context/ContextHandlerInterface.php
@@ -74,6 +74,9 @@ interface ContextHandlerInterface {
*
* @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 = []);
diff --git a/core/modules/block/src/BlockAccessControlHandler.php b/core/modules/block/src/BlockAccessControlHandler.php
index 2652251..e0abc0d 100644
--- a/core/modules/block/src/BlockAccessControlHandler.php
+++ b/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 @@ class BlockAccessControlHandler extends EntityAccessControlHandler implements En
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 @@ class BlockAccessControlHandler extends EntityAccessControlHandler implements En
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 @@ class BlockAccessControlHandler extends EntityAccessControlHandler implements En
}
$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 c203c50..c6f786d 100644
--- a/core/modules/node/tests/src/Functional/NodeBlockFunctionalTest.php
+++ b/core/modules/node/tests/src/Functional/NodeBlockFunctionalTest.php
@@ -141,15 +141,32 @@ class NodeBlockFunctionalTest extends NodeTestBase {
$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 f1ca26e..81a1ee9 100644
--- a/core/tests/Drupal/Tests/Core/Plugin/ContextHandlerTest.php
+++ b/core/tests/Drupal/Tests/Core/Plugin/ContextHandlerTest.php
@@ -9,6 +9,7 @@ namespace Drupal\Tests\Core\Plugin;
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 @@ class ContextHandlerTest extends UnitTestCase {
$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 @@ class ContextHandlerTest extends UnitTestCase {
$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);
}