Validate cache contexts (+ cache contexts in some views plugins wrong)

Making the issue summary more readable. I thought those were two sentences... :-)

Seems @Wim Leers found this as well, in #2451679: Validate cache contexts (+ cache contexts in some views plugins wrong).

Yes, I've been seeing and fixing this too, in two places even: #606840-40: Enable internal page cache by default & #2432837-58: Make cache contexts hierarchical (e.g. 'user' is more specific than 'user.roles').

I'm repurposing this issue to fix the general problem: cache contexts are never validated.

Hrm, turns out we already have validation:

      if (!in_array($context_id, $this->contexts)) {
        throw new \InvalidArgumentException(String::format('"@context" is not a valid cache context ID.', ['@context' => $context_id]));
      }

… but it happens only when converting cache contexts to keys. This avoids making Cache::mergeContexts() dependent on the container. Should we change that strategy? We're already seeing bubbling being a significant percentage of the page building time, this would make that *much* worse, and impossible to improve significantly.

Actually, I think we can do this by using the cache_contexts container parameter. Receiving that once and comparing with it is quite cheap.

1. +++ b/core/lib/Drupal/Core/Cache/Cache.php
   @@ -37,11 +37,52 @@ public static function mergeContexts() {
   +    static $valid_contexts;
   +    if (!isset($valid_contexts)) {
   +      $valid_contexts = \Drupal::getContainer()->getParameter('cache_contexts');
   +    }
   

   Please use a static class property instead.

2. +++ b/core/lib/Drupal/Core/Cache/Cache.php
   @@ -37,11 +37,52 @@ public static function mergeContexts() {
   +    foreach ($context_tokens as $value) {
   +      if (!is_string($value)) {
   +        throw new \LogicException('Cache contexts must be strings, ' . gettype($value) . ' given.');
   +      }
   

   This should probably use a look-up table for performance so that isset() can be used.

#8:

1. Why? It's irrelevant to the class, but it's relevant to the function. Why pollute the class with something that's function-specific?
2. I don't see how that can work, because once we know it's a string, we need to "parse" the context ID out of the string to validate it.

Straight reroll to chase HEAD.

#9:

1. I don't think we should mix static in functions and class static.

2. What I meant was a) using array_flip and b) caching already looked up implementations (with parameters)

#12: Thanks, that was almost identical to my first iteration :D

But FabianX expressly meant that to include the parameters of cache contexts that accept parameters. So, doing that too.

+++ b/core/lib/Drupal/Core/Cache/Cache.php
@@ -69,22 +68,35 @@ public static function validateContexts(array $context_tokens) {
+    if (!isset(static::$validContextTokens)) {
+      static::$validContextTokens = \Drupal::getContainer()->getParameter('cache_contexts');
     }
...
+    $valid = array_flip(static::$validContextTokens);
...
+      if (isset($valid[$context_id])) {
+        static::$validContextTokens[] = $context_token;
       }

Can we array_flip once() and then set

static::$validContextTokens[$context_token] = TRUE;

please?

But yes that is what I meant :).

#16: argh that's what I first was doing, but I thought you wanted #15 :P Will do, unless @dawehner beats me to it. (Calling it a night here — *hungry*! This is my first thing in the morning.)

Addressed #15.

Fixing most of the test failures. Many thanks to Fabianx & alexpott to help debug this. (I first thought something was wrong with the module installer.)

This should fix the majority of the test failures.

(I worked on this in parallel with #26. Same diff as @dawehner posted, but I also enhanced the debug output. So rerolling just for that.)

+++ b/core/modules/views/src/Plugin/views/field/Field.php
@@ -961,10 +961,7 @@ public function isCacheable() {
-    $contexts[] = 'user';
+    $contexts = $this->getEntityTranslationRenderer()->getCacheContexts();

It's this change in #12 that's the root cause of the last failures; the tests weren't adjusted accordingly. This caused 2 types of failures:

1. the 'user' cache context's presence was being verified, but is no longer present
2. some tests (EntityReferenceRelationshipTest, FieldEntityTest, ViewEntityDependenciesTest) were failing because something goes wrong in Drupal\views\Plugin\views\HandlerBase->getEntityType(). I got stuck there. Hoping dawehner can fix that.

#26:

1. Done.
2. All good changes, thanks! (It was late and I just wanted something that worked.)

The additional validation is costly, so I@m curious whether we can move it to a level, at which its just needed once. Maybe on cache set?

I considered this too. But:

1. That results in "less local" stack traces, making it harder to spot the culprit.
2. That's inconsistent with how Cache::mergeTags() works. I think we should make both work similarly, and then optimize both in a follow-up. (The same performance concern applies to tag merging.)

1. +++ b/core/lib/Drupal/Core/Cache/Cache.php
   @@ -37,11 +44,70 @@ public static function mergeContexts() {
   +    // The set of existing (thus valid) cache contexts is stored on the
   +    // container; it's safe to statically cache this because it cannot change
   +    // during the request. Initialize the set of valid context tokens with this.
   

   This is no longer true as the ::refresh method shows.

2. +++ b/core/lib/Drupal/Core/Cache/Cache.php
   @@ -37,11 +44,70 @@ public static function mergeContexts() {
   +      if (strpos($context_id, ':') !== FALSE) {
   +        $context_id = substr($context_id, 0, strpos($context_id, ':'));
   +      }
   

   If we want to micro-optimize this can save the result of strpos.

   $colon_pos = strpos(...);
   if ($colon_pos !=== FALSE) {
   $c_id = substr($c_id, 0, $colon_pos);
   }

3. +++ b/core/lib/Drupal/Core/Cache/Cache.php
   @@ -62,10 +128,10 @@ public static function mergeTags() {
   -      static::validateTags($tags);
          $cache_tags = array_merge($cache_tags, $tags);
        }
        $cache_tags = array_unique($cache_tags);
   +    static::validateTags($cache_tags);
   

   Nice change :).

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

   These are great!

5. +++ b/core/modules/views/src/Plugin/CacheablePluginInterface.php
   @@ -8,7 +8,10 @@
   + * For caaching on the render level we rely on cache bubbling of the cache
   + * contexts.
   

   caaching :)

Besides those this looks quite ready, could use a beta evaluation.

Fixed all remarks in #33.

Added beta evaluation.

+++ b/core/modules/views/src/Plugin/CacheablePluginInterface.php
@@ -10,8 +10,7 @@
- * For caaching on the render level we rely on cache bubbling of the cache
- * contexts.
+ * For caching on the render level ,we rely on bubbling of the cache contexts.

nit - "level ,we"

-- can be fixed on commit.

=> RTBC

Fixed the nit.

Also:

10:45:47 WimLeers: ok cool
10:49:28 dawehner: WimLeers: so we need 2 follow ups
10:49:34 dawehner: WimLeers: a) optimize Cache::mergeTags()
10:49:43 dawehner: WimLeers: b) convert the validation into an assert() statement
10:51:15 WimLeers: +1 to both
10:51:23 WimLeers: and would love to see assert()
10:51:25 WimLeers: *love*

Filed #2454643: Optimize cacheability bubbling (Cache::mergeTags(), ::mergeContexts(), BubbleableMetadata) and #2454649: Cache Optimization and hardening -- [PP-1] Use assert() instead of exceptions in Cache::merge(Tags|Contexts).

If I run all the phpunti tests using PHPUnit I get errors.

There were 11 errors:

1) Drupal\Tests\Core\Render\RendererBubblingTest::testContextBubblingEdgeCases with data set #2 (array(array(array('set_test'), array('foo', 'bar', 'baz'))), array('bar', 'baz', 'foo'), array(array(array(), array(array('bar', 'baz', 'foo'), array(), -1), array(), '')))
LogicException: "bar" is not a valid cache context ID.

