Add #cache[max-age] to disable caching and bubble the max-age

In https://www.drupal.org/node/2222293 it was stated that the replacement for DRUPAL_CACHE_NONE was to set max_age = 0.

This does not work in a cache bubbling enabled caching world.

Why not? Because render arrays don't have a #cache['max_age'] support? Should they? In which case, we could bubble that up, right (where merge = min())? In which case, would setting that to 0 be preferable to an 'uncacheable' context?

Ok, so let's do #cache['max-age'], per #2 & #3.

This is a first, rough iteration. I won't continue until I get some initial feedback whether we indeed want to continue down this road.

Failures don't matter, this needs a review.

Code makes sense to me. Can we get an issue summary update? Are we tackling downstream-ttl here too?

1. +++ b/core/lib/Drupal/Core/Render/BubbleableMetadata.php
   @@ -52,14 +59,17 @@ class BubbleableMetadata {
   +  public function __construct(array $contexts = [], array $tags = [], $max_age = Cache::PERMANENT, array $attached = [], array $post_render_cache = []) {
   

   The constructor is starting to get hairy here, and given all arguments have default values, a builder-pattern might be of merit for DX here

   BubbleableMetadata::create()->setMaxAge($max_age)->setContexts($contexts);
   

2. +++ b/core/lib/Drupal/Core/Render/BubbleableMetadata.php
   @@ -52,14 +59,17 @@ class BubbleableMetadata {
   +    $this->maxAge = $max_age;
   

   This is the first argument in the constructor that isn't type-hinted. I think we should be doing some validation of the incoming value here and throwing an Exception if something unexpected is encountered. We're doing min() comparisons with the value at a later date, so before we get that far we should be ensuring the incoming value is sane

3. +++ b/core/lib/Drupal/Core/Render/MainContent/HtmlRenderer.php
   @@ -138,14 +138,21 @@ public function renderResponse(array $main_content, Request $request, RouteMatch
   +var_dump($cache_max_age);
   

   :)

4. +++ b/core/modules/filter/src/FilterProcessResult.php
   @@ -49,9 +52,15 @@
   + *   $result->setCacheContexts(['language']);
   

   unrelated?

5. +++ b/core/modules/filter/src/FilterProcessResult.php
   @@ -218,6 +235,30 @@ public function setCacheContexts(array $cache_contexts) {
   +    $this->cacheMaxAge = $max_age;
   

   Same story here re validation?

There's an existing issue about being able to prevent caching by setting a flag, sounds like we can close that as a duplicate of this... doing so now: #2344243: Support a kill flag in $render_array[#cache']

#7: thanks for the general +1 and the actual review! Now rerolling.

#8: cool — thanks!

Now rerolling to be more than just a quick POC.

First: a straight reroll with the conflict caused by #2445761: Add a X-Drupal-Cache-Contexts header to aid in debugging and testing fixed.

#7

1. Agreed in principle, not entirely in practice:
   1. There are only 2 places in all of core that pass arguments to this: tests and FilterProcessResult.
   2. Almost everything uses the ::createFromRenderArray() helper.
   3. That increases the number of function calls massively.

   But writing the above actually convinced me of your point! :D

   The thing is: you want BubbleableMetadata to support chained calls to set various things, but… FilterProcessResult already has this! So if we: A) move all the get/set/add methods from FilterProcessResult to BubbleableMetadata and B) make FilterProcessResult extend BubbleableMetadata… then we have what you wanted. And we even end up with less code overall! And without an API change! (With the exception of removing FilterProcessResult::getBubbleableMetadata(), but that's something that affects the .1%, the rest of the API affects the 99.9%.)

2. Agreed, fixed. But note that cache contexts & tags must be arrays of strings, so string[], not array. But you're right that that at least gives a certain level of assurance.
3. :P
4. Yeah, strictly speaking that is unrelated, but since I have to update that docblock, I might as well do what we (I) forgot to do in #2329101: CacheableInterface only has a getCacheKeys() method, no getCacheContexts(), leads to awkward implementations.
5. Fixed.

In the next reroll, I'm going to change the constructor of BubbleableMetadata, so that we're forced to use the "builder pattern" everywhere.

Tests still needed, but besides that, this should be mostly done.

This is the right interdiff for #13.

Updated IS to match #2 + #3.

+1 RTBC

This was broken with a recent core update, reroll attached.

Boink!

Reroll due to #2381217: Views should set cache tags on its render arrays, and bubble the output's cache tags to the cache items written to the Views output cache.

Wrong patch, sorry.

+++ b/core/lib/Drupal/Core/Cache/Cache.php
@@ -71,6 +71,36 @@ public static function mergeTags() {
+    if (in_array(Cache::PERMANENT, $max_ages)) {
+      $max_ages = array_filter($max_ages, function ($max_age) {
+        return $max_age !== Cache::PERMANENT;
+      });

I wonder if it is just quicker to do the array_filter all the time rather then searching for Cache::PERMANENT?

You're right when dealing with very large sets of max-ages (100):

• With in_array(): http://3v4l.org/kFF5V/perf#tabs
• Without: http://3v4l.org/nalqv/perf#tabs

But not when dealing with smaller ones (10) — and we almost always are handling only 2 values.

• With in_array(): http://3v4l.org/UHAfY/perf#tabs ()
• Without: http://3v4l.org/vs4vA/perf#tabs

(But the differences are very small.)

That was actually a bad example. This comparison is better. 10000 times running a set of typically encountered max-age values, in the different (typical) combinations, to exercise all code paths of this function. That's a much fairer comparison.

• With in_array(): http://3v4l.org/6pKmr/perf#tabs (137/141/128/115 ms user time on PHP 5.4/5.5/5.6/7)
• Without in_array(): http://3v4l.org/0ptco/perf#tabs (144/168/151/130 ms user time on PHP 5.4/5.5/5.6/7)

And that apparently leads to the same conclusion. So I think no changes are necessary.

https://www.drupal.org/node/2451555

+++ b/core/lib/Drupal/Core/Cache/Cache.php
@@ -71,6 +71,36 @@ public static function mergeTags() {
+    if (in_array(Cache::PERMANENT, $max_ages)) {

Hmm this is micro-optimization but I wonder if a straight foreach() would be quicker.

$min = FALSE;
foreach ($max_ages as $age) {
  if ($age !== CACHE_PERMANENT && ($min === FALSE || $age < $min)) {
    $min = $age;
  }
}

return $min !== FALSE ? $min : CACHE_PERMANENT;

Also it's not clear from a quick scan how this affects:
1. Internal page cache
2. Max age header for external caches.

I think it does not affect the internal page cache, but it does affect external caches.

#37

• RE: micro-optimization: doesn't seem like there's a noticeable difference: http://3v4l.org/sfiMN/perf#tabs vs. http://3v4l.org/VVNBe/perf#tabs
• RE: page-level cache header. Good question!

  We do bubble up the max-age to the page-level header:

  +++ b/core/lib/Drupal/Core/Render/MainContent/HtmlRenderer.php
  @@ -135,21 +135,29 @@ public function renderResponse(array $main_content, Request $request, RouteMatch
  +    // If an explicit non-infinite max-age is specified by a part of the page,
  +    // respect that by applying it to the response's headers.
  +    if ($cache_max_age !== Cache::PERMANENT) {
  +      $response->setMaxAge($cache_max_age);
  +    }
  

  … but interestingly, it doesn't have any effect right now! Because we still conflate "page cache settings" with "internal page cache" (i.e. simple built-in reverse proxy for anon users). Consequently, in FinishResponseSubscriber::onRespond(), we do this:

      $is_cacheable = ($this->requestPolicy->check($request) === RequestPolicyInterface::ALLOW) && ($this->responsePolicy->check($response, $request) !== ResponsePolicyInterface::DENY);
  
      // Add headers necessary to specify whether the response should be cached by
      // proxies and/or the browser.
      if ($is_cacheable && $this->config->get('cache.page.max_age') > 0) {
        if (!$this->isCacheControlCustomized($response)) {
          // Only add the default Cache-Control header if the controller did not
          // specify one on the response.
          $this->setResponseCacheable($response, $request);
        }
      }
      else {
        // If either the policy forbids caching or the sites configuration does
        // not allow to add a max-age directive, then enforce a Cache-Control
        // header declaring the response as not cacheable.
        $this->setResponseNotCacheable($response, $request);
      }
  

  So even with the internal page cache turned on, the "page cache maximum age" setting (cache.page.max_age) at /admin/config/development/performance trumps whatever max-age was bubbled.

  But… I think that belongs in a separate issue.

  This issue is about marking it possible to mark parts of the page uncacheable, and propagating that uncacheability. The patch does that just fine. Making sure it also bubbles to the response level as expected (it is bubbled to response, but overwritten by the above) can be a separate issue, because it also relates to the internal page cache. Doing that separately would be just like we did e.g. #2445761: Add a X-Drupal-Cache-Contexts header to aid in debugging and testing separately.

This was what I meant in terms of the comparison, but also not anything in it: http://3v4l.org/EYE5S/perf#tabs

The internal cache overriding is consistent with what we do now so yes separate issue if we decide to change the behaviour - I'm not sure we should anyway.

Committed/pushed to 8.0.x, thanks!

Awesome, now #2099137: Entity/field access and node grants not taken into account with core cache contexts is unblocked! :) And we now have a much more sensible Render API, which now finally is starting to map to HTTP closely :)

Published the CR.

Oops: #2453891: Renderer::getCacheableRenderArray() does not include max-age.