Core issue where this was fixed in the HAL module: #2864816: HAL LinkManager doesn't add 'url.site' cache context when needed.

Discovered in #2883086-45: [PP-1] Port RequestHandler + ResourceResponseSubscriber improvements from REST module to JSON API.

CommentFileSizeAuthor
#4 2966384-4.patch1.89 KBwim leers
#4 interdiff.txt889 byteswim leers
#3 2966384-3.patch1.01 KBwim leers

Comments

Wim Leers created an issue. See original summary.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

(#2952714: Group module's GroupAccessResult::allowedIfHasGroupPermission(s)() does not include cacheability was originally created for this purpose, but that got derailed. So, this is a resurrection of the original issue and scope.)

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Active » Needs review
StatusFileSize
new 2966384-3.patch1.01 KB

Fix the @todo. This should fail.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
StatusFileSize
new interdiff.txt889 bytes
new 2966384-4.patch1.89 KB

Implementing the same solution as #2864816: HAL LinkManager doesn't add 'url.site' cache context when needed seems overkill. Because every JSON API document/response contains absolute URLs. So, let's just make everything that that uses \Drupal\jsonapi\Normalizer\JsonApiDocumentTopLevelNormalizer vary by url.site.

The last submitted patch, 3: 2966384-3.patch, failed testing. View results

gabesullice’s picture

gabesullice
Primary language English
commented
Status: Needs review » Needs work

I believe there are some cases in the resource tests that need to be (un)commented by this issue. Otherwise, LGTM.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs work » Needs review

If I search for 2966384, I find only this one?

gabesullice’s picture

gabesullice
Primary language English
commented
Status: Needs review » Reviewed & tested by the community

Derp, looks like I reviewed the interdiff, not the patch.

  • gabesullice committed a8fc0bc on 8.x-1.x authored by Wim Leers 
    Issue #2966384 by Wim Leers: JSON API's LinkManager does not handle...

  • gabesullice committed df34add on 8.x-2.x authored by Wim Leers 
    Issue #2966384 by Wim Leers: JSON API's LinkManager does not handle...
gabesullice’s picture

gabesullice
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Status: Fixed » Closed (fixed)

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