Problem/Motivation

Page caching and Dynamic caching modules are caching notification callbacks even when setting the max age to 0.

Proposed resolution

Use CacheableJsonResponse instead of JsonResponse for notification callback responses.

Remaining tasks

Patch.

User interface changes

None

API changes

None

Data model changes

None

Release notes snippet

None

CommentFileSizeAuthor
#2 3127942-cacheablejsonresponse-2.patch1.83 KBpenyaskito

Issue fork lingotek-3127942

Command icon 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:

About issue forks

Comments

penyaskito created an issue. See original summary.

penyaskito’s picture

penyaskito
he/him
Primary language Spanish
Location 🧑🏽‍🌾 Seville 💃 Andalusia, UTC+2 🇪🇺
commented
Status: Active » Needs review
StatusFileSize
new 3127942-cacheablejsonresponse-2.patch1.83 KB

Attached patch.

  • penyaskito committed 534c29c on 8.x-2.x 
    Issue #3127942 by penyaskito: Use CacheableJsonResponse instead of...
penyaskito’s picture

penyaskito
he/him
Primary language Spanish
Location 🧑🏽‍🌾 Seville 💃 Andalusia, UTC+2 🇪🇺
commented
Status: Needs review » Fixed

Committed 534c29c and pushed to 8.x-2.x. Thanks!

  • penyaskito committed d00e943 on 8.x-2.x 
    Fixes for Issue #3127942 by penyaskito: Use CacheableJsonResponse...
penyaskito’s picture

penyaskito
he/him
Primary language Spanish
Location 🧑🏽‍🌾 Seville 💃 Andalusia, UTC+2 🇪🇺
commented

Committed d00e943 and pushed to 8.x-2.x. Thanks!

Only time that I don't wait for tests to come back, tests are broken. This fixed those and an introduced bug when no valid callback arrived.

penyaskito credited juampynr.

penyaskito credited m4olivei.

penyaskito’s picture

penyaskito
he/him
Primary language Spanish
Location 🧑🏽‍🌾 Seville 💃 Andalusia, UTC+2 🇪🇺
commented

The last failures were because of a 'LogicException: The controller result claims to be providing relevant cache metadata, but leaked metadata was detected'. We were rendering the url when recalculating hash because the metadata is included. https://www.lullabot.com/articles/early-rendering-a-lesson-in-debugging-... helped a lot on finding this, so crediting @m4olivei and @juampynr for writing that.

  • penyaskito committed 984092b on 8.x-2.x 
    Fixes tests for Issue #3127942 by penyaskito, m4olivei, juampynr: No...
juampynr’s picture

juampynr commented

Glad to hear that it helped you, @penyaskito! :-D

m4olivei’s picture

m4olivei
he/him
Primary language English
Location Grimsby, ON
commented

Wow, good stuff! Thanks for sharing @penyaskito.

Status: Fixed » Closed (fixed)

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

PabloNicolas made their first commit to this issue’s fork.