Split the internal page cache from the rest of the render cache

Big +1 on this, it'll help mitigate some of the trade-offs in #2526150: Database cache bins allow unlimited growth: cache DB tables of gigabytes! as well.

Very much in favor of splitting up the bins again. I honestly have no idea what the reason was to combine render cache and page cache in the first place.

This should be green now.

Very much in favor of splitting up the bins again. I honestly have no idea what the reason was to combine render cache and page cache in the first place.

We never had a dedicated render_cache bin, so render cached items (in 7.x) tended to end up in cache or cache_block depending on what was specified in the render array. In 8.x we renamed cache_page to cache_render to include those items (since it's a cache of rendered HTML), but that was long before we enabled render caching by default.

Why cache.page_cache and not cache.page?

#14: hah, nice catch! So this is why (quoting HEAD):

    // This form allows page compression settings to be changed, which can
    // invalidate cached pages in the render cache, so it needs to be cleared on
    // form submit.
    $this->renderCache->deleteAll();

Page Cache used to store its cache items in the render cache bin, now it stores it in the page_cache cache bin. So #14 is changing too much; rather than adding a new thing to clear, it should be updating which thing is being cleared.

Furthermore… #2775381: response.gzip is redundant and should be removed just landed. Which means we don't have any of this "page compression" stuff anymore. So it's completely obsolete. #2775381 failed to remove it because… there was zero test coverage for this.

Fixed :)

To answer your question about injection: that's pure refactoring, it's out of scope for this issue. Would be great to clean up in another issue. The only reason AFAIK: legacy tests.

#15: Hah, awesome catch!

I think this is ready now.

Oops.

I'm curious here, wouldn't page_cache_maximum_age theoretical requires page cache changes, when you lower the amount of time we store page cache entries? Maybe this is just a super edge case so its not worth carrying about it though.

#22: I think that makes sense, but it's out of scope here. We're just moving things to a different cache bin here :)

Well, maybe I just don't get why it is in scope to remove the cache clearing then :(

You're right, it isn't. @fgm's changes in #14 triggered that. Reverted all changes in PerformanceForm.

Created #2892179: Follow-up for #2775381: clearing render cache in PerformanceForm::submitForm() is obsolete for this.

Oops.

For behaviour equality, shouldn't we clear now both buckets?

I think you mean not only

    $this->renderCache->deleteAll();

but also

    $this->pageCache->deleteAll();

?

That's exactly what @fgm did in #14 and I explained in #17 why that is not necessary. We now have #289179: Idea, need input : Book review rating system to remove $this->renderCache->deleteAll();.

If that's not what you meant, can you please clarify?

Well, I still think about the max age time bit.

I think @dawehner has a point here. This is not only about max-age but also js and css aggregation settings which affect all (cached) pages. Hence I'd rather leave that piece of code in there and just swap out the bin.

We can continue the bikeshed about whether or not it is necessary to flush the page cache in #2892179: Follow-up for #2775381: clearing render cache in PerformanceForm::submitForm() is obsolete.

Looks good I think, surprisingly small patch, nice.

Although we possibly want to do a cache record for this, in case people (incorrectly) the render cache bin directly or something like that?

I'm also pretty sure we had existing issues about having a way to separate the page cache from other render caches, e.g. with a special cache tag, but can't find them right now. I think that's no longer necessary now.

One thing on the cache bin name, though.

cache.page_cache is a bit repetitive, but it is a) the module name/namespace and b) would be consistent with cache.dynamic_page_cache. Thoughts?

I didn't even think about it being the same as the module name, I guess because it's Internal Page Cache in the UI. So... I think it's OK to go in as cache.page.

@fgm's idea is a good one on a wider rename.

Committed 9fa0f16 and pushed to 8.4.x. Thanks!

9fa0f16 returns a bad object and I'm not seeing the commit in Git either. Did you actually push this?

@fgm++
@catch++

This actually makes DX so much better depending on what kind of site it is, that it's probably worth a mention in the release notes?

Yes it's not an API change, but it's worth an informational change record for site admins who might want to tweak memcache allowances and a release notes mention.

Draft CR is at https://www.drupal.org/node/2896679.

Added a sentence about that, but I think that's going to vary heavily on the specific site as I commented. You're welcome to edit if you have a better idea on how to write that. Published.