Bubbling of elements' max-age to the page's headers and the page cache

In principle I think supporting this use case is OK.

In practice I'm not sure about it.

The Drupal 7 model was that block caching was really about authenticated users, and page caching was for anonymous, and these were separate things. It opens up the potential for contrib or custom modules (especially ones that don't ignore/don't understand cache tags) to break page caching in fun and unexpected ways.

So a clear yes from me:

This is why #cache['ttl'] or max-age is independent from #cache expires.

I think we can easily combine this with the place holder approach, but bubbling should probably be configurable.

But having that information besides the cache context is important to know if we want to render a place holder.

So I definitely think we can make this a developer decision - always bubble up max_age. Then it's OK to file a bug report if people are doing it wrong.

Where there's a UI for setting the cache time of a block, then that should probably also include whether it should affect parent/page caching ttl too - since that's where people will get into trouble (thinking of the Views UI specifically here).

#7 The smallest one that we want to bubble up - yes.

Not to the page cache though what this was originally about ;).

Are we sure we want to override the internal page cache? I'm not completely opposed but that feels tricky to explain how it works with the max_age setting. For example render cached content is assumed to be permanent if there's no max age set, so the setting would represent a cap on that - but then if someone sets max_age to a week in #cache and the config is set to 10 minutes, which wins?

Currently the max-age is already set on the Response object via setMaxAge(), the page cache can use that setting.

If I understand correctly, what is requested in this issue is actually what we are removing in #2467041: max-age on HTML responses wrongly set to `max-age=0, private` instead of `max-age=N, public` (breaks reverse proxies and client-side caching). Until that issue lands, max-age is actually bubbled up the stack and the smallest element max-age value makes it into the cache-control header. However, since some elements are setting max-age=0, this ends up marking each page request as "max-age=0, private".

So, is this issue about:
a) re-enabling what we are removing in #2467041: max-age on HTML responses wrongly set to `max-age=0, private` instead of `max-age=N, public` (breaks reverse proxies and client-side caching) after all elements correctly set max-age <> 0
b) changing the way we handle previously set cache headers in FinishResponseSubscriber::onRespond()

In any case, it seems that this issue is postponed until we have fixed max-age being set to 0 on some elements that are currently causing the entire page to have max-age=0?

Coming from #2429617-270: Make D8 2x as fast: Dynamic Page Cache: context-dependent page caching (for *all* users!).
SmartCache puts debug info in its own X-Drupal-SmartCache header, with values HIT/MISS, and UNCACHEABLE when max-age = 0.

This issue here is where we should, similarly, output UNCACHEABLE in X-Drupal-Cache when applicable ?

I opened #2592555: Time based cache does not work as expected for anonymous users today that seems to be related to this issue (thanks @catch to have pointed this one to me).

What I understand here is that if I want to implement the very common use case of a piece of generated content with a time based cache (i.e. a RSS aggregation or a randomized view), anonymous users are going to always see the same thing unless I manually clear the cache.
Am I right?

It is kind of silly, but you sold that to me as a feature a while ago :)

Having a separate external and internal max age can definitely be a useful feature, especially if you can't invalidate the external cache. You can set it to a short time, so that it frequently re-validates using the internal page cache.

I think we can do that with an X-Internal-Page-Cache-Max-Age header specially for that purpose. Akamai has similar with Edge-control, and Wim pointed out in irc there is s-max-age.

#30: Yes, Symfony and the FosHttpCache bundle uses s-max-age for that purpose, e.g. $response->setSharedMaxAge(600);

I'm a bit confused why we talk about headers. If you are able to set a header, then you can already set the Expires header and page cache will respect that? The problem IMHO is when you can't but want it to respect whatever max-age was set by a block, statistics.module, a views or whatever.

Isn't what we want here exactly what you implemented in #2527126-143: Only send X-Drupal-Cache-Tags and -Contexts headers when developer explicitly enables them? To be clear, I'm not against doing that at all, just against doing it in that issue. That would finally allow to properly use statistics.module also for anonymous users, something that in 7.x only worked by setting the page cache expiration manually to 1h or whatever frequency you wanted to have it updated. And right now doesn't work at all.

But we need to make sure that we no longer send out max-age 0 in common cases, as that would avoid those pages from being cached. Maybe we should only respect a value != 0. It's a bit weird but would be a minimal behavior change compared to HEAD...

I'm a bit confused why we talk about headers. If you are able to set a header, then you can already set the Expires header and page cache will respect that?

You can but then Expires gets passed to everything after page cache too. Same with max-age once we make page caching respect max-age.

#28: Per marking #2592555: Time based cache does not work as expected for anonymous users as a duplicate of this, that makes this essentially a 8.0.x bug.

If we don't want this, we need to split this out again.

Marking #2578627: Drupal Cache for Anonymous users does not respect render array's #cache['max-age'] as a duplicate of this.

IMHO, before considering this, I think we need to introduce a concept to say that e.g. a block is not worth caching. Right now, you can achieve that by returning max-age 0, but it's not the same. As the result would be that page cache would be disabled.

For example something like \Drupal\system\Plugin\Block\SystemPoweredByBlock, it's totally pointless that cache a hardcoded #markup string , just calling it will always be faster since we have to instantiate the block anyway. But you still want it to be cached in the page cache.

Also, at this point this would IMHO be an API change?

#40: I think you can now do this within system module via https://api.drupal.org/api/drupal/core%21modules%21block%21block.api.php....

Just unsetting the #cache 'keys' is enough.

I still would love to have a $block->alterBuild() call to do the same as the hook, but Wim was against it so maybe in 8.9.x ;).

