Our cacheability debug headers show completely wrong information

Created on 3 May 2019, about 6 years ago
Updated 13 March 2023, over 2 years ago

Exposed in Abstract RenderCache into a separate service that is capable of cache redirects in a non-render array-specific way Fixed so you can read the full thought process there but here's the TLDR:

Both X-Drupal-Cache-Tags and X-Drupal-Cache-Contexts are set in FinishResponseSubscriber (FRS). This is before page cache gets the response but after dynamic page cache has handled it. This is wrong for both tags and contexts.

To add insult to injury, FRS folds the cache contexts before setting them in the header, but does not similarly affect the cache tags. This is very misleading for numerous reasons:

  • Only the cache contexts gathered up until DynamicPageCacheSubscriber (DPCS) truly matter because that's where they'll affect the cache. Any contexts added later on -such as in RouteAccessResponseSubscriber (RARS)- do nothing at all because page cache doesn't leverage contexts, only tags.
  • By folding the contexts in FRS, we therefore expose incorrect data. Suppose we got to DPCS with user.permissions and not user, but RARS would add user to the response, then X-Drupal-Cache-Contexts would simply show user, even though the DPC stored the page with user.permissions
  • If we are folding contexts, it affects the tags. This is not represented in X-Drupal-Cache-Tags

The reason this currently works with our thousands of tests checking for the 2 headers is twofold:

  1. RenderCache (accidentally?) bleeds information about folded contexts/tags, which causes it to bubble up to the response. By rewriting RenderCache in the issue linked at the top, this became very obvious through many test fails.
  2. Most of our tests check for the wrong contexts because their writers seemingly had no clue how broken the headers were. We even have tests checking for user after folding user.permissions, but not the tags set by said folding.

Proposed solution

In the long run, we should move X-Drupal-Cache-Tags to PageCache, where it actually makes sense. We should also move X-Drupal-Cache-Contexts to DPCS where it makes sense for contexts to be debugged.

This will, however lead to ambiguity in X-Drupal-Cache-Tags because we might want to debug tags for DPC and PC individually. So I suggest we add a third header X-Drupal-Dynamic-Cache-Tags and rename the context one to X-Drupal-Dynamic-Cache-Contexts.

We can finally add X-Drupal-Cache-Contexts-Deprecated to PageCache and allow failing tests to check that header while we clean them up on a smaller scale in follow-ups. The end goal would be to remove said header altogether.

🐛 Bug report
Status

Needs work

Version

10.1

Component
Cache 

Last updated 12 days ago

Created by

🇧🇪Belgium kristiaanvandeneynde Antwerp, Belgium

Live updates comments and jobs are added and updated live.
  • Needs tests

    The change is currently missing an automated test that fails when run with the original code, and succeeds when the bug has been fixed.

Sign in to follow issues

Comments & Activities

Not all content is available!

It's likely this issue predates Contrib.social: some issue and comment data are missing.

Production build 0.71.5 2024