Simplify performance settings page

Looks good to me! I'd say RTBC if it comes back green :)

I do not agree that this is useful. In Drupal 7 it was possible to use the internal cache without having to set Page cache maximum age. In my opinion this is a regression and #2177461: Refactor page caching into a service addresses that.

Use internal page cache is the checkbox formerly used to enable the page cache. I found where the regression was introduced: #1853086: Remove cache.page.enabled in favor of an explicit internal cache setting. The problem is this hunk:

--- a/core/lib/Drupal/Core/EventSubscriber/FinishResponseSubscriber.php
+++ b/core/lib/Drupal/Core/EventSubscriber/FinishResponseSubscriber.php
@@ -91,7 +91,8 @@ public function onRespond(FilterResponseEvent $event) {
     //   use partial page caching more extensively.
     // Commit the user session, if needed.
     drupal_session_commit();
-    if (config('system.performance')->get('cache.page.enabled') && ($cache = drupal_page_set_cache($response->getContent()))) {
+    $max_age = config('system.performance')->get('cache.page.max_age');
+    if ($max_age > 0 && ($cache = drupal_page_set_cache($response->getContent()))) {
       drupal_serve_page_from_cache($cache);
       // drupal_serve_page_from_cache() already printed the response.
       $response->setContent('');

In Drupal 7 it was necessary to enable internal page caching in order make external page caching work. This is admittedly nonsense. In Drupal 8 the nonsense was just reversed and now internal page caching is only possible when external caching is configured.

In my opinion those two things simply do not need to be coupled together.

I agree with 1. However 2 does not match the current implementation. The maximum age setting does not have any influence on the expire-setting of the cache-object. It is true that drupal_page_set_cache will set the expire-attribute of the cache-object to a custom timestamp, but it will only do it if the Expire header was set by a custom module (core only sets it in drupal_serve_page_from_cache which comes after the cache object was saved). Normally the expire-attribute of the cache-object is set to CACHE_TEMPORARY (D7) and Cache::PERMANENT (D8) respectively.

Also I'd like to point out that it may be desirable to specify different expire-values for internal and external caches when delivering directly to the browser (no reverse-proxy). The internal cache should be as long living as possible, because we can purge it if necessary, while the external ttl should be set to a lower value on sites with frequent updates.

I would rather get rid of the Use the actual timestamp from an Expires header, if available. business. It does not make much sense to bind the expiry-attribute of the internal cache to any of the Expire / Cache-Control headers of the response.

The maximum age you configure on the performance settings page is only set when a page is served from cache!

No, that is actually not the case. On a cache-miss drupal_page_set_cache returns the cache-object and that is sent just like a cache-hit using drupal_serve_page_from_cache where the headers are applied. This is actually where the tight coupling between internal and external cache is coming from.

Regarding the new proposal: I like it very much. Setting Cache-Control: max-age is not only necessary for turning on the browser cache but also for operation behind a reverse proxy. This should be specified more explicit.

Maybe this could work in the description text:

The maximum time a page may be cached by a browser or a proxy server.

Anything else?

A patch to review? :)

I'm *big favor* of getting this in, we should better protect our users against the tight relation between these two settings. Hopefully that will also reduce the amount of sites running without internal page caching that don't have a internal cache/rproxy in front of them. This often fuels competitors with the "Drupal is slow" argument so moving more towards "fast by default" (or almost default with hiding the checkbox at none) is better.

I've been testing #2124957 and related sub issues recently and with that shaping well up this is all becoming more and more important, as the internal page cache will be "less in the way" and less of a irritation to site users but something that just works and should be enabled as much as possible.

Niels

Making this a sub-issue of #2257695: [meta] Modernize the page cache.

Open for anyone to move ahead as Wim has other focus issues these days.

External cache (adding Cache-Control header) is now separated from the internal cache, see #2257709: Remove the interdependence between the internal page cache and management of the Cache-Control header for external caches. The settings should be updated to reflect this.

Reroll of #1.

#2368987: Move internal page caching to a module to avoid relying on config get on runtime is going to remove a few options from that page.

This is how the page looks now.

No idea if there's still something to simplify or improve?

I tried to read up on this issue (since I was the one who made the change being objected to here), but this issue is all over the place now. Maybe someone could start a new, more focussed issue for the problem being solved and proposed solution?

I think there is still stuff to do here. The description which says

Note: Drupal provides an internal page cache module that is recommended for small to medium-sized websites.

is largely irrelevant because this setting has nothing to do with that. It only affects the headers that are outputted from Drupal. Additionally we have a clash of terminology because we're setting the

Page cache maximum age

value and this has nothing to do with the internal page cache module.

Here's a patch that changes the Page cache title to

Browser and proxy cache maximum age

so there can be no confusion and only displays something about the Internal Page Cache module if it is not installed.

Link to screenshot of what it now looks like when page_cache is not installed: https://www.drupal.org/files/issues/2018-03-12/Screen%20Shot%202018-03-1...

Updated the Issue summary.

This is the current page:

This is how it looks after the patch is applied

With the page cache module enabled.

With the page cache module not installed.

@borisson_ I think you might have the

With the page cache module enabled.

and

With the page cache module not installed.

images around the wrong way.

#40, I did! Thanks! Sorry. Edited the comment to fix the order.

Nice to see small UI cleanups like this happening.

Committed 2f1a5b3 and pushed to 8.6.x. Thanks!