Problem/Motivation

Debugging caching can be hard.
https://www.previousnext.com.au/blog/ensuring-drupal-8-block-cache-tags-...
...

Although http.response.debug_cacheability_headers: true lets you see your tags and contexts, there's no easy way to see WTF is going on with max-age.

Unfortunately, just knowing what your #cache['max-age'] is doesn't tell you much until this pit of snakes is untangled:

But, at least knowing you're trying to do the right thing could help in the meanwhile. ;)

It also seems very useful to be able to compare Expires vs. Cache-Control vs. X-Drupal-Cache-Max-Age in various circumstances and configurations as we try to navigate the snake pit.

Proposed resolution

If http.response.debug_cacheability_headers: true is in your services.yml file, also set a X-Drupal-Cache-Max-Age response header.

Remaining tasks

  1. Write code
  2. Write tests
  3. Reviews + refinements
  4. RTBC
  5. Commit
  6. Reach back down into the snake pit and try to make progress on making max-age actually work. :/

User interface changes

None.

API changes

None.

Data model changes

None.

Release notes snippet

Probably none.

CommentFileSizeAuthor
#18 3089957.7_18.interdiff.txt3.61 KBdww
#18 3089957-18.patch5.67 KBdww
#7 3089957.4_6.interdiff.txt598 bytesdww
#7 3089957-6.patch5.43 KBdww
#4 3089957.2_4.interdiff.txt3.47 KBdww
#4 3089957-4.patch4.73 KBdww
#2 3089957-2.patch1.64 KBdww
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dww created an issue. See original summary.

dww’s picture

dww
we/he/they
CreditAttribution: dww as a volunteer commented
Issue summary: View changes
Status: Active » Needs review
Issue tags: +Needs tests
FileSize
3089957-2.patch1.64 KB
Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Related issues: +#2951814: Improve X-Drupal-Cache and X-Drupal-Dynamic-Cache headers, even for responses that are not cacheable

This is a subset of #2951814: Improve X-Drupal-Cache and X-Drupal-Dynamic-Cache headers, even for responses that are not cacheable, let's push that issue forward instead :)

dww’s picture

dww
we/he/they
CreditAttribution: dww as a volunteer commented
Issue summary: View changes
Issue tags: -Needs tests
Related issues:
FileSize
3089957-4.patch4.73 KB
3089957.2_4.interdiff.txt3.47 KB

Re: #3: I see this as a stand-alone, orthogonal feature request to aid in debugging. Nothing more. ;) Looking at the latest patch in there, I don't see anything like what I'm proposing here.

But, I added that to the growing list of issues in the snake pit. ;)

Meanwhile, here's test coverage.

RTBC?

The last submitted patch, 2: 3089957-2.patch, failed testing. View results

Status: Needs review » Needs work

The last submitted patch, 4: 3089957-4.patch, failed testing. View results

dww’s picture

dww
we/he/they
CreditAttribution: dww as a volunteer commented
Status: Needs work » Needs review
Related issues:
FileSize
3089957-6.patch5.43 KB
3089957.4_6.interdiff.txt598 bytes

re: https://www.drupal.org/pift-ci-job/1447632 -- wow:

1) Drupal\Tests\ComposerIntegrationTest::testExpectedScaffoldFiles with data set #18 ('sites/default/default.services.yml', 'assets/scaffold/files/default...es.yml')
Scaffold source and destination files must have the same contents.
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-  # get X-Drupal-Cache-Tags and X-Drupal-Cache-Contexts headers.\n
+  # get X-Drupal-Cache-Tags, X-Drupal-Cache-Contexts and X-Drupal-Max-Age\n
+  # headers.\n
...

I guess the attached is the right way to fix this?

dww’s picture

dww
we/he/they
CreditAttribution: dww as a volunteer commented
Issue summary: View changes

Re: #3: I added this to the summary:

It also seems very useful to be able to compare Expires vs. Cache-Control vs. X-Drupal-Cache-Max-Age in various circumstances and configurations as we try to navigate the snake pit.

I think resolving those issues (and writing test for them) will be easier if we have this, so we can compare expectations with reality.

Meanwhile, to warn others of the fun, I made these edits to the main Cache max-age doc:

https://www.drupal.org/node/2541404/revisions/view/11105792/11617646

(added a "Limitations of max-age" header at the bottom of the page).

Cheers,
-Derek

mr.baileys’s picture

