Improve X-Drupal-Cache and X-Drupal-Dynamic-Cache headers, even for responses that are not cacheable

1. Only CacheableResponseInterface response objects can get X-Drupal-Dynamic-Cache: UNCACHEABLE (i.e. the response varies by a high-cardinality cache context or a high-invalidation frequency cache tag, or a max-age=0 — see \Drupal\dynamic_page_cache\EventSubscriber\DynamicPageCacheSubscriber::shouldCacheResponse()).
2. Any response object that does not implement CacheableResponseInterface cannot ever be processed by Dynamic Page Cache, and thus doesn't get any response header at all.

I do understand that this can be confusing though. Would setting X-Drupal-Dynamic-Cache: INELIGIBLE for case 2 help?

Does it make sense?

Yes and no. For Page Cache, we also don't set a X-Drupal-Cache header on every response. Why should Dynamic Page Cache do so then? But you're right that Dynamic Page Cache is harder to understand.

Let's do this!

This patch should cause failures in the existing test coverage.

This updates the expectations per the changes in #5.

Apparently the layout_builder module has impressively detailed test coverage! 👏

1. +++ b/core/modules/dynamic_page_cache/src/EventSubscriber/DynamicPageCacheSubscriber.php
   @@ -156,6 +156,7 @@ public function onResponse(FilterResponseEvent $event) {
   +      $response->headers->set(self::HEADER, 'UNCACHEABLE (no cacheability)');
   
   @@ -167,7 +168,7 @@ public function onResponse(FilterResponseEvent $event) {
   +      $response->headers->set(self::HEADER, 'UNCACHEABLE (poor cacheability)');
   
   @@ -191,6 +193,7 @@ public function onResponse(FilterResponseEvent $event) {
   +      $response->headers->set(self::HEADER, 'UNCACHEABLE (policy)');
   

   These 3 are explicitly tested by \Drupal\Tests\dynamic_page_cache\Functional\DynamicPageCacheIntegrationTest::testDynamicPageCache().

2. +++ b/core/modules/dynamic_page_cache/src/EventSubscriber/DynamicPageCacheSubscriber.php
   @@ -184,6 +185,7 @@ public function onResponse(FilterResponseEvent $event) {
   +      $response->headers->set(self::HEADER, 'UNCACHEABLE (subrequest)');
   

   This one is not yet, and is also the one I was least certain about. Based on the comment, this should probably not set a X-Drupal-Dynamic-Cache header.

   That last remaining failure confirms this suspicion:

   --- Expected
   +++ Actual
   @@ @@
   -MISS
   +UNCACHEABLE (subrequest)
   

   So, reverting that one line.

IS updated to clarify the changes.

@Chi: Thoughts? :)

What if we apply the same to Page cache module?

I do like the idea of keeping both modules similar/consistent in their headers!

Let's see which tests fail.

Cool :) Changing component again — sorry for the nitpicking!

Note that we should still add page_cache test coverage.

+

We should have a change record so people are informed about this. The CR should also document what the new header values mean. Especially UNCACHEABLE (poor cacheability) is that is a bit cryptic.

Agreed! CR created: https://www.drupal.org/node/2958442.

Test coverage for page_cache.

And I think we can actually add X-Drupal-Cache: UNCACHEABLE (no cacheability) to page_cache too? At which point page_cache's headers would entirely match dynamic_page_cache's, except for UNCACHEABLE (poor cacheability), which does not make sense for page_cache.

Forgot #28's interdiff apparently.

Let's untangle request policy vs response policy, and be consistent about it.

#30: Yes, but first let's see whether A) it actually works, B) whether you and Alex Pott think it's a good idea :)

This should make many of the existing tests pass! Maybe even all.

#37 was rolled against 8.5. Oops. Rebased on top of 8.6.

The remaining test failures are all old WebTestBase tests that needed updating now that page_cache communicates the reason for not caching explicitly.

Ready for final review! :)

+++ b/core/modules/dynamic_page_cache/src/EventSubscriber/DynamicPageCacheSubscriber.php
@@ -156,6 +156,7 @@ public function onResponse(FilterResponseEvent $event) {
     if (!$response instanceof CacheableResponseInterface) {
+      $response->headers->set(self::HEADER, 'UNCACHEABLE (no cacheability)');
       return;

@@ -167,7 +168,7 @@ public function onResponse(FilterResponseEvent $event) {
-      $response->headers->set(self::HEADER, 'UNCACHEABLE');
+      $response->headers->set(self::HEADER, 'UNCACHEABLE (poor cacheability)');

I'm curious what are potential risks that code out there relies on this exact output? Could we reduce the risk by maybe doing something like having an additional -explanation header or so? I don't see any discussion about this yet. I could imagine that for example tests in contrib modules stop working

Rebased because it did not apply on 8.6.2

@yobottehg Thanks! Seems something went wrong though. Rebased it too.

#43: Great point! I think an additional *-Explain or *-Reason header is worth considering. OTOH, the updates to test coverage this may require in some projects are A) trivial, B) valuable. They're valuable because they result in explicitly documenting why some responses are uncacheable!

Hence #44's proposal sounds reasonable to me. I'm happy to help create patches for affected contrib modules.

What do you think, @dawehner?

Patch does not apply to latest 8.7 dev, nor 8.6.3 stable. Needs a re-roll.

Here is the reroll for current 8.7 dev

@ndobromirov++

RTBC - Looks great! Love it :)

Needs a reroll

I've tried to reroll the patch.

Rebased #57.

Note that with JSON:API having landed, most JSON:API integration tests will also fail with this change; they'll need to be updated.

There is one case where Dynamic Page Cache doesn't yet set a header:

    // Don't cache the response if Dynamic Page Cache's request subscriber did
    // not fire, because that means it is impossible to have a Dynamic Page
    // Cache hit. This can happen when the master request is for example a 403
    // or 404, in which case a subrequest is performed by the router. In that
    // case, it is the subrequest's response that is cached by Dynamic Page
    // Cache, because the routing happens in a request subscriber earlier than
    // Dynamic Page Cache's and immediately sets a response, i.e. the one
    // returned by the subrequest, and thus causes Dynamic Page Cache's request
    // subscriber to not fire for the master request.
    // @see \Drupal\Core\Routing\AccessAwareRouter::checkAccess()
    // @see \Drupal\Core\EventSubscriber\DefaultExceptionHtmlSubscriber::on403()
    $request = $event->getRequest();
    if (!isset($this->requestPolicyResults[$request])) {
      return;
    }

That case was being hit.

This should allow more tests to pass. I haven't touched the JSON:API tests yet.

This updates many JSON:API assertions but may not result in a much higher number of passing tests, due to the way that DrupalCI counts tests (if even one of 10 test methods in a test class fails, that's counted as a single failing test).

The presence of X-Drupal-Cache and X-Drupal-Dynamic-Cache on even uncacheable responses is A) pointless, B) extremely disruptive. So let's not do that anymore.

I actually found this patch really useful lately in debugging some caching issues with a REST resource.
I didn't actually understand at first that a missing X-Drupal-Cache header means it's not cacheable with internal page cache. It was nice to actually still see the header stating it's actually uncacheable.

I did find it strange that Insomnia shows `X-Drupal-Cache UNCACHEABLE (request policy)` whereas Postman shows `X-Drupal-Cache →UNCACHEABLE (response policy)` for the same URL.

I actually found this patch really useful lately in debugging some caching issues with a REST resource.

🥳 That's what this is aiming to achieve!

I didn't actually understand at first that a missing X-Drupal-Cache header means it's not cacheable with internal page cache. It was nice to actually still see the header stating it's actually uncacheable.

Exactly! The problem is not you not being smart enough, the problem is Drupal's response caching modules not communicating explicitly enough.

I did find it strange that Insomnia shows `X-Drupal-Cache UNCACHEABLE (request policy)` whereas Postman shows `X-Drupal-Cache →UNCACHEABLE (response policy)` for the same URL.

Are you sure that one of them wasn't X-Drupal-Dynamic-Cache?

Would you like to take a stab at finishing this issue by solving the last test failures? The number of failing tests is high, but many of them have the same root cause. For example:

There were 3 failures:

1) Drupal\Tests\datetime\Functional\EntityResource\EntityTest\EntityTestDateonlyTest::testPost
Failed asserting that false is true.

/var/www/html/core/tests/Drupal/KernelTests/AssertLegacyTrait.php:35
/var/www/html/core/modules/rest/tests/src/Functional/ResourceTestBase.php:415
/var/www/html/core/modules/rest/tests/src/Functional/ResourceTestBase.php:465
/var/www/html/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php:854

2) Drupal\Tests\datetime\Functional\EntityResource\EntityTest\EntityTestDateonlyTest::testPatch
Failed asserting that false is true.

