Controllers render caching at the top level and setting a custom page title lose the title on render cache hits

Oh I see, we probably need to add #title to #cache_properties by default on controller resuts?

This is quite disconcerting and might be a general issue (although entity titles are OK, but we do special things there anyway). Bumping to major.

I think the right fix is something like this.

+1 for merging test coverage together here ...

@olli

I tried hard to reproduce that, but I could not. Are you sure this is now still a problem?

We have tests now.

Uh. Yeah I'd consider this critical. Even though it's confined to one feature, it's a pretty important one.

I'm pretty sure this is a Views-specific problem in HEAD. But it's a problem that any controller that is using render caching for the entire render array it returns can experience. Because:

class Controller {
  public function something() {
    return [
      '#cache' => ['keys' => […], …],
      '#title' => 'Llamas are awesome',
      'children' => […],
    ];
  } 
}

means that on a cache hit, #title will not be set.

I noticed this in page manager a while ago, moved to a nested element as a workaround.

Hmm, is there any way we could add a general test for the scenario described in #11 and/or somehow flag render arrays like that?

Also I think we really need to document #11 thoroughly somewhere.

#14: #9 is trying to fix it generically :)

I'm pretty sure this is a Views-specific problem in HEAD. But it's a problem that any controller that is using render caching for the entire render array it returns can experience. Because:

Yeah just like #9 fixes it.

Working on a test.

and a better title too. Thanks!

Mh, I'm not sure its worth to unit test \Drupal\Core\Render\MainContent\HtmlRenderer

Regarding the general fix, I think its not possible. We want to get rid of pretty much everything beside #markup, so well, people need to use the #cache_properties api.

Thank you to alexpott for helping me here, well its still not fixed actually, mhh.

Regarding the general fix, I think its not possible. We want to get rid of pretty much everything beside #markup, so well, people need to use the #cache_properties api.

Well, I'd argue that the changes in HtmlRenderer are the generic fix.