/Volumes/devdisk/dev/drupal/core/lib/Drupal/Core/Cache/Cache.php:97
/Volumes/devdisk/dev/drupal/core/lib/Drupal/Core/Cache/Cache.php:47
/Volumes/devdisk/dev/drupal/core/lib/Drupal/Core/Render/BubbleableMetadata.php:70
/Volumes/devdisk/dev/drupal/core/lib/Drupal/Core/Render/Renderer.php:426
/Volumes/devdisk/dev/drupal/core/lib/Drupal/Core/Render/Renderer.php:357
/Volumes/devdisk/dev/drupal/core/lib/Drupal/Core/Render/Renderer.php:132
/Volumes/devdisk/dev/drupal/core/tests/Drupal/Tests/Core/Render/RendererBubblingTest.php:93
phar:///usr/local/bin/phpunit/phpunit/TextUI/Command.php:152
phar:///usr/local/bin/phpunit/phpunit/TextUI/Command.php:104

2) Drupal\Tests\Core\Render\RendererBubblingTest::testContextBubblingEdgeCases with data set #3 (array(array(array('set_test'), array('foo', 'baz', 'bar'))), array('bar', 'baz', 'foo'), array(array(array(), array(array('bar', 'baz', 'foo'), array(), -1), array(), '')))
LogicException: "bar" is not a valid cache context ID.

