Only send X-Drupal-Cache-Tags and -Contexts headers when developer explicitly enables them

While I not necessarily agree with the DDOS argument, there are so many ways anyway by default, I totally agree with the performance point.

#2241377: [meta] Profile/rationalise cache tags actually has currently that problem

This for sure will fail a LOT, but it this basically what we want?

I fear that people only set that setting in case they want the logging to set the right IP. So I think it needs further scrutiny.

But yes, it sure seems that simple :)

Also, does this mean you propose having WebTestBase set the reverse_proxy setting to TRUE? Or do you want to introduce a separate flag (config perhaps?) to expose it anyway?

Well, we want these headers when debugging anyway, so we want a flag either in Settings or in Config to turn it on. Then settings.local.php can turn it on again.

Large headers slow down communication between client and server.

Indeed! Especially concerning is that this header is in the first 14 KB of the TCP packet, so that means less space for more valuable information.

With the headers attackers may be able to identify sections of a page that are not cached and may start a targeted DDoS.

No. By that argument, literally everything in Drupal 7 is DDoSable, with the sole exception of image styles.

Talked with wim about the name of that debugging flag.

Manually tested the patch and it seems to work!

+++ b/core/lib/Drupal/Core/EventSubscriber/FinishResponseSubscriber.php
@@ -117,7 +117,9 @@ public function onRespond(FilterResponseEvent $event) {
+    // We just want to send the cacheability headers in case it is actually
+    // needed, so a reverse proxy is used, or when we want to debug its values.
+    if ((Settings::get('reverse_proxy', FALSE) || Settings::get('send_cacheability_headers', FALSE)) && $response instanceof CacheableResponseInterface) {

Why not only a simple flag to enable the headers without checking reverse_proxy setting?

#12: As soon as you use a reverse proxy in 99% of the cases you want the cache tags and cache contexts.

RTBC, we have enough test coverage for that.

This needs change record and also tests for this change.

Some rewording for consistency/clarity and nitpicking.

I also agree with Fabianx that we don't need tests, because WebTestBase sets this new setting, and we have dozens upon dozens of tests doing cacheability header assertions, so this'd fail immediately otherwise.

As for a CR, I've updated https://www.drupal.org/node/2259531.

Rewrote the IS.

Back to RTBC.

Actually, shouldn't we do this using a container parameter instead of a setting?

In case we want that we should move all of the reverse proxy settings to the container.

Perhaps I'm misremembering, but wasn't general rule we don't add NEW things to settings.php? Don't we favor container parameters over that now?

<WimLeers> dawehner: just earlier today, Alex mentioned we're getting rid of settings eventually, right?
<WimLeers> dawehner: Which made me vaguely remember that as a general rule we don't add NEW settings?
<dawehner> WimLeers: the thing is the following
<amateescu> WimLeers: I think it's better to keep together for now, if reverse proxy settings are moved to container param, the new one will be moved as well
<dawehner> WimLeers: once we have the container of fabian we can have dynamic container params
<dawehner> WimLeers: so override container params from settings.php
<WimLeers> ohhhhhhhhhhhh
<WimLeers> that sounds awesome
<dawehner> yeah work for a follow up
<WimLeers> dawehner: can you put that on the issue? I'll re-RTBC.

This issue addresses a major bug and is allowed per https://www.drupal.org/core/beta-changes. Committed 7115108 and pushed to 8.0.x. Thanks!

Do we have recommended configurations for frontend caches somewhere? I think we need to document this so varnish admins do not miss to remove the headers in their cache servers, too.

This does not belong into this issue. Please create a new one.

Updated https://www.drupal.org/developing/api/8/render/arrays/cacheability (diff: https://www.drupal.org/node/2456295/revisions/view/8437059/8721102).

This broke the Page Cache. Page Cache cache items now have no cache tags, and thus are never invalidated.

Page Cache now only works if you are either behind a reverse proxy or have that new flag enabled. The reason tests pass is because we made web test enable the new "send cacheability headers" setting.

Ughhhh, lets revert for now and make page cache use the cacheable metadata on the response instead.

In other words it is a horrible idea to have a different test environment than actual code running in production.
I think we should enable the cache tags debug output in places where we need it.

In other words it is a horrible idea to have a different test environment than actual code running in production.

+1000

Reverted.

For now this removes the autoenabling out of WebTestBase, but this should also get some dedicated test.

Patch didn't apply. New patch for testing.

Attempting a straight reroll from #35.

@anavarre
Ha, good try to simply don't add the new code.

While talking with @Fabianx we realized that we can also strip after the page cache.

While talking with @Fabianx we realized that we can also strip after the page cache.

Indeed. That was the plan all along.

But it needs to be configurable. Very often, it's a good idea to still use Page Cache (and Dynamic Page Cache) even when you're using a CDN, because it still reduces the load on the origin: many edges may simultaneously request the same page from the origin, and then (Dynamic) Page Cache still helps.

#43: This has nothing to do with dynamic page cache vs. non-dynamic page cache.

It just means we remove the headers in the reverse proxy middleware instead of in the FinishResponseSubscriber.

Hence PageCache and all other things running in a Middleware or ResponseEvent subscriber are fine.

If you use a CDN or varnish, set reverse_proxy = TRUE or set the send_cacheability_headers = TRUE - easy enough.

I think this only needs the part of ensuring our tests have the cacheability headers enabled from #16 as there is nothing using headers after ReverseProxyMiddleware.

I only read #40, so I thought it was the Page Cache's middleware that was going to strip it.

Fixing a small oversight that caused those many failures.

Wow, this are actually quite a few failures for not returning a response at all.

Why not again unconditionally on WebTestBase?

Yes, we screwed up once, but only because we removed those headers too early.

Overall that seems like a way less invasive change and also not breaking contrib tests, which we can't do at this point in time anymore.

Well, ulike the last time we should ensure that it still works without them

+++ b/core/modules/simpletest/src/WebTestBase.php
@@ -3002,4 +3004,20 @@ protected function assertNoCacheTag($cache_tag) {
+    $settings['settings']['send_cacheability_headers'] = (object) [
+      'value' => TRUE,

Should be 'value' => $value ;).

Should be 'value' => $value ;).

Ah perfect, thank you for catching that!
First I had two methods but then considered that as a little bit too silly.

Fail: [Other] Line 0 of sites/default/files/simpletest/phpunit-1224.xml:
PHPunit Test failed to complete

Unit tests for ReverseProxyMiddlewareTest need to be updated, too - to account for the new calls to settings.

Sure

Back to RTBC - finally.

Hope this can go in during RC.

1. +++ b/core/lib/Drupal/Core/StackMiddleware/ReverseProxyMiddleware.php
   @@ -55,7 +55,17 @@ public function handle(Request $request, $type = self::MASTER_REQUEST, $catch =
   +    // X-Drupal-Cache-Contexts and X-Drupal-Cache-Tags header respectively, when
   +    // either a reverse proxy is being used (so the reverse proxy or CDN can be
   +    // invalidated when appropriate) or when developing/debugging.
   

   Seems like a verb is missing at the beginning of this.

2. +++ b/core/modules/simpletest/src/WebTestBase.php
   @@ -3019,4 +3021,20 @@ protected function assertNoCacheTag($cache_tag) {
   +   * Sets the setting whether to send cacheability headers.
   +   *
   +   * @param bool $value
   +   *   (optional) Should the cache headers be send.
   

   Needs language love.

3. +++ b/core/modules/simpletest/src/WebTestBase.php
   @@ -3019,4 +3021,20 @@ protected function assertNoCacheTag($cache_tag) {
   +  protected function setCacheHeaders($value = TRUE) {
   

   s/setCacheHeaders()/setCacheabilityHeaders()/

4. +++ b/core/modules/simpletest/src/WebTestBase.php
   @@ -3019,4 +3021,20 @@ protected function assertNoCacheTag($cache_tag) {
   +    // Send cacheability headers so tests can check their values.
   

   Pointless comment.

Seems like a verb is missing at the beginning of this.

Yes, there is certainly a word .

Pointless comment.

Agreed.

Shaddup, PIFT.

Ahem.

Two nits that can be fixed on commit:

1. +++ b/core/modules/simpletest/src/WebTestBase.php
   @@ -3022,13 +3022,12 @@ protected function assertNoCacheTag($cache_tag) {
       *   (optional) Should the cache headers be send.
   

   Whether the cacheability headers should be sent.

2. +++ b/sites/example.settings.local.php
   @@ -73,6 +73,13 @@
   + * Send cacheablity headers for debugging purposes.
   

   s/cacheblity/cacheability/

To avoid the "paper bag" release that we had several betas ago, when #16 got committed and turned out to break the Page Cache (see #29). The root cause for this going unnoticed back then was: The reason tests pass is because we made web test enable the new "send cacheability headers" setting.

@dawehner described it very well why that was stupid in #31:

In other words it is a horrible idea to have a different test environment than actual code running in production.
I think we should enable the cache tags debug output in places where we need it.

This time, however, we do it in the reverse proxy middleware that runs after the page cache (it wraps the Page Cache middleware). So we cannot possibly break the Page Cache anymore.

To make that absolutely certain, however, we now test that even when you explicitly set send_cacheability_headers = FALSE, it still continues to work:

+++ b/core/modules/page_cache/src/Tests/PageCacheTest.php
@@ -50,6 +50,10 @@ protected function setUp() {
   function testPageCacheTags() {
+    // Ensure that the sending out of the cache tags / contexts is independent
+    // from the page cache.
+    $this->setCacheabilityHeaders(FALSE);

And finally, I did manual testing, verifying that both the Page Cache and the Dynamic Page Cache, and it all indeed still works out of the box, unlike last time.

We discussed this on a committer call today and decided that it's better for this to go in during RC than just before, so tagging. While it's unfortunate that some RC testers will thus get WSOD on some hosting environments (according to the issue summary), we expect this to be a very small minority.

---
Well, I think you might underestimate which hosting environments this are.

If they are big hosting environments that care about Drupal, then they (or we) will get a lot of bug reports until they raise the header limit.

Also if they offer reverse proxies, wouldn't you want to set $settings['reverse_proxy'] = TRUE to take advantage of that? So this ends up being a workaround where you disable all reverse proxy behaviour despite there actually being one on your hosting.

I think we should keep going with issues like #2542868: Allow a header value size limit to be specified regardless of this one.

Yep, agreed with the sentiment of #73. This is just a band-aid for the typical use case. It helps improve front-end performance in the process.

But we still need #2542868: Allow a header value size limit to be specified together with this, because if your setup/architecture needs the cacheability headers, then you still need to ensure it doesn't exceed the supported header length.

During the D8 EU criticals call of this morning, we (Alex Pott, dawehner, Wim) discussed this issue, and its sister issue #2542868: Allow a header value size limit to be specified.

1. There are two purposes to the cacheability headers: A) debugging/testing ("is my cacheability metadata showing up?" and ("what cacheability metadata does this response have?"), and B) reverse proxies (efficient invalidation of cached responses).
2. For purpose A, this does not need to be on by default at all.
3. For purpose B, just having that header is insufficient, you also need code that talks to the reverse proxy (Varnish, a CDN …) to inform it about the invalidations as they happen. (And given that I've ported both the Fastly and CloudFlare modules to Drupal 8, and added support for this, I can add an additional data point: they each require a specifically named header anyway, so we already need to remove/rename the X-Drupal-Cache-Tags header.)
4. Therefore, it is pointless that HEAD always sends the cacheability headers.
5. Therefore, we can have this be an opt-in thing for developers.
6. … and since you never need to change this on the fly, this can be a container parameter — just like the Twig debug output setting (twig.config.debug), which is a container parameter. i.e. add a new container parameter to core.services.yml, document it in default.services.yml.

NW for that simpler, clearer, more sensible direction.

Note that with #75's approach, the necessity of #2542868: Allow a header value size limit to be specified becomes questionable.

one thing we could do:

1. add a new module http_headers, which takes a cacheable response and adds the HTTP headers to it, basically what #2542868: Allow a header value size limit to be specified partially does
2. let page_cache require it
3. Make it enabled in most tests

Sadly this is not the minimal change.

One clarification: PageCache doesn't need the X-Drupal-Cache-Tags header anymore. It currently parses that header to get the cache tags of the response, but, ever since we got CacheableResponse, that's not necessary anymore. That's why it's totally fine to not even have the X-Drupal-Cache-(Tags|Contexts) headers even when (Dynamic) Page Cache is enabled: because the response already carries the cacheability metadata in a more structured form.

Yeah I agree this would not be the minimal change at all.

Small details matter.

1. +++ b/core/core.services.yml
   @@ -16,6 +16,7 @@ parameters:
   +  http.cacheability_headers.send: false
   

   I like the name :) http makes sense.

   cacheability_headers makes sense.

   But perhaps send should become debug? That'd match twig.config's debug parameter better. But that also doesn't sound quite as good.

   Hrm, not sure… Ideas?

2. --- a/core/lib/Drupal/Core/StackMiddleware/ReverseProxyMiddleware.php
   +++ b/core/lib/Drupal/Core/StackMiddleware/ReverseProxyMiddleware.php
   --- a/core/tests/Drupal/Tests/Core/StackMiddleware/ReverseProxyMiddlewareTest.php
   +++ b/core/tests/Drupal/Tests/Core/StackMiddleware/ReverseProxyMiddlewareTest.php
   

   Well, these changes don't really make sense anymore after #75's conclusion, IMO.

   We can make it simpler still: don't generate these headers at all… unless the container parameter tells us to.

   Then there is no reason for the reverse proxy middleware to strip them.

3. +++ b/sites/default/default.services.yml
   @@ -142,3 +142,7 @@ parameters:
   +  # Send cacheablity headers for debugging purposes.
   +  # By default, cacheability headers are only sent when behind a reverse proxy.
   +  # http.cacheability_headers.send: false
   

   Nit: Rather than appending this to the end of the file, I think it makes more sense to put after Twig & Renderer's settings.

Working on addressing all that.

#78 is unfortunately wrong.

We specifically also support other responses right now - that send cache tags and max-age values themselves.

Overall I am okay to remove unless that container parameter or setting is set though - and making that opt-in.

#83: that's not intentional, that's pure coincidence. There's no test coverage verifying that. It's not supported.

Well, just to be clear, I think people should be able to add the tags, if needed but we should simply not add them in the beginning.

+++ b/core/lib/Drupal/Core/StackMiddleware/ReverseProxyMiddleware.php
@@ -55,7 +65,18 @@ public function handle(Request $request, $type = self::MASTER_REQUEST, $catch =
+    // Strip X-Drupal-Cache-Contexts and X-Drupal-Cache-Tags header
+    // respectively, when either a reverse proxy is being used (so the reverse
+    // proxy or CDN can be invalidated when appropriate) or when
+    // developing/debugging.

The comment does not seem to match the implemented logic. How about:

Leave the X-Drupal-CacheContexts and X-Drupal-Cache-Tags header on the response when either a reverse proxy is being used or when developing/debugging. Otherwise remove them.

Implemented #82.

This will cause any test relying on X-Drupal-Cache-(Tags|Contexts) to fail. I'll update the other tests in the next reroll. That keeps this patch easier to review, and it's this patch that contains the crucial changes.

And now updated every test that uses assert(No)Cache(Tag|Context)().

1. +++ b/core/core.services.yml
   @@ -16,6 +16,7 @@ parameters:
   +  http.cacheability_headers.send: false
   

   I'm still not fully convinced by this name.

   I wonder if it'd be better to stress the "debug" aspect of this more?

   Then perhaps http.cacheable_response.debug_headers: false or something like that would make more sense?

2. +++ b/core/modules/page_cache/src/Tests/PageCacheTest.php
   @@ -257,6 +257,9 @@ function testPageCache() {
   +    $this->setCacheabilityHeaders(TRUE);
   

   I think "set cacheability headers" is also relatively confusing, I think "send cacheability headers" would be better? Of course, the answer to this point depends on the feedback to the first point.

+++ b/core/modules/menu_ui/src/Tests/MenuTest.php
@@ -543,6 +543,9 @@ function testSystemMenuRename() {
+    $this->setCacheabilityHeaders(TRUE);
+    $this->rebuildContainer();

Should we move the rebuilderContainer() code into the set... call? I think this would make the code a little bit less repetitive.

I think "set cacheability headers" is also relatively confusing, I think "send cacheability headers" would be better? Of course, the answer to this point depends on the feedback to the first point.

Well, I don't care that much. Afaik it should mirror the parameter as much as possible and be done, so set(Camelize($parameter_name))

I'm still not fully convinced by this name.

I wonder if it'd be better to stress the "debug" aspect of this more?

Then perhaps http.cacheable_response.debug_headers: false or something like that would make more sense?

Its a total odd thing that the cacheability information are part of the FinalResponseSubscriber, which makes it a little bit confusing to find a proper name here. I would argue that the cachability information should be moved to the list bit, as its about displaying cachability debug headers, not any kind of headers, so what about http.response.debug_cacheability_headers?

The setting being called cacheability headers suggests it'd include Cache-control, so something more specific sounds good.

Should we move the rebuilderContainer() code into the set... call? I think this would make the code a little bit less repetitive.

+1

http.response.debug_cacheability_headers

Hah, that's exactly what I had written locally in a patch already, before I decided to first gather feedback :) +1

The setting being called cacheability headers suggests it'd include Cache-control, so something more specific sounds good.

Exactly! :)

Assigning to myself to address those points.

Should we move the rebuilderContainer() code into the set... call? I think this would make the code a little bit less repetitive.

Done.

Also rebased, #92 didn't have a single fail like DrupalCI misleadingly suggested: the patch simply did not apply anymore.

The rest will follow tomorrow.

This will hopefully be the first green patch.

The nastiest remaining problem was:

Exception: _theme() may not be called until all modules are loaded. in Drupal\Core\Theme\ThemeManager->render() (line 145 of /var/www/html/core/lib/Drupal/Core/Theme/ThemeManager.php). Drupal\Core\Theme\ThemeManager->render('field', Array)

the rebuildContainer() call caused $this->moduleHandler->isLoaded() to return FALSE. A call to resetAll() fixes that (that's also how WebTestBase::setUp() works).

+++ b/sites/default/default.services.yml
@@ -115,6 +115,9 @@ parameters:
+  # By default, cacheability headers are only sent when behind a reverse proxy.

This seems to be no longer true?

To be able to link to proper documentation, I wrote https://www.drupal.org/developing/api/8/response and particularly created the https://www.drupal.org/developing/api/8/response/cacheable-response-inte... child page. I also updated https://www.drupal.org/developing/api/8/cache and https://www.drupal.org/developing/api/8/render/arrays/cacheability#headers accordingly. And I added https://www.drupal.org/developing/api/8/cache/cacheable-response-interface.

With all that out the way, I was able to finally able to rename the container parameter to http.response.debug_cacheability_headers and rename the method on WebTestBase accordingly.

Well that was rather stupid: I renamed it everywhere, except where it's actually being used.

+++ b/sites/default/default.services.yml
@@ -115,9 +115,17 @@ parameters:
+  # For more information about debugging cacheable responses, se

se -> see

#104: Thanks, fixed now! #100 was fixed in #101.

Fixed up the IS, after chatting with Wim. Looks solid to me.

Forgot to adjust the issue title for the new direction since #75.

Minor update to IS.

Is there any reason why we're not turning the headers on in all test environments where they matter - this would limit the impact on any contrib or custom tests that depend on this.

Is there any reason why we're not turning the headers on in all test environments where they matter - this would limit the impact on any contrib or custom tests that depend on this.

Well, I was originally in favour of doing that, but it required quite a lot of enabling, see https://www.drupal.org/files/issues/2527126-51.patch as a start (wasn't everything),
so it would not be the minimal change, this is for sure.

@dawehner the latest patch is doing this everywhere:

+++ b/core/modules/block/src/Tests/Views/DisplayBlockTest.php
@@ -241,6 +241,8 @@ public function testViewsBlockForm() {
   public function testBlockRendering() {
+    $this->setHttpResponseDebugCacheabilityHeaders(TRUE);

Why don't we just enable this for KernelTestBases, WebTestBase and BrowserTestBase - for all tests.

#110: Because then the tests don't match reality. And that's what caused Page Cache to be broken in beta 15 in the first place. See #29 and #31:

In other words it is a horrible idea to have a different test environment than actual code running in production.
I think we should enable the cache tags debug output in places where we need it.

Of course…

1. with the approach in the current patch, it's actually impossible to break the Page Cache by enabling/disabling these headers, because it doesn't rely on them anymore.
2. and the current patch breaks every cacheability test, in both core and contrib.

So… I think I'm +1 to @alexpott's proposal. This is probably the most reasonable approach.

+++ b/core/modules/simpletest/src/WebTestBase.php
@@ -3019,4 +3023,18 @@ protected function assertNoCacheTag($cache_tag) {
+  /**
+   * Enables/disables the cacheability headers.
+   *
+   * Sets the http.response.debug_cacheability_headers container parameter.
+   *
+   * @param bool $value
+   *   (optional) Whether the debugging cacheability headers should be sent.
+   */
+  protected function setHttpResponseDebugCacheabilityHeaders($value = TRUE) {
+    $this->setContainerParameter('http.response.debug_cacheability_headers', $value);
+    $this->rebuildContainer();
+    $this->resetAll();
+  }

In fact, this is now something that is necessary for only the most exotic tests. Nothing in core uses it.

So let's remove that too.

So let's remove that too.

/me sighs loud.

We need a test for the parameter ... to ensure that a) we don't send out the header, unless needed (we had that in earlier versions of the patch, I'm quite sure about that)
b) page caching is still working if the parameter is set to FALSE (we had a critical regression, let's ensure this doesn't happen anymore).

IMO this is ready again now. With a smaller patch than ever before. And now with zero disruption.

IS updated again. https://www.drupal.org/node/2259531 updated.

Okay, adding an explicit regression test for Page Cache. I don't think that's particularly helpful now, but totally agreed: better safe than sorry in this regard :)

Fully agreed on adding a test that verifies the debugging cacheability headers are sent or not when the container parameter is set to true or false respectively.

Great, thank you!

RTBC - I will miss looking at D8 websites and see their contexts online ;) - but yes using CachableResponseInterface is much simpler overall.

+1, a lot cleaner than before.

I like this solution, it's very clear.

I wonder if we should remove the X-Drupal-Dynamic-Cache header as well? While that one isn't as big as the contexts/tags header I think this serves the same purpose: debugging / development.
We can do that in a followup though.

I think we might need an explicit change record for https://www.drupal.org/node/2527126 in case anyone has a reverse proxy that is using the tags OR is implementing a module

#117:

RTBC - I will miss looking at D8 websites and see their contexts online ;) - but yes using CachableResponseInterface is much simpler overall.

+1 :)

#119:

I wonder if we should remove the X-Drupal-Dynamic-Cache header as well?

I see your point, but I think we don't want that, because:

• There's no danger of WSODs with that header.
• We also always expose the X-Drupal-Cache header for Page Cache, so it's consistent with that.
• There's no additional logic/module necessary for that header to be useful.

#120: CR created: https://www.drupal.org/node/2592471

Again improved the IS, the title, and the issue tags.

@Wim Leers
Why is that no longer a security improvement? I mean of course its kind of small one but I think its not the worst to not shout out internal IDs to the public everywhere.

This never was about security. The OP added that tag, and it should've been removed a long time ago. When we added this header originally, we had it thoroughly vetted by the security team. The conclusion was there was no security risk at all. If there were, we would never have sent this header by default.

Ah well, the original issue summary had it, but it got deleted ...

#124: it's still there, but crossed through, because it's wrong/silly.

It's super easy to find uncached things to "DDoS" sites: just append a random query string to any Drupal URL that returns a HTML response. You can fill up the Page Cache with millions of duplicate frontpage entries with just a different query string. This is well-known, and applies to all Drupal versions.

That's much more damaging than knowing which entities would cause a certain response to change, because an attacker would not be able to modify those entities. If they can modify those entities, they don't need these cache tags to know that anyway.

Therefore, that argument is IMHO entirely moot.

From request.module's RedirectRequestSubscriber:

      $headers = [
        'X-Redirect-ID' => $redirect->id(),
        'X-Drupal-Cache-Tags' => implode(' ', $redirect->getCacheTags()),
      ];
      $response = new TrustedRedirectResponse($url->setAbsolute()->toString(), $redirect->getStatusCode(), $headers);

This change *will* break that.

I'm a bit confused now, do we now have a cacheable redirect response object or not? I think that wasn't committed yet?

I could implement my own I guess but maybe we can find a way to keep BC there (we are in RC, after all..) by falling back to the header if it doesn't implement the interface?

I'm a bit confused now, do we now have a cacheable redirect response object or not? I think that wasn't committed yet?

No, it was not: #2573923: Introduce a CacheableRedirectResponse and use it where appropriate.

But redirect.module could have its own ConfiguredRedirectResponse extends SecuredRedirectResponse implements CacheableResponseInterface {} implementation so that it fully controls the response.

I could implement my own I guess but maybe we can find a way to keep BC there (we are in RC, after all..) by falling back to the header if it doesn't implement the interface?

I think you maintain the only module that does this.

You're right that we're breaking the ability to create any kind of response that has this header and have it work with Page Cache. But doing that allows us to have a very simple explanation: Page Cache and Dynamic Page Cache support cacheability metadata if the response contains cacheability metadata, i.e. if it implements CacheableResponseInterface. That's very easy to understand.

Do you think there are (many) other modules that would be affected by this?

I think you maintain the only module that does this.

Why do I hear that all the time ;)

I probably am, as far as contrib is concerned.. who knows what custom code is doing out there already.

You're right that we're breaking the ability to create any kind of response that has this header and have it work with Page Cache. But doing that allows us to have a very simple explanation: Page Cache and Dynamic Page Cache support cacheability metadata if the response contains cacheability metadata, i.e. if it implements CacheableResponseInterface. That's very easy to understand.

Yes, I fully agree that is much easier. But... RC ;) I think we need to be very, very careful in regards to breaking things unless there is no other way. We've had plenty of freezes, beta's and so on, and every single time, within days/weeks we more or less fell back into the old pattern of breaking things all over the place. That really has to stop now.

And this wouldn't be a complicated to do in a way that keeps BC. Just have an if/else with a @todo to remove it in 9.x.

So it is true that it breaks the module.

I'm not sure the implementation detail of 1. the naming of the header 2. internal page cache consuming that header is something we should consider an API though.

That is more for #2550249: [meta] Document @internal APIs both explicitly in phpdoc and implicitly in d.o documentation - clearly we did not document the contents/consumption of custom headers there.

Fair.

Done.

IS updated accordingly.

RTBC - the BC layer makes sense to me.

+++ b/core/modules/simpletest/src/WebTestBase.php
@@ -812,6 +812,10 @@ protected function initSettings() {
+    // During tests, cacheable responses should get the debugging cacheability
+    // headers by default.
+    $this->setContainerParameter('http.response.debug_cacheability_headers', TRUE);
...
+++ b/core/modules/page_cache/src/StackMiddleware/PageCache.php
@@ -224,7 +225,17 @@ protected function fetch(Request $request, $type = self::MASTER_REQUEST, $catch
+    // The above (superior) mechanism to retrieve a response's cache tags was
+    // introduced during the RC phase. This exists to maintain backwards
+    // compatibility.
+    // @todo Remove in Drupal 9.0.0.
+    elseif ($response->headers->has('X-Drupal-Cache-Tags')) {
+      $tags = explode(' ', $response->headers->get('X-Drupal-Cache-Tags'));
+    }

I don't like this combination. Both the name of the parameter and the first hunk setting it differently for tests than the production default implies that the 'X-Drupal-Cache-Tags' header is for debugging. But the second hunk means that there's a non-debugging aspect to the header as well. That then puts us into some fuzzy territory with potential for bugs and WTFs.

I see a couple ways out of this:

1. Either accept breaking a little BC, and remove that second hunk. But in order to not make use cases like #126 result in invalid caching, make PageCache only cache CacheableResponseInterface responses.
2. Or accept that 'X-Drupal-Cache-Tags' is not a debugging header, but a functional header, and revert PageCache to what it is in HEAD. And instead of solving this overall issue with a container parameter that implies "debugging", solve it with another middleware that runs at a higher priority than PageCache and strips out the headers.

Not changing issue status in case another committer disagrees with me and wants to commit as-is.

But the second hunk means that there's a non-debugging aspect to the header as well. That then puts us into some fuzzy territory with potential for bugs and WTFs.

I completely agree.

2. […] And instead of solving this overall issue with a container parameter that implies "debugging", solve it with another middleware that runs at a higher priority than PageCache and strips out the headers.

More middlewares mean worse performance.

I vote for option 1, of course.

Discussed in irc with berdir.

I personally don't think the custom header counts as an API - if we'd thought of it prior to rc, it'd be documented in https://www.drupal.org/node/2562903 (which I've just added a note to for any similar issues that come up in the future).

However, before this issue lands (or as part of it) we should ensure that for path redirect, redirect responses stop getting cached altogether.

i.e. if we'd never had that header, then path_redirect would not have been able to use it, so the only option would have been to not have any redirect caching. That fact that we cache redirects in core now without #2573923: Introduce a CacheableRedirectResponse and use it where appropriate is a bug. That might have been a contributed project blocker, but it wouldn't have been any kind of API change then. Sometimes nothing is better than something...

Approached that way, it's a bug fix to stop caching any redirects, and an API addition to start caching some of them. And path_redirect then has some dead code that relied on a core implementation detail (and via berdir also broken tests that were testing the behaviour). Just breaking tests doesn't necessarily mean we changed an API though, a string change can break a test too.

For that reason I think we should not add a bc layer here, because it's an 'API that should never have been', but we need to think very carefully about this and make sure the documentation reflects any decisions taken like this. And it'd be good to make sure people on this issue actually agree with those arguments - this is going to come up more places than here and we need to be as consistent as we can.

#136++

Per #136, re-uploading #115. Interdiff compared to #129 is then of course the inverse of #129's interdiff.

It looks like we've considered every possible angle. We've tried to maintain a BC layer but as per @effulgentsia this ends up being more confusing and breaking redirect caching is sensible given that this was relying on a side effect rather than an API. Given the discussions here I think we should agree that #2573923: Introduce a CacheableRedirectResponse and use it where appropriate is an RC target.

@catch had the idea of postponing this on #2573923: Introduce a CacheableRedirectResponse and use it where appropriate. I agree. That way we have no regression.

#126 is actually not cached in page cache, because of #2476407: Use CacheableResponseInterface to determine which responses should be cached, which went in before RC and did affect both page cache and caching in reverse proxies.

So unless contrib also modified the Expires and Cache-Control headers - which is not the case for redirect module, then the point of BC is very moot.

If however this continues to be cached - despite #2476407: Use CacheableResponseInterface to determine which responses should be cached then that is a bug in PageCache - however that is unlikely.

Opened #2593887: Follow-up: Do not cache private or no-store responses in PageCache, berdirs tests passing is a bug. They should be failing by now.

+++ b/core/modules/page_cache/src/StackMiddleware/PageCache.php
@@ -224,7 +225,7 @@ protected function fetch(Request $request, $type = self::MASTER_REQUEST, $catch
-    $tags = explode(' ', $response->headers->get('X-Drupal-Cache-Tags'));
+    $tags = ($response instanceof CacheableResponseInterface) ? $response->getCacheableMetadata()->getCacheTags() : [];
     $this->set($request, $response, $expire, $tags);

This doesn't quite implement my suggestion in #134.1. My suggestion there is that the ->set() should not be done for a $response that isn't CacheableResponseInterface.

Following on #140, I don't think #2476407: Use CacheableResponseInterface to determine which responses should be cached is enough of an equivalent to that. Because that issue still allows a Response that isn't a CacheableResponseInterface to use Cache-Control headers and therefore be cacheable by something, which is correct. But say such a custom Response also sets some other headers that manage tag-like invalidation (e.g., Surrogate-Keys, if this is a custom module that's catered to a particular deployment architecture). In such a case, since PageCache doesn't know about such headers, it should simply not be in the business of trying to cache such a Response.

Or in other words, modules that want to work with caching generically rather than deployment-specifically should return CacheableResponseInterface responses. Then PageCache can cache it and various contrib modules can provide additional middlewares to transform CacheableResponseInterface information to the headers needed by a corresponding proxy/CDN. But, if a module chooses to not return CacheableResponseInterface, but instead manages its own response headers, then PageCache should skip over it and leave caching to whatever software that module catered the headers to.

#142 is eminently sensible. It'd make the entire response caching infrastructure in Drupal 8 super simple to understand:

Drupal 8 itself will only ever cache CacheableResponseInterface responses. In both the Page Cache and Dynamic Page Cache.

(i.e. Dynamic Page Cache already has this restriction.)

This patch implements that, and adds the necessary test coverage. (And fixes the broken bits in \Drupal\page_cache\Tests\PageCacheTest::testCacheableResponseResponses().)

#140: I found why #2476407: Use CacheableResponseInterface to determine which responses should be cached's test coverage did not do what you expected it to do: because the assertions do

$this->assertFalse(in_array('X-Drupal-Cache', $this->drupalGetHeaders()), 'Drupal page cache header not found');

… but $this->drupalGetHeaders() returns lower-cased header names.

1. +++ b/core/modules/page_cache/src/StackMiddleware/PageCache.php
   @@ -220,12 +230,9 @@ protected function fetch(Request $request, $type = self::MASTER_REQUEST, $catch
   +    $max_age = $response->getCacheableMetadata()->getCacheMaxAge();
   +    $expire = ($max_age === Cache::PERMANENT) ? Cache::PERMANENT : (int) $request->server->get('REQUEST_TIME') + $max_age;
   

   Not sure about this change, I think that should be done/discussed separately?

   So that we can test what this actually means for a standard install or when you add statistics.module to the mix (which now defines a max age of 3600 by default if you can view the node counter. Should have test coverage and so on...

2. +++ b/core/modules/page_cache/src/Tests/PageCacheTest.php
   @@ -420,15 +451,25 @@ public function testCacheableResponseResponses() {
        // Try to fill the cache.
        $this->drupalGet('/system-test/respond-reponse');
   -    $this->assertFalse(in_array('X-Drupal-Cache', $this->drupalGetHeaders()), 'Drupal page cache header not found');
   ...
        // Try to fill the cache.
   +    $this->drupalGet('/system-test/respond-public-response');
   
   @@ -444,12 +485,8 @@ public function testCacheableResponseResponses() {
        // Try to fill the cache.
   

   I think that comment is still weird, considering that we don't actually try to do that?

Other than that, looks great.

+++ b/sites/default/default.services.yml
@@ -115,6 +115,17 @@ parameters:
+  http.response.debug_cacheability_headers: false

Would be cool to add this to development.services.yml with a true value IMO. (That might also mitigate some of @FabianX's sadness ;-))

Can be a follow-up, though, and should in case that's in any way controversial.

#146.1: can you explain what the problem with that is?

I can't reproduce the fail in https://www.drupal.org/files/issues/2527126-143.patch locally… re-testing.

#146

1. I don't see what's controversial about that TBH? #2352009: [pp-3] Bubbling of elements' max-age to the page's headers and the page cache is what is controversial, because it also affects the header that we send out to end users, and thus will affect how long stale content may be shown to the user. This on the other hand is still invalidated instantaneously thanks to cache tags and merely ensures that the responses cached are refreshed more often.

   Can you explain in what way this could be problematic? Happy to add test coverage. Happy to defer to a follow-up too, but I want to understand it better.

2. Agreed that those comments were very confusing. Fixed all of them.

#147: I'm afraid that is highly controversial. More so because if this would be in there, then twig.config.debug etc should be too. I totally see your reasoning though: it currently requires too many steps/is too confusing to enable any of those "debug" container parameters. Could you file a separate issue for that?

Would be cool to add this to development.services.yml with a true value IMO. (That might also mitigate some of @FabianX's sadness ;-))

Great idea!

Not sure about this change, I think that should be done/discussed separately?

You are totally right IMHO. This bit does NOT solve the issue we have, its a separate issue which needs separate tests. Due to the usage of time() vs. REQUEST_TIME the testbot might exceed the time a bit. When I debugged it locally it most of the time one test failure occured, but on some circumstances the debugger slowed it down even more, so that sometimes even two test failed. Let's revert that bit, its not needed in this particular issue.

(crosspost with #150/151)

1. I'm not sure there is a problem, but it is IMHO an unrelated change, one that needs a dedicated issue and testing. It changes behavior that is not related to this issue but #2352009: [pp-3] Bubbling of elements' max-age to the page's headers and the page cache.

To see one example of how this changes things: install statistics.module, enable content view counter, give permission to anon users. Without this patch, page cache doesn't care and caches forever. With this patch, the expire is now set to 1h. That's a great improvement, actually, but it has nothing to with this issue IMHO. Lets do that in the issue mentioned above where we should also discuss how Cache-Control: max-age should behave exactly, right now, that doesn't respect this either.

#147: I'm afraid that is highly controversial. More so because if this would be in there, then twig.config.debug etc should be too. I totally see your reasoning though: it currently requires too many steps/is too confusing to enable any of those "debug" container parameters. Could you file a separate issue for that?

Well, here is one: #2594543: Put http.response.debug_cacheability_headers into default.services.yml ...

+1 to this direction and thanks for fixing the test. Given this direction I think I can close #2593887: Follow-up: Do not cache private or no-store responses in PageCache as a duplicate of this :).

Or maybe we should do the cache only cacheable responses in its own issue first?

#152 + #153: ahh, of course! Thanks for the explanation, both of you! :)

#154: thanks for filing that!

#155: No, that is still necessary. Or is at least a valuable thing to discuss, come to consensus, and formalize the expectations in the form of a test. Because even if a response implements CacheableResponseInterface, if it's marked as private or no-store, it could be argued that Page Cache should in fact not cache it.

#152 looks great to me.

+++ b/core/modules/page_cache/src/StackMiddleware/PageCache.php
@@ -208,10 +209,19 @@ protected function lookup(Request $request, $type = self::MASTER_REQUEST, $catch
+    // Page Cache only works with cacheable responses. It does not work with
+    // plain Response objects.
+    if (!$response instanceof CacheableResponseInterface) {
+      return $response;
+    }

If this is really the case, then please give some justification at least in the issue summary, but probably also in the comment.

Update: I understand that this comes from #142, but still needs documentation.

#2573923: Introduce a CacheableRedirectResponse and use it where appropriate landed a few hours ago, so that blocker is out of the way.

@effulgentsia is going to reply to #157.

Then we can hopefully go to RTBC.

As long the final response object can be used to extract somehow those caching information I'm happy with it, because well, I had usecases to fetch these information already to be honest.

This adds the docs requested in #157. I'm open to feedback on how to improve it or make it more concise.

+++ b/core/modules/page_cache/src/StackMiddleware/PageCache.php
@@ -207,33 +207,56 @@
+    // - Custom applications that wish to provide internal page cache support
+    //   for responses that do not implement CacheableResponseInterface may do
+    //   so by replacing/extending this middleware service or adding another
+    //   one.
     if (!$response instanceof CacheableResponseInterface) {
       return $response;
     }

When I wrote this, I thought, hm, maybe this check should be moved to a response policy instead of being baked into this class. But I'm unclear on whether the page_cache_response_policy service tag is to only control what is cached by page_cache module, or whether those policies should also control what gets cached by reverse proxies and CDNs (via the corresponding integration module). If the latter, then this should not move to a response policy, since we only want this affecting page_cache itself. Do we have/need an issue to discuss/document this aspect of response policies?

Updated summary to reflect the #160 patch. The last update was in #130, so was behind by several patches.

When I wrote this, I thought, hm, maybe this check should be moved to a response policy instead of being baked into this class.

Doing so would not be an API change. It would be a behavior change that results in the same exact behavior by default, but would open the door to sites removing or changing this policy. Which means this would be a pure API addition. So we could do so at a later time, when the need arises.

It's better to start simpler & more strict and add more abilities later.

Do we have/need an issue to discuss/document this aspect of response policies?

I think we need to discuss that, yes. Given that these are used by both the PageCache middleware and the FinishResponseSubscriber to determine the Cache-Control headers that are sent to the world, the page_cache_response_policy policies actually do affect both Drupal's internal page cache and external proxies.

I think it's worth taking this aspect into consideration in #2540684: Rename Drupal\Core\PageCache namespace, so I propose we continue that discussion there.

#163: Also the FinishResponseSubscriber already has a check hard-coded for CacheableDependencyInterface and marks the response as uncacheable then (unless headers have been modified).

And unless we want to support the full HTTP spec like Symfony's HttpCache middleware, I don't think it would be wise to store anything besides CacheableResponseInterface responses, where we can say:

Just like smart_cache, just like render_cache. _Only_ CacheableDependencyInterface information is taken into account.

That makes it (as Wim has already said several times) very simple and easy to understand.

So +1 to hard-coding for now.

RTBC + 1 for #160 - not sure what is missing here, so not setting status value, but patch looks good to me.

+++ b/core/modules/page_cache/src/StackMiddleware/PageCache.php
@@ -206,25 +207,55 @@ protected function lookup(Request $request, $type = self::MASTER_REQUEST, $catch
+    // encode cache tags in a response. Different CDNs implement their own
+    // approaches, and configurable reverse proxies (e.g., Varnish) allow for
+    // custom implementations. To keep Drupal's internal page cache simple, we
+    // only cache CacheableResponseInterface responses, since those provide a
+    // defined API for retrieving cache tags. For responses that do not
+    // implement CacheableResponseInterface, there's no easy way to distinguish
+    // responses that truly don't depend on any site data from responses that
+    // contain invalidation information customized to a particular proxy or
+    // CDN.
+    // - Drupal modules are encouraged to use CacheableResponseInterface
+    //   responses where possible and to leave the encoding of that information
+    //   into response headers to the corresponding proxy/CDN integration
+    //   modules.
+    // - Custom applications that wish to provide internal page cache support
+    //   for responses that do not implement CacheableResponseInterface may do
+    //   so by replacing/extending this middleware service or adding another
+    //   one.
+    if (!$response instanceof CacheableResponseInterface) {
+      return $response;
+    }
+
+    // Currently it is not possible to cache binary file or streamed responses:
+    // https://github.com/symfony/symfony/issues/9128#issuecomment-25088678.
+    // Therefore exclude them, even for subclasses that implement
+    // CacheableResponseInterface.
     if ($response instanceof BinaryFileResponse || $response instanceof StreamedResponse) {

So can anyone explain me actually why anyone of this is in scope of this issue? The issue is that we sent out a too big HEADER, and this is all. Other changes should live in its own issue, IMHO

#157 is addressed, I think #153 and previous concerns from @Berdir are addressed as well according to #2573923-76: Introduce a CacheableRedirectResponse and use it where appropriate and the change record looks right.

• Given that this was already RTBC at #116 by dawehner and confirmed in #117, #118, #119 by three others (Fabianx, znerol, borisson_)
• Alex Pott then RTBC'd in #138 but un-RTBC'd one comment later to postpone on #2573923: Introduce a CacheableRedirectResponse and use it where appropriate, which has landed since
• effulgentsia raised a concern in #142 that we should be even stricter/simpler/clearer about what to cache; that's what the reroll in #143 was for; the subsequent few rerolls refined that and minimized the amount of change

That means every single concern here has been addressed and carefully weighed against all other concerns. We have minimal changes, extremely carefully determined decisions.

What's holding back RTBC?

EDIT: cross-posted with #166. Reply to that: Because we are no longer sending the X-Drupal-Cache-Tags header always, which means PageCache cannot rely on it, which means it must rely on CacheableResponseInterface responses' metadata, which led to further discussions that are inherently intertwined with that change, which then after carefully achieved consensus has resulted in this. Ideally, yes, it would live in a separate issue, but it's so closely intertwined that it doesn't really make sense to split it up IMO.

EDIT: cross-posted with #167 too. Thanks @znerol.

Thanks everyone - good discussion of what to do here with respect to backwards compatibility - and sorry @Berdir for breaking yet another of your modules but at least this time we had the solution in first.

Committed ff622b5 and pushed to 8.0.x. Thanks!