1. +++ b/core/lib/Drupal/Core/Render/MainContent/HtmlRenderer.php
   @@ -182,7 +182,10 @@ protected function prepare(array $main_content, Request $request, RouteMatchInte
          if (!empty($main_content)) {
   +
            $this->renderer->executeInRenderContext(new RenderContext(), function() use (&$main_content) {
   

   This is an unnecessary change.

2. +++ b/core/lib/Drupal/Core/Render/MainContent/HtmlRenderer.php
   @@ -182,7 +182,10 @@ protected function prepare(array $main_content, Request $request, RouteMatchInte
   +          // Ensure that we at least store the page title in cache.
   

   What about something like Retain #title; otherwise dynamically generated titles would be missing for controllers whose entire returned render array is render cached.

3. +++ b/core/lib/Drupal/Core/Render/MainContent/HtmlRenderer.php
   @@ -182,7 +182,10 @@ protected function prepare(array $main_content, Request $request, RouteMatchInte
   +          $main_content['#cache_properties'][] = '#title';
   

   We could wrap this in

   if (isset($main_content['#cache']['keys']) {
     …
   }
   

giddyup

So the failing test is because the title retrieved from cache is sanitized again.
Working on it

This is a alex thing atm.

So it turns out we need to know which # properties are safe when we render cache them.

+++ b/core/lib/Drupal/Core/Render/MainContent/HtmlRenderer.php
@@ -182,7 +182,10 @@ protected function prepare(array $main_content, Request $request, RouteMatchInte
+          // Ensure that we at least store the page title in cache.
+          $main_content['#cache_properties'][] = '#title';

The problem is that the #cache_properties API only works with children to mark them as Safe again.

There is two cases here:

a) #title is already at top-level, in which case we could check if its safe and wrap in a SafeString instead.

b) #title is bubbled up dynamically, which is really hard.

( and #post_render does not work as call_user_func does not pass $elements by reference ), which in strict mode could be considered a regression compared to Drupal 7 ...

So not many options.

Probably the best is to do it like we do it with drupal_set_message() and check SafeMarkup::isSafe() for each string property and then either store as array with [ safe => TRUE ] case and unwrap array on retrieval or wrap directly in a SafeString object.

Asked plach & Fabianx to review the changes in RenderCache, since they brought that to core.

+++ b/core/lib/Drupal/Core/Render/Renderer.php
@@ -257,16 +257,20 @@ protected function doRender(&$elements, $is_root_call = FALSE) {
+        // Mark root element cached properties as safe. This always includes
+        // #markup.
+        foreach ($elements['#safe_cache_properties'] as $cache_property) {
+          SafeMarkup::set($cached_element[$cache_property]);
+        }
+

Love it, lets clean this internal property up by unsetting it here.

RTBC once that change is made from me.

1. +++ b/core/lib/Drupal/Core/Render/MainContent/HtmlRenderer.php
   @@ -183,6 +183,8 @@ protected function prepare(array $main_content, Request $request, RouteMatchInte
            $this->renderer->executeInRenderContext(new RenderContext(), function() use (&$main_content) {
   +          // Ensure that we at least store the page title in cache.
   +          $main_content['#cache_properties'][] = '#title';
              return $this->renderer->render($main_content, FALSE);
            });
   

   See #21.2 & 3, I think that'd help make this significantly more understandable/less mysterious.

2. +++ b/core/lib/Drupal/Core/Render/RenderCache.php
   @@ -335,6 +336,15 @@ public function getCacheableRenderArray(array $elements) {
   +      // Store whether any of the cache properties are safe strings. #markup is
   +      // always safe at this point.
   +      $data['#safe_cache_properties'] = ['#markup'];
   +      foreach (Element::properties(array_flip($elements['#cache_properties'])) as $cache_property) {
   +        if (isset($elements[$cache_property]) && SafeMarkup::isSafe($elements[$cache_property])) {
   +          $data['#safe_cache_properties'][] = $cache_property;
   +        }
   +      }
   

   Shouldn't this also require updates to the test coverage in \Drupal\Tests\Core\Render\RendererTest::testRenderCacheProperties()?

3. +++ b/core/modules/system/tests/modules/test_page_test/src/Controller/Test.php
   @@ -6,6 +6,7 @@
    namespace Drupal\test_page_test\Controller;
   +use Drupal\Component\Utility\SafeMarkup;
   

   Nit: should have a \n in between.

After that, RTBC from me.

Fixed #30 and #32.

Fingers-crossed for green-ness.

Fixing the unit tests...

The fail in #35 was because this got a PHP5.5 bot without APCu.

IS updated.

RTBC.

Looks great from my point of view!

Doxygen? RendererInterface::render mentions #cache_properties so I presume we would want to put #safe_cache_properties there as well.

Good point.

I’m concerned about the implementation.

At the moment if you cached the following render array

[
  ‘#title’ => ‘<em>I love emphasis</em>’,
  ‘#custom_property’ => [‘some’ => ‘random array’],
  ‘#markup’ => ‘<some html>’,
]

With the #custom_property it will break. Let’s make this more robust.

Also added docs to RenderCacheInterface - this doesn't belong along side #cache_properties in RendererInterface::render because it is never return from render as it is unset there. Perhaps we should move all of this SafeMarkup logic inside RenderCache::get()?

Yep we can deal with #safe_cache_properties internally in RenderCache.. I think the solution attached is much neater as #safe_cache_properties is never exposed to the render system at large.

This is so much more elegant. Excellent :)

1. +++ b/core/tests/Drupal/Tests/Core/Render/RendererTest.php
   @@ -690,10 +691,13 @@ public function testRenderCacheProperties(array $expected_results) {
   +      '#custom_property' => SafeMarkup::checkPlain('custom_value'),
   +      '#custom_property_array' => ['custom value'],
   

   Nit: I think it'd be clearer if these were named, #custom_property_safe and #custom_property_unsafe, respectively.

2. +++ b/core/tests/Drupal/Tests/Core/Render/RendererTest.php
   @@ -708,9 +712,14 @@ public function testRenderCacheProperties(array $expected_results) {
   +    // #custom_property_array can not be a safe_cache_property.
   

   Though this makes it sufficiently clear. Consider the previous point a soft suggestion :)

1. +++ b/core/lib/Drupal/Core/Render/RenderCache.php
   @@ -327,6 +333,7 @@ public function getCacheableRenderArray(array $elements) {
   +      '#safe_cache_properties' => []
   

   Do we need to expose this here?

2. +++ b/core/lib/Drupal/Core/Render/RenderCache.php
   @@ -335,6 +342,13 @@ public function getCacheableRenderArray(array $elements) {
   +      // 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;
   +        }
   +      }
   

   Should we not check for is_scalar() instead?

I still do not see #safe_cache_properties documented...

@chx I don't think it needs to be. It is set in RenderCache::set() and removed in RenderCache::get() - and also will be removed when we do #2506581: Remove SafeMarkup::set() from Renderer::doRender. It is never exposed in the render system.

Why was this changed? With the patch applied, this method is not called anymore on cache hit.

To be clear, this is exactly what auto escaping is doing for you. There is no longer a need to do that manually.

re #51

Should we also add a case where title contains some html that is not escaped? Somethink like "Cached title with some html"

- well this is not possible with a Node - since it is alway checkPlain'd (before and after this patch) but I do think it is worth discussing was is and what is not allowed in a title. Just not in this issue.

re #50.2 - nope spans are removed for the title... See template_process_html

  // Construct page title.
  if (!empty($variables['page']['#title'])) {
    $head_title = array(
      'title' => trim(strip_tags($variables['page']['#title'])),
      'name' => $site_config->get('name'),
    );
  }

And now we're not checkPlain'ing this NodeViewController::title() this actually works as desired.

Let's do that in a separate issue: #2530474: Discuss whether | which html tags to allow in entity labels

I think with the response of alex, the feedback from olli got adressed

Committed/pushed to 8.0.x, thanks!

Hmm, #47 was not addressed :/.

So asking post-commit #47 again:

1. Why do we expose #safe_cache_properties in getCacheableRenderArray()? It should be an internal implementation detail only ...
2. Why not use is_scalar() instead of the is_array check? What happens with objects?

Both could be fixed in a non-critical follow-up, but would really love to have those answered.

Why do we expose #safe_cache_properties in getCacheableRenderArray()? It should be an internal implementation detail only ...

Darn! I completely overlooked that :(

Let's open a major follow-up to fix the things we didn't get right here. Sorry for having missed that, Fabianx! :(

Re #58/#59 this will be addressed in #2506581: Remove SafeMarkup::set() from Renderer::doRender