mr.baileys
Primary language Dutch
Location 🇧🇪 (Ghent)
CreditAttribution: mr.baileys at SQLI - Belgium commented
Issue tags: +DX (Developer Experience), +Needs change record, +needs documentation updates

Caching in Drupal is governed by the combination of cache tags, cache contexts and max-age. When providing information meant for debugging purposes, it makes sense to output all three pieces of information, instead of just the first two. This change seems to fall outside of the current scope of #2951814: Improve X-Drupal-Cache and X-Drupal-Dynamic-Cache headers, even for responses that are not cacheable, so +1 to keep these issues separate.

The patch and tests look good to me. One thing I would consider is replacing the 'special' values with descriptive text (-1 = "Permanent", 0 = "Uncacheable")

If this gets commited, and since this is a DX improvement, we should issue a CR explaining the new header, and update existing documentation.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Krzysztof Domański’s picture

Krzysztof Domański
Location Poland
CreditAttribution: Krzysztof Domański at abventor commented
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs change record, -

I added change record X-Drupal-Cache-Max-Age header.

dww’s picture

dww
we/he/they
CreditAttribution: dww as a volunteer commented

Wonderful, thanks @Krzysztof Domański!

I did a few edits to the CR, but mostly it looked great.

Cheers,
-Derek

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 7: 3089957-6.patch, failed testing. View results

Krzysztof Domański’s picture

Krzysztof Domański
Location Poland
CreditAttribution: Krzysztof Domański at abventor commented
Status: Needs work » Reviewed & tested by the community

Unrelated random test failure. Back to RTBC.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Status: Reviewed & tested by the community » Needs review

The patch and tests look good to me. One thing I would consider is replacing the 'special' values with descriptive text (-1 = "Permanent", 0 = "Uncacheable")

From #9 hasn't been addressed. I'm ambivalent - on one hand it makes it easier to read on the other it obscures the real value set in code. @dww that do you think?

Krzysztof Domański’s picture

Krzysztof Domański
Location Poland
CreditAttribution: Krzysztof Domański at abventor commented

I am voting for a real value... Since it's debug value, can we combine both values?

$cache_max_age_debug_message = $response_cacheability->getCacheMaxAge();
if ($cache_max_age_debug_message == 0) {
  $cache_max_age_debug_message == '0 (Uncacheable)';
}
elseif ($cache_max_age_debug_message == -1) {
  $cache_max_age_debug_message == '-1 (Permanent)';
}
$response->headers->set('X-Drupal-Cache-Max-Age', $cache_max_age_debug_message);
dww’s picture

dww
we/he/they
CreditAttribution: dww as a volunteer commented
FileSize
3089957-18.patch5.67 KB
3089957.7_18.interdiff.txt3.61 KB

I'm ambivalent as well. Seems like anyone using this feature is going to be doing low-level debugging and can handle interpreting the values themselves. I still prefer #7.

But here's a working version of #17 if y'all prefer. #17 is using == for both comparison and assignment. ;) So I borrowed a = from each assignment and gave it to each comparison. ;) Also shorted the variable name.

Not sure about translating these special values, since this seems like some mighty low-level code, and I'm not sure we can assume we have a working translation system at this point. Seems like some potential bloat and complication for only limited value.

Krzysztof Domański’s picture

Krzysztof Domański
Location Poland
CreditAttribution: Krzysztof Domański at abventor commented
Status: Needs review » Reviewed & tested by the community

Not sure about translating these special values, since this seems like some mighty low-level code, and I'm not sure we can assume we have a working translation system at this point. Seems like some potential bloat and complication for only limited value.

IMO we don't need to translate these special values, because only developers can see these values. Additionally, it adds extra work, that we can avoid.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Status: Reviewed & tested by the community » Fixed

Committed 488870e and pushed to 9.1.x. Thanks!

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented

I committed the patch in #18 - the additional detail is useful.

  • alexpott committed 488870e on 9.1.x 
    Issue #3089957 by dww, Krzysztof Domański, Wim Leers, mr.baileys: Set X-...
dww’s picture

dww
we/he/they
CreditAttribution: dww as a volunteer commented
Issue summary: View changes

Sweet, thanks!

I see the CR is now published with the right version info, thank to whomever took care of that.

Cheers,
-Derek

Status: Fixed » Closed (fixed)

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

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented

Let's continue this work in #2951814: Improve X-Drupal-Cache and X-Drupal-Dynamic-Cache headers, even for responses that are not cacheable!