Include Cache-Control header on 301 redirects.

Thanks for all the research you did here and over at #3016776-24: 301 redirects don't contain cache-control header, @mr.baileys!

This indeed still needs test coverage. AFAICT \Drupal\Tests\page_cache\Functional\PageCacheTest and \Drupal\Tests\system\Functional\Routing\RouterTest::testFinishResponseSubscriber() are the appropriate places to expand test coverage.

Symfony uses the case-senstive array_key_exists to check for "cache-control", this wrongly concluding that no cache-control header is present...

Woah, really!? If true, that'd be a bug in Symfony. Symfony stores everything in lowercase (just look at \Symfony\Component\HttpFoundation\HeaderBag::get() and \Symfony\Component\HttpFoundation\HeaderBag::set()), so I'm surprised to hear that. But you're right, I see the case-sensitive check in \Symfony\Component\HttpFoundation\RedirectResponse::__construct(). We should file an upstream bug for that.

That being said, rather than changing the event subscriber priority, I think an alternative solution is to change \Drupal\Component\HttpFoundation\SecuredRedirectResponse::createFromRedirectResponse(), which does:

$safe_response = new static($response->getTargetUrl(), $response->getStatusCode(), $response->headers->allPreserveCase());

Note the ->allPreserveCase()! Changing it to ->all() presumably fixes this edge case too.

