Issue #2505989 by alexpott, dawehner, olli: Controllers render caching at the...

Issue #2505989 by alexpott, dawehner, olli: Controllers render caching at the top level and setting a custom page title lose the title on render cache hits

core/lib/Drupal/Core/Render/MainContent/HtmlRenderer.php

@@ -183,6 +183,12 @@ protected function prepare(array $main_content, Request $request, RouteMatchInte
       // @todo Remove this once https://www.drupal.org/node/2359901 lands.
       if (!empty($main_content)) {
         $this->renderer->executeInRenderContext(new RenderContext(), function() use (&$main_content) {
+          if (isset($main_content['#cache']['keys'])) {
+            // Retain #title, otherwise, dynamically generated titles would be
+            // missing for controllers whose entire returned render array is
+            // render cached.
+            $main_content['#cache_properties'][] = '#title';
+          }
           return $this->renderer->render($main_content, FALSE);
         });
         $main_content = $this->renderCache->getCacheableRenderArray($main_content) + [

core/lib/Drupal/Core/Render/RenderCache.php

@@ -7,6 +7,7 @@
 
 namespace Drupal\Core\Render;
 
+use Drupal\Component\Utility\SafeMarkup;
 use Drupal\Core\Cache\Cache;
 use Drupal\Core\Cache\CacheableMetadata;
 use Drupal\Core\Cache\Context\CacheContextsManager;
@@ -76,6 +77,11 @@ public function get(array $elements) {
       if (isset($cached_element['#cache_redirect'])) {
         return $this->get($cached_element);
       }
+      // Ensure that any safe properties are marked safe.
+      foreach ($cached_element['#safe_cache_properties'] as $cache_property) {
+        SafeMarkup::set($cached_element[$cache_property]);
+      }
+      unset($cached_element['#safe_cache_properties']);
       // Return the cached element.
       return $cached_element;
     }
@@ -327,6 +333,7 @@ public function getCacheableRenderArray(array $elements) {
         'tags' => $elements['#cache']['tags'],
         'max-age' => $elements['#cache']['max-age'],
       ],
+      '#safe_cache_properties' => []
     ];
 
     // Preserve cacheable items if specified. If we are preserving any cacheable
@@ -335,6 +342,13 @@ public function getCacheableRenderArray(array $elements) {
     // the cache entry size.
     if (!empty($elements['#cache_properties']) && is_array($elements['#cache_properties'])) {
       $data['#cache_properties'] = $elements['#cache_properties'];
+      // Store whether any of the cache properties are safe strings.
+      foreach (Element::properties(array_flip($elements['#cache_properties'])) as $cache_property) {
+        if (isset($elements[$cache_property]) && !is_array($elements[$cache_property]) && SafeMarkup::isSafe($elements[$cache_property])) {
+          $data['#safe_cache_properties'][] = $cache_property;
+        }
+      }
+
       // Extract all the cacheable items from the element using cache
       // properties.
       $cacheable_items = array_intersect_key($elements, array_flip($elements['#cache_properties']));

core/modules/node/src/Controller/NodeViewController.php

@@ -57,7 +57,7 @@ public function view(EntityInterface $node, $view_mode = 'full', $langcode = NUL
    *   The page title.
    */
   public function title(EntityInterface $node) {
-    return SafeMarkup::checkPlain($this->entityManager->getTranslationFromContext($node)->label());
+    return $this->entityManager->getTranslationFromContext($node)->label();
   }
 
 }

core/modules/system/src/Tests/System/PageTitleTest.php

@@ -55,6 +55,7 @@ function testTitleTags() {
 
     $node = $this->drupalGetNodeByTitle($edit['title[0][value]']);
     $this->assertNotNull($node, 'Node created and found in database');
+    $this->assertText(SafeMarkup::checkPlain($edit['title[0][value]']), 'Check to make sure tags in the node title are converted.');
     $this->drupalGet("node/" . $node->id());
     $this->assertText(SafeMarkup::checkPlain($edit['title[0][value]']), 'Check to make sure tags in the node title are converted.');
   }
@@ -137,6 +138,24 @@ public function testRoutingTitle() {
     $this->assertTitle('Dynamic title | Drupal');
     $result = $this->xpath('//h1');
     $this->assertEqual('Dynamic title', (string) $result[0]);
+
+    // Ensure that titles are cacheable and are escaped normally if the
+    // controller does not escape them.
+    $this->drupalGet('test-page-cached-controller');
+    $this->assertTitle('Cached title | Drupal');
+    $this->assertText(SafeMarkup::checkPlain('<span>Cached title</span>'));
+    $this->drupalGet('test-page-cached-controller');
+    $this->assertTitle('Cached title | Drupal');
+    $this->assertText(SafeMarkup::checkPlain('<span>Cached title</span>'));
+
+    // Ensure that titles are cacheable and are escaped normally if the
+    // controller escapes them use SafeMarkup::checkPlain().
+    $this->drupalGet('test-page-cached-controller-safe');
+    $this->assertTitle('Cached title | Drupal');
+    $this->assertText(SafeMarkup::checkPlain('<span>Cached title</span>'));
+    $this->drupalGet('test-page-cached-controller-safe');
+    $this->assertTitle('Cached title | Drupal');
+    $this->assertText(SafeMarkup::checkPlain('<span>Cached title</span>'));
   }
 
 }

core/modules/system/tests/modules/test_page_test/src/Controller/Test.php

@@ -7,6 +7,8 @@
 
 namespace Drupal\test_page_test\Controller;
 
+use Drupal\Component\Utility\SafeMarkup;
+
 /**
  * Defines a test controller for page titles.
  */
@@ -49,6 +51,26 @@ public function dynamicTitle() {
     return 'Dynamic title';
   }
 
+  /**
+   * Defines a controller with a cached render array.
+   *
+   * @param bool $mark_safe
+   *   Whether or not to mark the title as safe use SafeMarkup::checkPlain.
+   *
+   * @return array
+   *   A render array
+   */
+  public function controllerWithCache($mark_safe = FALSE) {
+    $build = [];
+    $build['#title'] = '<span>Cached title</span>';
+    if ($mark_safe) {
+      $build['#title'] = SafeMarkup::checkPlain($build['#title']);
+    }
+    $build['#cache']['keys'] = ['test_controller', 'with_title'];
+
+    return $build;
+  }
+
   /**
    * Returns a generic page render array for title tests.
    *

core/modules/system/tests/modules/test_page_test/test_page_test.routing.yml

@@ -21,6 +21,21 @@ test_page_test.static_title:
   requirements:
     _access: 'TRUE'
 
+test_page_test.cached_controller:
+  path: '/test-page-cached-controller'
+  defaults:
+    _controller: '\Drupal\test_page_test\Controller\Test::controllerWithCache'
+  requirements:
+    _access: 'TRUE'
+
+test_page_test.cached_controller.safe:
+  path: '/test-page-cached-controller-safe'
+  defaults:
+    _controller: '\Drupal\test_page_test\Controller\Test::controllerWithCache'
+    mark_safe: true
+  requirements:
+    _access: 'TRUE'
+
 test_page_test.dynamic_title:
   path: '/test-page-dynamic-title'
   defaults:

core/tests/Drupal/Tests/Core/Render/RendererBubblingTest.php

@@ -179,6 +179,7 @@ public function providerTestContextBubblingEdgeCases() {
           'max-age' => Cache::PERMANENT,
         ],
         '#markup' => 'parent',
+        '#safe_cache_properties' => [],
       ],
     ];
     $data[] = [$test_element, [], $expected_cache_items];
@@ -202,6 +203,7 @@ public function providerTestContextBubblingEdgeCases() {
           'max-age' => Cache::PERMANENT,
         ],
         '#markup' => '',
+        '#safe_cache_properties' => [],
       ],
     ];
     $context_orders = [
@@ -243,6 +245,7 @@ public function providerTestContextBubblingEdgeCases() {
           'max-age' => 3600,
         ],
         '#markup' => 'parent',
+        '#safe_cache_properties' => [],
       ],
     ];
     $data[] = [$test_element, ['bar', 'baz', 'foo'], $expected_cache_items];
@@ -291,6 +294,7 @@ public function providerTestContextBubblingEdgeCases() {
           'max-age' => Cache::PERMANENT,
         ],
         '#markup' => 'parent',
+        '#safe_cache_properties' => [],
       ],
     ];
     $data[] = [$test_element, ['bar', 'foo'], $expected_cache_items];
@@ -370,6 +374,7 @@ public function testConditionalCacheContextBubblingSelfHealing() {
         'max-age' => Cache::PERMANENT,
       ],
       '#markup' => 'parent',
+      '#safe_cache_properties' => [],
     ]);
 
     // Request 2: role B, the grandchild is accessible => bubbled cache
@@ -395,6 +400,7 @@ public function testConditionalCacheContextBubblingSelfHealing() {
         'max-age' => 1800,
       ],
       '#markup' => 'parent',
+      '#safe_cache_properties' => [],
     ]);
 
     // Request 3: role A again, the grandchild is inaccessible again => bubbled
@@ -431,6 +437,7 @@ public function testConditionalCacheContextBubblingSelfHealing() {
         'max-age' => Cache::PERMANENT,
       ],
       '#markup' => 'parent',
+      '#safe_cache_properties' => [],
     ]);
 
     // Request 4: role C, both the grandchild and the grandgrandchild are
@@ -458,6 +465,7 @@ public function testConditionalCacheContextBubblingSelfHealing() {
         'max-age' => 300,
       ],
       '#markup' => 'parent',
+      '#safe_cache_properties' => [],
     ]);
 
     // Request 5: role A again, verifying the merging like we did for request 3.
@@ -477,6 +485,7 @@ public function testConditionalCacheContextBubblingSelfHealing() {
         'max-age' => Cache::PERMANENT,
       ],
       '#markup' => 'parent',
+      '#safe_cache_properties' => [],
     ]);
 
     // Request 6: role B again, verifying the merging like we did for request 3.
@@ -496,6 +505,7 @@ public function testConditionalCacheContextBubblingSelfHealing() {
         'max-age' => 1800,
       ],
       '#markup' => 'parent',
+      '#safe_cache_properties' => [],
     ]);
   }
 
@@ -529,7 +539,7 @@ public function testBubblingWithPrerender($test_element) {
     // - … is not cached DOES get called.
     \Drupal::state()->set('bubbling_nested_pre_render_cached', FALSE);
     \Drupal::state()->set('bubbling_nested_pre_render_uncached', FALSE);
-    $this->memoryCache->set('cached_nested', ['#markup' => 'Cached nested!', '#attached' => [], '#cache' => ['contexts' => [], 'tags' => []]]);
+    $this->memoryCache->set('cached_nested', ['#markup' => 'Cached nested!', '#attached' => [], '#cache' => ['contexts' => [], 'tags' => []], '#safe_cache_properties' => ['#markup']]);
 
     // Simulate the rendering of an entire response (i.e. a root call).
     $output = $this->renderer->renderRoot($test_element);

core/tests/Drupal/Tests/Core/Render/RendererPlaceholdersTest.php

@@ -137,6 +137,7 @@ public function providerPlaceholders() {
           'tags' => [],
           'max-age' => Cache::PERMANENT,
         ],
+        '#safe_cache_properties' => [],
       ],
     ];
 
@@ -179,6 +180,7 @@ public function providerPlaceholders() {
           'tags' => [],
           'max-age' => Cache::PERMANENT,
         ],
+        '#safe_cache_properties' => [],
       ],
     ];
 
@@ -312,6 +314,7 @@ public function testCacheableParent($test_element, $args, $placeholder_cid_keys,
         'tags' => [],
         'max-age' => Cache::PERMANENT,
       ],
+      '#safe_cache_properties' => [],
     ];
     $expected_element['#attached']['placeholders'][$expected_placeholder_markup] = $expected_placeholder_render_array;
     $this->assertEquals($cached_element, $expected_element, 'The correct data is cached: the stored #markup and #attached properties are not affected by placeholder #lazy_builder callbacks.');
@@ -579,6 +582,7 @@ public function testRenderChildrenPlaceholdersDifferentArguments() {
         'tags' => [],
         'max-age' => Cache::PERMANENT,
       ],
+      '#safe_cache_properties' => [],
     ];
 
     $dom = Html::load($cached_element['#markup']);

core/tests/Drupal/Tests/Core/Render/RendererTest.php

@@ -7,6 +7,7 @@
 
 namespace Drupal\Tests\Core\Render;
 
+use Drupal\Component\Utility\SafeMarkup;
 use Drupal\Core\Access\AccessResult;
 use Drupal\Core\Access\AccessResultInterface;
 use Drupal\Core\Cache\Cache;
@@ -690,10 +691,13 @@ public function testRenderCacheProperties(array $expected_results) {
       ],
       // Collect expected property names.
       '#cache_properties' => array_keys(array_filter($expected_results)),
-      'child1' => ['#markup' => 1],
-      'child2' => ['#markup' => 2],
-      '#custom_property' => ['custom_value'],
+      'child1' => ['#markup' => '1'],
+      'child2' => ['#markup' => '2'],
+      // Mark the value as safe.
+      '#custom_property' => SafeMarkup::checkPlain('custom_value'),
+      '#custom_property_array' => ['custom value'],
     ];
+
     $this->renderer->renderRoot($element);
 
     $cache = $this->cacheFactory->get('render');
@@ -708,9 +712,14 @@ public function testRenderCacheProperties(array $expected_results) {
       $this->assertEquals($cached, (bool) $expected);
       // Check that only the #markup key is preserved for children.
       if ($cached) {
-        $this->assertArrayEquals($data[$property], $original[$property]);
+        $this->assertSame($data[$property], $original[$property]);
       }
     }
+    // #custom_property_array can not be a safe_cache_property.
+    $safe_cache_properties = array_diff(Element::properties(array_filter($expected_results)), ['#custom_property_array']);
+    foreach ($safe_cache_properties as $cache_property) {
+      $this->assertTrue(SafeMarkup::isSafe($data[$cache_property]), "$cache_property is marked as a safe string");
+    }
   }
 
   /**
@@ -723,14 +732,15 @@ public function testRenderCacheProperties(array $expected_results) {
   public function providerTestRenderCacheProperties() {
     return [
       [[]],
-      [['child1' => 0, 'child2' => 0, '#custom_property' => 0]],
-      [['child1' => 0, 'child2' => 0, '#custom_property' => 1]],
-      [['child1' => 0, 'child2' => 1, '#custom_property' => 0]],
-      [['child1' => 0, 'child2' => 1, '#custom_property' => 1]],
-      [['child1' => 1, 'child2' => 0, '#custom_property' => 0]],
-      [['child1' => 1, 'child2' => 0, '#custom_property' => 1]],
-      [['child1' => 1, 'child2' => 1, '#custom_property' => 0]],
-      [['child1' => 1, 'child2' => 1, '#custom_property' => 1]],
+      [['child1' => 0, 'child2' => 0, '#custom_property' => 0, '#custom_property_array' => 0]],
+      [['child1' => 0, 'child2' => 0, '#custom_property' => 1, '#custom_property_array' => 0]],
+      [['child1' => 0, 'child2' => 1, '#custom_property' => 0, '#custom_property_array' => 0]],
+      [['child1' => 0, 'child2' => 1, '#custom_property' => 1, '#custom_property_array' => 0]],
+      [['child1' => 1, 'child2' => 0, '#custom_property' => 0, '#custom_property_array' => 0]],
+      [['child1' => 1, 'child2' => 0, '#custom_property' => 1, '#custom_property_array' => 0]],
+      [['child1' => 1, 'child2' => 1, '#custom_property' => 0, '#custom_property_array' => 0]],
+      [['child1' => 1, 'child2' => 1, '#custom_property' => 1, '#custom_property_array' => 0]],
+      [['child1' => 1, 'child2' => 1, '#custom_property' => 1, '#custom_property_array' => 1]],
     ];
   }