So not worth caching is taken care of and yes probably the new behavior would be opt-in via a $conf flag or such, e.g. maybe even enabled for new installations ...

I created #2732129: FinishResponseSubscriber::setResponseCacheable() does not respect Cacheable Metadata for Cache-Control header but it looks like it might be a duplicate of this issue?

I just ran into this as a part of #2772847: Add support for private uploads / presigned URLs. I'm a bit lost in all of the discussion, but is there any need for this to be more complex then this patch? At the least, it's working for me in the basic case.

The test fails show why this is a problem. There are max-age 0 elements on many pages, doing this results in them no longer being cacheable. That's a behavior change that I don't think we can just introdue like that.

I've commented about this multiple times before :)

As a start, we need to at least figure out what max-age 0 elements we have and figure out what to do about them. Then respecting this could be an option, I don't know..

That's all depends on "strategy" used - all places are replaced with placeholder to make whole page cachable if I remember right

And more importantly, we are talking about the internal/anon page cache, that doesn't support that anyway, only the dynamic page cache does :)

#48 I think the only way to fix this is to be honest about it and remove the max-age=0 e.g. for forms when used by the anonymous user, etc.

And all other places, too, where we currently allow caching in page cache and external proxies.

Because if we allow that caching, then we should not be declaring max-age=0 in the first place (for anon user).

So the patch above didn't actually do anything useful yet, because max-age 0 was skipped and ignored (which should result in it not being cacheable) while a -1 actually resulted in expire being lower than request time and was then not cached.

The 4xx caching expire also changed this quite a bit, so didn't apply anymore.

New patch that should work correctly with 0 and -1. Also checking expires first, that's more backwards compatible I think.

Lets see how many test fails we have now. The aggregator test is now actually passing.

Interesting, was wondering if that was going to happen. That means we're actually doing a much better job at setting max-age than I thought. Good. The question is how many pages we are not testing.

The only fail we have is in GlossaryTest, because that view is marked as uncacheable and that now bubbles up. And the test actually doesn't care about testing that the page is cached, it just uses the standard method from the trait to assert this. And the cache tags/contexts are actually there on the response, the problem is just that there is no cache entry now anymore for this view.

Also weird is that \Drupal\system\Tests\Cache\AssertPageCacheContextsAndTagsTrait::assertPageCacheContextsAndTags() does a negative MISS check on the first request, but it does *not* check for a HIT on the second. But it would implicitly fail as shown in this example.

The reason why the glossary view is not cacheable is interesting, though. And that's \Drupal\views\Plugin\views\style\Table::getCacheMaxAge(). That returns 0 and I can't really say why? Best I can tell is that this is actually a bug that we introduced in #2464427: Replace CacheablePluginInterface with CacheableDependencyInterface. We went from isCacheable() TRUE to max-age 0.

That means we have two ways of fixing this test. I first converted the test to not use that trait, but by fixing the table style plugin, the test actually passes unchanged.

This looks like it will cause all pages in my current project to become uncacheable in a very fun way.

I have a couple of facet blocks in my search page, and these have max-age = 0. I have noticed that this currently causes all my pages to be marked with max-age = 0, not just the search page. At the moment this is not a huge problem since, hey, the page cache still caches pages with max-age = 0 at the moment! :D

I could not find an issue about it so I created #2828136: A single uncacheable block can make the entire website uncacheable. We might have a look at this since this will mean that merging this issue will affect every site that uses uncacheable blocks, for example because they use the Facets module.

Commented there, but yes, I think that is the main thing that we have to worry about. Core is either doing pretty fine or we simply don't have the explicit assertions to detect unexpected page cache misses. But who knows that will happen to actual sites.

What we could do is declare it an (experimental?) opt-in behavior that you have to enable with a setting or so. And declare that we'll make it the default in 9.x. Kind of the opposite of a deprecation. And speaking of that, if this would just work, it would mean that we could replace most if not all use cases of the kill switch service with setting max-age on the response/some render array and eventually deprecate that?

I think that is the main thing that we have to worry about. Core is either doing pretty fine or we simply don't have the explicit assertions to detect unexpected page cache misses. But who knows that will happen to actual sites.

What we could do is declare it an (experimental?) opt-in behavior that you have to enable with a setting or so. And declare that we'll make it the default in 9.x. Kind of the opposite of a deprecation. And speaking of that, if this would just work, it would mean that we could replace most if not all use cases of the kill switch service with setting max-age on the response/some render array and eventually deprecate that?

I agree that we cannot predict all cases that will happen in live sites, but even though I did find #2828136: A single uncacheable block can make the entire website uncacheable which would turn my entire site uncacheable, I actually think it's fine to fix this for real. I don't think we should default to the buggy behaviour and opt-in to the fixed behaviour. I fear that this will just complicate things in the long run. The current behaviour is just wrong.

I do think we need enough lead time to discover other potential caching problems that are now hidden because of this issue. 8.3.0 is scheduled for April next year, if we can get this in before the end of December this will probably give enough time. After that, we should maybe consider a kill switch or postpone it to 8.4.x.

Doesn't the current patch fail to account for FinishResponseSubscriber.php, which currently has this code:

    $max_age = $this->config->get('cache.page.max_age');
    $response->headers->set('Cache-Control', 'public, max-age=' . $max_age);

The max age is always taken from the core config setting.

EDIT: The issue I described is documented in #2732129: FinishResponseSubscriber::setResponseCacheable() does not respect Cacheable Metadata for Cache-Control header, but as it stands it's not clear if this issue is intended to fix that as well as the Page Cache module, or just the Page Cache module.

We should split this into two issues:

1. For deciding on and implementing support for a new #cache property which can be used to set s-max-age or Surrogate-Control or similar.

2. For bubbling max-age itself (except I don't think we should do that because it could very easily break existing sites).

About Max-age bubbling we could create a checkbox in the performance settings to enable this option then have an upgrade path that would disable it for existing sites to avoid breaking BC. That setting could be marked as deprecated and removed in D9 (or not).

+1 for @DuaelFr's idea of providing this as a configuration option which is disabled for existing sites and enabled for new installations. I don't think it is necessary to make this option visible in the UI, but if we do this is then the performance settings are indeed ideal for it.

we are trying to solve two different problems here (at least in code level):
1. Adding expire to page cache - gets saved in a storage to serve when Drupal receives a request
Current: This has been done by the value of response's 'expires' header. If there is no 'expires' header, cached permanently.
Solution: patch #53 allows to fallback to bubbled-up max age, which is set by setCacheMaxAge(),

2. Adding max-age header - Allows other systems (browser, cdn, reverse proxy) in front of drupal/server to deal with the response caching
Current: If a page is cacheable, use the config value system.performance:cache.page.max_age to set max-age in cache-control header
Solution: We have two parts mentioned in #59 here. We can use #2732129: FinishResponseSubscriber::setResponseCacheable() does not respect Cacheable Metadata for Cache-Control header for 59.1 and this issue for 59.2 AND page_cache expire.
As per #60 and #61, we can provide an option, so that user can select to use setCacheMaxAge() instead of fixed system.performance:cache.page.max_age

Below patch adds option in performance page and use it as max-age in response header. Now it's possible to fallback on getCacheableMetadata()->getCacheMaxAge() for both page_cache and max-age on Cache-Control header.

Quick schema update...

Fixing last test fail.

Thanks @Wim Leers, for the review.

#67.1 - FIXED
#67.2 - NOT FIXED: Not sure how to set permanent. probably we shouldn't set max-age derivative at all?
#67.3 - NOT FIXED: Yeah, its for page cache. @Berdir mentioned, we might split the views related changes to different issue. probably leave it for now.
#67.4 - FIXED.
#67.5 - NOT FIXED: hmm, I know I wasn't totally happy about the labels either. Should we do ['override' => 'Fixed period', 'bubbled' => 'Dynamic']
#67.6 - NOT FIXED: we have hook_update_N. we might need to change the number, but it would work right?
#67.7 - :)

Thanks @Wim Leers. Updated for #67.6 and re-rolled.

@Wim Leers It was a reroll as the patch in #70 was against 8.2.x. However here is the diff-ui (after all we have UX initiative in core :D)

What I don't understand (and I literally mean "don't understand" - it's not a rhetoric question or rant or anything): How could Drupal 8 have ever been released without that feature? Why is there some talk above in this issue about it being "nice-to-have"? Why do I diligently set my max-age-keys within my preprocess functions of paragraphs, views all the time only to find out they have no effect whatsoever, because the page cache is overriding all of this anyway? It's totally unexpected. Or was the full-page-cache never intended to be used at the same time with the dynamic page cache caching individual parts? Was it my mistake to enable both or expect both of them to work at the same time? Would be great if someone could enlighten me :-)

Well, I liked the solution in this patch much better, because you can set the Max-Age explicitly to "Dynamic". The module uses the setting "Permanent" to disable the dynamic Max-Age. So you have to explicitly set a max-age for it to work. The patch, however, didnt work with the current Drupal version 8.3.4 (mostly Array syntax changes), I've modified the patch and posted it here. Perhaps it's also useful for someone else.

totally agree with 76. wasnt the "new" cache system such a great new deal of d8? So many things dont work out of the box, or just not well, it still surprises me today how d8 could be released :(

so, this is still a bug and the only workaround is that contrib module?

Updated patch, because #78 failed to apply on 8.4.x branch.

#2232375: Support CacheOptionalInterface in BlockViewBuilder, use it in language switcher block to prevent max-age 0 bubbling is one of possibly many issues why this change/issue is not as trivial as it may look. Any multilingual site with a language switcher would then be completely uncacheable. So we likely need to fix that issue first and identify what else would be impacted but is not explicitly tested.

We also need still need to move the Views Table style fix to a separate issue and get it in.

I just got caught out by this. It seems very un-intuitive to me that I would go to the effort of setting a max age for it to be ignored by page cache... Not sure I'm in a position to deal with the various complications of resolving this issue (e.g. translation stuff), but when I get time I'll at least try to get the documentation updated so it's clear...

Another problematic issue for this is #2521956: Missing contexts prevent caching of block access. Combined with this, that would disable page caching on all non-node pages as soon as you use a node type visibility condition.

Given that we do not have test fails for either that or the issue mentioned in #84 shows how problematic this is, it is quite likely that we are missing test coverage for a whole bunch of cases that would result in partially or completely disabled page cache for a lot of sites. Even a configuration option IMHO isn't an OK solution as users have no way of seeing/knowing about those issues.

See also #2888838: Optimize render caching for some ideas on what kind of features would also be quite important, for example a way of saying "this block isn't worth caching", so that we can e.g. stop render-caching the "Powered By" block without that bubbling up and resulting in not caching the whole site.

I haven't read through all the comments in this issue, but my hunch is that there's really important information in some of them that isn't reflected in the issue summary, so if anyone is inspired to update the issue summary with where this issue is at, that would be helpful. Thanks!

I was messing around last weekend, and I ran into this problem as well. I was expecting "max-age=0" to bubble up and not cause the page to be cached. I was surprised it still got cached until I found this issue; it looks like it gets other people confused as well. I worked around it with a kill switch but it feels like a hack; see screenshot.

in my case, a workaround for this:
uninstall "Internal Page Cache" and let only "Internal Dynamic Page Cache" enabled

Related https://www.drupal.org/project/drupal/issues/2968485 .

I will keep this issue as a personal reminder that drupal is in slow death. If @dries #89 don't understand how drupal cache system (don't) works their is a serious problem.

Another example of why this is not easy: #2578855: Form tokens are now rendered lazily, allow forms to opt in to be cacheable. Any page with a form, including any page with a lazy builder that is then uncacheable would be uncacheable by the internal page cache module.

@Berdir: Absolutely, it might not be easy and I don't want to defend @gagarine and I see Drupal thriving more than ever and not in slow death, but I have to admit I do share somewhat of his harsh criticism, because: Drupal 8.6 is now approaching - the caching system was introduced with 8.0 and advertised as the next cool thing. And then I read about what features are planned for 8.6 (https://www.drupal.org/core/roadmap) I read about a lot of work (or so it seems) going into migration of old legacy sites, Media support etc. Certainly all great features. But shouldn't something basic like the caching system work reliably? And isn't it a least a tad bit disturbing that even Dries is surprised by this bug? I really shared this part of @gargarine's sentiment. But don't get me wrong: Drupal is a cool piece of software and D8 is really a huge step forward. And I do appreciate all the work. But I sometimes think those problems should really be given a bit more priority. These things IMHO are also part of the "Out-of-the-box" experience, because you need to do a lot of researching and reading to make seemingly simple things work properly as you just don't expect them to be buggy in an 8.5/8.6 release. We're out of alpha state, I suppose.

But shouldn't something basic like the caching system work reliably?

The render caching system, considering it was almost entirely new in 8.x (at least in terms of its application and several new concepts like cache tags and contexts) does work reliably. This issue is a lot more about expectations for a specific use case. Also the render cache API and the caching system are two different things, the former is based on the latter. Most projects do not have an equivalent of the render caching API which handles variation and invalidation in the way we do.

If someone wants to help get the issue closed, then updating the issue summary would be a good start. Documenting issues that block this one like #2578855: Form tokens are now rendered lazily, allow forms to opt in to be cacheable would help as part of that.

We should probably postpone this issue on the 4 issues that @mpp linked in #96.

My site has an events page - Only future events are in the view by filter. The internal page cache does not respect the filter criterion in the view, anonymous users always get outdated views. Do I have to disable the internal page cache for all of my site, just to have the events page show current affairs?

@Hungerbug: That's what I had to do in that situation. You may find that the combination of the dynamic page cache and bigpipe makes the site almost as fast as it was with the anonymous page cache anyway.

What we implemented in our project is a response subscriber that converts a non-zero, non-permanent max age to the Expires header, which allows time-based expiration without disabling page cache entirely for pages that set max-age 0.

@Berdir we are doing the same; Wim mentioned https://www.drupal.org/project/cache_control_override earlier in this thread. Adding that workaround to the issue summary may help some people who are encountering this without having to dive that deep into core until there is a permanent fix.

@malcom_p The cache_control_override module makes filters on the view active, even for anonymous users. Performance does not suffer noticeably. Deservers more prominence indeed, I probably never learned of it, if it was not for your post!

i think you won't be able to solve such problems without client side javascript code that updates part of the page outside the context of the whole page caching .

Changed issue link for a better overview

I'm not sure to understand why this has to be postponed.
Some pages can have no forms, no book navigation block, no language switcher and still need to have a proper bubbling of their max-age.
Fixing this issue just won't change anything for people that do have one of the above on their pages but could really help all other websites.

Am I right or am I missing something?

"I'm not sure to understand why this has to be postponed."
Neither I do, certainly because this issue is simply to hard. Seriously this should be a blocker: a cache system with a broken cache invalidation is useless.

And don't make me wrong, this is a complicated issue. But it's why you use a system like drupal at the first place, so you don't have to take care of super complicated things like cache.

This major bug in the cache system is now 4 years old. It's unbelievable.

If someone know how to fix it, but need money to work on that, please give us a price.

Some pages can have no forms, no book navigation block, no language switcher and still need to have a proper bubbling of their max-age.
Fixing this issue just won't change anything for people that do have one of the above on their pages but could really help all other websites.

This is true, but when they have one of those elements the page should behave in an a correct way. Because the current behavior, without those issues fixed is not correct, this will lead to far less hitrate in the page cache/reverse proxy. If I understand it correctly, that is the reason why this issue is postponed by those issues.

Seriously this should be a blocker: a cache system with a broken cache invalidation is useless.

This might be overstating it, the cache system as it is implemented right now is not useless.

Atm #2888838: Optimize render caching is marked as a blocker for #2232375: Support CacheOptionalInterface in BlockViewBuilder, use it in language switcher block to prevent max-age 0 bubbling which in turn is a blocker for this issue.

I updated the summary to reflect this.

Hmmmm. I've been experimenting again with the above mentioned CCO module recently to see if it can be of any help with this long standing issue:

https://www.drupal.org/project/cache_control_override

The situation is: I have a block somewhere on the page which is showing dynamic data from an external source (weather). Now, I want it to be updated regularly. First attempt: Set #maxage of this block to 0. Without the CCO module, this is ignored by the static page cache. I get X-Drupal-Cache: HIT and the weather never updates. Now I install the CCO module. It detects that the maxage 0 has bubbled up and then switches off all caching (static and dynamic). Now, the weather info is updating. Nice. But: I wonder whether this is really correct. IMHO only the static page cache should be disabled, because other blocks on the remaining page could still of course benefit from the dynamic cache.

Now, in addition, it would be even better to be able to set #maxage to 120 so the weather only updates every 2 minutes. The CCO module doesnt support that. It only detects whether the value is 0. Which is understandable as the static page cache itself AFAIK doesnt yet support a max-age. It would of course be cool if the static page cache would update after 2 minutes.

IMHO, It should simply work like this: I have a website. There are several blocks/paragraphs. They define their separate max-age: 50, 120, 900, whatever. The minimum of all max-ages bubbles to the top and is used for the static page cache and cache-control header.

Well, looks like we're 25% of the way to unpostponing...

this is so frustrating. this should really work for a cms before releasing it :(

who released drupal 8 seriously. the most basic shit doesnt work!

Lets try to be more constructive.

Patches welcome!

Hi all,

Are the patches very olds? Is a good point to start? Or is better start from scratch.

Greetings!

I have just found this issue and after spending some time reading through I tried to do the IS update to help future readers. Most of the people who have commented on this issue already know the facts better than me, so please correct me if I misunderstood.

I have raised #3075116: Document limitations of Cache Control Override asking CCO to document the limitations on the project page.

Adding #2985253: content_moderation_entity_access() wrongly adds uncacheable dependencies as related issue, since if it doesn't get fixed before this one, that's another place where a max-age=0 ends up being generated and bubbled up.

Attached a patch for 9.x that checks max-age from cache metadata when storing response. It could be used as a workaround when a site does have issues with block caching (#2828136: A single uncacheable block can make the entire website uncacheable).

Few years ago, I wanted my pages to be cached the whole day, because one block changed at midnight every day... and I didn't manage to do that before I noticed that it seems that the age is lost when we get the cached_element in RenderCache.php So I substracted the request time when I get the cached element in RendereCache :

      // Return the cached element.

      // three lines appended
      if ($cached_element['#cache']['max-age'] !== Cache::PERMANENT) {
        $cached_element['#cache']['max-age'] = $cache->expire - \Drupal::time()->getRequestTime();
      }

      return $cached_element;

And I wrote a sample module, a kind of proof of concept : https://github.com/mathieu71/render_cache which allowed me to do what I wanted.

I just want to make sure I'm reading this correctly. The page_cache module is enabled by default and so is dynamic_page_cache. If page_cache module is enabled, dynamic_page_cache (and max-age) essentially doesn't work for anonymous users?

If that's correct, then shouldn't there be an explanation of this at /admin/config/development/performance? There isn't even an indication on that page (the only one that I know of where I can configure Drupal core caching) that two caching modules are involved. I really think that if this is by design (as Berdir says here), then there ought to be an explanation on this page that explains the differences between the two modules and when it might be best to simply disable page_cache entirely (such as when most of your visitors are anonymous but you have a lot of dynamic content). I think we should even be able to enable or disable the two modules from this page.

If this is truly by design, then this seems like mostly a communication issue doesn't it?

Another example: In the "Cache API" documentation there is no explanation that two modules are involved. It doesn't explain that most of the documentation is actually about functionality provided by the dynamic_page_cache module and completely unrelated to (and in fact negated by) the page_cache module (as far as i can tell). In fact you don't learn this until you read to the bottom of "Cache max-age". There is a page about Internal Page Cache and another about Dynamic Page Cache but they're not linked to the the main "Cache API" documentation and don't even link to each other. It's chaos.

I'm frustrated I wasted time wondering why max-age wasn't working because of bad documentation. If I knew enough to fix this documentation I would, but I can't even say with certainty that what I'm saying now is 100% correct.

That said, if someone in the know confirms what I said is correct, I will hack at this documentation and link these pages together.

After fighting for a very long time with this bug, I've come to the conclusion to switch off page_cache module. IMHO it just shouldn't be enabled by default. It would be much better the other way round. If you really have only static pages and a massively visited site - THEN it might make sense to enable this and shortcut many requests while being fully aware that you need to be extra careful when you do changes. Currently, it's automatically installed and you might think that it might work for most cases when in fact it causes huge problems. My mistake was to actually try to make this work. And it gives the superb dynamic_cache a bad rep, because you thin that it doesn't work when in fact only the page_cache causes all the trouble. Just uninstall page_cache is the recommendation I can give.

Here's patches that include updates to the `Cache-Control` header, as well as the cache storage expiry time.

For people that stumble upon this issue and look for a solution to cache blocks for a specific amount of time there is a fairly simpel solution.

Just add your own cache tag to the block and create a cron job that invalidates this tag. This will bubble up to the page cache.
In the cron job you will be able to specify how often this job has to run, eg daily in my case.

See the following post that I found regarding the issue: https://ashday.com/blogs/custom-cache-tags-max-age-drupal

I strongly agree with #130

I also agree with #130 and #134. But it's not a solution. If I disable page cache then server response time is increased and the page took time to load in less network. Is this a definite solution?

@heni_deepak: As this is not really moving forward for a long time, I guess the Cache Control Override module (see issue description) is your best bet if you want to keep page_cache. Another, perhaps a bit controversial/unconvential tip that nevertheless worked for us: Don't necessarily search for solution to your performance problems in the cache. Disable page_cache. And if your website now gets slower, check if there is sth. else you can do. Perhaps do some profiling (XDebug, Tideways, Blaxckfire) if you have other bottlenecks in your code. And, often overlooked, hardware isnt really that expensive these days IMHO. Why not use a second server? Or faster CPU? Or faster disk (SSD). All these things can make a huge difference. Is your APCu/OPcache/Memcache configured correctly? They are very important even if you only use dynamic_cache!

i'm sorry to repeat but what about using angular ?
https://www.drupal.org/project/drupal/issues/2352009#comment-12814040

i'm sorry to repeat but what about using angular ?

Isn't that what BigPipe is for?

Just ran into this issue, and want to get page_cache working correctly on a site that has a max-age set from render cache.

There appears to be two currently possible alternative solutions:

• Latest patch from this issue #131
• Using cache_control_module and patch from https://www.drupal.org/project/cache_control_override/issues/2962699#com...

Both of these solutions are working OK for me.

But I'm not sure which of these is the best option. The core patch from this issue is smaller, but looks like it's unlikely to be merged in anytime soon.
Patch for CCO module also does the trick, but that module seems to be more intended to solve the issue for external cache's/CDN's (so again it may not get merged in).

Possibly I'm using page_cache in a way it was unintended, i.e. a poor-mans Varnish. But I can't really see how else it is supposed to work? (Perhaps we are scared of breaking the internet by messing with page_cache)?

I was also bit by this and came up with a patch before finding this issue. It's available at https://www.drupal.org/project/drupal/issues/2938524#comment-14457583 in case anyone is interested.

mass.gov needs this bubbling as well.

@plach - #141 patch looks good to me. is there a reason you chose to use Expires header instead of Cache-Control? Looks like Cache-Control is the modern way. Or call $response->setMaxAge().

@moshe weitzman

No good reason, let's switch to Cache-Control :)

There is a reason, expires is what the internal page cache module uses.\Drupal\page_cache\StackMiddleware\PageCache::storeResponse().

To make it clear again. There are several issues that are blocking this, without having those resolved, this will absolutely go nowhere.

Per issue summary, https://www.drupal.org/project/cache_control_override is an option, but the same blocks apply to that. Language switcher block? no caching for you. Forms? no caching for you. certain views? no caching for you.

We worked around that by ignoring max-age 0 and only explicitly supporting specific age expiration definitions, essentially this, in a KernelEvents::RESPONSE event:

  public function onKernelResponse(ResponseEvent $event) {
    $response = $event->getResponse();
    // Do nothing if a response does not have cacheable metadata.
    if (!$response instanceof CacheableResponseInterface) {
      return;
    }

    if (!\Drupal::currentUser()->isAnonymous()) {
      return;
    }

    $cache_max_age = $response->getCacheableMetadata()->getCacheMaxAge();
    if ($cache_max_age > 0) {
      $response->setExpires(new \DateTime($cache_max_age . 'sec'));
    }
  }

If you want to hep move this forward, we need help to review and complete the documented child issues.

Thanks for the clarification/update. One thing I don't understand, though: Currently those mentioned page elements emit a max-age:0 but then we are almost "happy" about the fact that this has no effect ;-) ? If the language switcher (for example) is currently not cachable, then, well it's not cachable right? Why would we want the page cache to serve a page with a language switcher on it from the cache anyway? I thought this issue here is only about the idea that when a page contains elements that are not cachable then the whole page simply should not be served from the cache.

This patch is based on #141 but it uses Cache-Control. This is better for sites that dont use Drupal Page Cache module (i.e. every site I've worked on). Before you use this patch, read @Berdir comment in #146 to make sure you site doesn't use the known problematic bits of Drupal, and do your response header testing.

Update the issue summary to link to the referenced comment.

This patch is based upon #131, but ignores cacheable metadata's max-age if it is 0. This should work around the issues like suggested by @Berdir in #146

Turns out, skipping max-age of 0 (that is, handling max-age=0 as permanent in page cache) is not a good solution; if you use max-age to ensure a cached page is rebuild after a certain timestamp, and if a request happens on exactly the expiration time, the response must not be cached (as the max-age is 0). At least in such cases, page cache must not handle max-age=0 as cache permanently.

And since we (currently) cannot determine why max-age was set to 0, I think page cache should never cache responses with a max-age>=0. Even if this means that certain (or all) pages will no longer be cached by the page cache. Note this means that for example multilingual sites using the language switcher block (#2232375: Support CacheOptionalInterface in BlockViewBuilder, use it in language switcher block to prevent max-age 0 bubbling) no longer have their pages cached by the page cache module, but at least the page cache won't be incorrectly caching pages permanently that have a max-age>=0. Also, those pages will probably still be cached by the dynamic page cache.

I think we should deprecate using the Expires header as the expire time for the page cache and just use the max-age of the cacheable metadata of the response. There are several places (like access checks) that don't have access to the final reponse and can only pass cacheable metadata.

I also think this issue is actually about two different issues:

1. Page cache incorrectly ignoring the max-age of the cacheable metadata of the response (patch #131)
2. The Cache-Control response header not containing/reflecting the max-age of the cacheable metadata of the response (patch #148 or something like cache contol override module)

For now, if your site depends on time-based cache invalidations you at least need the patch from #3447821: Stacked caches result in max-age drift in caches, even if your site is not using the page_cache module.

If you are using the page_cache module, you also need the patch from #131 as a complete solution.

If you are not using the page_cache module (but are using another public/managed caching solution, like a reverse proxy like Varnish), the patch from #148 might work.

The Cache Control Override module can be used additionally if you want to enforce a minimum and/or maximum on the max-age of the Cache-Control response header.

This patch is a combination of the patch in #131 and #3447821: Stacked caches result in max-age drift in caches for usage in sites <=D10.2

Patch #154 incorrectly did request_time - expire instead of expire - request_time to recalculate mag-age

I'm still subscribed to this issue 8 years later and I re-read the duplicate I created and I'm still thinking about this:

There are existing issues for this, it's mostly by design and I'm not sure if it can be changed. It would result in many pages no longer being cacheable that currently are, for example as forms declare themself as uncacheable.

#2732129-4: FinishResponseSubscriber::setResponseCacheable() does not respect Cacheable Metadata for Cache-Control header

And now I'm wondering.... so what? Like if a bunch of things become uncachable in Drupal 11.0 and we go and fix those things (individually) in 11.1+ that seems... fine? Am I missing something here?

maybe consider my comment once again ?
https://www.drupal.org/project/drupal/issues/2352009#comment-12814040

i think you won't be able to solve such problems without client side javascript code that updates part of the page outside the context of the whole page caching .

Committed #155 to an issuefork so tests actually run on that code.

i think you won't be able to solve such problems without client side javascript code that updates part of the page outside the context of the whole page caching .

That seems fine?

In my mind, undercaching is always prefered to overcaching. In an ideal world, it would be perfectly cached, but if the form (or whatever) declares that the whole page is uncachable because of it's inclusion, then.... it is what it is. The only thing you can do is refactor that element to be cacheable (i.e. generate the dynamic bits with JS and/or WebAssembly)

Or we figure out that we don't actually need to declare these things as completely uncachable anymore.

For instance, what if we decide to rely on SameSite=Lax rather than using our custom CSRF protection? This is what other projects like Next.js do and maybe that would be fine for us as well?

CSRF protection doesn't kick in unless you have a session, in which case page caching doesn't happen anyway. However if forms are uncacheable purely because they might have CSRF protection, then that would be breaking page caching for no reason then.

And now I'm wondering.... so what? Like if a bunch of things become uncachable in Drupal 11.0 and we go and fix those things (individually) in 11.1+ that seems... fine?

I have personally seen multiple sites that only stay online because of Drupal's internal page cache or sometimes because they cache pages in a CDN. If we suddenly make those sites uncacheable, they will go down. Something to test would be this patch + a site with anonymous comments enabled (and the form displayed on node pages) to see if those pages still go into the page cache. It would be OK if some poorly behaving contrib modules break the page cache after this lands, although we still might want to make the behaviour configurable as a first step, but not if lots of pages output by core do.

#2578855: Form tokens are now rendered lazily, allow forms to opt in to be cacheable would make forms render cacheable.

Committed #155 to an issuefork so tests actually run on that code.

Made outdated patches hidden, MR#8155 contains the latest changes _always_.

Read through chunks of this issue again and as far as I can tell two separate things are being discussed here and while both of them are related to the cache max-age and its (lack of) bubbling, only one of them seems to be the cause for the seeming "intractability" of this issue:

1. Things having an explicit nonzero, finite max-age
2. Things setting their max-age to 0 to declare themselves as uncacheable

While I would absolutely applaud also getting the second use-case somehow properly integrated with page cache it seems getting the first use-case to work is technically just as simple but does not have any of the repercussions in terms of page cache integration that the second one does.

So I would like to propose to solve that issue first, i.e. to bubble nonzero max-ages into the page cache and explicitly leave the "max-age = 0"-case as a todo for the future.

The only place in core we ever set a nonzero, finite max-age, as far as I know, is the time-based caching plugin for views, so the only affect this would have in core is those views actually being properly expired by page cache. But the benefit would be that any contrib or custom modules that either (knowingly or unknowingly) do not work with Page Cache or somehow have to employ hacks to set the response expiry manually will now work with Page Cache automatically (and without any hacks).

Would love to hear if people agree with the splitting up of this problem and if so, whether or not the first (nonzero) part should happen in this issue and the zero part be handled in a follow-up or if we should keep the zero part here and split out the nonzero part.

I do not have all the specific elements in mind, but generally, yes, I'm in favor of splitting this kind of big old issue in several smaller ones, so that they can be reviewed more easily. That's the kind of practice encouraged by core reviewers: https://www.youtube.com/watch?v=_uCfmFH4rUs

I think #2232375: Support CacheOptionalInterface in BlockViewBuilder, use it in language switcher block to prevent max-age 0 bubbling could be handled without evaluating the render cache i think. Hoepfully we can get some activity there.

For this issue, we should try and keep the blockers as small as possible, seems it is a pretty short list.

This patch contains a snapshot of the current state of the MR so it can be safely used with composer-patches.

Note: cache_control_override, at least in it's current from, does _not_ deal with page_cache, see #2916705: Page cache isn't invalidated

There was lot of activity on #2888838: Optimize render caching and related issues in the last few weeks and months. That said, I don't think that is actually blocking this at all, as it's just optimizing (the scope of that meta changed a few times).

So removing it and reducing pp, although i have no idea what the correct number for that is. The most obvious one is still the language block, but I'm working on that.

> Things setting their max-age to 0 to declare themselves as uncacheable

> Would love to hear if people agree with the splitting up of this problem and if so, whether or not the first (nonzero) part should happen in this issue and the zero part be handled in a follow-up or if we should keep the zero part here and split out the nonzero part.

In fact, our custom response subscriber does exactly that, it ignores a 0 max-age. But we've also been using the language block patch for years. The problem is that in this scenario, anything setting a max age 0 will remove the ability for anything else to set a finite specific cache max age. Most multilingual sites have a language switcher (or multiple) on every page, so this won't do anything for them.

I recommend rebasing this so we can see the current effect this has on the performance tests.

The patch in # 148 for Drupal 11

The language block blocker is resolved. Started a look at the remaining test failures, but it gets "interesting" quickly.

PageCacheTest fails on the fact that with this, 404 pages are no longer cacheable in page cache. That is because \Symfony\Component\HttpKernel\EventListener\RouterListener::onKernelRequest throws a non-cacheable exception (as symfony doesn't know about cacheable exceptions), and then in \Drupal\Core\EventSubscriber\DefaultExceptionHtmlSubscriber::makeSubrequest(), we run this code:

      // Persist the exception's cacheability metadata, if any. If the exception
      // itself isn't cacheable, then this will make the response uncacheable:
      // max-age=0 will be set.
      if ($response instanceof CacheableResponseInterface) {
        $response->addCacheableDependency($exception);
      }

So this was kind of always the intention, but at the same time, we also want those responses to be cacheable and have a built a way to deal with invalidations with the 4xx-response cache tag. On HEAD, we set Cache-Control to must-revalidate, no-cache, private, set the debug header to Uncacheable... but we actually still cache it. I'm not sure what the right change here is. Should we change DefaultExceptionHtmlSubscriber::makeSubrequest() to not break caching? That will mean it will also be cached externally and in dynamic page cache. Maybe only ignore it for a client error to keep it in sync with \Drupal\Core\EventSubscriber\ClientErrorResponseSubscriber::onRespond? At a glance, the dynamic page cache test coverage does not seem to have explicit test coverage for a 404, so it might not cause test failures, but it will be a behavior change.

"That will mean it will also be cached externally and in dynamic page cache" => If we approach that question from a mere functional POV, I'd spontaneously say that's even desirable, right? 404 pages are exactly those pages that IMHO would profit the most from an external cache like Varnish etc. considering how many invalid page calls are raining down on a usual Drupal website per day. Stuff like /wpconfig.php (WP login attempts etc). The only exception would be if the 404 page is reacting individually on the specific request. But that's rarely the case and if someone is implementing this, they should probably send a bubbling max-age 0 with that content. Just my few thoughts on this...

I agree, but it's a behavior change and to be clear, it does not reflect the current state of this MR. Right now, 404 pages are completely uncacheable, internally and externally. We'll need to open another issue and align this, which means this is again/still blocked.

A couple of related issues for 404s #3516169: Adding url.path to breadcrumb cache context results in every 404 page getting a different dynamic page cache entry #3516173: Block status code visibility condition should use a status code cache context - we do have some bad behaviour including in core on 404s but it's also fixable.

Re #171:

That is because \Symfony\Component\HttpKernel\EventListener\RouterListener::onKernelRequest throws a non-cacheable exception (as symfony doesn't know about cacheable exceptions)

OK, couldn't/shouldn't we work around that, then? So that instead of the non-cacheable exception a cacheable is thrown directly by an overridden/alternative listener as we know the 404 just depends on the url cache context (or similar, didn't check in detail, but it definitely seems "knowable") or alternatively we check for the exception thrown from RouterListener over in DefaultExceptionHtmlSubscriber (which also seems doable albeit somewhat hacky) and convert it into a cacheable one.

So I think we could still leave the general applying of the exception cacheability (including uncacheable ones) and just fix the one(s) that are actually problematic. That would still make it a behavior change as random contrib/custom controllers that throw NotFoundHttpExceptions are now now longer cached were they were before, but with a change record explaining to convert to the cacheable exceptions that might be acceptable?

Also not 100% what specific issue you want to open in #173, i.e. which part you think cannot/should not be fixed here directly. Would love to push this forward, so would appreciate your input.

Yes, I think we should address it, just not quite what the best option is, there are going to be some behavior changes either way if we cache it consistently (dynamic page cache + external caches). I think a separate issue makes sense because I'm sure there are going to be other issues with other failing tests and then we can discuss and decide on this specifically with a dedicated change record.

another note: This includes code from #3447821: Stacked caches result in max-age drift in caches, or an earlier version of that. That might not be a hard blocker as it's already an issue with existing stacked caches, but this does make a larger issue. But I think that one is close.

OK, that's fair enough. Does the fact that this is then again blocked on at least one other issue which is anticipated to be at least somewhat contentious change your opinion on #165, i.e. could we maybe fix this for all cases where the max age is strictly larger than 0 with an explicit @todo to resolve the === 0 case once the other issue is solved?

> which is anticipated to be at least somewhat contentious change your opinion on #165

It might not be, I didn't get around to doing it, but I think if we find a good technical solution then we might be able to get this done reasonably quick. If you give it a try then I can help with reviews.

> could we maybe fix this for all cases where the max age is strictly larger than 0 with an explicit @todo to resolve the === 0 case once the other issue is solved?

I'm not against it if the 404 issues turns out to be complicated. This is a big enough change that the whole thing maybe should be behind a feature flag anyway for BC (off by default for now, only non-zero, all). And then we either drop the flag completely or switch the default later.

I'm a bit surprised that there is only one test fail left now, two previous ones were contact, which is removed from core now and one was a weird performance change, so this might be closer than I thought.

OK, taking that as a mild "no" ;-)

Took stab at the cacheable 404 thing, though, now, so would love some thoughts over in #3580545: Make empty route lookup cacheable.

#3580545: Make empty route lookup cacheable is in, this seems to have a surprising amount of test failures again though. Some are expected, the generic rest tests are repeating many times.

What's concerning is that the random fails are back that I've seen before:

     'X-Drupal-Cache-Max-Age' => Array &9 [
-        0 => '799',
+        0 => '800',
     ],

Drupal\Tests\jsonapi\Functional\EntityTestComputedFieldTest::testGetIndividual in https://git.drupalcode.org/project/drupal/-/pipelines/806973/test_report...

This is actually due to the changes of #3447821: Stacked caches result in max-age drift in caches, which I think we'll need to resolve first separately in that issue. The stacked max-age fixes now sometimes cause off-by-one assertions on the actual max-age on those requests. I'll also add a comment on that other issue on that.