Support an independent max-age for 4xx responses

mr.baileys is working on a different issue for now.

I added this in the FinishResponseSubscriber

Looking good!

Failed because #2368987: Move internal page caching to a module to avoid relying on config get on runtime landed in the mean time.

A bunch of nitpicks. Point 3 needs feedback from others, e.g. Fabianx. Pinged him.

1. +++ b/core/modules/system/config/schema/system.schema.yml
   @@ -163,6 +163,13 @@ system.performance:
   +              label: 'Page cache for 4xx results'
   

   s/results/responses/

2. +++ b/core/modules/system/config/schema/system.schema.yml
   @@ -163,6 +163,13 @@ system.performance:
   +                  label: 'Max age for 4xx results page cache'
   

   Max age for 4xx responses

3. +++ b/core/modules/system/src/Form/PerformanceForm.php
   @@ -134,6 +134,13 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   +    $form['caching']['page_cache_4xx_maximum_age'] = array(
   +      '#type' => 'select',
   +      '#title' => t('Page cache 4xx maximum age'),
   +      '#default_value' => $config->get('cache.page.4xx.max_age'),
   +      '#options' => $period,
   +      '#description' => t('The maximum time a page with a 4xx response can be cached. This is used as the value for max-age in Cache-Control headers.'),
   +    );
   
   @@ -197,6 +204,7 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
   +      ->set('cache.page.4xx.max_age', $form_state->getValue('page_cache_4xx_maximum_age'))
   

   I personally think we don't want a UI for this. Which would mean these changes are unnecessary.

   Let's see what others think.

4. +++ b/core/modules/system/src/Tests/Bootstrap/PageCacheTest.php
   @@ -318,15 +318,24 @@ function testPageCacheAnonymous403404() {
   +    $config = $this->config('system.performance');
   +    $config->set('cache.page.use_internal', 1);
   +    $config->set('cache.page.max_age', 300);
   +    $config->save();
   

   The page cache is already enabled; this is unnecessary.

a) I don't think we want a GUI.
b) I don't see why we need this at all. After all you can just add a subscriber if you want to change the response and use a different max-age.

This needs a IS update.

I changed the comments wim had in #8 but left the UI in for now.

I discussed the change to fix test failures with znerol and he agreed this was a good way to do this.

I don't see why we need this at all. After all you can just add a subscriber if you want to change the response and use a different max-age.

@znerol, @pwolanin, please chime in.

I fixed the failures testbot failures, an unrelated test in the same testClass fails for me, but I believe this is unrelated. Let's see what testbot thinks.

Pinged @znerol for a review.