+++ b/core/modules/system/tests/modules/system_test/src/Controller/SystemTestController.php
@@ -398,4 +400,18 @@ public function getCacheableResponseWithCustomCacheControl() {
+  public function cacheableRedirectResponse($status_code) {
...
+  public function localRedirectResponse($status_code) {

Nit: I find this naming a bit confusing: it makes it sound like a redirect response is either cacheable or local.

But class LocalRedirectResponse extends CacheableSecuredRedirectResponse: it is cacheable too!

Still, it's worth testing both. But if we're testing both, shouldn't we also test TrustedRedirectResponse?

Happy to switch from ->allPreserveCase() to ->all() if losing capitalisation is acceptable.

Let's see if it causes test failures. The capitalization of headers isn't meaningful anyway (hence Symfony lowercasing everything!).

Not changing event subscriber priority will make this much easier to land.

I have applied the patch at #16 to a Drupal 8 codebase I help to look after, and have got the latest stable redirect module enabled.

$ curl -sIXGET 'https://www.SITE.com/includes/webtrends.min.js' | grep -iE 'cf-|cache|varnish'
cache-control: public, max-age=2764800
via: varnish
x-cache: HIT
x-cache-hits: 7
cf-cache-status: HIT
cf-ray: 4e2d78bebc51d6a9-SYD

The result is perfect, I am seeing the correct cache-control header being sent, and Varnish and Cloudfare are able to now cache this for 30 days.

Thanks @mr.baileys

RTBC +1

1. +++ b/core/lib/Drupal/Component/HttpFoundation/SecuredRedirectResponse.php
   @@ -27,7 +27,7 @@ abstract class SecuredRedirectResponse extends RedirectResponse {
   -    $safe_response = new static($response->getTargetUrl(), $response->getStatusCode(), $response->headers->allPreserveCase());
   +    $safe_response = new static($response->getTargetUrl(), $response->getStatusCode(), $response->headers->all());
   

   I spent some time thinking about this change. Note the change here is whether or not the header keys are lowercased - the values remain untouched. I believe that the http spec doesn't care if it is Cache-Control or cache-control. As per the spec - https://tools.ietf.org/html/rfc7230#section-3.2 - header field names are case-insensitive

2. +++ b/core/lib/Drupal/Core/EventSubscriber/FinishResponseSubscriber.php
   @@ -202,7 +202,7 @@ public function onRespond(FilterResponseEvent $event) {
   -    return $cache_control != 'no-cache, private' && $cache_control != 'private, must-revalidate';
   +    return !is_null($cache_control) && $cache_control != 'no-cache, private' && $cache_control != 'private, must-revalidate';
   

   I've ummed and ahhed over this for quite a bit of time. I think this change makes it impossible for use to emit a response without a cache header. Which feels limiting but maybe in a good way. But it is definitely surprising because before if I do what Symfony's RedirectResponse does - ie. $this->headers->remove('cache-control'); - what I expect to happen happens. A response without cache headers is emitted. I think we might only want to change this behaviour if we're dealing with 301s.

   Also regardless of whether we make that change the extensive docs of this method need changing to mention the NULL case.

   I think this code could look like

     protected function isCacheControlCustomized(Response $response) {
       if ($response->getStatusCode() === 301 && !$response->headers->has('Cache-Control')) {
         return FALSE;
       }
       $cache_control = $response->headers->get('Cache-Control');
       return $cache_control != 'no-cache, private' && $cache_control != 'private, must-revalidate';
     }
   

3. I spent sometime debating whether or not Drupal should be emitting a cache-control header for 301s. Or whether this should be up to configuring the reverse proxy correctly. Certainly Symfony has made the decision that 301 caching is the responsibility of the layers above the application hence removing the cache-control header. I'm torn on this. Because 301 is a permanent redirect therefore it strictest terms varnish / cloudflare would be well within scope to cache these forever. OTOH the reality is the cloudflare and Acquia take the same approach - https://support.cloudflare.com/hc/en-us/articles/200168326-Are-301-and-3... - that is, short term caching if there is no cache-control header. The default varnish behaviour is odd but then again varnish configuration seems to be aspire to create its own job market with its arcaneness.

   So... I think if we limit the not set cache header behaviour to 301s then this might be a good change because out-of-box 301s cache like any other redirect. Which kinda makes sense.

> Because 301 is a permanent redirect therefore it strictest terms varnish / cloudflare would be well within scope to cache these forever.

Hm, if you take redirect.module as an example, you can change those redirects again and on many sites, most redirects are created automatically and it's not uncommon to change it back to not being a redirect, e.g. /article/foobar => article/foobar-update => /article/foobar. Redirect adds cache tags to invalidate them internally, that works fine, but caching them externally is actually somewhat tricky.

https://stackoverflow.com/questions/9130422/how-long-do-browsers-cache-h... so chrome at least will cache the current 301s emitted by core permanently. It feels to me that we should be using 301s very sparingly. Because hint is in the name permanent redirect. In the example in #23 these should not be 301s.

Just a reroll for 8.8.1 (latest 8.8.x anyway...)

This is a weird issue. Had to read it half a dozen times to really grok the interactions.

1. +++ b/core/lib/Drupal/Component/HttpFoundation/SecuredRedirectResponse.php
   @@ -27,7 +27,7 @@ abstract class SecuredRedirectResponse extends RedirectResponse {
   -    $safe_response = new static($response->getTargetUrl(), $response->getStatusCode(), $response->headers->allPreserveCase());
   +    $safe_response = new static($response->getTargetUrl(), $response->getStatusCode(), $response->headers->all());
   

   This has been fixed in Symfony 3.4, but for now we need to work around that bug.

   This has sat around long enough its going to land in 9.x which means will be requiring 4.x+. Reading back through the comments and the Symfony issue it sounds that means this chunk isn't required anymore.

2. +++ b/core/lib/Drupal/Core/EventSubscriber/FinishResponseSubscriber.php
   @@ -201,6 +201,13 @@ public function onRespond(FilterResponseEvent $event) {
   +    // Symfony explicitly removes the Cache-Control header for 301 redirects
   +    // which do not have a custom Cache-Control header. Treat those redirect
   +    // responses as not customized.
   +    if ($response->getStatusCode() === 301 && !$response->headers->has('Cache-Control')) {
   +      return FALSE;
   +    }
   +
   

   Man their logic really puts us in a sticky situation here. This docblock is a bit confusing. I tried to come up with a way of explaining how but I'm struggling. Maybe we can just add @see Symfony\Component\HttpFoundation\RedirectResponse::__construct so the next person struggling with this can follow where the logic is we're matching.

   Corollary, if I make a Response with a 301 status code this is going to match I think. Is that ok? I don't have the answer to that.

3. +++ b/core/tests/Drupal/Tests/Component/HttpFoundation/SecuredRedirectResponseTest.php
   @@ -37,11 +37,8 @@ public function testRedirectCopy() {
   -    // We unset cache headers so we don't test arcane Symfony weirdness.
   -    // https://github.com/symfony/symfony/issues/16171
   -    unset($headers1['Cache-Control'], $headers2['Cache-Control']);
   

   That's weird... who wrote that? Wait what? Oh, hey look over there at that other thing that's not a hacky test.

   Glad we're on versions where that hack is not needed. Sorry I didn't follow up.

This patch solved a rather annoying problem on my Drupal 9.3.x-dev installation.
Let me explain the characteristics of what I think is a bug: without the patch the redirection (only for anonimous user) from https://mydomain.ext/node/6 to the specifict node 6 path alias raised an exception:

The website encountered an unexpected error. Please try again later.
TypeError: Argument 1 passed to str_contains() must be of the type string, null given, called in /var/www/vhosts/scarpepervocazione.it/httpdocs/vendor/symfony/http-foundation/Response.php on line 314 in str_contains() (line 29 of /var/www/vhosts/scarpepervocazione.it/httpdocs/vendor/symfony/polyfill-php80/bootstrap.php).

str_contains() (Line: 314)
Symfony\Component\HttpFoundation\Response->prepare() (Line: 720)
Drupal\Core\DrupalKernel->handle() (Line: 19)

On line 314 of vendor/symfony/http-foundation/Response.php there is this check:

        // Check if we need to send extra expire info headers
        if ('1.0' == $this->getProtocolVersion() && str_contains($headers->get('Cache-Control'), 'no-cache')) {
            $headers->set('pragma', 'no-cache');
            $headers->set('expires', -1);
        }

I have only noticed this behaviour for anonymous users; if the user is logged in, the redirection from https://mydomain.ext/node/6 to https://mydomain.ext/page/this-is-a-sample-path-alias is completed without error.

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

The upstream Symfony issue from #20 is fixed, so we could proceed here, but now it seems unclear, what's the status in Drupal 10. Is the issue still existing and is someone still using a patch from this issue?

MAYBE things might get worse if this is not fixed and we merge #3311406: .htaccess ExpiresDefault (2W) is much too low. Should be ~1Y?

This is indeed still an issue in Drupal 10. And indeed, this patch can be simplified as we should no longer need to deal with the different case of the cache-control header anymore.

Vanilla re-roll of #27 (8.x) against 10.1.x

A vanilla re-roll of the patch for 11.x and running tests.

Lint

Lint, passing tests

So that's passing, but needs reviewing feedback of #29 in #38 given upstream changes in Synfony.

Patch applies still, setting to NR.

Nitpicky stuff, can we add typehints and returns for new functions and parameters through.

Converting to an MR is also preferred if possibe

Updated with a MR and incorporates feedback from #29 regarding no need for the case-insensitive header logic update.

Also updated with typehints for params + returns on affected / new methods as requested by #46.

There was 1 error:
1) Drupal\Tests\page_cache\Functional\PageCacheTest::testCacheabilityOfRedirectResponses
Behat\Mink\Exception\ExpectationException: Current response header "Cache-Control" is "", but "max-age=300, public" expected.
/builds/issue/drupal-3054821/vendor/behat/mink/src/WebAssert.php:794
/builds/issue/drupal-3054821/vendor/behat/mink/src/WebAssert.php:161
/builds/issue/drupal-3054821/core/modules/page_cache/tests/src/Functional/PageCacheTest.php:577
/builds/issue/drupal-3054821/vendor/phpunit/phpunit/src/Framework/TestResult.php:728
ERRORS!
Tests: 14, Assertions: 183, Errors: 1.
PHPUnit 9.6.15 by Sebastian Bergmann and contributors.
Testing Drupal\Tests\Component\HttpFoundation\SecuredRedirectResponseTest
.                                                                   1 / 1 (100%)

Test-only feature showed test-coverage.

Appears all feedback has been addressed.

Believe this is good.

This needs a rebase.

It needs a bit of rewriting now too, as we have AssertPageCacheContextsAndTagsTrait for enabling page cache and checking header(s). Might as well use it.

If I get time later on I'll do it...

Copy of patch #40 but with correct extension (without the '.txt').
Looks like this patch would be able to work on D10.2.x

apparently my previous patch (rename from #40) would not apply to 10.2.x
Made some changes and this one should.

As I was saying in #53, we have AssertPageCacheContextsAndTagsTrait available here - how would you feel about changing the test from say;

/**
 * Verify that the cache-control header is added by FinishResponseSubscriber.
 */
public function testCacheabilityOfRedirectResponses() {
  $config = $this->config('system.performance');
  $config->set('cache.page.max_age', 300);
  $config->save();

  $this->getSession()->getDriver()->getClient()->followRedirects(FALSE);
  $this->maximumMetaRefreshCount = 0;

  foreach ([301, 302, 303, 307, 308] as $status_code) {
    foreach (['local', 'cacheable', 'trusted'] as $type) {
      $this->drupalGet("/system-test/redirect/${type}/${status_code}");
      $this->assertResponse($status_code);
      $this->assertHeader('Cache-Control', 'max-age=300, public');
    }
  }
}

To something like;

/**
 * Verify that the cache-control header is added by FinishResponseSubscriber.
 */
public function testCacheabilityOfRedirectResponses() {
  $this->enablePageCaching();

  $this->getSession()->getDriver()->getClient()->followRedirects(FALSE);
  $this->maximumMetaRefreshCount = 0;

  foreach ([301, 302, 303, 307, 308] as $status_code) {
    foreach (['local', 'cacheable', 'trusted'] as $type) {
      $this->drupalGet("/system-test/redirect/${type}/${status_code}");
      $this->assertResponse($status_code);
      $this->assertCacheMaxAge(300);
    }
  }
}

#56 makes sense to me, we have those helpers so why not use them.

Patches rolled into the MR by Sean, hiding patches.

I read the MR with wiifm, focusing on the comments. I noticed different casing of 'Cache-Control' and the tense of a verb that should change. Those are being looked into. The code changes read well to me and are easy to understand, however I can't RTBC. I asked if the change was specific to a version of Symfony which he did not think so.

Adding credit for contributors during the DS2024 Code sprint.

Just 1 final question left on the MR.

Reverted change to SecuredRedirectResponse and tests still pass so don't believe it was needed. That resolved the last thread I believe.

Looks like a well-tested and tightly scoped change, with potentially big scalability improvements (far fewer origin requests to respond to)!

Committed and pushed 2fbe22aa19 to 11.x and ec45fd5604 to 10.3.x. Thanks!