Page cache isn't invalidated

I found debugging stuff like this extremely hard. I had some success going to "lib/Drupal/Core/Cache/CacheableMetadata.php" and e.g. doing a backtrace in merge() when an unwanted max-age:0 is about to get merged in.

Besides that, I still think this issue is very valid. This module currently only fixes the fact that a default Drupal installation doesnt properly bubble the Max-Age to the Cache-Control HTTP header. It doesnt do anything about the fact that the page cache doesnt support time-based expiration *at all*. Of course, I could write another module for this. But I think it might make sense to integrate it here, so we don't get to a point where we require a whole library of cache fixing modules to get a decent working caching mechanism in Drupal...

Ah. I see, I'm wrong. Page Cache seems to use $expire in core/modules/page_cache/src/StackMiddleware/PageCache.php, storeResponse. But I also found that only -1 ends up in the cache_page table. Weird.

Hello,

The issue here is that PageCache::storeResponse() uses the Expires header but not Cache-Control to invalidate the page cache.
The attached patch helps workaround this problem.

I just noticed that #2962699 contains a similar patch.

Here is a reroll for 2.0.0-rc2.

Rerolled for 8.x-1.x.

Hi,

Thanks for the reroll on 2.0.0-rc2. I use this patch now instead of #2962699-5: Expires header not set for the version 8.x-1.x.

cache_control_override needs to take global max-age into account.

Here is the patch from MR for Composer usage.

We should probably decide which patch to keep between #2962699: Expires header not set and #2916705: Page cache isn't invalidated.

MR rebased for version 2.0.1

Hiding patches to make it clear the MR should be reviewed.

Added a comment on the MR.

FWIW, I disagree with considering the max age and that is a change to how core handles this.

Core specifically only sets the max age setting on the response and the internal page cache is forever. If you have no reverse proxy or something like this then it is common to set cache.page.max_age to a low value, because you have no means to invalidate it.

The UI label for the core setting is specifically " Browser and proxy cache maximum age".

Then again, our custom implementation of this only sets the Expires header and doesn't touch max-age at all. This is also how I imagine the core implementation will behave, it will consider cacheability max-age for the internal page cache, without altering the Cache-Control header.

@berdir I understand your point but I think people using cache_control_override often use a reverse proxy and a high max age setting.

However I agree the patch should not use the cache.page.max_age setting from core.
Since #3003716: Coerce bubbled max-age to a floor or ceiling, cache_control_override has a specific max age setting different from the core one and the patch should use that.

Yeah, it's tricky.

FWIW, the project page explicitly talks about solving the problem of the referenced core issues, which it does in fact not do yet, it does something different. So that seems a quite weird. This issue then actually makes it do what it says there, but it still does the max-age, which might result in quite unpredictable results.

The max max age check is already there above, so yes, I think the extra check can be removed.

Still, the problem IMHO is the combination of the two headers and applying the same rules to them.

For external/proxy caching, having a maximum max-age makes a lot of sense, most sites I think don't really have any means of cache tag based invalidation, definitely not in browsers ([#/3350593]). But applying that same max-age on the internal page cache could result in considerably lower cache hit rates. Then again, it will only run if there is a specific max-age set that's not permanent. permanent is still default core setting.

It all seems quite unpredictable. I'm not a maintainer, I just ended up here because I've been telling people in the past, including in those core issues to use this project, only to realize that it doesn't actually do what I expect.

As you said, it probably also depends on your use case and how you handle caching. For us, that's basically two things:
1. We set a low max-age (like 15min), which means that every 15min or so, a given page needs to be revalidated against the internal page cache, which caches by default forever.
2. For a few specific pages, we want to have limited and lower max age/expire in the internal page cache, for example event listings.

So this currently, with and without this issue, isn't a good fit for us and I'd assume also many other small to medium sites that don't have a custom reverse proxy with invalidation support.

I think the most sensible thing for this project would be to have 3 different on/off settings for max-age, s-max-age and expires, each with it's own separate min and max. And for a consistent result, I'd then always enforce the given max-age setting, even if the cacheability metadata says permanent. Then you can customize it to fit your use case.

I added logging if setExpires() fails.

And I stopped using the page cache max age setting as @berdir suggested.