This was originally opened in the context of #3278759: Access cacheability is not correct when "view own unpublished content" is in use (see in particular https://git.drupalcode.org/project/drupal/-/merge_requests/8198#note_321385) and came up again in #3580545: Make empty route lookup cacheable.
Currently 40x responses have a very inconsistent behavior in terms of the X-Drupal-Dynamic-Cache Dynamic Page Cache response header:
Request a 404 page in HTML and JSON format for anonymous and authenticated users and be throughly confused about the different values of the X-Drupal-Dynamic-Cache response header.
We already detect the 40x (and also 50x) responses in DynamicPageCacheSubscriber. Move this code to run first to prevent any leakage of the sub-request-response to the main response and - in the case there was a sub-request - handle this explicitly. Concretely:
UNCACHEABLE (403) response headers for 403s (and similar for responses with a different 40x/50x status code) as the value of the X-Drupal-Dynamic-Cache headerUNCACHEABLE (403, sub-request: MISS)As part of that various tests that explicitly test the value of the X-Drupal-Dynamic-Cache header are updated.
One incorrect test assertion from StandardTestTrait is removed. It was introduced all the way back in #2429617-316: Make D8 2x as fast: Dynamic Page Cache: context-dependent page caching (for *all* users!) without much discussion and it is (and presumably always was) actually accidentally testing the leakage of the 403 sub-request-response header to the main response. It is not actually testing the profile page, as the comment claimed, but the access denied page, because the logged in user is not user 1 but /user/1 is being accessed. Actually "fixing" that by fixing the user ID then yields the "UNCACHEABLE (poor cacheability)" response header, which makes sense because of the user cache context. So removing that whole assertion is the best course of action.
The X-Drupal-Dynamic-Cache response header value is changed for 4xx and 5xx responses. (See above for details.)
| Comment | File | Size | Author |
|---|
Show commandsStart within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes, plain diff MR !8240
Comments
Comment #4
mxr576Comment #5
mxr576@fabianx @wim.leers or others, what does "UNCACHEABLE" means exactly? Because the current fix in the blocked issue is changing the order of
if (!isset($this->requestPolicyResults[$request])) {}andif (!$this->shouldCacheResponse($response)) {n. The other potential alternative is also addingX-Drupal-Dynamic-Cache: UNCACHABLEwhenif (!isset($this->requestPolicyResults[$request])) {}is true... but is not that rather "UNHANDLED"?https://git.drupalcode.org/project/drupal/-/merge_requests/8198/diffs?co...
Comment #7
mxr576I see that I was not the only one who wanted to clarify how many "UNCACHEABLE" scenarios/reasons exists and explicit headers to early returns \
Drupal\dynamic_page_cache\EventSubscriber\DynamicPageCacheSubscriber::onResponse()#2951814: Improve X-Drupal-Cache and X-Drupal-Dynamic-Cache headers, even for responses that are not cacheable
Comment #8
mxr576Comment #9
mxr576Comment #10
mxr576Now that user view access is also going to vary per user, we also hit this bug there.
See #3452426: Insufficient cacheability information bubbled up by UserAccessControlHandler.
Comment #11
mxr576Comment #12
mxr576Comment #13
quietone commentedComment #16
tstoecklerComing over from #3580545: Make empty route lookup cacheable where I also stumbled upon this and fixed this, but wanted to split that part out in order to keep that issue a bit more on scope there. Adding my proposed fix to the issue summary, but that solution is just my proposal so far, so both for people here already and for those coming from #3580545: Make empty route lookup cacheable as well, feel free to suggest improvements/alternatives for the concrete response header string.
Comment #17
tstoecklerComment #18
tstoecklerComment #19
tstoecklerRegarding the removed assertion in
StandardTestTraitin case anyone is wondering: This was introduced all the way back in #2429617-316: Make D8 2x as fast: Dynamic Page Cache: context-dependent page caching (for *all* users!) without much discussion and it is (and presumably always was) actually accidentally testing the leakage of the 403 sub-request-response header to the main response. It is not actually testing the profile page, as the comment claimed, but the access denied page, because the logged in user is not user 1 but /user/1 is being accessed. Actually "fixing" that by fixing the user ID then yields the "UNCACHEABLE (poor cacheability)" response header, which makes sense because of the user cache context. So as far as I can tell, just removing that whole assertion is the best course of action here.Comment #20
needs-review-queue-bot commentedThe Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. 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.
Comment #21
tstoecklerJust a test bot hickup.
Comment #22
smustgrave commentedShould this get a CR since it's returning a different status code then before.
Comment #23
tstoecklerComment #24
tstoecklerSo the status code doesn't actually change, just the value of the
X-Drupal-Dynamic-Cacheresponse header. Made the issue summary more explicit about that. Also added a draft change notice for the change just in case, feel free to disregard if it's not actually needed. (Also feel free to re-word/improve it in case it is warranted, I found it a bit hard to explain the actual change in an understandable way.)Comment #25
berdirThis is just optional debug information and kind of self-explanatory. Not sure if that really needs a CR. That said, there are quite a lot of tests asserting this, maybe a few of them will need an update, so why not, setting to needs work for that, but then I think this is RTBC.
One last thing, could you add the comment on the user page test change either as a MR comment or the issue summary so it's easy to find for the core committer reviewing this?
Comment #26
tstoecklerFair enough, copied that comment to the issue summary.
Not sure what specifically you meant to set this to "Needs work" for. I did already write a draft change notice in case that's what you meant.
Comment #27
berdir#25 was a crosspost with #24, that's why.
CR looks great to me.
I doubt this really qualifies as a major bug, the debug header is confusing, but it's just a debug header. But it's useful to have it fixed either way.
Comment #30
catchThis looks good to me, didn't comment here but did comment on the changes in the other issue already. Committed/pushed to main and cherry-picked to 11.x, thanks!
Comment #33
wim leersFantastic to see this improved! 🤩
The one thing that confused me in the change record was that it seemed to imply that the response header previously already included the status code as the reason for uncacheability. But the diff shows what really happened:
UNCACHEABLE (poor cacheability)👇
UNCACHEABLE (404)IOW: the reason for the response being uncacheable was wrong, and it was kind of the catch all "poor cacheability", which is misinforming developers.
Makes sense!
(Did I understand that right? 🤞)
Comment #34
tstoecklerYes, that's 100% correct. Please feel free to improve the wording of the change record if you have any ideas. I definitely did not mean to imply what you seem to have gotten from it, but, yeah, kind of had a hard time putting such a technicality into words in a way that is understandable - and didn't quite succeed it seems, sorry about that.
Comment #35
wim leersPlease don't apologize! This is very hard to explain. If anything: thank you for even creating a CR! 😊 Otherwise I'd never have seen this 🤓
I updated the CR based on you confirming #33 is accurate, WDYT?
Comment #36
tstoecklerGreat, yes, that's more explicit, thanks!