/Volumes/devdisk/dev/drupal/core/lib/Drupal/Core/Cache/Cache.php:97
/Volumes/devdisk/dev/drupal/core/lib/Drupal/Core/Cache/Cache.php:47
/Volumes/devdisk/dev/drupal/core/lib/Drupal/Core/Render/BubbleableMetadata.php:70
/Volumes/devdisk/dev/drupal/core/lib/Drupal/Core/Render/Renderer.php:426
/Volumes/devdisk/dev/drupal/core/lib/Drupal/Core/Render/Renderer.php:357
/Volumes/devdisk/dev/drupal/core/lib/Drupal/Core/Render/Renderer.php:132
/Volumes/devdisk/dev/drupal/core/tests/Drupal/Tests/Core/Render/RendererBubblingTest.php:93
phar:///usr/local/bin/phpunit/phpunit/TextUI/Command.php:152
phar:///usr/local/bin/phpunit/phpunit/TextUI/Command.php:104

3) Drupal\Tests\Core\Render\RendererBubblingTest::testContextBubblingEdgeCases with data set #4 (array(array(array('set_test'), array('bar', 'foo', 'baz'))), array('bar', 'baz', 'foo'), array(array(array(), array(array('bar', 'baz', 'foo'), array(), -1), array(), '')))
LogicException: "bar" is not a valid cache context ID.

/Volumes/devdisk/dev/drupal/core/lib/Drupal/Core/Cache/Cache.php:97
/Volumes/devdisk/dev/drupal/core/lib/Drupal/Core/Cache/Cache.php:47
/Volumes/devdisk/dev/drupal/core/lib/Drupal/Core/Render/BubbleableMetadata.php:70
/Volumes/devdisk/dev/drupal/core/lib/Drupal/Core/Render/Renderer.php:426
/Volumes/devdisk/dev/drupal/core/lib/Drupal/Core/Render/Renderer.php:357
/Volumes/devdisk/dev/drupal/core/lib/Drupal/Core/Render/Renderer.php:132
/Volumes/devdisk/dev/drupal/core/tests/Drupal/Tests/Core/Render/RendererBubblingTest.php:93
phar:///usr/local/bin/phpunit/phpunit/TextUI/Command.php:152
phar:///usr/local/bin/phpunit/phpunit/TextUI/Command.php:104

4) Drupal\Tests\Core\Render\RendererBubblingTest::testContextBubblingEdgeCases with data set #5 (array(array(array('set_test'), array('bar', 'baz', 'foo'))), array('bar', 'baz', 'foo'), array(array(array(), array(array('bar', 'baz', 'foo'), array(), -1), array(), '')))
LogicException: "bar" is not a valid cache context ID.

/Volumes/devdisk/dev/drupal/core/lib/Drupal/Core/Cache/Cache.php:97
/Volumes/devdisk/dev/drupal/core/lib/Drupal/Core/Cache/Cache.php:47
/Volumes/devdisk/dev/drupal/core/lib/Drupal/Core/Render/BubbleableMetadata.php:70
/Volumes/devdisk/dev/drupal/core/lib/Drupal/Core/Render/Renderer.php:426
/Volumes/devdisk/dev/drupal/core/lib/Drupal/Core/Render/Renderer.php:357
/Volumes/devdisk/dev/drupal/core/lib/Drupal/Core/Render/Renderer.php:132
/Volumes/devdisk/dev/drupal/core/tests/Drupal/Tests/Core/Render/RendererBubblingTest.php:93
phar:///usr/local/bin/phpunit/phpunit/TextUI/Command.php:152
phar:///usr/local/bin/phpunit/phpunit/TextUI/Command.php:104