/var/www/html/core/tests/Drupal/KernelTests/AssertLegacyTrait.php:35
/var/www/html/core/modules/rest/tests/src/Functional/ResourceTestBase.php:415
/var/www/html/core/modules/rest/tests/src/Functional/ResourceTestBase.php:465
/var/www/html/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php:1080

3) Drupal\Tests\datetime\Functional\EntityResource\EntityTest\EntityTestDateonlyTest::testDelete
Failed asserting that false is true.

/var/www/html/core/tests/Drupal/KernelTests/AssertLegacyTrait.php:35
/var/www/html/core/modules/rest/tests/src/Functional/ResourceTestBase.php:415
/var/www/html/core/modules/rest/tests/src/Functional/ResourceTestBase.php:465
/var/www/html/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php:1322

All three are due to the first line here not succeeding:

    else {
      $this->assertTrue($response->hasHeader('X-Drupal-Cache'));
      $this->stringStartsWith('UNCACHEABLE (', $response->getHeader('X-Drupal-Cache')[0]);
    }

Why not? Because the 404 response is not triggered by Drupal, but very early by Symfony, in \Symfony\Component\HttpKernel\EventListener\RouterListener::onKernelRequest(). So we'll either need to refine our expectations or ensure that 404 responses for non-existent routes also have a X-Drupal-Cache response header.

