Include Cache-Control header on 301 redirects.

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 ✨ .htaccess ExpiresDefault (2W) is much too low. Should be ~1Y Fixed ?

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 (9.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 and checking headers. 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.

wiifm → changed the visibility of the branch 3054821-include-cache-control-header to hidden.

wiifm → changed the visibility of the branch 3054821-include-cache-control-header to active.

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!

Automatically closed - issue fixed for 2 weeks with no activity.