5) Drupal\Tests\Core\Render\RendererBubblingTest::testContextBubblingEdgeCases with data set #6 (array(array(array('set_test'), array('baz', 'foo', 'bar'))), array('bar', 'baz', 'foo'), array(array(array(), array(array('bar', 'baz', 'foo'), array(), -1), array(), '')))
LogicException: "bar" is not a valid cache context ID.

/Volumes/devdisk/dev/drupal/core/lib/Drupal/Core/Cache/Cache.php:97
/Volumes/devdisk/dev/drupal/core/lib/Drupal/Core/Cache/Cache.php:47
/Volumes/devdisk/dev/drupal/core/lib/Drupal/Core/Render/BubbleableMetadata.php:70
/Volumes/devdisk/dev/drupal/core/lib/Drupal/Core/Render/Renderer.php:426
/Volumes/devdisk/dev/drupal/core/lib/Drupal/Core/Render/Renderer.php:357
/Volumes/devdisk/dev/drupal/core/lib/Drupal/Core/Render/Renderer.php:132
/Volumes/devdisk/dev/drupal/core/tests/Drupal/Tests/Core/Render/RendererBubblingTest.php:93
phar:///usr/local/bin/phpunit/phpunit/TextUI/Command.php:152
phar:///usr/local/bin/phpunit/phpunit/TextUI/Command.php:104

6) Drupal\Tests\Core\Render\RendererBubblingTest::testContextBubblingEdgeCases with data set #7 (array(array(array('set_test'), array('baz', 'bar', 'foo'))), array('bar', 'baz', 'foo'), array(array(array(), array(array('bar', 'baz', 'foo'), array(), -1), array(), '')))
LogicException: "bar" is not a valid cache context ID.

/Volumes/devdisk/dev/drupal/core/lib/Drupal/Core/Cache/Cache.php:97
/Volumes/devdisk/dev/drupal/core/lib/Drupal/Core/Cache/Cache.php:47
/Volumes/devdisk/dev/drupal/core/lib/Drupal/Core/Render/BubbleableMetadata.php:70
/Volumes/devdisk/dev/drupal/core/lib/Drupal/Core/Render/Renderer.php:426
/Volumes/devdisk/dev/drupal/core/lib/Drupal/Core/Render/Renderer.php:357
/Volumes/devdisk/dev/drupal/core/lib/Drupal/Core/Render/Renderer.php:132
/Volumes/devdisk/dev/drupal/core/tests/Drupal/Tests/Core/Render/RendererBubblingTest.php:93
phar:///usr/local/bin/phpunit/phpunit/TextUI/Command.php:152
phar:///usr/local/bin/phpunit/phpunit/TextUI/Command.php:104

7) Drupal\Tests\Core\Render\RendererBubblingTest::testContextBubblingEdgeCases with data set #8 (array(array(array('parent'), array('foo', 'bar', 'baz')), 'parent', array(array(array('foo', 'baz'), 3600))), array('bar', 'baz', 'foo'), array(array(array(), array(array('bar', 'baz', 'foo'), array(), 3600), array(), 'parent')))
LogicException: "bar" is not a valid cache context ID.

/Volumes/devdisk/dev/drupal/core/lib/Drupal/Core/Cache/Cache.php:97
/Volumes/devdisk/dev/drupal/core/lib/Drupal/Core/Cache/Cache.php:47
/Volumes/devdisk/dev/drupal/core/lib/Drupal/Core/Render/BubbleableMetadata.php:70
/Volumes/devdisk/dev/drupal/core/lib/Drupal/Core/Render/Renderer.php:426
/Volumes/devdisk/dev/drupal/core/lib/Drupal/Core/Render/Renderer.php:357
/Volumes/devdisk/dev/drupal/core/lib/Drupal/Core/Render/Renderer.php:132
/Volumes/devdisk/dev/drupal/core/tests/Drupal/Tests/Core/Render/RendererBubblingTest.php:93
phar:///usr/local/bin/phpunit/phpunit/TextUI/Command.php:152
phar:///usr/local/bin/phpunit/phpunit/TextUI/Command.php:104

