Problem/Motivation
Original report:
\Drupal\block\BlockAccessControlHandler::mergeCacheabilityFromConditions()
causes the current user's cache tag to be associated with the access result.
Even though there should not be such a cache tag, only the user.roles
cache context (which is also present).
I tried to find the root cause, but quickly got lost in the complexity of the context system.
Discovered in #2765959: Make 4xx REST responses cacheable by (Dynamic) Page Cache + comprehensive cacheability test coverage.
Proposed resolution
Bug since fixed elsewhere, and the MR just removes the workaround and @todo added in #2765959
Comment | File | Size | Author |
---|---|---|---|
#18 | 2867881-17.patch | 1.15 KB | smustgrave |
| |||
#5 | 2867881-5.patch | 2.45 KB | Wim Leers |
Issue fork drupal-2867881
Show commands
Start 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:
Comments
Comment #3
Wim LeersComment #4
Wim LeersThis also affects the API-First Initiative. #2765959: Make 4xx REST responses cacheable by (Dynamic) Page Cache + comprehensive cacheability test coverage had to add these work-arounds for this bug, which this issue will be able to remove:
Comment #5
Wim LeersIOW: this patch fails today, this issue should make it green! 🙂
Comment #17
smustgrave CreditAttribution: smustgrave commentedThis came up as a daily bugsmash target.
Rerolled #5 the changes in core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php were previously commented.
Lets see what fails.
Comment #18
smustgrave CreditAttribution: smustgrave commentedUpdate
Comment #22
catchConverted the patch to an MR. Given #18 was green, we might have just fixed this elsewhere without removing the @todo.
Comment #23
catchSince all I did was convert the patch to an MR, I think I can RTBC this. Updated the issue title and summary.
Comment #24
dwwTagging that this came up as a random triage target today in #bugsmash, which is where the new life came from.
Comment #25
dwwAlso reviewed the MR code and test results. RTBC++.
Hah, this was random #bugsmash ~1.5 years ago for patch #17, but wasn't tagged then, either. 😅
Saving credits for everyone.
Comment #26
dwwMinor summary edit, but I didn't bother with the full default template since none of the other headings are relevant here.
Comment #27
alexpottBackported to 10.2.x as a test only change.
Committed and pushed eed7ea3a50 to 11.x and 0d4a5883d4 to 10.3.x and 4d9ec8d74a to 10.2.x. Thanks!