#3089957: Set X-Drupal-Cache-Max-Age to aid in debugging #cache fun landed. Let's land this too now!

Since all of the failing hunks from #70 patches were tests, I just applied partially to make the patch work on 8.9. Need to go back and review those tests.

#79 apply successfully on 8.9 so, just run test case for 8.9

re-roll patch for 9.2

I just want to add that after my attempt, I found that it did not solve my issue. As anonymous user, I always get a HIT on a REST response that should have max-age cacheability. Logged-in users correctly invalidate the cache. I will try to debug this but I don't have much time for this personal project so I might just give up and go with a tag + cron.

This is not meant to solve anything, it just adds debug output (it is a task not a bugfix). What you're looking for is #2352009: Bubbling of elements' max-age to the page's headers and the page cache and https://www.drupal.org/project/cache_control_override or a similar custom solution. But beware, there be dragons, there are reason why this isn't happening atm, which are documented in that issue and just enabling that module could cause all kinds of pages to not be cached anymore.

Fixed few test cases.

Fixed more test cases.

@sanjayk Thanks for pushing this forward! 🙏👏

Could you please add interdiffs for the patches you posted, and for all future patches? Otherwise it's rather difficult to see what you changed 😅🤓

Thanks @Wim Leers for reminding me. I was totally forgot to upload.
Sure, I will keep in mind and upload interdiff in future :)

Took a stab at moving this forward.

1. Needed a reroll, conflicted with #3228634: Move tests for integrations between QuickEdit and other modules into QuickEdit so that it can more easily be moved into contrib. That seems to be because #2266809: Make QuickEditEntityFieldAccessCheck::access() use the $account that's passed in was accidentally merged with this so I reverted those changes.
2. A bunch of tests seem to be failing because EntityResourceTestBase::testDelete was asserting cache headers but they where removed on uncacheable methods in an earlier patch.

Rebased the MR on 9.5.x.

However, I'm not sure about the choice of strings such as 'UNCACHEABLE (poor cacheability)'. So far, they are single words all in caps - clear and legible.

Could we find different single words for the different flavours of UNCACHEABLE?

Updated title. And the latest MR needs to have its base update to 9.5 or 10.1.

rerolled. Open to alternative names but don't have any suggestions. Good enough for developer experience header?

I'd love to RTBC this but it's not passing tests anymore 🙈

Could we find different single words for the different flavours of UNCACHEABLE?

I don't see how. The all-caps word is the result: HIT, MISS or UNCACHEABLE. The parenthetical provides the reason. You may be interested in just the result and not care about the reason. The reason is especially helpful as a developer who aims to make a response cacheable. So … I think this is already as simple and clear as it can be? 😊

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.

Updated the change record to match the MR.

Narrowed from 268 to 8 failures.

Moving to NW for the failures.

This would be fantastic to ship at last! 🤞

Needs a new MR against 11.x. Let's finally land this — we now have fantastic new tooling such as StandardPerformanceTest (thanks to #3352851: Allow assertions on the number of database queries run during tests and many follow-ups) that this would be a valuable addition to, especially for real-world sites. Just today, I helped out a colleague in Acquia's Professional Services department, and it'd have been far simpler to narrow down root causes if this would've existed. #3421164: Log every individual query in performance tests landed earlier today too, so I'm convinced we can do this one as well! 😊