8) Drupal\Tests\Core\Render\RendererBubblingTest::testContextBubblingEdgeCases with data set #9 (array(array(array('parent'), array('foo'), array('yar', 'har')), 'parent', array(array(array('bar'), array('fiddle', 'dee')), '')), array('bar', 'foo'), array(array(true, array(array('parent'), array('bar', 'foo'), array('dee', 'fiddle', 'har', 'yar'))), array(array(), array(array('bar', 'foo'), array('dee', 'fiddle', 'har', 'yar'), -1), array(), 'parent')))
LogicException: "bar" is not a valid cache context ID.

/Volumes/devdisk/dev/drupal/core/lib/Drupal/Core/Cache/Cache.php:97
/Volumes/devdisk/dev/drupal/core/lib/Drupal/Core/Cache/Cache.php:47
/Volumes/devdisk/dev/drupal/core/lib/Drupal/Core/Render/BubbleableMetadata.php:70
/Volumes/devdisk/dev/drupal/core/lib/Drupal/Core/Render/Renderer.php:426
/Volumes/devdisk/dev/drupal/core/lib/Drupal/Core/Render/Renderer.php:357
/Volumes/devdisk/dev/drupal/core/lib/Drupal/Core/Render/Renderer.php:288
/Volumes/devdisk/dev/drupal/core/lib/Drupal/Core/Render/Renderer.php:132
/Volumes/devdisk/dev/drupal/core/tests/Drupal/Tests/Core/Render/RendererBubblingTest.php:93
phar:///usr/local/bin/phpunit/phpunit/TextUI/Command.php:152
phar:///usr/local/bin/phpunit/phpunit/TextUI/Command.php:104

9) Drupal\Tests\Core\Render\RendererBubblingTest::testConditionalCacheContextBubblingSelfHealing
LogicException: "user.roles" is not a valid cache context ID.

/Volumes/devdisk/dev/drupal/core/lib/Drupal/Core/Cache/Cache.php:97
/Volumes/devdisk/dev/drupal/core/lib/Drupal/Core/Cache/Cache.php:47
/Volumes/devdisk/dev/drupal/core/lib/Drupal/Core/Render/BubbleableMetadata.php:70
/Volumes/devdisk/dev/drupal/core/lib/Drupal/Core/Render/Renderer.php:426
/Volumes/devdisk/dev/drupal/core/lib/Drupal/Core/Render/Renderer.php:357
/Volumes/devdisk/dev/drupal/core/lib/Drupal/Core/Render/Renderer.php:288
/Volumes/devdisk/dev/drupal/core/lib/Drupal/Core/Render/Renderer.php:132
/Volumes/devdisk/dev/drupal/core/tests/Drupal/Tests/Core/Render/RendererBubblingTest.php:340
phar:///usr/local/bin/phpunit/phpunit/TextUI/Command.php:152
phar:///usr/local/bin/phpunit/phpunit/TextUI/Command.php:104

10) Drupal\Tests\Core\Render\RendererBubblingTest::testBubblingWithPrerender with data set #0 (array(array(array('Drupal\Tests\Core\Render\Bubb...Render'))))
LogicException: "child.cache_context" is not a valid cache context ID.

/Volumes/devdisk/dev/drupal/core/lib/Drupal/Core/Cache/Cache.php:97
/Volumes/devdisk/dev/drupal/core/lib/Drupal/Core/Cache/Cache.php:47
/Volumes/devdisk/dev/drupal/core/lib/Drupal/Core/Render/BubbleableMetadata.php:70
/Volumes/devdisk/dev/drupal/core/lib/Drupal/Core/Render/Renderer.php:426
/Volumes/devdisk/dev/drupal/core/lib/Drupal/Core/Render/Renderer.php:357
/Volumes/devdisk/dev/drupal/core/lib/Drupal/Core/Render/Renderer.php:288
/Volumes/devdisk/dev/drupal/core/lib/Drupal/Core/Render/Renderer.php:288
/Volumes/devdisk/dev/drupal/core/lib/Drupal/Core/Render/Renderer.php:132
/Volumes/devdisk/dev/drupal/core/lib/Drupal/Core/Render/Renderer.php:103
/Volumes/devdisk/dev/drupal/core/tests/Drupal/Tests/Core/Render/RendererBubblingTest.php:504
phar:///usr/local/bin/phpunit/phpunit/TextUI/Command.php:152
phar:///usr/local/bin/phpunit/phpunit/TextUI/Command.php:104

