Condition plugins should provide cache contexts AND cacheability metadata needs to be exposed

This blocks #2429287: [meta] Finalize the cache contexts API & DX/usage, enable a leap forward in performance.

Ok, this is a very naive first attempt, no testing additions or anything, mostly curious to see how it tests.

Rationale:

1.) The CacheableMetadata class had methods I wanted and needed, I couldn't extend from it because I needed to add these into the core plugin context class, so I extracted an Interface from it. The static methods proved to be problematic. If we like this approach we should address that.

2.) Instead of implementing this directly on conditions (which I did as well) I actually took it up a layer and implemented it on the core plugin Context object. This is REALLY nice because it means that when the context is set (through the event system or anything contrib devises) we can programmatically set tags/contexts/max-age as desired right then regardless of the actual context value object.

The specific implementation should probably change a bit. And I'm sure the interfaces here will change, but I think this is an ok first attempt just to see where it goes.

Eclipse

1. +++ b/.htaccess
   @@ -176,3 +176,4 @@ AddEncoding gzip svgz
   +php_value display_errors 1
   

   ?

2. +++ b/core/lib/Drupal/Core/Cache/CacheableMetadata.php
   @@ -11,7 +11,7 @@
   -class CacheableMetadata implements CacheableDependencyInterface {
   +class CacheableMetadata implements CacheableMetadataInterface {
   

   1) this new interface is missing from the patch
   2) this is intended to be a value object; it's not using an interface for good reason: A) for the interface to be of use, there'd need to be (yet another) a layer of indirection to instantiate whatever preferred implementation of that interface a site wanted to use, B) the set of cacheability metadata does not actually change; it is controlled by Drupal. I.e. value objects are sufficient.