I have rerolled to 11.x and have brought down the test failure to 6 :), please review the reroll and suggest path forward to fix the remaining test failures.
Thanks !

Believe the failures are showing that the solution needs to be relooked at.

All test Passed , moving to review !

Hiding patches and old MRs for clarity.

Updated CR version

Updated issue summary to include release notes, as I think this is a good highlight.

Believe the reroll is correct.

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

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

Back to review \o/

Even if this gets merged, this other issue still remains relevant.

Ran test-only feature https://git.drupalcode.org/issue/drupal-2951814/-/jobs/1740771 which shows test coverage

CR is well written with the examples

+1 from me

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

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

Back to RTBC after rebase.

Meh... ofc simple rebase wasn't enough

Drupal\Tests\jsonapi\Functional\UserTest::testGetIndividual
Failed asserting that false is identical to true.

core/modules/jsonapi/tests/src/Functional/ResourceTestBase.php:1030

This should be also caused by the recently merged https://git.drupalcode.org/project/drupal/-/commit/9fc1bc9ac5a785ebf6e88......

Rebase seems good.

(rebased)

This will further improve and simplify cache debugging! Thank you all very much for your hard work on this. Very much looking forward to this functionality!

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

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

Gitlab says MR still applies, back to RTBC.

Checking GL, it does say "Merge conflicts must be resolved." so think this needs another rebase.

Indeed... :sadpanda: Rebasing again... PHPStorm could auto-resolve all :fingers-crossed:.

 git rebase origin/11.x
Auto-merging core/modules/basic_auth/tests/src/Functional/BasicAuthTest.php
Auto-merging core/modules/jsonapi/tests/src/Functional/NodeTest.php
CONFLICT (content): Merge conflict in core/modules/jsonapi/tests/src/Functional/NodeTest.php
Auto-merging core/modules/jsonapi/tests/src/Functional/ResourceTestBase.php
CONFLICT (content): Merge conflict in core/modules/jsonapi/tests/src/Functional/ResourceTestBase.php
Auto-merging core/modules/layout_builder/tests/src/Functional/LayoutSectionTest.php
Auto-merging core/modules/node/tests/src/Functional/NodeBlockFunctionalTest.php
Auto-merging core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
error: could not apply d8f6085835... Applying https://www.drupal.org/files/issues/2021-02-24/2951814-85.patch
hint: Resolve all conflicts manually, mark them as resolved with
hint: "git add/rm <conflicted_files>", then run "git rebase --continue".
hint: You can instead skip this commit: run "git rebase --skip".
hint: To abort and get back to the state before "git rebase", run "git rebase --abort".
Could not apply d8f6085835... Applying https://www.drupal.org/files/issues/2021-02-24/2951814-85.patch

believe rebase is good.

Rebased the forked branch ~~ RTBC++

Rebased, there was only one conflict in a test file that PHPStorm could autoresolve easily.

❯ git rebase origin/11.x
Auto-merging core/modules/jsonapi/tests/src/Functional/ResourceTestBase.php
Auto-merging core/modules/language/tests/src/Functional/LanguageBrowserDetectionAcceptLanguageTest.php
Auto-merging core/modules/page_cache/tests/src/Functional/PageCacheTest.php
CONFLICT (content): Merge conflict in core/modules/page_cache/tests/src/Functional/PageCacheTest.php
Auto-merging core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
Auto-merging core/modules/rest/tests/src/Functional/ResourceTestBase.php
Auto-merging core/modules/system/tests/src/Functional/Session/SessionTest.php
Auto-merging core/modules/system/tests/src/Functional/System/ErrorHandlerTest.php
Auto-merging core/modules/user/tests/src/Functional/UserPasswordResetTest.php

May I recommend this feature to 10.4.0 release notes?

This would certainly have saved me hours of debugging. Perhaps this is a bridge too far, but it would also help to identify which modules are adding which tags/contexts.

Updated issue credit. Removed credit for partial rerolls and tried to credit everyone who contributed test fixing & comments that help direct the work.

Committed 19f80f6 and pushed to 11.x. Thanks!

Let's backport this to 10.4.x

Created the backport MR - I wanted a full test run. There were no merge conflicts so I'm going to set to RTBC in the hope it is green. If it is, I will cherry-pick the 11.x commit to 10.4.x