11) Drupal\Tests\Core\Render\RendererBubblingTest::testBubblingWithPrerender with data set #1 (array('common_test_render_element', array(array('Drupal\Tests\Core\Render\Bubb...Render'))))
LogicException: "child.cache_context" is not a valid cache context ID.

/Volumes/devdisk/dev/drupal/core/lib/Drupal/Core/Cache/Cache.php:97
/Volumes/devdisk/dev/drupal/core/lib/Drupal/Core/Cache/Cache.php:47
/Volumes/devdisk/dev/drupal/core/lib/Drupal/Core/Render/BubbleableMetadata.php:70
/Volumes/devdisk/dev/drupal/core/lib/Drupal/Core/Render/Renderer.php:426
/Volumes/devdisk/dev/drupal/core/lib/Drupal/Core/Render/Renderer.php:357
/Volumes/devdisk/dev/drupal/core/lib/Drupal/Core/Render/Renderer.php:288
/Volumes/devdisk/dev/drupal/core/lib/Drupal/Core/Render/Renderer.php:132
/Volumes/devdisk/dev/drupal/core/tests/Drupal/Tests/Core/Render/RendererBubblingTest.php:492
/Volumes/devdisk/dev/drupal/core/lib/Drupal/Core/Render/Renderer.php:275
/Volumes/devdisk/dev/drupal/core/lib/Drupal/Core/Render/Renderer.php:132
/Volumes/devdisk/dev/drupal/core/lib/Drupal/Core/Render/Renderer.php:103
/Volumes/devdisk/dev/drupal/core/tests/Drupal/Tests/Core/Render/RendererBubblingTest.php:504
phar:///usr/local/bin/phpunit/phpunit/TextUI/Command.php:152
phar:///usr/local/bin/phpunit/phpunit/TextUI/Command.php:104

The static caching got in the way: \Drupal\Tests\Core\Cache\CacheTest::testValidateContexts() ran first, and then \Drupal\Tests\Core\Render\RendererBubblingTest() runs, both of which set a different set of valid cache contexts.

Having looked at #41 — I much, much prefer #41. No hassling with statics at all! I checked with Fabianx, and he also thinks #41 is better.

Just did some clean-up:

1. made the patch apply (3 rebase conflicts)
2. renamed CacheContexts::validate() to CacheContexts::validateTokens(), so that it matches CacheContexts::parseTokens()
3. also moved it to sit next to ::parseTokens()
4. and restored its docs
5. fixed Views' RendererBase, which was still returning language instead of languages
6. removed one extraneous newline
7. … and made tests pass by adding a single line to RendererTestBase :)

Since this was RTBC and these are only cosmetic changes relative to @dawehner's changes in #41, I'm very tempted to already RTBC this, but I guess I better leave it to somebody else.