3. +++ b/core/lib/Drupal/Core/Plugin/Context/Context.php
   @@ -10,13 +10,17 @@
   +class Context extends ComponentContext implements ContextInterface, CacheableMetadataInterface {
   

   s/CacheableMetadataInterface/CacheableDependencyInterface/

4. +++ b/core/lib/Drupal/Core/Plugin/Context/Context.php
   @@ -35,6 +39,27 @@ class Context extends ComponentContext implements ContextInterface {
   +  protected $contexts = [];
   

   A Context class with a $contexts property. Super confusing.

   I'd prefix all of these new properties with "cache".

5. +++ b/core/lib/Drupal/Core/Plugin/Context/Context.php
   @@ -97,4 +122,118 @@ public function validate() {
   +  public function getCacheContexts() {
   +    $context = $this->getContextValue();
   +    $contexts = $this->contexts;
   +    if ($context instanceof CacheableDependencyInterface) {
   +      $contexts = Cache::mergeContexts($this->contexts, $context->getCacheContexts());
   +    }
   +    return $contexts;
   +  }
   

   I don't understand this logic.

   AFAICT, getContextValue() always returns the same value. Yet this code keeps updating the stored cache contexts with the cache contexts of getContextValue().

   Why not replace all of this with:

   public function getCacheContexts() {
     $context = $this->getContextValue();
     if ($context instanceof CacheableDependencyInterface) {
       return $context->getCacheContexts();
     }
     return [];
   }
   

   And similarly for the other methods.

6. +++ b/core/lib/Drupal/Core/Plugin/Context/Context.php
   @@ -97,4 +122,118 @@ public function validate() {
   +  public function addCacheTags(array $cache_tags) {
   ...
   +  public function setCacheTags(array $cache_tags) {
   ...
   +  public function addCacheContexts(array $cache_contexts) {
   ...
   +  public function setCacheContexts(array $cache_contexts) {
   ...
   +  public function setCacheMaxAge($max_age) {
   ...
   +  public function merge(CacheableMetadataInterface $other) {
   ...
   +  public function applyTo(array &$build) {
   ...
   +  public static function createFromRenderArray(array $build) {
   ...
   +  public static function createFromObject($object) {
   

   So… these methods are I guess the reason why you're storing cacheability metadata on the object. But why? Why do you need to be able to *set* cacheability metadata on a context? Why doesn't the context value already provide the necessary data?

   If there really is a reason for this, then IMO the right way to do this is not by duplicating all of this logic, but by adding a CacheableMetadata object as a property on this class, and aliasing these methods to that class.

7. +++ b/core/modules/block/src/EventSubscriber/NodeRouteContext.php
   @@ -43,6 +43,7 @@ public function onBlockActiveContext(BlockContextEvent $event) {
   +        $context->addCacheContexts(['request']);
   

   Aha, so here's the first example. (Note that what you actually meant was "vary by route", not "vary by request" — so you wanted 'route', not 'request').

   Wouldn't it be much better to instead to have a Context::setCacheability(CacheableMetadata $cacheability) method? Then all of the complexity above can be drastically simplified. Composition rather than repetition.

Yeah like I said, super naive first attempt. Given your points in 7, which I think are great, I'll work on another patch.

Eclipse

Wim++

Most of your review didn't matter after I adopted the approach from comment 3.7.

All around I think a much better approach. I didn't populate this out to all the EventSubscribers that provide contexts, but there aren't many, I want to see if we like this approach first.

Eclipse

Not trying to fix test issue yet, just fleshing this out a bit more.

Eclipse

Ok, fixing some failures.

Eclipse

FWIW, All callers for far have been merging the render array into an object, then applying it in the end.

e.g. something like:

BubbleableMetadata::createFromRenderArray($build)
  ->merge($object)
  ->applyTo($build);

I did never like that DX though ...

I want to stress that the reason #3.7 makes sense, is that contexts apparently don't have *inherent* cacheability. As is shown by the change in NodeRouteContext, the cacheability is *external* to the context. Therefore it makes sense that it doesn't implement CacheableDependencyInterface, but instead has a CacheableMetadata property.

1. +++ b/core/lib/Drupal/Core/Block/BlockBase.php
   @@ -303,21 +304,42 @@ public function setTransliteration(TransliterationInterface $transliteration) {
   +    $cache_contexts =[];
   ...
   +    $tags =[];
   

   Missing space.

2. +++ b/core/lib/Drupal/Core/Cache/CacheableMetadata.php
   @@ -153,9 +153,9 @@ public function merge(CacheableMetadata $other) {
      public function applyTo(array &$build) {
   -    $build['#cache']['contexts'] = $this->contexts;
   -    $build['#cache']['tags'] = $this->tags;
   -    $build['#cache']['max-age'] = $this->maxAge;
   +    $build['#cache']['contexts'] = isset($build['#cache']['contexts']) ? Cache::mergeContexts($build['#cache']['contexts'], $this->contexts) : $this->contexts;
   +    $build['#cache']['tags'] = isset($build['#cache']['tags']) ? Cache::mergeTags($build['#cache']['tags'], $this->tags) : $this->tags;
   +    $build['#cache']['max-age'] = isset($build['#cache']['max-age']) ? Cache::mergeMaxAges($build['#cache']['max-age'], $this->maxAge) : $this->maxAge;
      }
   

   This is a big API change.

   See #14 on how this is intended to be used. Changing that is out of scope for this issue.

3. +++ b/core/lib/Drupal/Core/Plugin/Context/ContextInterface.php
   @@ -32,4 +33,22 @@ public function getContextData();
   +   * @return \Drupal\Core\Cache\CacheableMetadata|NULL
   

   When can this be NULL?

4. +++ b/core/modules/block/src/EventSubscriber/CurrentUserContext.php
   @@ -53,9 +54,12 @@ public function __construct(AccountInterface $account, EntityManagerInterface $e
   +    $cache = CacheableMetadata::createFromObject($current_user);
   +    $cache->addCacheContexts(['user']);
   
   +++ b/core/modules/block/src/EventSubscriber/NodeRouteContext.php
   @@ -43,13 +44,20 @@ public function onBlockActiveContext(BlockContextEvent $event) {
   +        $cache = CacheableMetadata::createFromObject($node);
   +        $cache->addCacheContexts(['route']);
   +        $context->setCacheableMetadata($cache);
   ...
   +      $cache = CacheableMetadata::createFromObject($node);
   +      $cache->addCacheContexts(['route']);
   +      $context->setCacheableMetadata($cache);
   

   Much better :)

Ok, I based this patch off of my work in 9 because I think tim's work in 12 was attempting to expose how we changed the applyTo() method stuff. If this comes back with the same errors my apologies.

Basically I just tried to fix all the stuff Wim outlined in 15 and incorporate Fabien's howto in 14.

Eclipse

Almost there :) Still needs tests, integration tests (probably KTB), to verify that a block that depends on contexts gets the cacheability metadata of the contexts applied.

1. +++ b/core/lib/Drupal/Core/Plugin/Context/Context.php
   @@ -53,6 +62,12 @@ public function getContextValue() {
      public function setContextValue($value) {
   +    if ($value instanceof CacheableDependencyInterface) {
   +      $this->setCacheableMetadata(CacheableMetadata::createFromObject($value));
   +    }
   +    else {
   +      $this->setCacheableMetadata(new CacheableMetadata());
   +    }
   
   @@ -97,4 +112,20 @@ public function validate() {
   +  public function getCacheableMetadata() {
   +    return $this->cache;
   +  }
   

   Doesn't this mean the getter may still return NULL? Or is it somehow guaranteed that ::setContextValue() will be called?

   Also, the quoted if/else can be replaced by simply

   $this->setCacheableMetadata(CacheableMetadata::createFromObject($value));
   

   ::createFromObject() already handles the case that the CacheableDependencyInterface is not implemented.

2. +++ b/core/lib/Drupal/Core/Plugin/Context/ContextInterface.php
   @@ -32,4 +33,22 @@ public function getContextData();
      public function setContextData(TypedDataInterface $data);
    
   +
   +  /**
   

   Extraneous newline.

#17: Yes and no. Would that not mean that e.g. passing in an entity would set max-age = 0 as if the object does not implement CacheableDependencyInterface we deem it not cacheable, but that is not the case here as it is very generic.

I agree though the the NULL case should be fixed unless we depend on setContextValue() being called first in which case an assert() would be nice to use here ...

This is now actively blocking #2429617: Make D8 2x as fast: Dynamic Page Cache: context-dependent page caching (for *all* users!): it causes the test failures in BlockTest. The per-role visibility should add the user.roles cache context, the request path visibility should add the url cache context.

(The request path visibility condition could in the future provide an optimized cache context, but for now url will do.)

I was wrong. Turns out this is not directly blocking #2429617: Make D8 2x as fast: Dynamic Page Cache: context-dependent page caching (for *all* users!), but #2495171: Block access results' cacheability metadata is not applied to the render arrays is.

This issue is still necessary if we want any blocks using visibility conditions to be render cached at all.

This only partially belongs here, but...

I've actually had the problem with missing condition contexts in my project, what I've done is making the block plugin responsible for returning cache keys according to the injected context. See #2287073: Allow views contextual filters to expose the context using argument validation plugins for an example. That's just cache keys and not contexts, so no bubbling, so it doesn't work for smart cache. Also not for conditions, obviously...

Wondering how to properly implement cache contexts for #2284005: Implement static contexts. I'm using it to display many instances of the same views block with different arguments as part of the same page. How is a context like that supposed to define cache context? There can be many of those and they are based on configuration.

The only thing I can imagine right now is a dummy cache context that just passes through anything as as and then using UUID's (so the cache context would be page_manager.static:node:UUID and the context class would just return that 1:1. So basically bubbling cache keys ;)

Just a re-roll and trying to fix the non-null issue. Given my change, I'm wondering if we really need setCacheMetadata() ? Isn't there a risk of replacing existing metadata? Why not just only have get() and always require the caller to merge?

Interesting point on the max age problem but what about cache tags, for example? Using a context with a node should also result in adding that cache tag?

Also, does this mean we can remove the hardcoded max age form the block access control handler? Wondering what will happen with tests when I do that.

This is a hard blocker for #2495171: Block access results' cacheability metadata is not applied to the render arrays, which is critical and as accepting a max-age=0 for all blocks (effectively disabling block caching) is not a good option to ship Drupal 8, hence this is also critical ... (sorry @stats)

+++ b/core/lib/Drupal/Core/Plugin/Context/Context.php
@@ -70,10 +70,8 @@ public function getContextValue() {
+      $this->getCacheableMetadata()
+        ->merge(CacheableMetadata::createFromObject($value));

That is problematic, because CacheableMetadata objects are partially immutable and merge returns a new object, so this is essentially a no-op, which means we seem to miss test-coverage.

(https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Cache%21C...)

Oh, so we do need the set, should we make that so that it merges an existing object? Is that intuitive and always desired?

We're not caching the access result right now, we always check that, so this would only break when combined with the smartcache patch I assume.

Lets add a mergeCacheableMetadata() function instead? I don't think set() should merge ...

#27: Oh, okay makes sense then.

#22: I commented on #2287073: Allow views contextual filters to expose the context using argument validation plugins and #2284005: Implement static contexts. Thanks for the links. Let's keep those discussions in those issues.

#23: Using a context with a node should also result in adding that cache tag? — this is indeed an excellent question, and one that came to mind while looking at those two aforementioned issues as well. I think the answer is: it depends on how that context is used. If you e.g. get a user entity from a route parameter, then if you're just showing "nodes created by this user", then the user's cache tag is not needed, because you don't show anything from the user. But if the title of the view would be something like "Bob's posts", then you're using the user entity's name, and then it's up to the part that is render "Bob's posts" to mark the user entity as a cacheable dependency.

#26 is right, this is basically a no-op. I think the solution to this problem is quite simple:

+++ b/core/lib/Drupal/Core/Plugin/Context/Context.php
@@ -113,4 +126,23 @@ public function validate() {
+  public function setCacheableMetadata(CacheableMetadata $cache) {

IMHO this should be replaced with addCacheableDependency(), just like we have on \Drupal\Core\Cache\CacheableResponseInterface::addCacheableDependency() and \Drupal\Core\Render\RendererInterface::addCacheableDependency().

That'd remove all confusion.

cache tags: Make sense. We're adding cache metadata for the context value. The context value of the current user doesn't change when the user is saved again. Note that this *will* change when we build UI's that will understand complex data structure, like rules will and page_manager/panels eventually too. (E.g you don't pass along the uid but a certain field value). Because then, the cache tag becomes relevant again. Not sure who will be responsible for adding it at that point. But that's a different problem.

I almost 1:1 copied the CacheableResponseInterface + trait now. Agree that the new names make more sense.

Still no tests.

+++ b/core/modules/block/src/EventSubscriber/NodeRouteContext.php
@@ -43,6 +44,8 @@ public function onBlockActiveContext(BlockContextEvent $event) {
+        $cache = $context->getCacheableMetadata();
+        $cache->addCacheContexts(['route']);
       }
       $event->setContext('node.node', $context);

@@ -50,6 +53,8 @@ public function onBlockActiveContext(BlockContextEvent $event) {
+      $cache = $context->getCacheableMetadata();
+      $cache->addCacheContexts(['route']);
       $event->setContext('node.node', $context);

What is these local variables doing since the cache metadata doesn't seem to get set anytime?

What is these local variables doing since the cache metadata doesn't seem to get set anytime?

I think the point is that those bits depend on the route match object and so upon the route

Just a nitpick

+++ b/core/lib/Drupal/Core/Plugin/Context/Context.php
@@ -35,6 +37,12 @@ class Context extends ComponentContext implements ContextInterface {
+  protected $cacheabilityMetadata;
+  /**

newline missing

So, having read through the interdiffs berdir's provided here, I have nothing to complain about, this all seems super sensible to me. A quick comment.

@Wim re #30:

Yes, the default implementation would assume that if you need a context, we should cache by it. I think that's a pretty sane default, and individual plugins consuming context can override it as necessary. That should place the burden of understanding how caching works on people who actually need to understand it because the default didn't work for them.

And a review question:

+++ b/core/modules/block/src/BlockAccessControlHandler.php
@@ -105,13 +105,8 @@ protected function checkAccess(EntityInterface $entity, $operation, $langcode, A
+      return $access->cacheUntilEntityChanges($entity);

Doesn't this need to get all the contexts used in this block (block and conditions) and cache based on that instead of just the block entity? Or is there some magic on the block entity that bubbles that up properly?

Eclipse

@EcliseGc: That line just adds the default. Each context/condition/access check will add additional stuff as necessary, like this does for node/user conditions.

Wrote some unit tests for Context.

Almost there :) Still needs tests, integration tests (probably KTB), to verify that a block that depends on contexts gets the cacheability metadata of the contexts applied.

Except that doesn't really happen right now. Not without #2495171: Block access results' cacheability metadata is not applied to the render arrays. Or at least it's not visible, because access cache metadata is not bubbled up.

Which means its quite hard to write tests for it. We could maybe write a unit test for the two context class for that additional line, but they don't have any unit tests at all right now so that would be quite complex. And for a kernel test, we need to create fake route matches and so on as well.

I'm wondering if we shouldn't just merge that issue into this one, it's blocked on this already, is very related only actually does anything in combination with that anyway. Web tests are easy then (we already have some over there now in PageCacheIntegrationTest).

Here are the reasons/thoughts why I think it would make sense to merge those two issues/patches into one:

* Despite what Wim wrote over there, there is no actual security issue with block access in HEAD, this would only be a problem when combined with smartcache.

* The condition context patch isn't complete without the other one. Those contexts are not actually visible anywhere, because they get lost.

* That fact, as written above, also makes it very hard to write meaningful tests. We would have to set up relatively complex unit or kernel tests, with a lot of faked objects/complex moking. Combined, it's very easy to add some assertions on the page cache context level. That's not perfect, but I think enough to get it committed.. we could improve unit test coverage in a normal follow-up anytime later.. and smartcache will I guess also result in considerable additional indirect test coverage.

* They are closely related, block access is blocked on this and pretty small (15kb and 2/3 of that is unit test fixes).

Suggested issue title: "Condition plugins should provide cache contexts and be exposed together with block access cache metadata in render arrays" (Shorter would also be great ;))

Doing as #36 said, lets merge #2495171: Block access results' cacheability metadata is not applied to the render arrays into this one, no need to to have two criticals that are just dependency-chain.

I'm looking into writing tests for this but there are actually quite a few things missing it seems...

In HEAD, we only use contexts on blocks indirectly, through visibility conditions. We however only add context cache contexts from BlockBase, but that doesn't know about the visibility conditions at all. Also the code there is bogus as well, because Context isn't using CacheableDependencyInterface, it uses methods for CacheableMetadata.

A bit confused by all this right now.

And then there's the next confusing part... take NodeRouteContext for example. It adds the cache context to the context object (too much context), if it is able to create one. But isn't the point of cache context that they are always added when relevant? But that's not possible because if there's no route that matches our two cases, we can't create a context object so how could we pass along the context?

Maybe, instead of context, it's the ConditionInterface *and* the classes/services that define context that need to expose cache metadata, not really contexts?

Need to discuss this with Wim & Fabianx. I have some local code, so assigning to me for now.

Right, it's technically possible, but there's no UI for that in core (yet). Not suggesting to remove that, but it's also definitely not working yet, due to the CacheableDependency/CacheableMetadata mismatch.

But isn't the point of cache context that they are always added when relevant? But that's not possible because if there's no route that matches our two cases, we can't create a context object so how could we pass along the context?

This gets at the heart of the problem.

Let's look at the code:

  public function onBlockActiveContext(BlockContextEvent $event) {
    if (($route_object = $this->routeMatch->getRouteObject()) && ($route_contexts = $route_object->getOption('parameters')) && isset($route_contexts['node'])) {
      $context = new Context(new ContextDefinition($route_contexts['node']['type']));
      if ($node = $this->routeMatch->getParameter('node')) {
        $context->setContextValue($node);
      }
      // THIS IS WHERE THE PATCH ADDS A CACHE CONTEXT
      $event->setContext('node.node', $context);
    }
    elseif ($this->routeMatch->getRouteName() == 'node.add') {
      $node_type = $this->routeMatch->getParameter('node_type');
      $context = new Context(new ContextDefinition('entity:node'));
      $context->setContextValue(Node::create(array('type' => $node_type->id())));
      // THIS IS WHERE THE PATCH ADDS A CACHE CONTEXT
      $event->setContext('node.node', $context);
    }
  }

NodeRouteContext ALWAYS depends on the route, because it uses the current route to determine which logic to use. So, the route cache context should be added unconditionally.
And the problem is that the cache context is used only if we enter either the if-branch or the elseif-branch. i.e. when we enter the (not visible, but actually-there) else-branch, we should still add the route cache context. That looks like quite a big problem indeed.

So..

This isn't pretty yet, but my test is green and that's a start. Requires lots of changes in BlockAccessControlHandler, ContextHandler and some Context internals, but I don't see many other ways. More details below.

Note that this also includes the block access patch from the other issue.

Some of the changes, in no specific order:

* ConditionInterface implements CacheableDependencyInterface now, with almost identical implementation in ConditionPluginBase as BlockBase
* ContextInterface also extends CachealeDependencyInterface, removed getCacheableMetadata(), still using that internally.
* Removed the exception in getContextValue(). This makes no sense, because it only works as the provided context is required and throws an exception there. This is also against the documention, which says it returns NULL and doesn't say anything about an exception.
* Started to refactor ContextHandler to explicitly check for required on the plugin context definition and check for a context value. Also throws an exception, but I really dislike that we are using exceptions there. Throwing an exception on /user when you have a block with a node type visibility condition is *bad*. I think that method should just return a boolean. What this refactoring allows is to still assign all context and pass through the cacheability metadata.. which gets me to the next point.
* ContextAwarePluginBase and ContextHandler only passed the context *value* to the plugins. That means they never got the cacheability metadata, which made all the existing code completely bogus :) I added code to explicitly pass that through to the context objects on the plugins as well.
* BlockAccessControlHandler refactored so that we don't abort on an exception but continue to build all conditions with as much context as we have available. Then collect cacheability metadata and add it to the access result. Still have the max-age: 0, which prevents caching the fact that a node-type-visibility-condition block is not visible on /user. Which kills smart-cache on that page.. which is.. you guessed it.. *bad*.
* Changed NodeRouteContext to always return a Context object, even if there's no matching route, so that we can return the cacheability metadata. Making the type dynamic in one case when it was hardcoded in two others made no sense anyway :)

Expecting lots of unit test fails and probably some kernel/web test fails too because of the context changes, too late to look into that now, wanted to post what I had so far.

I think the most important part is getting rid of the max-age in as many cases as possible. We could just drop it now, then it's the responsibility of context providers to ensure that they always return a context object with cacheability metadata if applicable, or we could just do it on missing values vs. missing contexts. Then we need two different exceptions or even better, no exceptions at all for those scenarios.

PS: Sorry for the ranting about exceptions and stuff. I feel better now, thanks for asking :)

Fixing the fatal errors and unit test fails.

The general reasoning in #42 makes sense to me, but it's very hard for me to judge the details, since I don't know the Context/Condition system well enough. I'm hopeful that @tim.plunkett is able to review this better than I could — he's also using this in D8 contrib.

(With all due respect: I find Context/Condition quite dense and confusing, and lack of docs and the fact that there are e.g. two ContextAwarePluginBase classes and more such oddities don't help. I think docs explaining the architecture would be extremely valuable.)

Also want to call out the irony here: since conditions and contexts are all about providing things (plugins) with dependency information, it is quite ironic that we're now seeing problems with how conditions/contexts themselves don't have enough dependency information :P
(I've been saying this since October last year: #2349679-4: Support registration of global context.)

This fixes BlockLanguageTest in two different ways :)

The test was actually broken because the visibility condition was incomplete, it was missing the context mapping. So it was not able to run the block properly, was throwing a random exception somewhere which resulted in the block being hidden. Since the test method was just about making sure that the block configuration was updated, this wasn't failing anything.

With the new code flow, I have to add one more check for missing context, when there's no mapping *and* the plugin and provided context ID's don't match. I also updated the test to configure the block properly.

One more unit test fixed.

Ok. Added some unit test mocks and assertions to ContextHandlerTest for the new required checks and cache metadata. Also did a code review and removed some parts that I think are not required. We no longer need the part that was adding visibility conditions in BlockViewBuilder and I think adding block cache metadata in case of an access denied is not needed, I was just experimenting there.

I'm also attaching a patch that combined this with the smartcache patch. This fixes 3 of the 4 test fails in BlockTest. The last one is a test-only scenario I think, since it's an access check that just depends on state. So I'm adding a cache tag invalidation there for the block, which I think is actually a useful test on it's own. With that, BlockTest is green. Let's see if it helps with other test fails as well.

Oh, almost forgot. I think we should open a follow-up issue to deal with the exception code and the max-age 0. It's not a blocker for smartcache but it will prevent smartcache from doing anything useful on any non-node page as soon as you have a block with a node type visibility condition, so it's major I think.

Wow, amazing work! I know @tim.plunkett is deep in #2263569: Bypass form caching by default for forms using #ajax. — but I'm sure that once he has time, he'll start reviewing this :)

1. +++ b/core/lib/Drupal/Component/Plugin/Context/Context.php
   @@ -57,8 +57,7 @@ public function getContextValue() {
          if (!isset($default_value) && $definition->isRequired()) {
   -        $type = $definition->getDataType();
   -        throw new ContextException(sprintf("The %s context is required and not present.", $type));
   +        return NULL;
          }
   

   What about having two different return values: One for required contexts and one for optionals? I think we should at least document things properly

2. +++ b/core/lib/Drupal/Core/Block/BlockBase.php
   index e0b6115..c53059d 100644
   --- a/core/lib/Drupal/Core/Condition/ConditionInterface.php
   
   --- a/core/lib/Drupal/Core/Condition/ConditionInterface.php
   +++ b/core/lib/Drupal/Core/Condition/ConditionInterface.php
   
   +++ b/core/lib/Drupal/Core/Condition/ConditionInterface.php
   +++ b/core/lib/Drupal/Core/Condition/ConditionInterface.php
   @@ -9,6 +9,7 @@
   
   @@ -9,6 +9,7 @@
    
    use Drupal\Component\Plugin\ConfigurablePluginInterface;
    use Drupal\Component\Plugin\PluginInspectionInterface;
   +use Drupal\Core\Cache\CacheableDependencyInterface;
    use Drupal\Core\Executable\ExecutableInterface;
    use Drupal\Core\Executable\ExecutableManagerInterface;
    use Drupal\Core\Plugin\PluginFormInterface;
   @@ -46,7 +47,7 @@
   
   @@ -46,7 +47,7 @@
     *
     * @ingroup plugin_api
     */
   -interface ConditionInterface extends ExecutableInterface, PluginFormInterface, ConfigurablePluginInterface, PluginInspectionInterface {
   +interface ConditionInterface extends ExecutableInterface, PluginFormInterface, ConfigurablePluginInterface, PluginInspectionInterface, CacheableDependencyInterface {
    
      /**
       * Determines whether condition result will be negated.
   

   I'm curious, do we want to add the CacheableDependencyInterface to every condition plugin, or just some of them, so make the interface optional ...

3. +++ b/core/lib/Drupal/Core/Plugin/Context/ContextHandler.php
   @@ -74,14 +75,40 @@ public function getMatchingContexts(array $contexts, ContextDefinitionInterface
   +        $missing_value[] = $plugin_context_id;
   

   $missing_contexts maybe a better name?

4. +++ b/core/lib/Drupal/Core/Plugin/Context/ContextInterface.php
   @@ -32,4 +33,22 @@ public function getContextData();
    
   +  /**
   +   * Adds a dependency on an object: merges its cacheability metadata.
   +   *
   +   * E.g. when a context depends on some configuration, an entity, or an access
   +   * result, we must make sure their cacheability metadata is present on the
   +   * response. This method makes doing that simple.
   +   *
   +   * @param \Drupal\Core\Cache\CacheableDependencyInterface|mixed $dependency
   +   *   The dependency. If the object implements CacheableDependencyInterface,
   +   *   then its cacheability metadata will be used. Otherwise, the passed in
   +   *   object must be assumed to be uncacheable, so max-age 0 is set.
   +   *
   +   * @return $this
   +   *
   +   * @see \Drupal\Core\Cache\CacheableMetadata::createFromObject()
   +   */
   +  public function addCacheableDependency($dependency);
   

   This sounds like a more generic concept. Should we extract that into its own interface?

5. +++ b/core/modules/block/src/BlockAccessControlHandler.php
   @@ -87,31 +90,58 @@ protected function checkAccess(EntityInterface $entity, $operation, $langcode, A
              catch (ContextException $e) {
   -            return AccessResult::forbidden()->setCacheMaxAge(0);
   +            $missing_context = TRUE;
              }
   

   Yeah its a little bit odd how we use exceptions here in general, context api is odd, because its not something which happens rarely, but rather it can happen easily as part of a normal page flow, right?

6. +++ b/core/modules/block/src/BlockAccessControlHandler.php
   @@ -87,31 +90,58 @@ protected function checkAccess(EntityInterface $entity, $operation, $langcode, A
   +        // @todo Find a reliable way to avoid max-age 0 in all or more cases.
   +        //   Treat missing context vs. context without value as a different
   +        //   exception, for example.
   +        $access = AccessResult::forbidden()->setCacheMaxAge(0);
   +      }
   +      elseif ($this->resolveConditions($conditions, 'and') !== FALSE) {
   

   Agreed, its also hard to understand why or under which circumstances max-age has to be 0

7. +++ b/core/modules/block/src/BlockAccessControlHandler.php
   @@ -87,31 +90,58 @@ protected function checkAccess(EntityInterface $entity, $operation, $langcode, A
   +  protected function mergeCacheabilityFromConditions($conditions, AccessResult $access) {
   

   let's use array $conditions ... I'd expected to be $access the first parameter ...

8. +++ b/core/modules/block/src/BlockAccessControlHandler.php
   @@ -87,31 +90,58 @@ protected function checkAccess(EntityInterface $entity, $operation, $langcode, A
   +        if ($condition->getCacheMaxAge() != Cache::PERMANENT && $condition->getCacheMaxAge() < $access->getCacheMaxAge()) {
   +          $access->setCacheMaxAge($condition->getCacheMaxAge());
   

   We could at least use \Drupal\Core\Cache\Cache::mergeMaxAges

9. +++ b/core/modules/block/src/EventSubscriber/NodeRouteContext.php
   @@ -39,8 +40,8 @@ public function __construct(RouteMatchInterface $route_match) {
   +    $context = new Context(new ContextDefinition('entity:node', NULL, FALSE));
   ...
   -      $context = new Context(new ContextDefinition($route_contexts['node']['type']));
   

   +1 for explicit code

10. +++ b/core/modules/block/tests/src/Unit/BlockRepositoryTest.php
    @@ -89,10 +89,7 @@ public function testGetVisibleBlocksPerRegion(array $blocks_config, array $expec
    -      $block->expects($this->once())
    -        ->method('access')
    -        ->will($this->returnValue($block_config[0]));
    -      $block->expects($block_config[0] ? $this->atLeastOnce() : $this->never())
    +      $block->expects($this->atLeastOnce())
    

    ++

11. +++ b/core/modules/block/tests/src/Unit/Plugin/DisplayVariant/BlockPageVariantTest.php
    @@ -55,6 +56,18 @@ class BlockPageVariantTest extends UnitTestCase {
    +    $container = new ContainerBuilder();
    ...
    +    $container->set('cache_contexts_manager', $cache_context_manager);
    

    Afaik you don't need a container builder but you can use a Container directly, as there is a set() method on there

12. +++ b/core/modules/block/tests/src/Unit/Plugin/DisplayVariant/BlockPageVariantTest.php
    @@ -99,18 +112,48 @@ public function providerBuild() {
    +            '#cache' => [
    +              'contexts' => [],
    +              'tags' => [],
    +              'max-age' => -1,
    ...
    +            '#cache' => [
    +              'contexts' => [],
    +              'tags' => [],
    +              'max-age' => -1,
    ...
    +            '#cache' => [
    +              'contexts' => [],
    +              'tags' => [],
    +              'max-age' => -1,
    ...
    +            '#cache' => [
    +              'contexts' => [],
    +              'tags' => [],
    +              'max-age' => -1,
    +            ],
    ...
    +            '#cache' => [
    +              'contexts' => [],
    +              'tags' => [],
    +              'max-age' => -1,
    +            ],
    
    @@ -124,16 +167,40 @@ public function providerBuild() {
    +            '#cache' => [
    +              'contexts' => [],
    +              'tags' => [],
    +              'max-age' => -1,
    +            ],
    ...
    +              'contexts' => [],
    +              'tags' => [],
    +              'max-age' => -1,
    ...
    +            '#cache' => [
    +              'contexts' => [],
    +              'tags' => [],
    +              'max-age' => -1,
    +            ],
    +          ],
    ...
    +            '#cache' => [
    +              'contexts' => [],
    +              'tags' => [],
    +              'max-age' => -1,
    +            ],
    

    What about moving this to a protected variable on the $test so its not needed to be there so many times?

13. +++ b/core/modules/page_cache/src/Tests/PageCacheTagsIntegrationTest.php
    @@ -71,13 +71,11 @@ function testPageCacheTags() {
    -      'route.menu_active_trails:account',
    -      'route.menu_active_trails:footer',
    -      'route.menu_active_trails:main',
    -      'route.menu_active_trails:tools',
    

    What happened with them?

14. +++ b/core/modules/system/src/Tests/Cache/AssertPageCacheContextsAndTagsTrait.php
    @@ -77,8 +77,8 @@ protected function assertPageCacheContextsAndTags(Url $url, array $expected_cont
    +      debug('Unwanted cache tags: ' . implode(',', array_diff($cache_entry->tags, $expected_tags)));
    +      debug('Missing cache tags: ' . implode(',', array_diff($expected_tags, $cache_entry->tags)));
    

    Yeah this is all about the message: Did you test code misses cache tags or the actual site. What putting on site at the end?

Thanks for the detailed review!

1. What would the two values be? The behavior that is now implemented is the one that is documented, there's no mention of an exception. The problem is that there are really two different kind of context objects. Context that is offered by someone. There, required is really weird. Right now in HEAD, an exception is thrown when getContextValue() is called on a required context. But we are checking the offering object, not the one that the plugin defined that it needs. Which means that it would also throw an exception if the plugin defines an optional context as we don't check that (in HEAD).

If we keep the exception then we need a hasContextValue() method to be able to check it. Because we do need the object so that we can pass around cache contexts. Or we introduce special objects that make it clear that they only contain "context context" and not an actual value. Doesn't sound that great to me and would be a bigger API change.

I plan to open a follow-up issue to improve the handling of missing context and values without exceptions.

2. Yes we do. Same as blocks, we want to automatically inherit cacheablity from contexts as soon as a condition defines those. So we need the methods on the base class and the interface there. There are likely very few reasons to override them.

3. Maybe. When I added it, only missing_value was a thing, the second check was added later for missing contexts. Maybe just $missing?

4. Hm, not sure. We do have that method in another place (CacheableResponseInterface) but it's slightly different there (like one word in the descrption is different ;)). Should we really add an interface for one method? On the other hand, being able to do this on AccessResults would be useful...

5. & 6. Exactly. Create one block with a node type visibility condition, and you have an exception on every single non-node route. And killed smartcache as a bonus ;) As mentioned in 1, I'd prefer to work on that in a non-critical follow-up although it would be an API change, so maybe better while we have a a critical?

7. Changed. I'm not even sure we need that method, I initially added it because I thought it would be needed in two places. As mentioned in 4, if we'd have $access->addCacheableDependency() then it would be quite a bit easier to understand.

8. I always forget we have that method :)

11. Good point. I'd rather not have it at all :)

12. You mean something like this?

13. They were inherited by the new route cache context. Since something is adding that, we don't need to vary by those "lesser" contexts, they are implied by the route context.

14. I was thinking about this multiple times and almost reverted it. It's tricky, but unwanted IMHO means that I didn't define it in the exceptions and it's still returned? What else could it mean? Instead of on site, maybe "in response" ? Unwanted cache tags in response: and Missing cache tags in response: seems pretty clear?

I was thinking about adding more tests and I think that the current user should *not* add the user cache context. Because it depends on how a condition uses that. For example, the UserRole condition only needs user.roles. But here it gets complicated. Because the provided user could also *not* be the current user and what then? Then it depends on the roles of some arbitrary user? For example, you might want to show a block on the user profile page based on the roles that the user you're viewing has. Or maybe keep the context and replace 'user' with 'user.roles' in that condition? Tried that now.

Also added some more cache contexts to relevant condition plugins.

Condition/Context cache contexts are hard...

Let's try like this for now.

+++ b/core/modules/user/src/Plugin/Condition/UserRole.php
@@ -87,4 +87,17 @@ public function evaluate() {
+    foreach (parent::getCacheContexts() as $context) {
+      $contexts[] = $context == 'user' ? 'user.roles' : $context;
+    }

Agree, this is a good solution.

Likely better if we had a current_user.role service and injected a role directly instead.

Not know enough of the patch, yet.

Fixed the failing test.

1. +++ b/core/lib/Drupal/Core/Plugin/Context/Context.php
   @@ -48,8 +63,7 @@ public function getContextValue() {
   -        $type = $definition->getDataType();
   -        throw new ContextException(SafeMarkup::format("The @type context is required and not present.", array('@type' => $type)));
   +        return NULL;
   

   Why are we removing this logic?

2. +++ b/core/lib/Drupal/Core/Plugin/Context/Context.php
   @@ -60,6 +74,11 @@ public function getContextValue() {
   +    // Add the value as a cacheable dependency only if implements to interface
   +    // to prevent it from disabling caching with a max-age 0.
   +    if ($value instanceof CacheableDependencyInterface) {
   +      $this->addCacheableDependency($value);
   +    }
   

   This feels like a mistake to me. Having setContextValue() performing cacheable metadata merges feels like we're begging for problems. If someone sets the context value to node 1 and then later sets it to node 2, we only merge cacheable metadata so now we'll have both. This also feels like it could be problematic down stream in the call stack since plugins could technically manipulate this and that would affect all plugins using the same context. This is probably a good argument for Context objects losing their setters and becoming 100% pure data objects with just constructor injection. I'm not sure of all the ramifications of doing that, but it might be worth exploring.

3. +++ b/core/lib/Drupal/Core/Plugin/Context/ContextHandler.php
   @@ -74,14 +75,40 @@ public function getMatchingContexts(array $contexts, ContextDefinitionInterface
   +        $plugin_context = $plugin->getContext($plugin_context_id);
   +        if ($plugin_context instanceof ContextInterface && $contexts[$context_id] instanceof CacheableDependencyInterface) {
   +          $plugin_context->addCacheableDependency($contexts[$context_id]);
   +        }
   

   This all happens via getContext() and doesn't ever call getContextValue() which is responsible for throwing our ContextException (before this patch is applied). The rest of the code changed in this file is an adaptation around getContextValue() returning NULL for missing required contexts and I don't think it's necessary. Pretty sure you can go back to throwing the exception in the Context classes.

4. +++ b/core/modules/block/src/EventSubscriber/NodeRouteContext.php
   @@ -39,8 +40,8 @@ public function __construct(RouteMatchInterface $route_match) {
   +    $context = new Context(new ContextDefinition('entity:node', NULL, FALSE));
   
   @@ -48,10 +49,12 @@ public function onBlockActiveContext(BlockContextEvent $event) {
   +    $event->setContext('node.node', $context);
   

   errr... everything about this change makes me very very very concerned. Why do you want node context objects on pages that have no nodes? This bodes badly for any page based management of contexts in Drupal and having control over this through any sort of UI. This feels show-stopping to me.

To elaborate more on that last point, the context system is intended to allow modules like Rules or PageManager to add contexts arbitrarily on a given page. If I have a block that requires a Feed Entity, do I now have to register every empty feed context on any page_manager page ANYWHERE ALWAYS? That seems show-stopping. Someone needs to explain this to me.

Also, to Berdir's earlier questions about block context mapping UI, that's exactly why #2377757: Expose Block Context mapping in the UI exists, and the approach I was attempting in that issue is exactly what inspired the code in this issue. Increasingly, there are some competing/cooperating issues and we need to sort that all out. I want a block context mapping UI in core very badly, I've spent a lot of time and effort on doing that, and have side-lined it all for this issue since there is so much code-overlap. Point being, YES we need a block context mapping UI in core and a bunch of work already exists to make that happen. if we want to do yet another issue merge with this one, consider me ++.

Eclipse

Did you read #42? Quite a few parts of what follows is already mentioned there although with less explanation of the problems we need to solve.

Here's the simple but MUST requirement for cache contexts:
If there is conditional code that affects the output, then the cache context must *always* be added. A typical case is a permission check. If you have that, then you MUST add the user.permissions cache context outside of the if. The render cache system MUST be aware that there might a different version of the same output.

Translating this to the condition/context system:
If you have a block with a node type visibility using a context that depends on something, then we always need to know about that cache context, even if the block is currently not visible. More so, if the block has two conditions and both depend on context, then we need to know about all those contexts. There's a pretty long list of blockers for that as listed in #42.

See my test exceptions NodeBlockFunctionalTest, that's what needs to happen when you add a node type visibility condition. All pages need to have that cache context. This specific example is not too useful since it's the route cache context and we'd cache per page anyway. But a cache context could also be a specific cookie for example that sometimes exists and sometimes doesn't on the same page.

The code in HEAD tries to work around that by marking the block access result as max-age: 0 in case of a missing cache context. That's a *huge* problem with smartcache. Having just one max-age: 0 element on a page means that smartcache is skipped (at least for a first implementation until we have automatic placeholdering and so on that can re-calculate just the problematic element on the page). So as soon as you add such a node type visiblity condition to just a single block means you killed smartcache for every user, term, view and any other page that doesn't have a node context. That's just not an option. This patch doesn't fully address that, but starts to make it possible.

To fully solve it, we need to get rid of that max-age: 0. Which means we need to have all the relevant cacheability metadata, so we can cache it.

Now to your review:

1. Because the code currently gives us no choice at all to check if a context object has a value and prevents us from merging any cacheability metadata. As I noticed while debugging this in #42, it's also actually completely wrong. In HEAD, we call getContextValue() on the *provided* context object. Which means we check the required flag of the provided context definition object and *not* the one from the plugin that actually defines if it should be required or not. The provided context object shouldn't even have any understanding of required.. It just exists or it doesn't, it can't be required.

If you in HEAD define a block or condition that has a non-required node context then it will throw an exception because trying to access the context value will throw an exception, if the object even exists. That doesn't make sense.

2. This part of the code has been added by you in #16. We renamed stuff a bit since then, but it's still the same logic :) That said, nothing in HEAD actually depends on it in the current page, we could just as well drop it and require that those who create context object add the cacheability metadata separately, which they all already do.

3. See above why we can *not* throw an exception. We MUST pass along the cacheability metadata even if there is no value. Those lines basically require all the other changes about not throwing exceptions.

4. Because the Context object is our only way to pass cacheability metadata to the plugins even if there is no context value. As explained above, even if we currently don't have a value, then we MUST have the knowledge that there could be a value and which external thing influences it, so that smartcache can cache the page with the relevant contexts. Again, NodeBlockFunctionalTest has the expectations what needs to happen and #42 has my changes and reasons what's IMHO necessary to get there. If you have ideas for a different implementation, you're welcome to provide an alternative solution. The only alternative I see is that we somehow use onBlockAdministrativeContext() and add the cacheability metadata there as well. Then we always know what contexts could exist and based on which conditions (naming overlap is fun.. we have condition plugins that have context injected that defines cache contexts so that we know under which conditions a given context is available)

Block Context UI: I know that issue, you might have misunderstood my questions (not sure what questions, actually). We already have a context UI for conditions and are actively using them. #2495171: Block access results' cacheability metadata is not applied to the render arrays is a hard blocker for smartcache and this issue is a hard blocker for that, it goes beyond what you need for your UI issue. However, I don't see how it contradicts/blocks anything in that issue. I also don't see how we could possibly simplify it by merging those two issues. I'd actually propose the opposite: You ignore the cacheability problem/this completely, add some workarounds/hacks (e.g, set max-age: 0 if a block has context) to make the current tests work and point to this issue which is a release blocker anyway. Then both issues can continue on their own.

So.. It's almost 2am and I need to get up at 6am again to catch my flight.. two weeks away in Ireland, so I won't have much time for this issue I think. I hope I was able to explain what is needed and why I implemented it like I did.

Ok, so duly noted. The Context event subscribers are absolutely the wrong place to do this. Their job is to provide to plugins assurances that a context does or will exist. They can be leveraged for cache contexts needs without a problem because again that's active information... that being said, when they are NOT available, we need to get the information that this is going to be a requirement from somewhere else.

Reasons:
NodeRouteContext is the node... FROM THE ROUTE. It's a very specific way to get a node, and any contrib module anywhere can provide its own node contexts. Each of these context event subscribers can't be responsible for this, it won't make any sense.

Alternatives:
Probably need to seriously consider what getting this from the plugin definitions looks like instead. As you pointed out, our isRequired() check is wrong, well the specifics of this condition/block/whatever's cache contexts are based upon what is mapped into them (if anything). Is it important to the caching mechanism that NodeRouteContext provides a node via routing? or just that it's a node? If the former, this is going to be REALLY REALLY ugly... like really.

Does this matter for EVERY condition that COULD be used on a block for visibility? CTools and Rules both are going to add lots of conditions... the existing block ui for conditions is kind of awful from this perspective and makes setting expectations for when a condition applies very difficult.

Setting all these matters aside, the bigger problem here is that core has 0 page management. It doesn't even know which blocks are configured to appear on which pages, so it has to test ever damn thing and cache every thing. I'll dig into this issue more this week and see if I can make sense of it for myself in light of your explanations here, but this latest patch concerns me.

Eclipse

Also, have a good time in Ireland.

NodeRouteContext is the node... FROM THE ROUTE. It's a very specific way to get a node, and any contrib module anywhere can provide its own node contexts. Each of these context event subscribers can't be responsible for this, it won't make any sense.

Why not? "From the route" is *exactly* the information that we need. When caching the page, we need to know what can influence if it changes. If there's a context that can come from the route, then we need to cache by route. If there's a alternative context that can be set or not based on a cookie then we need to konw that as well and smartcache needs to cache different variants of the page, based on the existence and value of that cookie.

As mentioned, the route is not a great example because smartcache obviously caches per URL/route anyway. But the cookie information would be very important. And someone might implement block-region caching as well for example, so we can cache whole regions. And then we need to know that it varies by route.

Thanks :)

Ok, but ideally I'd have a context stack where the class responsible for upcasting url parameters registers any upcast entities into the stack appropriately... what you're suggesting would require that class to not only care about the current route's upcast parameters, but EVERY route's upcast parameters if it's ever been used as a context for a block or condition... all the time. The existing block context subscribers are just a "limp-along" solution. Yes I think they can be made to work for core, but in the context of discussing contrib's needs, this whole conversation is VERY concerning to me.

To elaborate more on that last point, the context system is intended to allow modules like Rules or PageManager to add contexts arbitrarily on a given page. If I have a block that requires a Feed Entity, do I now have to register every empty feed context on any page_manager page ANYWHERE ALWAYS?

If you have a block that requires a feed entity, then whenever that block might be rendered, the cache context needs to be set. So say this is an entity reference from the current user to their favourite feed, wherever you check 'does the user have the entity reference and which feed does it reference if so', then the user cache context (or user.property') cache context must also get set.

A few minor observations

1. +++ b/core/lib/Drupal/Core/Block/BlockBase.php
   @@ -303,21 +304,42 @@ public function setTransliteration(TransliterationInterface $transliteration) {
   +      /** @var $context \Drupal\Core\Cache\CacheableDependencyInterface */
   

   Out of interest I think you don't need this if you use instanceof, IDEs are smart enough to identify the class the conditional instanceof.

2. +++ b/core/lib/Drupal/Core/Plugin/Context/Context.php
   @@ -60,6 +74,11 @@ public function getContextValue() {
   +    // Add the value as a cacheable dependency only if implements to interface
   

   %s/to/the

3. +++ b/core/lib/Drupal/Core/Plugin/Context/ContextHandler.php
   @@ -74,14 +75,40 @@ public function getMatchingContexts(array $contexts, ContextDefinitionInterface
   +    $missing_value = [];
   ...
   +          $missing_value[] = $plugin_context_id;
   ...
   +        $missing_value[] = $plugin_context_id;
   ...
   +    if ($missing_value) {
   +      throw new ContextException(SafeMarkup::format("Required contexts without a value: @contexts.", array('@contexts' => implode(', ', $missing_value))));
   

   Shouldn't this be $missing_values?

4. +++ b/core/modules/block/tests/src/Unit/Plugin/DisplayVariant/BlockPageVariantTest.php
   @@ -192,11 +240,31 @@ public function testBuild(array $blocks_config, $visible_block_count, array $exp
   +      ->willReturn(-1);
   

   Should we use the constant here for readability/ease of refactoring?

Catch,

Great example, thank you. To unpack your example more though, that feed is a reference to some arbitrary feed set by the current user. So this page has to be cached, by page by user now, and the specific feed that specific user used has to be part of the cache context as well, so the contexts in this case are like... route, user-3, user.property, feed-23 or something? This feels sane-ish to me, what feels less sane to me is that if a context comes through the route it must always be present in the cache tags/context/max-age if it's used on any block anywhere (this is because core can't differentiate between blocks used on one route or another unless the condition for that is explicitly set). Efforts were made during the D8 cycle to make route upcast items "context" elements, and with good reason, but now it seems if I were to actually expose that, it would bloat the cache... am I misunderstanding?

Eclipse

Okay:

To clear up some misunderstanding here.

We have to distinguish two things:

- Cache keys (identifying the thing and its configuration) => No external dependencies, strong coupling.

and

- Cache contexts (a dependency on some external factor, that can change - without the thing itself changing).

Lets take a simple example for an injected context (the page) into a view:

If you do:

$view = new View('my_view');
$view->setPage(5);
$build = $view->getRenderable();

Then this affects the cache keys, the view will look like (example, not exactly like this):

  $build[
    '#pre_render' => View::preRenderRenderable(),
    '#cache' => [
      'keys' => [ 'view', 'my_view', 'display', 'default', 'page', 5 ],
   ]
  ];

The 'page context' is a part of the views configuration and hence part of the cache keys as it was set externally.

If _that_ view however is cached again via smart-cache, then we need to distinguish where the Page 5 comes from.

In our case this looks like this:

$build = [];

$page = 0;
if (Moon::phase() == Moon::halfMoon) {
  $page = 5;
}

// Unconditionally add the cache context as we did variate on it.
$build['#cache']['contexts'][] = 'moon';

$view = new View('my_view');
$view->setPage($page);
$build['view'] = $view->getRenderable();

Rule of thumb: Whenever writing an if-statement that depends on an external factor, add a cache context.

On the other hand, if the following code is used:

$view = new View('my_view');
$build['view'] = $view->getRenderable();

And the view has a rule to get the current page from the $_REQUEST['page'].

That means the 'page' is _not_ part of the cache contexts, therefore the resulting renderable looks like this:

  $build[
    '#pre_render' => View::preRenderRenderable(),
    '#cache' => [
      'keys' => [ 'view', 'my_view', 'display', 'default' ],
      'contexts' => [ 'url.query_args.pager:0' ],
   ],
  ];

And that is the difference.

We discussed this issue at the SCOTCH meeting today (see notes)! EclipseGC's concern is essentially that he's worried that the cache contexts for block visibility will get specified on every page, even when blocks are being placed outside of cores mechanism for placing block, for example, in page_manager. It'd be great to test this patch with page_manager and make sure that blocks visibility conditions only cause cache contexts to be added to the page in question, and not everywhere. Hopefully, I'm describing that correctly, if not I hope that EclipseGC can clearify. :-)

Not sure I fully understood that:

Cache contexts are only added if there are blocks that have visibility conditions that are using contexts with cache contexts. We always add them through the condition plugins, not directly. Unused contexts don't have their cache metadata added to the page. This is easily visible in the test coverage added to NodeBlockFunctionalTest. The route context is not visibly initially, but as soon as there is a block that has a node type visiblity condition, it's added to all pages, even those the block is currently not displayed on.

Page manager won't be doing anything with this *yet*. I will definitely work on integrating access cacheability metadata into page manager as soon as this in, but it can't happen yet. For the same reason as #2495171: Block access results' cacheability metadata is not applied to the render arrays wasn't possible without this. Right now, block access is forced to max-age: 0, which would disable block level caching. But after this issue, page manager will need to be updated, or it won't work with smartcache.

But when that work is done, then page manager and core blocks will work in the same way but completely separate from each other. Both will add whatever cacheability metadata that is needed for their respective block access checks. page manager will likely get (better) caching for the whole page that will allow to cache the full page without repeating the access checks (which needs to happen right now) and all the cacheability metadata will be summed up and used by smartcache. I don't see any problems there.

Unassigned from me, as mentioned, still mostly away for this week and next.

It would be awesome if someone could start working on a real issue summary. Many long comments here to cover, #70 might be a good one to start with, as that tries to explain the problem and some of the changes that are done here, also #42 and all the related discussions.

Cache contexts are only added if there are blocks that have visibility conditions that are using contexts with cache contexts. We always add them through the condition plugins, not directly. Unused contexts don't have their cache metadata added to the page.

While cache contexts are only added when needed, unfortunately condition context event listeners run whether they're needed or not, see #2354889: Make block context faster by removing onBlock event and replace it with loading from a ContextManager. This may be the source of some of the confusion here.

#82:

So should #2354889: Make block context faster by removing onBlock event and replace it with loading from a ContextManager not actually be critical? What if I have 100s of contexts on a site? They will all run automatically and load everything that might apply?

I think introducing onBlock event was an architectural mistake (and hence the criticality as IMHO the only sane way is to remove the onBlock event again).

There is a language for contexts, so we know exactly what each block has for context, so a block should ask a ContextManager service for the contexts it needs (lazy loaded for all blocks together for speed reasons).

Just because SCOTCH did not go in, does not mean we can take short cuts, we have that debt and we need to solve it properly.

Indeed, there are a lot of confusing twists that make this hard to reason about.

Updating the IS per #81, and c/p'ing what I'm including there in this comment. This is hopefully a clear synthesis for what @Fabianx, @berdir and @catch have been saying.

If something (a block, or generally: anything that can be rendered) is rendered, then we need to know its cacheability metadata. We need cache tags to know when the cached version of the renderable becomes stale. And we need cache contexts to know which variations of the renderable exist.
Perhaps most counter-intuitively: checking access is part of the rendering process, because an inaccessible thing is not rendered. Not being rendered is equivalent with rendering to the empty string. If the cache tags associated with the access check become invalidated (e.g. the node:77 cache tag, node 77 is published, which suddenly makes the block with the node:77 accessible, unlike before), or a different value for the cache context is specified (e.g. a different set of permissions, because permissions have changed for a role), then that may make previously inaccessible (== empty string) things accessible (== non-empty string).

In summary: if something may be rendered, then we always need the associated cacheability metadata.

Applied to the conditions/contexts system/concepts: if you have a block with a "node type" visibility using a context (as in \Drupal\Core\Plugin\Context\ContextInterface) that depends on some external information, then we always need to know about the cache context (as in \Drupal\Core\Cache\CacheContextInterface) associated with that external information. Even if the block is currently not visible.
The block not being visible is analogous to access checking above: it causes the empty string to be rendered (== invisible block), but we still need the cacheability metadata associated with that choice to not render the block.

Think about it this way: we need to know the cacheability of the information that was used to determine whether the block was visible or not; otherwise Drupal would be forced to assume that no external data was involved in coming to the decision to make the block invisible: the absence of cache tags and cache contexts means there are no dependencies on either data (cache tags) or request context (cache contexts) to come to that conclusion.

(Related: http://wimleers.com/talk-making-drupal-fly-fastest-drupal-ever-near/#/2/3.)

@WimLeers;
Thanks, problem description sounds about right, I guess we're still missing a more detailed solution with the changes that this patch is making and API changes/additions.

Any feedback on the implementation/changes that this patch is proposing, anything that you see could be easier? I'll continue to work on this next week and especially in the week after that. Scheduling a discussion or so around this with interested people might also be a good idea then. More than happy to explain the patch there and my decisions/changes in detail.

It might be worth discussing if the changes proposed/being worked on in #2354889: Make block context faster by removing onBlock event and replace it with loading from a ContextManager will affect this, maybe they could make it easier? If we have a more direct way of talking with the services that provide the context objects, then we might be able to transport the cache contexts/tags in a more direct way? I kind of doubt it because we still have a 1:N connection between services and contexts.. and not all the contexts have the same cacheability metadata. Will cross-post there as well.

Thanks, problem description sounds about right

Glad to hear that!

I guess we're still missing a more detailed solution with the changes that this patch is making and API changes/additions.

Yeah, I specifically did not do that because it's quite complex to follow. Which brings me to…

Any feedback on the implementation/changes that this patch is proposing, anything that you see could be easier?

I'd love to do that, and I'll try to do so this week, but… I'm not actually the best person to review this. EclipseGc and others involved (perhaps Tim Plunkett?) are better able to judge the API changes here, because they know the rationale behind the current API much better.

(It's difficult to do solid reviews of an API change if you don't understand why the current version of the API works the way it does.)

Worked on the issue summary, try to explain the problems and my solutions/workarounds for them.

Thanks, berdir!

• I added some minor clarifications.
• Points 8, 9 and 10 in the "Problems" section that you added aren't clear enough yet I think. At least I don't grok them. (Fixed the abrupt end to point 8 with what berdir told me to put there.)
• The proposed solution makes sense at a high level.
• It feels like this issue is hard to make progress on unless we do some parts in child issues.

@EclipseGc: a diagram like https://www.drupal.org/developing/api/8/render/pipeline would be most useful in understand the current flow, being able to point out the problems in the current flow, and would allow us to more clearly describe what the desired flow would be.

Extracted 9. into a separate issue: #2513244: ContextHandler incorrectly checks required/optional contexts of plugins

#2513244: ContextHandler incorrectly checks required/optional contexts of plugins is RTBC, this is a reroll on top of that.

Hurray! 10 K smaller patch. One less problem to list in the IS. Could you also update the IS? I'd do it, but I'm not sure if that'll affect coherence.

@timplunkett: Yes, that method name doesn't make sense anymore. I'm wondering if we can do something similar to other places, where we pass in a CacheabilityMetadata object and then use that to collect access information. Or maybe a list of $access objects, because the problem is that we need to add the cacheability metadata to each block render array if we do have access. I think. Or actually, we might not? I'll discuss that with WimLeers.

#97: We need to add the cacheable metadata regardless if we have access.

However, yes we could indeed collect the metadata and just add it in full the to region.

Because only smart cache is affected (or a region cache or anything that runs before blocks), that should be fine.

Not totally sure how it would affect page manager though as it would need to ensure the cacheability metadata is present.

Yes. Merging it into the block when we have access is actually *wrong* I think. That cacheability metadata just affects *if* the block is rendered, so it affects the region. It doesn't affect how the block itself is cached. For example, we shouldn't be adding the route context to a block that has a node type visibility condition unless the block itself also depends on the node.

@Wim, can you confirm that?

What I was wondering if we need to do it per region or if just $build is enough. Right now, we add non-accessible blocks metadata just to $build, but we should probably add it all to region, so that we could later also introduce per-region caching?

Also, page manager doesn't use the block repository (each page is it's own block repository ;)), so I don't think this is relevant for that.

#99: If you explain it like that, this gets way easier ;).

Yes, lets add it to the region - not sure we need region wide caching, but it is the right hierarchical thing to use.

Region caching would also be quite difficult with how block manager works right now, but that should be okay.

1. +++ b/core/modules/block/src/Plugin/DisplayVariant/BlockPageVariant.php
   @@ -143,6 +156,10 @@ public function build() {
   +        CacheableMetadata::createFromRenderArray($build[$region][$key])
   +          ->merge(CacheableMetadata::createFromObject($access))
   +          ->applyTo($build[$region][$key]);
   

   Berdir is right:

   This is plain wrong.

   Access always needs to be added to the upper structure, not the block itself.

   Example:

   I have a block cacheable by user that has a node-type condition context.

   I want to ESI that block.

   As placeholdering runs after placement, the route cache context would as of now be present.

   But having ESI variate on the route would be non-sensical.

2. +++ b/core/modules/block/src/Plugin/DisplayVariant/BlockPageVariant.php
   @@ -12,6 +12,7 @@
   @@ -133,6 +134,18 @@ public function build() {
   
   @@ -133,6 +134,18 @@ public function build() {
        foreach ($this->blockRepository->getVisibleBlocksPerRegion($contexts) as $region => $blocks) {
   ...
   +          CacheableMetadata::createFromRenderArray($build)
   +            ->merge(CacheableMetadata::createFromObject($access))
   +            ->applyTo($build);
   

   If we always just merge this to the region, then we can also collect it in getVisibleBlocksPerRegion($contexts, $cacheable_metadata);

   And hence can make the function do again what it set out to do.

   Then we can merge everything just once() to the region, which is also quicker and more performant.

   This is still in the critical path, so a win on all fronts.

Regarding #97—#102: I'll answer this using parts of an IRC conversation, but condensed it to the key parts.

The root cause of the problem here is that HEAD implements block visibility conditions as if it were access, but it's not. It's different. Whether a block should be visible or not does is independent of whether that block is accessible to the current user. Just like you could have access to a block/entity/whatever but choose to hide it in the template.

IOW: visibility is like templates, and is therefore not actually about access, even though it is currently implemented like that.

Let's say we have a "WIPE THE SITE" block, that can only be seen by users with the right permission.
My "WIPE THE SITE" block has two links: "wipe my content" and "wipe all content". It depends on permissions whether you see only the first link or both links.
So there are two variants of the block
A: 1 link, B: 2 links
So we need to render cache two variants of the block.

Until here is where cacheability of access checking came into play.

Now let's add visibility to the equation.
It determines NOT whether variant A or B is shown
It determines whether the block is shown AT ALL.

Now we've demonstrated why visibility is in fact independent of access checking.

So therefore visibility cacheability metadata needs to be applied to the region.
But access cacheability metadata needs to be applied to the block.
Therefore visibility must run separately (earlier) from block access
Visibility applies to a different layer
Therefore it SHOULD not be part of block access checking, EVEN THOUGH it currently is.

So the key problem is that visibility is implemented inside \Drupal\block\BlockAccessControlHandler::checkAccess() rather than in an independent, earlier phase, e.g. in \Drupal\block\BlockRepository::getVisibleBlocksPerRegion().

Proposal

• Let \Drupal\block\BlockRepository::getVisibleBlocksPerRegion check block visibility conditions and set cacheability metadata on the region.
• Let \Drupal\block\BlockAccessControlHandler::checkAccess() check block access and set cacheability metadata on the block itself.

+1 to #103, this gets crucial when thinking about moving some blocks to render via ESI, e.g.

Visibility == depends on context the block is rendered in

Access == independent of context, except cache contexts specified by the access handler

Before we start discussing this in detail here, I disagree with #103 :) Give me some time to update the patch and hopefully that will help to explain things.

Ok, just posting what I have for tonight, we can discuss this tomorrow.

So there are two variants of the block
A: 1 link, B: 2 links
So we need to render cache two variants of the block.

This has absolutely *nothing* to do with *block* access or this issue.

There are two different things:

Visibility of the block/block access: block plugin access() method + visibility conditions + hook_block_access()
The result is of this is boolean. The block is visible or not, depending on a set of conditions. This is what this issue is mostly about.

partial/render access, variations: Any kind of logic inside build(). This already works just fine, either getCacheContexts() or the render array returned by the block needs to contain the relevant metadata and then multiple variations of the block are cached.

If you're not convinced, then implement that block/configuration as you described it. Then we can go through it together.

What my patch is now doing:
* BlockRepository stored to it's original purpose, additionally a list of cacheable metadata objects are returned, keyed by region.
* BlockPageVariant then adds those to each region.

There will be some test failures because we have the old problem again that empty regions that contain nothing but cache metadata aren't hidden anymore. Haven't figured out yet how to fix that, might need some help tomorrow.

Okay, I started testing this with ESI. EclipseGC said he designed blocks + context with ESI / cache placeholders in mind, and I can now proof that this is true with some additional work (hurray!).

The big picture is a little different, because context is a special case.

• - Because we know the contexts in advance, the 'context' should affect the cache key and be hardcoded e.g. set into the block.
• - However, because a cache key cannot be bubbled up, the contextual information should affect the upper levels.

Example 1: Block showing image field of a node

Example block showing an image of a node, the node (called also 'node' in this example) for this block got derived from the [route].

Lets assume the path is /node/1:

• - The cache key of the block changes to: block:[node=1]
• - The cache contexts of the block are not affected
• - The cache context that needs to bubble up is [route]
• - The block is shown as the context could be satisfied.

The reason the cache key changes (and not the cache context) is because every context is a new variation of that block based on the given configuration => cache key.

Lets assume the path is /not-a-node, that means context not found:

• - The cache key of the block is N/A
• - The cache contexts of the block are not affected
• - The cache context that needs to bubble up to region level is [route]
• - The block is not shown as the context could not be satisfied.

Example 2: Block depending on node_type visibility

Lets now take a node_type visibility condition and lets assume we want to show the block for articles, but not pages:

Lets assume the path is /article/1:

• - The cache key of the block is block_2:[node_type=article]
• - The cache contexts of the block are not affected
• - The cache context that needs to bubble up to region level is [route]
• - The block is shown as the condition matches.

Lets assume the path is /page/1:

• - The cache key of the block would be block_2:[node_type=page]
• - The cache contexts of the block are not affected
• - The cache context that needs to bubble up to region level is [route]
• - The block is not shown as the condition does not match.

Example 3: Block with custom access handler

Lets assume we have a custom access handler that gives access to the node on half-moon, but not on any other [moon]. (Anyone know the end of Secret of Mana? :-D)

Lets assume we have half-moon:

• - The cache key of the block is block_3
• - The cache contexts of the block are affected and [moon] is added to it.
• - The cache context that needs to bubble up to region level is [moon], which either happens automatically from the block, but which we can also set explicitly.
• - The block is shown as the access is true.

Lets assume we have crescent-moon:

• - The cache key of the block would be block_3
• - The cache contexts of the block would be affected and [moon] would be added to it.
• - The cache context that needs to bubble up to region level is [moon], which is set explicitly.
• - The block is not shown as the access is false.

Unlike context we cannot change the cache key, because as an access handler we can give or not give access, but conditions for that are external, in this case the phase of the moon.

Analysis of Example 3

Regardless of where or how the block is shown, the #access must only be granted when we have half-moon - as it is independent of anything else.

Also we would like to not bubble up the [moon] information in case we placeholder the block - but that is not possible in case of an access callback, because in the denied case, there is no block to placeholder, because we don't want to render all blocks all the time obviously.

Placeholdering for that could only happen at a higher level, e.g. region (or smartcache) as it runs before all access checks and hence only depends on cache contexts.

Or instead of an access callback, a custom condition inside of the block needs to be used.

However placeholdering could still happen for the access granted case _if_ [moon] was added as a cache context to the block itself.

Three Cases for putting #access cacheable metadata

Now there is three cases to choose from:

Case 1: [moon] added just to blocks' cache contexts

• - [moon] was added just to the block and bubbles up automatically
• - The page cache contexts do not vary on the [moon]
• - There is a placeholder for the block
• - The region is never empty
• - Depending on the moon phase the block is shown or not.

Case 2: [moon] added just to the region

• - [moon] was added just to the region explictly
• - The page cache contexts do vary on the [moon]
• - There cannot be a placeholder for the block based on [moon] as the block is not aware of dealing with [moon] at all. If there was a placeholder e.g. based on [user] in the cache contexts, it would allow access regardless of moon phase.
• - The region can be sometimes empty / sometimes not.
• - Depending on the moon phase the block is placed or not placed.

Case 3: [moon] added to the region and the block

• - [moon] was added to the block and to the region explictly
• - The page cache contexts do vary on the [moon]
• - There is a placeholder for the block and the cacheability of the block is [moon] dependent, but there is no advantage to the placeholdering anymore for the [moon] cache context. There is still an advantage for other cache contexts, though, e.g. user.
• - The region can be sometimes empty / sometimes not.
• - Depending on the moon phase the block is placed or not placed and shown or not shown.

Conclusion

Case 1 is my favorite, then Case 3 and Case 2 is making me feel nervous.

Case 3 however is probably the most correct at the moment, because Case 1 fails when access is denied (the page varies by [moon] so there is different behavior based on the [moon] value, but once the page has granted access and the page is cached (without moon), then a denied value leads just to an empty placeholder.

We should seriously think once we have this round of context issues done to always return #pre_render / #cache and add #access inside #pre_render as that makes placeholdering on #access feasible - this is independent of block visibility though as that can directly affect the cache key.

@Wim Leers: I hope this comment passes the 'legibility core gate' :D

TL;DR

• - Contexts need to affect the blocks' cache key and bubble up cache contexts to region level
• - Context Conditions need to affect the blocks' cache key and bubble up cache contexts to region level
• - Access checks should affect the blocks' cache contexts AND/OR bubble up cache contexts to region level

While #107 is correct, I think we can work with #106 for now (looks really great) and follow-up as a major bug to investigate any potential API changes we need to properly support contexts.

(09:42:42) berdir: Fabianx-screen: so.. my patch is adding the cacheable metadata to the region again. which means we have the empty region problem again :(
(09:43:10) Fabianx-screen: berdir: Can't we use the !Element::isEmpty() for the regions?
(09:43:14) berdir: where?
(09:43:16) berdir: classy page.html.twig has "{% if page.sidebar_first %}"
(09:43:20) berdir: and if that's set, then it adds a div
(09:43:40) Fabianx-screen: berdir: yeah
(09:44:00) berdir: Fabianx-screen: I did add the isEmpty() check in HtmlRenderer
(09:44:04) berdir: so the region wrapper isn't added
(09:44:15) Fabianx-screen: berdir: Okay, lets leave region in the API ( so we don't need to change it again ), but add it all to build() for now.
(09:44:32) berdir: Fabianx-screen: hah. that's exactly what I was thinking :p
(09:44:34) Fabianx-screen: That gives us Foreward-Compatibility.
(09:44:40) Fabianx-screen: And is a good in-between step.
(09:44:56) berdir: Fabianx-screen: so return a list, add it to build and if someone wants to add by-region caching, then it's their problem to solve, basically ;)
(09:45:03) Fabianx-screen: berdir: yes

So, did that. Also updated the unit test.

#106:

This has absolutely *nothing* to do with *block* access or this issue.

If you look at it purely from a separation of concerns POV, you're absolutely right.

But, given that visibility conditions are implemented inside Block's access checking, it is relevant to this issue. (As far as I can tell. I could be wrong. :) Let's discuss in person today.)

The result is of this is boolean. The block is visible or not, depending on a set of conditions. This is what this issue is mostly about.

Yes, visibility is boolean + cacheability metadata. But the problem is IMHO that we mix access & visibility.

partial/render access, variations: Any kind of logic inside build(). This already works just fine, either getCacheContexts() or the render array returned by the block needs to contain the relevant metadata and then multiple variations of the block are cached.

Yes and no: Yes, any kind of logic inside build(). But, access checking actually should be happening during the building as well. See Fabian's remarks about that in #107.

If you're not convinced, then implement that block/configuration as you described it. Then we can go through it together.

Ok :)

What my patch is now doing:
* BlockRepository stored to it's original purpose, additionally a list of cacheable metadata objects are returned, keyed by region.
* BlockPageVariant then adds those to each region.

There will be some test failures because we have the old problem again that empty regions that contain nothing but cache metadata aren't hidden anymore. Haven't figured out yet how to fix that, might need some help tomorrow.

In the worst case (I'm right), that sounds like an excellent intermediate step.
In the best case (you're right), that sounds like this issue is almost solved.

Either way, looks like we'll be able to solve this issue in the next few days!

#107:

Big picture, the two bullets ("Because…" + "However…")

Not convinced. See below.

Example 1, /node/1

If we buble the route cache context, then it'll end up in the cache ID too, so what is then the point of also generating a custom cache key?

(If it's to ensure that different routes that have the same context point to the same cache item, then this is in vain, because the route cache context will cause different cache items anyway. And if that is the case, then what is the point of even specifying a custom cache key?)

Example 2

Similar explanation; basically that custom cache key is implied by the cache context. So what is the point?

Analysis of example 3

but that is not possible in case of an access callback, because in the denied case, there is no block to placeholder, because we don't want to render all blocks all the time obviously.

This is a very interesting remark. We need to think that through. I think it'll be crucial.

Conclusion

Case 1 is my favorite, then Case 3 and Case 2 is making me feel nervous.

+1

[…] leads just to an empty placeholder.

And there's nothing wrong with that! What matters is that we were able to placeholder the block and send the majority of the content right away, and then BigPipe telling the page to render a certain block to the empty string (== not rendering it) is totally fine.

We should seriously think once we have this round of context issues done to always return #pre_render / #cache and add #access inside #pre_render as that makes placeholdering on #access feasible

Indeed! (Because then we're back to having a very simple principle: if it's cacheable, set #cache[keys], set a #pre_render/#lazy_builder callback and do *all* logic in that callback.)

#112: Changing the cache key and adding the context information statically into the lazy builder is necessary, because else how could the ESI callback get the block context, which simply does not exist there, but could be coming from _anywhere_.

Also:

Regardless of where the block is displayed and how the context is calculated (hard-coded, from route, from external service, from ...), block_1:[node=1] is always the same (except for any additional cache contexts added inside).

But kinda off-topic here, patch in #111 is indeed a great intermediate step and solves the critical, but we need some more work to make *SI, etc. play nice - both on the context level and on the access level. We will mainly need to ensure / investigate that no further API changes are needed to support it.

I think we can RTBC / go to final review of #111 once green. (and once blocker lands)

The blocker landed :)

Rerolled.

Only found a nit:

+++ b/core/lib/Drupal/Core/Plugin/Context/Context.php
@@ -120,4 +142,33 @@ public function validate() {
+  public function addCacheableDependency($dependency) {
+    $this->cacheabilityMetadata = $this->cacheabilityMetadata->merge(CacheableMetadata::createFromObject($dependency));
+    return $this;
+  }

Possible Follow-up: It would be nice to have a trait for that and have CacheableResponseTrait use that as well:

CacheableMetadataTrait

RTBC from my side! Great work!

Leaving for Wim to sign-off, too.

I will create the follow-up as part of the new Esi Battleplan [meta] for 8.1.

So I got distracted by #107, which is just talking about general ideas/areas for improvement in the future. Oops.

Before that, I got slightly distracted by #97–#102. I do think that #103 is correct and would be better, but doing so wouldn't change any APIs, so we can do it in the future if we want. In the mean time, this fixes the critical bug, so let's go ahead :)

1. +++ b/core/lib/Drupal/Component/Plugin/ContextAwarePluginBase.php
   --- a/core/lib/Drupal/Core/Block/BlockBase.php
   +++ b/core/lib/Drupal/Core/Block/BlockBase.php
   
   +++ b/core/lib/Drupal/Core/Block/BlockBase.php
   @@ -298,21 +299,42 @@ public function setTransliteration(TransliterationInterface $transliteration) {
      public function getCacheContexts() {
   -    return [];
   +    $cache_contexts = [];
   +    foreach ($this->getContexts() as $context) {
   ...
      public function getCacheTags() {
   -    return [];
   +    $tags = [];
   +    foreach ($this->getContexts() as $context) {
   ...
      public function getCacheMaxAge() {
   -    return (int)$this->configuration['cache']['max_age'];
   ...
   +    foreach ($this->getContexts() as $context) {
   +      /** @var $context \Drupal\Core\Cache\CacheableDependencyInterface */
   

   So this means that BlockBase now sets the cacheability metadata for the contexts used by a block.

   Initially, this concerned me, because it means that we almost force blocks to use this base class. But then I noticed

   abstract class BlockBase extends ContextAwarePluginBase implements BlockPluginInterface {
   

   So this actually perfectly in line with that. And the class docs already say:

    * This abstract class provides the generic block configuration form, default
    * block settings, and handling for general user-defined block visibility
    * settings.
   

   — the bit about visibility settings there makes this perfectly aligned.

   So I'd like to make this more clear/explicit by stressing this in updated the ::getCache(Contexts|Tags)() implementations as well, i.e. add something like:

   // Return the cache contexts of the context plugins (which includes those for visibility // settings) used by this block.
   

2. +++ b/core/lib/Drupal/Core/Block/BlockBase.php
   @@ -298,21 +299,42 @@ public function setTransliteration(TransliterationInterface $transliteration) {
   +    $max_age = (int)$this->configuration['cache']['max_age'];
   

   Nit: a @todo to https://www.drupal.org/node/2458763 would be nice.

3. +++ b/core/lib/Drupal/Core/Condition/ConditionPluginBase.php
   @@ -118,6 +120,48 @@ public function calculateDependencies() {
   +    foreach ($this->getContexts() as $context) {
   ...
   +    foreach ($this->getContexts() as $context) {
   ...
   +    foreach ($this->getContexts() as $context) {
   

   These bits are identical to the ones in BlockBase. Would it make sense to put these in a trait, with protected methods like mergeContexts(array $initial_contexts) et cetera?

4. +++ b/core/lib/Drupal/Core/Plugin/Context/Context.php
   @@ -67,6 +84,11 @@ public function hasContextValue() {
   +    // Add the value as a cacheable dependency only if implements to interface
   

   s/to/the/

5. +++ b/core/lib/Drupal/Core/Plugin/Context/ContextHandler.php
   @@ -84,6 +85,11 @@ public function applyContextMapping(ContextAwarePluginInterface $plugin, $contex
   +        $plugin_context = $plugin->getContext($plugin_context_id);
   +        if ($plugin_context instanceof ContextInterface && $contexts[$context_id] instanceof CacheableDependencyInterface) {
   +          $plugin_context->addCacheableDependency($contexts[$context_id]);
   +        }
   

   I think a comment here would be nice. (Like you just described what this does to me in person :))

6. +++ b/core/lib/Drupal/Core/Render/MainContent/HtmlRenderer.php
   @@ -11,6 +11,7 @@
   +use Drupal\Core\Render\Element;
   

   Nit: unnecessary/unintentional change. Can be removed.

7. +++ b/core/modules/block/src/BlockAccessControlHandler.php
   @@ -87,31 +90,56 @@ protected function checkAccess(EntityInterface $entity, $operation, $langcode, A
   +      if ($missing_context) {
   +        // @todo Find a reliable way to avoid max-age 0 in all or more cases.
   +        //   Treat missing context vs. context without value as a different
   +        //   exception, for example.
   +        $access = AccessResult::forbidden()->setCacheMaxAge(0);
   +      }
   

   The @todo is great, but I think it'd also be valuable to explicitly document the reasoning for setting max-age=0.

   AFAICT, the reasoning is "context is missing, something is probably wrong/broken in the system, hence forbid access and do not allow this to be cached — it will become cacheable again once the system is fixed, i.e. once the context is no longer missing".

8. +++ b/core/modules/block/src/BlockAccessControlHandler.php
   @@ -87,31 +90,56 @@ protected function checkAccess(EntityInterface $entity, $operation, $langcode, A
   +   * Merges cacheability metadata from the conditions
   

   […] onto the access result object.

9. +++ b/core/modules/block/src/BlockAccessControlHandler.php
   @@ -87,31 +90,56 @@ protected function checkAccess(EntityInterface $entity, $operation, $langcode, A
   +   * @param \Drupal\Core\Access\AccessResult $access
   

   s/AccessResult/AccessResultInterface/

10. +++ b/core/modules/block/src/BlockAccessControlHandler.php
    @@ -87,31 +90,56 @@ protected function checkAccess(EntityInterface $entity, $operation, $langcode, A
    +    // Add cacheability metadata from the conditions.
    

    Nit: Pointless comment, it's already in the function name and in the function's docblock. Let's delete this.

11. +++ b/core/modules/block/src/Tests/BlockLanguageTest.php
    @@ -90,6 +90,7 @@ public function testLanguageBlockVisibilityLanguageDelete() {
    +          'context_mapping' => ['language' => 'language.language_interface'],
    

    This was a missing context for the block probably? :)

12. +++ b/core/modules/block/tests/src/Unit/Plugin/DisplayVariant/BlockPageVariantTest.php
    @@ -96,7 +110,10 @@ public function providerBuild() {
    +          'max-age' => -1,
    
    @@ -121,7 +138,10 @@ public function providerBuild() {
    +          'max-age' => -1,
    
    @@ -152,7 +172,10 @@ public function providerBuild() {
    +          'max-age' => -1,
    
    @@ -226,6 +252,8 @@ public function testBuildWithoutMainContent() {
    +        'max-age' => -1,
    

    Nit: s/-1/Cache::PERMANENT/

13. +++ b/core/modules/page_cache/src/Tests/PageCacheTagsIntegrationTest.php
    @@ -71,16 +71,15 @@ function testPageCacheTags() {
    -      'route.menu_active_trails:account',
    -      'route.menu_active_trails:footer',
    -      'route.menu_active_trails:main',
    -      'route.menu_active_trails:tools',
    +      'route',
    

    This comes from one of the access results in ::getVisibleBlocksPerRegion().

14. +++ b/core/modules/page_cache/src/Tests/PageCacheTagsIntegrationTest.php
    @@ -71,16 +71,15 @@ function testPageCacheTags() {
    +      'user.roles:anonymous',
    

    This comes from the login block.

15. +++ b/core/modules/page_cache/src/Tests/PageCacheTagsIntegrationTest.php
    @@ -71,16 +71,15 @@ function testPageCacheTags() {
    +      'url',
    

    This is coming from the help block, but we should change the help block to return the route cache context instead. It doesn't need to be per URL at all. It just wasn't updated when we added the route cache context.

16. +++ b/core/modules/system/src/Plugin/Condition/RequestPath.php
    @@ -159,4 +159,13 @@ public function evaluate() {
    +    $contexts[] = 'url';
    

    Nit: this could use a more optimal cache context. Let's add a todo to add a url.path cache context.

Discussed and fixed almost everything.

BlockBase is actually not related to visibility at all anymore, especially not the cache contexts. That's about caching the output of the block that can vary by those.

11. Reason/Explanation for that is in #52.

Added comments about the changes in PageCacheIntegrationTest and where they are coming from. While doing so, we noticed that we can optimize the user login block to vary by route.name only. And that test is actually a surprisingly good integration test for all of this, we even test path-based visibility conditions, that's where the url context is coming from.

Opened #2521978: core/modules/system/src/Plugin/Condition/RequestPath should use the 'url.path' cache context and #2521956: Missing contexts prevent caching of block access

Awesome. No further remarks. Given #117 and #119, I feel like I can RTBC this :)

+++ b/core/lib/Drupal/Core/Condition/ConditionPluginBase.php
@@ -136,6 +138,8 @@ public function getCacheContexts() {
+    // Applied contexts can affect the cache tags of whatever i§s using

s/i§s/is/

Can be fixed on commit :)

A couple of nits that could be fixed on commit. Leaving at rtbc - still to do a deeper review.

1. +++ b/core/modules/block/src/BlockAccessControlHandler.php
   @@ -9,6 +9,9 @@
   +use Drupal\Core\Access\AccessResultInterface;
   

   Not used.

2. +++ b/core/tests/Drupal/Tests/Core/Plugin/Context/ContextTest.php
   @@ -43,6 +46,91 @@ class ContextTest extends UnitTestCase {
   +    $cacheable_dependency = $this->getMock('Drupal\Tests\Core\Plugin\Context\TypedDataCacheableDepencencyInterface');
   
   @@ -59,33 +147,14 @@ public function setUp() {
   +interface TypedDataCacheableDepencencyInterface extends CacheableDependencyInterface, TypedDataInterface { }
   

   TypedDataCacheableDepencencyInterface needs more d and less c

alexpott++

DepencencyInterface lol :)

RTBC + 1

1. The change is looking good to me. Setting to needs work to get an issue for the @todo and improved docs (points 3 and 4).

2. +++ b/core/lib/Drupal/Core/Block/BlockBase.php
   @@ -298,21 +299,51 @@ public function setTransliteration(TransliterationInterface $transliteration) {
      public function getCacheContexts() {
   -    return [];
   +    $cache_contexts = [];
   +    // Applied contexts can affect the cache contexts of blocks, so collect and
   +    // return them.
   +    foreach ($this->getContexts() as $context) {
   +      /** @var $context \Drupal\Core\Cache\CacheableDependencyInterface */
   +      if ($context instanceof CacheableDependencyInterface) {
   +        $cache_contexts = Cache::mergeContexts($cache_contexts, $context->getCacheContexts());
   +      }
   +    }
   +    return $cache_contexts;
      }
    
      /**
       * {@inheritdoc}
       */
      public function getCacheTags() {
   -    return [];
   +    $tags = [];
   +    // Applied contexts can affect the cache tags of blocks, so collect and
   +    // return them.
   +    foreach ($this->getContexts() as $context) {
   +      /** @var $context \Drupal\Core\Cache\CacheableDependencyInterface */
   +      if ($context instanceof CacheableDependencyInterface) {
   +        $tags = Cache::mergeTags($tags, $context->getCacheTags());
   +      }
   +    }
   +    return $tags;
      }
    
      /**
       * {@inheritdoc}
       */
      public function getCacheMaxAge() {
   -    return (int)$this->configuration['cache']['max_age'];
   +    // @todo Configurability of this will be removed in
   +    //   https://www.drupal.org/node/2458763.
   +    $max_age = (int)$this->configuration['cache']['max_age'];
   +
   +    // Applied contexts can affect the cache max age of blocks, so collect and
   +    // return them.
   +    foreach ($this->getContexts() as $context) {
   +      /** @var $context \Drupal\Core\Cache\CacheableDependencyInterface */
   +      if ($context instanceof CacheableDependencyInterface) {
   +        $max_age = Cache::mergeMaxAges($max_age, $context->getCacheMaxAge());
   +      }
   +    }
   +    return $max_age;
      }
   
   +++ b/core/lib/Drupal/Core/Condition/ConditionPluginBase.php
   @@ -118,6 +120,54 @@ public function calculateDependencies() {
      /**
       * {@inheritdoc}
       */
   +  public function getCacheContexts() {
   +    $cache_contexts = [];
   +    // Applied contexts can affect the cache contexts of whatever is using
   +    // conditions, collect and return them.
   +    foreach ($this->getContexts() as $context) {
   +      /** @var $context \Drupal\Core\Cache\CacheableDependencyInterface */
   +      if ($context instanceof CacheableDependencyInterface) {
   +        $cache_contexts = Cache::mergeContexts($cache_contexts, $context->getCacheContexts());
   +      }
   +    }
   +    return $cache_contexts;
   +  }
   +
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function getCacheTags() {
   +    $tags = [];
   +    // Applied contexts can affect the cache tags of whatever is using
   +    // conditions, collect and return them.
   +    foreach ($this->getContexts() as $context) {
   +      /** @var $context \Drupal\Core\Cache\CacheableDependencyInterface */
   +      if ($context instanceof CacheableDependencyInterface) {
   +        $tags = Cache::mergeTags($tags, $context->getCacheTags());
   +      }
   +    }
   +    return $tags;
   +  }
   +
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function getCacheMaxAge() {
   +    $max_age = Cache::PERMANENT;
   +    // Applied contexts can affect the cache max age of whatever is using
   +    // conditions, collect and return them.
   +    foreach ($this->getContexts() as $context) {
   +      /** @var $context \Drupal\Core\Cache\CacheableDependencyInterface */
   +      if ($context instanceof CacheableDependencyInterface) {
   +        $max_age = Cache::mergeMaxAges($max_age, $context->getCacheMaxAge());
   +      }
   +    }
   +    return $max_age;
   +  }
   

   Is this happening on the wrong level? Shouldn't ContextAwarePluginBase provide this functionality? This would mean that any context aware plugins would always be able to apply cacheability metadata.

3. +++ b/core/modules/block/src/BlockRepositoryInterface.php
   @@ -14,11 +14,13 @@
   +   * @param \Drupal\Core\Cache\CacheableMetadata[] $cacheable_metadata
   +   *   List of CacheableMetadata objects, keyed by region.
   

   I think it is worth mentioning in the docs that this is something that is not used by the method but is returned by reference to caller - where it is meant to be used. It's like a pseudo return. Also we're not mentioning that it is optional - however there is only one call in core which does not pass in the second argument (in a test) - should it be optional? I guess that is one way to show it is completely not used by the workings of the method.

4. +++ b/core/modules/block/src/Plugin/DisplayVariant/BlockPageVariant.php
   @@ -172,6 +174,15 @@ public function build() {
   +    // @todo The access cacheable metadata should be added per-region but that
   +    //   currently causes issues with empty regions being displayed. It will be
   +    //   needed in case we want to introduce by-region caching.
   

   Should we make an issue for this @todo?

Berdir is rerolling this one.

#130.2++ — this is exactly why I asked in #120.1 & #120.3 for a trait, but this is a ten times more elegant solution. Should've seen that :) Alex++

Also, regarding my criticism in #103, that I repeated in #120 to still think to be better for the future:

The root cause of the problem here is that HEAD implements block visibility conditions as if it were access, but it's not. It's different. Whether a block should be visible or not does is independent of whether that block is accessible to the current user. Just like you could have access to a block/entity/whatever but choose to hide it in the template.

IOW: visibility is like templates, and is therefore not actually about access, even though it is currently implemented like that.

Berdir convinced me otherwise yesterday :)

When I hear "visibility" I always immediately think "path-based visibility". From that limited POV, my reasoning in #103 is correct.
But, Berdir pointed out that we also have role-based visibility… which effectively is access checking. And from that POV, #103 does not make sense, and HEAD/this patch do make sense.

So, with this in, I no longer think any further changes will be necessary in the future wrt visibility conditions.

Addressed the review.

+++ b/core/lib/Drupal/Core/Block/BlockBase.php
@@ -295,55 +295,4 @@ public function setTransliteration(TransliterationInterface $transliteration) {
-  public function getCacheMaxAge() {
-    // @todo Configurability of this will be removed in
-    //   https://www.drupal.org/node/2458763.
-    $max_age = (int)$this->configuration['cache']['max_age'];

+++ b/core/lib/Drupal/Core/Condition/ConditionPluginBase.php
@@ -120,54 +120,6 @@ public function calculateDependencies() {
-  public function getCacheMaxAge() {
-    $max_age = Cache::PERMANENT;

+++ b/core/lib/Drupal/Core/Plugin/ContextAwarePluginBase.php
@@ -91,4 +93,55 @@ protected function contextHandler() {
+  public function getCacheMaxAge() {
+    // @todo Configurability of this will be removed in
+    //   https://www.drupal.org/node/2458763.
+    $max_age = (int)$this->configuration['cache']['max_age'];

This is a small oversight :)

Other than that, this is ready :)

(#130.4 does *not* need a todo, since it's not something we actually want to do, it's just documentation for our future selves in case we some day decide to do that after all.)

Yes, forgot that, this should be better.

Yay :)

(And more yay, #135 failed as expected.)

@Berdir thanks for addressing my feedback. This looks great.

Committed 7543540 and pushed to 8.0.x. Thanks!

Sorry if this has been mentioned before but :

+++ b/core/lib/Drupal/Core/Plugin/Context/Context.php
@@ -67,6 +84,11 @@ public function hasContextValue() {
   public function setContextValue($value) {
+    // Add the value as a cacheable dependency only if implements the interface
+    // to prevent it from disabling caching with a max-age 0.
+    if ($value instanceof CacheableDependencyInterface) {
+      $this->addCacheableDependency($value);
+    }

@@ -120,4 +142,33 @@ public function validate() {
+  public function addCacheableDependency($dependency) {
+    $this->cacheabilityMetadata = $this->cacheabilityMetadata->merge(CacheableMetadata::createFromObject($dependency));
+    return $this;
+  }

If I'm not mistaken, since we only ever merge, this means that the cacheability metadata of the Context keeps track of the history of values the context received over time - e.g changing the context value from node 1 to node 2 causes the context to have cache tags node:1 and node:2. while a fresh context created directly with node 2 will only have node:2.

This feels like the kind of histeresis we usually try to avoid ? I guess changing a Context value is not a very common situation, but then again the setContextValue() method is public, so we can't pretend contexts are immutable ?

@yched: This issue would make Context's immutable, which would be a good thing for other reasons: #2508884: Make contexts immutable

Oh nice, thanks for the pointer @dsnopek. Commented over there.