Changing #cache keys during #pre_render or anywhere else leads to cache redirect corruption

Test review:

1. +++ b/core/tests/Drupal/Tests/Core/Render/RendererBubblingTest.php
   @@ -502,6 +502,45 @@ public function providerTestBubblingWithPrerender() {
   +   * Tests that overwriting #cache keys does not corrupt the shared bubble cache.
   

   s/does not/cannot/

2. +++ b/core/tests/Drupal/Tests/Core/Render/RendererBubblingTest.php
   @@ -502,6 +502,45 @@ public function providerTestBubblingWithPrerender() {
   +    // Setup a shared cache redirect at llama::bar pointing to llama:bar:r.1
   

   Nit: s/llama::bar/llama:bar/

3. +++ b/core/tests/Drupal/Tests/Core/Render/RendererBubblingTest.php
   @@ -502,6 +502,45 @@ public function providerTestBubblingWithPrerender() {
   +    // Ensure the cached redirect is not overwritten when non-matching keys
   

   80 cols.

4. +++ b/core/tests/Drupal/Tests/Core/Render/RendererBubblingTest.php
   @@ -583,4 +622,27 @@ public static function bubblingPostRenderCache(array $element, array $context) {
   +   * #pre_render callback for testOverWriteCacheKeys().
   ...
   +      'keys' => ['llama', 'foo'],
   +      'contexts' => ['overwritefoo'],
   

   I expected this to only be overriding keys, but it's also overriding contexts?

Code review:

1. +++ b/core/lib/Drupal/Core/Render/Renderer.php
   @@ -201,6 +200,8 @@ protected function doRender(&$elements, $is_root_call = FALSE) {
   +          // @todo This does not work in all cases as recursive #post_render_cache
   +          // is not supported.
   

   That's not actually true, we support that. Since #2382667: #post_render_callback's that result from other #post_render_calback are not processed . Test coverage at \Drupal\Tests\Core\Render\RendererPostRenderCacheTest::testRenderRecursivePostRenderCache().

2. +++ b/core/lib/Drupal/Core/Render/Renderer.php
   @@ -212,16 +213,11 @@ protected function doRender(&$elements, $is_root_call = FALSE) {
   +    // Two-tier caching: set pre-bubbling elements for later comparison.
   +    // @see ::cacheGet()
   +    // @see ::cacheSet()
   +    $pre_bubbling_elements = $elements;
   
   @@ -381,10 +377,13 @@ protected function doRender(&$elements, $is_root_call = FALSE) {
   +    // Cache the processed element if both $pre_bubbling_elements and $elements
   +    // have the metadata necessary to generate a cache ID.
   +    if (isset($pre_bubbling_elements['#cache']['keys']) && isset($elements['#cache']['keys'])) {
   

   I'd think this also costs us extra memory. Is this really necessary?

   Can't we instead: 1) keep $pre_bubbling_cid, 2) add $pre_bubbling_cache_keys (or even just $cache_keys)?

   i.e. keep the same logic, but track the original cache keys and continue to use that, thus ignoring any cache keys set by #pre_render callbacks?

+++ b/core/tests/Drupal/Tests/Core/Render/RendererBubblingTest.php
@@ -502,6 +502,45 @@ public function providerTestBubblingWithPrerender() {
+    // Setup a shared cache redirect at llama:bar pointing to llama:bar:r.1

Oh, r.1 stands for ..
role 1

----

I didn't understand everything, but looks like most of the items from @Wim Leers 's #6 were addressed, except @Fabianx mentioned "I think I will add a comment there, why overwrite foo is needed." and I do not see that change.

---

+++ b/core/tests/Drupal/Tests/Core/Render/RendererBubblingTest.php
@@ -583,4 +622,27 @@ public static function bubblingPostRenderCache(array $element, array $context) {
+   * #pre_render callback for testOverWriteCacheKeys().
+   */
+  public static function bubblingCacheNoOverwritePrerender($elements) {
+    // Overwrite the #cache entry with new data.
+    $elements['#cache']['contexts'] = ['user.roles'];
...
+  public static function bubblingCacheOverwritePrerender($elements) {

Is "Overwrite the #cache entry with new data." a copy and paste error? Because this does is called "NoOverwrite" and it is the same comment as in bubblingCacheOverwritePrerender()

1. +++ b/core/lib/Drupal/Core/Render/Renderer.php
   @@ -212,16 +211,12 @@ protected function doRender(&$elements, $is_root_call = FALSE) {
   +    // Two-tier caching: set pre-bubbling elements for later comparison.
   

   s/set/track/
   s/elements/elements' #cache/

2. +++ b/core/lib/Drupal/Core/Render/Renderer.php
   @@ -381,10 +376,13 @@ protected function doRender(&$elements, $is_root_call = FALSE) {
   +      if ($pre_bubbling_elements['#cache']['keys'] != $elements['#cache']['keys']) {
   

   Let's use strict comparison.

3. +++ b/core/lib/Drupal/Core/Render/Renderer.php
   @@ -381,10 +376,13 @@ protected function doRender(&$elements, $is_root_call = FALSE) {
   +        throw new \LogicException('Cache keys cannot be changed after initial setup. Use the contexts property instead to bubble added metadata.');
   

   s/cannot/may not/
   s/added/additional/

4. +++ b/core/lib/Drupal/Core/Render/Renderer.php
   @@ -551,15 +549,15 @@ protected function cacheGet(array $elements) {
   +   *   The pre-bubbling render cache array.
   

   The pre-bubbling cacheability metadata.

5. +++ b/core/lib/Drupal/Core/Render/Renderer.php
   @@ -574,11 +572,14 @@ protected function cacheSet(array &$elements, $pre_bubbling_cid) {
   +    // Calculate pre_bubbling_cid.
   

   Calculate the pre-bubbling CID.

6. +++ b/core/tests/Drupal/Tests/Core/Render/RendererBubblingTest.php
   @@ -502,6 +502,45 @@ public function providerTestBubblingWithPrerender() {
   +    // Setup a shared cache redirect at llama:bar pointing to llama:bar:r.1
   

   s/Setup/Set up/

7. +++ b/core/tests/Drupal/Tests/Core/Render/RendererBubblingTest.php
   @@ -502,6 +502,45 @@ public function providerTestBubblingWithPrerender() {
   +    $redirect_after = $this->memoryCache->get('llama:bar');
   +
   +    $this->assertEquals($redirect, $redirect_after, 'Invalid bubbled keys cause redirect to be the same.');
   

   These are never executed because the exception is triggered by the ::render() call. Let's remove them.

Seems like a novice re-roll, I'll do it.

Here's a patch with all changes proposed in #16.

@marcvangend: can you post an interdiff? :) Thanks!

Thanks for #21, I was going to mark it NW for that :)

Patch looks great, but the test seems to run a bunch of lines of code without asserting anything related to them. Since the fix that we ended up with here is to simply disallow changing #cache['keys'] mid-rendering (completely independently of anything having to do with contexts), how about simplifying the test to only testing that? See patch.

I also expanded the docs of the $pre_bubbling_elements parameter in cacheSet().

Committed and pushed to 8.0.x. Thanks all! (Yay, my first commit)

I don't think this needs a change record, because if a D7 or D8 module was changing #cache['keys'] after the initial cache get, it was broken anyway. This issue just changes it from being broken silently to having an explicit exception.

Maybe the problem of this bug report (https://www.drupal.org/node/2589703) is related with changes in this issue.

Ok, I've started my period with 'maybe' :)