+++ b/core/modules/node/src/Plugin/views/filter/Access.php
@@ -53,8 +53,7 @@ public function query() {
-    // Node access is potentially cacheable per user.
-    $contexts[] = 'cache.context.user';
+    $contexts[] = 'node_view_grants';

This is wrong now that cache context hierarchy is in.

--

Rest looks fine. I wonder how that one slipped through all tests though.

#45: that could only have slipped through if we have zero integration test coverage for that Views plugin. Fixing that is out of scope for this issue.

+++ b/core/modules/node/src/Plugin/views/filter/Access.php
@@ -53,7 +53,7 @@ public function query() {
-    $contexts[] = 'node_view_grants';
+    $contexts[] = 'user.view_grants:node';
 

I was of the impression it is user.node_grants:view ...

Wow, epic fail much. It was very late yesterday — sorry for that stupid mistake.

Now it is RTBC :).

Thanks!

1. +++ b/core/lib/Drupal/Core/Cache/CacheContexts.php
   @@ -221,4 +221,54 @@ public static function parseTokens(array $context_tokens) {
   +   * Can be called before using cache tags in operations, to ensure validity.
   

   cache contexts no?

2. +++ b/core/lib/Drupal/Core/Cache/CacheContexts.php
   @@ -221,4 +221,54 @@ public static function parseTokens(array $context_tokens) {
   +        throw new \LogicException('Cache contexts must be strings, ' . gettype($context_token) . ' given.');
   

   Nit but shouldn't exception messages use sprintf, or did I miss something?

3. +++ b/core/lib/Drupal/Core/Cache/CacheContexts.php
   @@ -221,4 +221,54 @@ public static function parseTokens(array $context_tokens) {
   +        throw new \LogicException('"' . $context_id . '" is not a valid cache context ID.');
   

   Slightly concerned about the following scenario:

   1. Enable a module that provides a cache context
   2. Some stuff gets cached using that cache context
   3. The module is uninstalled
   4. The stuff is still cached and this condition gets hit when it's returned from cache.

   We're moving towards selectively clearing caches when modules are uninstalled, so this feels not impossible to hit as a condition.

   Is this worth thinking about more? If it's worth thinking about, I'd suggest making the exception a trigger_error() call instead, then opening a follow-up to make it an exception. That situation is something a real site could very possibly run into, and a developer probably wouldn't.

1. Fixed.
2. Fixed.
3. Interesting!

   First, I was going to say: "but installing a module clears all caches!" — but that's no longer true apparently.

   Hence: great catch!

   This is a pre-existing problem though. See the code that actually converts cache context tokens to cache keys:

     public function convertTokensToKeys(array $context_tokens) {
       $context_tokens = $this->optimizeTokens($context_tokens);
       sort($context_tokens);
       $keys = [];
       foreach (static::parseTokens($context_tokens) as $context) {
         list($context_id, $parameter) = $context;
         if (!in_array($context_id, $this->contexts)) {
           throw new \InvalidArgumentException(String::format('"@context" is not a valid cache context ID.', ['@context' => $context_id]));
         }
         $keys[] = $this->getService($context_id)->getContext($parameter);
       }
       return $keys;
     }
   

   As you can see, this is also (already in HEAD) throwing an exception. So the problem already exists; the only difference is that it happens in a slightly later stage.

   Therefore I propose to fix this in a follow-up issue entirely, and commit the patch as-is. (Unless I missed something which causes this patch to make HEAD worse.)

   Follow-up created: #2460013: Uninstalling modules containing cache contexts or #post_render_cache callbacks may break Drupal if they are cached somewhere.

I also think 3. is worth of a follow-up instead of doing it here.

We could compare the cache contexts lists before and after or force modules to uninstall properly by clearing the 'rendered' cache tag themselves.

Now that I think about it, there's another reason that not clearing the render cache after uninstalling a module is problematic: render cache items may include #post_render_cache callbacks… which may live in uninstalled modules.

Bumped the other issue to critical, but agreed the problem isn't introduced here.

Committed/pushed to 8.0.x, thanks!

This seems to introduce some strict warnings in views tests like:

Strict Standards: Declaration of Drupal\views\Tests\Handler\AreaEntityTest::setUpFixtures() should be compatible with Drupal\views\Tests\ViewUnitTestBase::setUpFixtures($import_test_views = true) in /var/www/core/modules/views/src/Tests/Handler/AreaEntityTest.php on line 23

Please see PostgreSQL Bot

#57: I don't see how this patch could've caused that. Are you 100% certain this is the first commit where that happens, and that it can be reproduced?

#58: I see this the first time today and non of the other hand full of commits yesterday seem related to this ;-) I just did a quick views test run again and it's reproducible.
qa.drupal.org shows the same messages while testing this patch in full review log...

+++ b/core/modules/views/src/Tests/ViewUnitTestBase.php
@@ -46,7 +58,7 @@ protected function setUp() {
-  protected function setUpFixtures() {
+  protected function setUpFixtures($import_test_views = TRUE) {

It seems this change is incompatible, when something inherits the ViewUnitTestBase.

Do we use this argument here anyway?

That looks like it's a c/p error indeed. Patch posted at #2460731: Strict warning in ViewUnitTestBase.