1. +++ b/core/lib/Drupal/Core/EventSubscriber/FinishResponseSubscriber.php
   @@ -146,17 +146,21 @@ public function onRespond(FilterResponseEvent $event) {
   +    } else {
   

   Opening brace should be on a separate line.

2. +++ b/core/lib/Drupal/Core/EventSubscriber/FinishResponseSubscriber.php
   @@ -146,17 +146,21 @@ public function onRespond(FilterResponseEvent $event) {
   +      if (!$this->isCacheControlCustomized($response) || $response->isClientError()) {
   

   It should not be necessary to change this condition. If isCacheControlCustomized() acts in weird ways on different machines, then we need to fix that. Also I cannot reproduce the test failure in #10 locally. Perhaps this was just a test-bot hiccup?

Fixed both changed suggested by znerol in #18. Don't have any test fails either locally without the changed if condition.

1. +++ b/core/modules/system/config/install/system.performance.yml
   @@ -1,6 +1,8 @@
    cache:
      page:
        max_age: 0
   +    4xx:
   +      max_age: 300
   

   If we default to 0 for normal pages, we also should use 0 for error pages.

2. +++ b/core/modules/system/config/schema/system.schema.yml
   +++ b/core/modules/system/config/schema/system.schema.yml
   @@ -160,6 +160,13 @@ system.performance:
   
   @@ -160,6 +160,13 @@ system.performance:
                max_age:
                  type: integer
                  label: 'Max age'
   +            4xx:
   +              type: mapping
   +              label: 'Page cache for 4xx responses'
   +              mapping:
   +                max_age:
   +                  type: integer
   +                  label: 'Max age for 4xx responses'
   

   We could move the 4xx mapping one level up in the structure. After the extraction of the page cache module, the cache key in the system.performance config has no other responsibility than governing the max_age directive of the Cache-Control header.

I changed the indentation of the config schema and changed all related settings in this patch. default value is now 0 as well.
I also added a new eventListener that listens to the configCrudEvent and invalidates the '4xx-response' tag. I adjusted the test to account for this as well.

Happily sent from the car between Montpellier and Belgium.

Happily sent from the car between Montpellier and Belgium.

:)

1. +++ b/core/modules/page_cache/src/EventSubscriber/ConfigSubscriber.php
   @@ -0,0 +1,36 @@
   +use Symfony\Component\EventDispatcher\EventSubscriberInterface;
   +
   +
   +/**
   

   Let's reduce this to a single newline.

2. +++ b/core/modules/page_cache/src/EventSubscriber/ConfigSubscriber.php
   @@ -0,0 +1,36 @@
   + * Clears some caches if the config has changed
   

   Suggested: Invalidates the '4xx-response' cache tag if the max-age for 4xx responses was changed.

   (If that fits.)

3. +++ b/core/modules/page_cache/src/EventSubscriber/ConfigSubscriber.php
   @@ -0,0 +1,36 @@
   +   * Causes the container to be rebuilt on the next request.
   

   This is not true. C/p remnanet, probably.

4. +++ b/core/modules/page_cache/src/EventSubscriber/ConfigSubscriber.php
   @@ -0,0 +1,36 @@
   +   * @param ConfigCrudEvent $event
   

   Needs FQCN.

5. +++ b/core/modules/page_cache/src/EventSubscriber/ConfigSubscriber.php
   @@ -0,0 +1,36 @@
   +      Cache::invalidateTags(array('4xx-response'));
   ...
   +    $events[ConfigEvents::SAVE][] = array('onConfigSave', 0);
   

   s/array()/[]/

6. +++ b/core/modules/page_cache/src/EventSubscriber/ConfigSubscriber.php
   @@ -0,0 +1,36 @@
   +  }
   +}
   

   Needs newline between these two.

Fixed coding standards as requested in #22.

+++ b/core/modules/page_cache/src/EventSubscriber/ConfigSubscriber.php
@@ -0,0 +1,36 @@
+    if ($saved_config->getName() == 'system.performance' && $event->isChanged('cache.4xx.max_age')) {

Use identity operator when comparing strings (i.e. '===').

I changed the code to use strict checking, i copied this from the ConfigSubscriber in language and that uses "==" as well. I also rerolled the patch to be an actual patch against HEAD, not just an interdiff.

Thanks, this looks good now.

Most routes are already fixed by the 4xx subscriber - if the menu router is rebuild.

I am still not sure we need it, but I am also not opposed to it. It seems like an advanced usage.

However, because the other issue has landed already:

This is not a major bug, such changing to a "normal task".

- This needs an issue summary update to show use cases, where this still breaks.
- This needs a beta evaluation.

Tagging D8 upgrade path. If it's not configurable (per #9) we wouldn't need the upgrade path.

But also I have the same question as Fabianx - it's possible that something not-an-entity could invalidate a 4xx page, but then that other thing could invalidate the cache tag. By the time you're at the point where you're tweaking the value, you could just do that.

I think it should be configurable to actually allow to switch between setting max-age or not in the first place. We don't cache the normal max-age by default either, because doing so, especially for a long value, requires a varnish/$reverse_proxy integration module that can deal with cache tag invalidations. I think enabling that by default and enforcing it would be a bad idea. Unlike page cache (where we already cache 404's, permamently), external caches don't work out of the box.

Going to unassign this issue, at least for now. I'm going to let you guys decide if this is really needed or not.

Assigning to catch based on the Varnish argument ...

We don't cache the normal max-age by default either, because doing so, especially for a long value, requires a varnish/$reverse_proxy integration module that can deal with cache tag invalidations.

This is true, but it's true for the default max-age as well.

So either:

1. You have varnish + no cache tag invalidation and probably a short TTL for all responses. This works fine for some sites.

or

2. You have varnish + cache tag invalidation and a longer TTL is fine because cache tags will work.

I don't see how 403/404 responses are different/worse from 200s if they get stale - usually in terms of cache invalidation people are worried about:

1. Deleted/unpublished content still showing up.
2. Edited content not showing up.
3. New content not showing up.

And usually more or less in that order.

Right, yes, it's the same for the normal max-age.

So you're not really arguing against making it configurable, you're arguing against making it configurable separately from the normal page max-age?

Which would basically mean that this issue is a won't fix. That would be fine with me, especially since we now have the tools to invalidate them in a fairly reliable way. Personally, I think the ability to cache 5xx errors with a different max age would be more interesting, as those are usually temporary and can't be invalidated automatically. E.g., you could cache the response when the database is down for a short time, to avoid hundreds of requests. We currently explicitly prevent that.

General note that #2699613: Set a shorter TTL for 404 responses in page_cache module is changing this for page_cache module, since permament cache items in the database cache can cause disk-filling.

Still think given varnish doesn't have that problem the existing behaviour with max-age might be OK, but posting this so I don't get confused which issues is which again - already happened twice this month.

How do we want to continue here?

Same question. This would an incredibly useful feature to land.

+++ b/core/modules/system/src/Form/PerformanceForm.php
@@ -135,6 +135,13 @@ public function buildForm(array $form, FormStateInterface $form_state) {
+    $form['caching']['page_4xx_maximum_age'] = array(
+      '#type' => 'select',
+      '#title' => t('Page cache 4xx maximum age'),
+      '#default_value' => $config->get('cache.4xx.max_age'),
+      '#options' => $period,
+      '#description' => t('The maximum time a page with a 4xx response can be cached. This is used as the value for max-age in Cache-Control headers.'),
+    );

@@ -180,6 +187,7 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
+      ->set('cache.4xx.max_age', $form_state->getValue('page_4xx_maximum_age'))

Is it just me, or is this something we should not expose via the UI, but just make it configurable?

Agreed with #43.

+++ b/core/modules/page_cache/src/EventSubscriber/ConfigSubscriber.php
@@ -0,0 +1,37 @@
+    if ($saved_config->getName() === 'system.performance' && $event->isChanged('cache.4xx.max_age')) {
+      Cache::invalidateTags(['4xx-response']);
+    }

The same should be true for 'cache.page.max_age', right?

#45: I don't think so, at least not for 4xx pages, the max age for those is different. If we'd want to do that for normal pages, we should do it in another issue.

I also agree with removing the UI, 2 years ago, this was one of my first core patches and I was too attached to this code to go ahead and remove it, while it was already suggested in #8 and #9.

A lot has changed in the meanwhile though, so this no longer applies.

Setting to needs work, because the patch no longer applies.

Attached is a re-roll, with UI options removed. Tests might need a review because of #2699613: Set a shorter TTL for 404 responses in page_cache module, but let's give is a go.

Quick fix of coding standards issues. Tests will still need work.

Reroll of #50.

I want to ask a question whether we could do actually better for entities. In the case of entities we know its path, so what about adding the path as cache tag to page cache and then on entity save build up a path using the 'canonical' link template.

On the other hand I'm not sure whether its worth special casing entities here?

Unassigning me.

...

#65 -- patches and tests for both the UI and non-UI options for cache.4xx.max_age.

+++ b/core/modules/page_cache/src/EventSubscriber/ConfigSubscriber.php
@@ -0,0 +1,37 @@
+ * Invalidates the '4xx-response' cache tag if the max-age for 4xx responses
+ * was changed.

Should be only one line with a max of 80 cols.

In any case, I don't think the version of the patch with the UI is needed anymore, enough people have said that it's not needed.

Easy fix. Thanks.

Awesome, thanks! Even though it's been over 3 years I don't think I'm allowed to rtbc that.

1. +++ b/core/lib/Drupal/Core/EventSubscriber/FinishResponseSubscriber.php
   @@ -160,13 +160,21 @@ public function onRespond(FilterResponseEvent $event) {
   +    // Support an independent max-age for 404/403 responses.
   

   The comment is inaccurate: it applies to all 4xx responses, not just 403 and 404.

   Honestly, I think this comment can simply be removed.

2. +++ b/core/lib/Drupal/Core/EventSubscriber/FinishResponseSubscriber.php
   @@ -160,13 +160,21 @@ public function onRespond(FilterResponseEvent $event) {
   +    if ($response->isClientError()) {
   +      $max_age = $this->config->get('cache.4xx.max_age');
   +    }
   +    else {
   +      $max_age = $this->config->get('cache.page.max_age');
   +    }
   

   Nit: Suggested change:

   $max_age = $response->isClientError()
     ? $this->config->get('cache.4xx.max_age')
     : $this->config->get('cache.page.max_age');
   

3. +++ b/core/modules/page_cache/src/EventSubscriber/ConfigSubscriber.php
   @@ -0,0 +1,36 @@
   +class ConfigSubscriber implements EventSubscriberInterface {
   

   This belongs in the system module, not in the page_cache module.

   Because:

   1. the configuration is owned by the system module
   2. the configuration is used by core's FinishResponseSubscriber, which doesn't live in the page_cache
      module either
   3. and therefore we need 4xx responses to be invalidated also when not using Page Cache, because they may be using an external reverse proxy,
      such as Varnish. If this subscriber only runs when Page Cache is installed,
      then Varnish will continue to serve stale 4xx responses

   Therefore this can be merged with \Drupal\system\SystemConfigSubscriber.

4. +++ b/core/modules/page_cache/src/Tests/PageCacheTest.php
   @@ -339,12 +339,16 @@ public function testPageCacheAnonymous403404() {
   +      $config->set('cache.4xx.max_age', 300)
   +        ->save();
   
   @@ -372,6 +376,12 @@ public function testPageCacheAnonymous403404() {
   +      $config->set('cache.4xx.max_age', 0)
   +        ->save();
   

   Nit: let's make these single lines.

5. +++ b/core/modules/system/config/install/system.performance.yml
   @@ -1,6 +1,8 @@
    cache:
      page:
        max_age: 0
   +  4xx:
   +    max_age: 0
   

   Why is 4xx a key on the same level as page?

   Wouldn't

   • cache.page.max_age (existing)
   • cache.page.4xx_max_age (new)

   be more appropriate names?

6. +++ b/core/modules/system/config/schema/system.schema.yml
   --- a/core/modules/system/tests/src/Kernel/Migrate/d6/MigrateSystemConfigurationTest.php
   +++ b/core/modules/system/tests/src/Kernel/Migrate/d6/MigrateSystemConfigurationTest.php
   
   +++ b/core/modules/system/tests/src/Kernel/Migrate/d6/MigrateSystemConfigurationTest.php
   --- a/core/modules/system/tests/src/Kernel/Migrate/d7/MigrateSystemConfigurationTest.php
   +++ b/core/modules/system/tests/src/Kernel/Migrate/d7/MigrateSystemConfigurationTest.php
   

   I don't know why this provides migrate paths. This is not something that existed in D6 or D7.

   It shouldn't provide a migration path. It should provide an update path.

   Because existing D8 sites still won't get this new setting.

Reroll and addressing the comments in #70. Probably could use some additional tests in the system module.

Needs review for the bot.

Ok. Green again. Notable changes form #68 considering #70:

• New config is now cache.page.4xx_max_age, was cache.4xx.max_age
• Initial value for 4xx_max_age in now null, was 0. Otherwise we will stop caching 4xx responses unless end users set a value for 4xx_max_age
• Moved ConfigSubscriber logic to system module
• Added an upgrade path and test
• Did not remove the migration code as I think we want cache.page.4xx_max_age in the $expectedConfig
• Various code formatting adjustments

Changes look very great to me. As I am not seeing anything missing, setting to RTBC.

1. +++ b/core/lib/Drupal/Core/EventSubscriber/FinishResponseSubscriber.php
   @@ -160,13 +160,18 @@ public function onRespond(FilterResponseEvent $event) {
   +      ? $this->config->get('cache.page.4xx_max_age')
   

   This can be $client_error_max_age - we've already got the value from config.

2. +++ b/core/modules/page_cache/tests/src/Functional/PageCacheTest.php
   @@ -387,6 +390,11 @@ public function testPageCacheAnonymous403404() {
   +      $config->set('cache.page.4xx_max_age', 0)->save();
   +      $this->drupalGet($content_url);
   +      $this->assertResponse($code);
   +      $this->assertEqual($this->drupalGetHeader('X-Drupal-Cache'), 'MISS');
   +      $this->assertEqual($this->drupalGetHeader('Cache-Control'), 'must-revalidate, no-cache, private');
   

   I think this should have something like

         $this->drupalGet($content_url);
         $this->assertResponse($code);
         $this->assertEqual($this->drupalGetHeader('X-Drupal-Cache'), 'HIT');
         // Setting cache.page.4xx_max_age should invalidate the 4xx cache tag.
   

   Before it so it is obvious we're testing that setting the value invalidates the cache.

3. +++ b/core/modules/system/config/schema/system.schema.yml
   @@ -152,6 +152,9 @@ system.performance:
   +            4xx_max_age:
   +              type: integer
   +              label: 'Max age for 4xx responses'
   

   This should also have nullable: true

4. Still need an issue summary update since the bug in the issue summary has been solved
5. We need a change record to tell people about the new config especially since there is no UI.

Addresses code comments in #81.

IS update and CR still needed.

+++ b/core/modules/page_cache/tests/src/Functional/PageCacheTest.php
@@ -387,6 +390,12 @@ public function testPageCacheAnonymous403404() {
+      // Setting cache.page.4xx_max_age should invalidate the 4xx cache tag.

As above we need to add

      $this->drupalGet($content_url);
      $this->assertResponse($code);
      $this->assertEqual($this->drupalGetHeader('X-Drupal-Cache'), 'HIT');

before this to show that the cache was HIT before this MISS.

Adding overlooked change from #83.

IS updated and CR drafted.

The change record https://www.drupal.org/node/2972025 looks good to me.

I meant to RTBC this actually.

I still have the same reservations here as I did in #33 - I can't see what the use-case for this is. i.e. there seem to be two situations:

1. You already have max_age short because your external proxy doesn't support cache tags. If this is the case, what makes 403/404 responses special? Everything will need a short max_age.

2. If the external proxy does support cache tags, then you probably don't need to configure it separately either, because cached 403 or 404 responses will in 99.9% of cases be invalidated when content is published and similar.

If there is a #3, it would be really useful to document that both in the issue summary and the change record.

@catch - consider this use case:

At the reverse proxy (e.g. Varnish VCL), say you enforce 30mn caching for all 404s to prevent DDoS attacks. This means when editors check if a page exists and get a 404 because it doesn't, Varnish is going to cache the page for 30mn. If it happens that editors do want to create this page, they'll do so and will notice the page is stubbornly throwing a 404 as anon. This creates a frustrating experience and is one of the reasons why https://www.drupal.org/project/http_cache_control was created, if I understand correctly.

Whether it should be supported by default in core is a different story.

Thanks for the review, @catch.

First, Core has $settings['cache_ttl_4xx'] = 3600; with applies only to the page_cache module. This issues allows similar functionality for external proxies.

Second, and a variation of your 1. in #90, you have an external proxy that doesn't support cache tags, but still want a reasonable max_age / CHR. For a max_age set to 60 min, let's say, newly published content could still 404 for an unacceptable amount of time. Being able to set cache.page.4xx_max_age to 1 min with keeping cache.page.max_age at a larger value still provides some baseline 4xx traffic stampede protection and quick expiration, while helping maintain a higher CHR on 200s.

@anavarre I understand that use case, but it's covered by using cache tags and varnish which is already supported: https://www.drupal.org/docs/8/api/cache-api/cache-tags-varnish

@bonus

First, Core has $settings['cache_ttl_4xx'] = 3600; with applies only to the page_cache module. This issues allows similar functionality for external proxies.

That's not why that setting exists, it's there because the default backend for the page cache is the database, and entries are stored permanently, so it was possible to DDOS a site simply by visiting 404s and fill the disk. See #2697795: [meta] Cache storage in database can fill the disk, especially via 404s.

Not caching 404s when a real page is created at the URL is handled instantaneously via cache tags - both with the database and reverse proxies that support cache tags.

So the only case would be a reverse proxy that does not allow cache tags - but again there's a limit to how long a site can have that set for non-4xx responses, you'd expect it to be pretty short and then also use the internal page cache as a backup (which is always up-to-date with the latest content, so can be cached permanently). If this is the only valid use case, I'm not sure it's worth supporting in core - would prefer to improve the documentation on the various options somewhere.

@catch, thanks for the clarification on cache_ttl_4xx.

#90:

Given that I just implemented a backport of that for Drupal 7 for one of our enterprise clients, let me elaborate:

It is perfectly fine to have 10 min of caching for all pages, but only 20s for 403 and 404 responses.

The reason is:

- Most CDNs support a shield-like functionality, so if you don't cache 403 and 404, then all requests are send to origin.
- But you definitely don't want to cache a 404 / 403 for longer than 20s as the page could then exist and this is an error state.

e.g. the only reason you need a max-age on 404 / 403 (and not uncacheable) is the shield like functionality of CDNs / Varnish as you want micro-caching, but a user should never see an error longer than necessary.

And most Varnish configurations do not yet support cache tags out of the box and e.g. Akamai is just now updating to have some kind of Surrogate-Keys.

I hope that helps as justification.

#95 sounds pretty reasonable. Just one question though:

It is perfectly fine to have 10 min of caching for all pages, but only 20s for 403 and 404 responses...But you definitely don't want to cache a 404 / 403 for longer than 20s as the page could then exist and this is an error state.

Isn't 10 min of caching a 200 in a reverse proxy that doesn't support cache tags just as much of an error state as 10 min of caching a 4xx? What makes the former error more acceptable than the latter one for the client that you helped with this?

#96:

The reason why a 403 / 404 page is more of an error state is, because they are very obvious errors and also more fatal for the UX.

e.g. if a title somewhere in a sidebar is wrong, it might be not a big deal to wait for the 10 min to expire, but if for example an access check is wrong and users are getting 403 pages, then this is an error state, which should be fixed ASAP as it is very obvious to the users that the 'site is broken'. And Drupal itself also handles those pages and conditions differently (logging it, etc.).

So while I agree that technically there is no difference between an error on a normal page and an error presented by a 404/403 page, there is a large difference in user perception.

It passed again.

There are many scenarios where it is desirable to have an independent max-age for 4xx responses.

Yep, #98 convinces me of that.

However, are there also common scenarios for wanting an independent max-age for 3xx responses and/or wanting 3xx responses to use the same max-age as 4xx responses? Just asking to make sure that 4xx_max_age is the config variable name that we're confident we'll want to keep for a while.

#101 For 30x the story is a little bit different from my experience:

Either we want to cache the 30x redirect not at all (e.g. a language redirect or based on some user session data) or we want to cache it permanently / per max-age (e.g. there is a way for it to expire) and then usually the default expiration time is fine (e.g. the same as max-age).

In all those cases the caller of "drupal_goto()" / the thrower of the RedirectResponse() is responsible for the cacheability of the actual redirect.

I have not come across the use-case in practice that we would need 3xx responses to work like 4xx and you never want to cache 5xx responses.

Therefore I would say that 4xx_max_age is a fine name and also matches what is in page_cache already as configuration variable (even if there it was a different reason).

Feels consistent.

We need a post update hook here with a test, but other than that, looks good.

I'd refactor the calculation of $max_age into a protected method to avoid else, but that's a personal preference.

See #2828793: Stop logging comment IP addresses by default for sample update path and test

Rerolled patch that is compatible with Drupal 8.6.10 (there was a duplicate `system_update_8601()` function in previous one).

Updated patch to be compatible with Drupal 8.7.7

Re-roll of #108 against 9.1.x. Changes from #104 and #105 pending.

Adding post update hook and resolving failed test cases from #111

Unrelated test failure.

Adjusted patch from #112 to core 8.9.x.

Re-roll of #112 against 9.2.x.

Rerolled patch in #116 and updated deprecated functions like changed $this->assertEqual($this->drupalGetHeader(''), '') to $this->assertSession()->responseHeaderEquals('', '') wherever necessary, thanks!

D10 version needed
At this time we need a D10.1.x patch or MR for this issue.

Rerolled patch for 10.1.x.
Needs review.

rerolled the patch for D10.1.x and fixed the issue of #125 . Please check and confirm.

     $is_cacheable = ($this->requestPolicy->check($request) === RequestPolicyInterface::ALLOW) && ($this->responsePolicy->check($response, $request) !== ResponsePolicyInterface::DENY);
+
     $client_err

1. Seems out of scope

     $max_age = $response->isClientError() && isset($client_error_max_age)
       ? $client_error_max_age
         : $this->config->get('cache.page.max_age');
+
     // Add headers 

2. Same

3. 3 more instances of spacing being added which is out of scope

and patch #129 is removing test coverage.

Using patch #124 on D 10.1.8 fixed my issue.

@smustgrave regarding extra lines in interdiff 124-128 (point 1., 2.) - an empty line was removed in 124, so these are OK in #128 I think.
Point 3. and test coverage should be fixed.

I think the assignment is not relevant after more than 1 year.