Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Problem/Motivation
Discovered while working on #2662090: Privacy Policy link no longer accepts external urls: When EU cookie compliance settings are changed (for example, changing the popup link setting), no cache invalidation is performed. This can cause the old settings to still be used.
Proposed resolution
(smart?) cache invalidation after submitting the settings form.
Remaining tasks
Write patch
Write tests
Review
User interface changes
None
API changes
None
Data model changes
None
Comment | File | Size | Author |
---|---|---|---|
#30 | no_cache_invalidation-2669028-30.patch | 4.47 KB | chishah92 |
#22 | no_cache_invalidation-2669028-22.patch | 4.47 KB | killua99 |
Comments
Comment #2
mr.baileysPatch attached adds a cache tag to EU Cookie Compliance's output, which will cause cached pages containing EUCC's settings to be invalidated when the EUCC configuration is saved.
Comment #3
killua99 CreditAttribution: killua99 commentedI'll accept this patch. But could be good add some test for it later.
Hard to find free time to write those test tho.
Comment #5
killua99 CreditAttribution: killua99 commentedComment #6
mr.baileysThanks!
Regarding the tests: this is actually implicitly tested in the tests from #2662090: Privacy Policy link no longer accepts external urls, since those tests nu explicitly clear the cache. I'll re-roll the patch now that this is no longer required.
Comment #8
nicobot CreditAttribution: nicobot as a volunteer commentedI've been rerolling the patch #2662090-14: Privacy Policy link no longer accepts external urls and when running the tests in that patch I've seen that the cache for the client settings is not being invalidated and is not considering its languages.
I'm attaching a new patch that invalidates the caches and makes the tests to work fine.
Comment #13
mr.baileysThanks @nicobot for the re-roll, and you're right in re-opening this issue: the earlier change (correctly) invalidates cached items that rely on the EU cookie settings configuration, however I missed the fact that EUCC also maintains it's own internal cache.
With regards to your patch though, we want to avoid adding those dependencies, and invalidating the cache explicitly. Your approach would also fail if settings are updating through other means that saving the form.
Instead, you should be able to pass the 'config:eu_cookie_compliance.settings' tag when saving the cache item. Doing so will ensure that this cache item too is cleared when the settings are saved, and should make the test in #2662090 turn green.
Something like this in
eu_cookie_compliance_page_attachments()
should do the trick:Comment #14
killua99 CreditAttribution: killua99 commentedPlus also this feature could have some sort of test?
It will be nice to have if make sense to have any here.
Comment #15
mr.baileys@killua99 => #2662090: Privacy Policy link no longer accepts external urls provides test-coverage for this scenario.
Comment #16
nicobot CreditAttribution: nicobot as a volunteer commentedRight, actually the tests from #2662090: Privacy Policy link no longer accepts external urls discovered this issue, but it has been adapted to avoid this issue.
Maybe the drupal_flush_all_caches() should be removed from the test, to make it fail; so this issue will be covered.
Comment #17
mr.baileysYes, ideally we get this issue resolved and committed first, then remove the explicit cache invalidation from the tests in #2662090 and commit that one too, and then we'll have test coverage for both this issues.
@nicobot: do you want / have time to create a new patch here?
Comment #18
killua99 CreditAttribution: killua99 commentedShould we create another issue to remove the explicit cache invalidation from the test and let this simple to review, and maybe we could advance faster on this subject.
I forgot this issue was covered on the other one.
Comment #19
mr.baileysMight not be necessary to open another ticket: if you're happy with #2662090: Privacy Policy link no longer accepts external urls you can commit that patch as-is, and then we can add the removal of the explicit cache invalidation to the patch here? That way we will have working test-coverage, and we can show that the patch in this issue indeeds takes care of proper cache invalidation when settings are updated.
Comment #20
naveenvalechaAdding parent issue.
Comment #21
killua99 CreditAttribution: killua99 commentedOk the patch had been committed now we need the patch for explicit cache invalidation.
Comment #22
killua99 CreditAttribution: killua99 commentedRerolling patch.
Needs review.
Comment #27
killua99 CreditAttribution: killua99 commentedComment #28
naveenvalechaThis needs a reroll as this went in #2656830: Export default setttings in config/install
Comment #29
chishah92 CreditAttribution: chishah92 at Blisstering Solutions commentedComment #30
chishah92 CreditAttribution: chishah92 at Blisstering Solutions commentedHave rerolled the patch
Comment #34
chishah92 CreditAttribution: chishah92 at Blisstering Solutions commentedComment #35
killua99 CreditAttribution: killua99 commentedNice rerolling, ofc all tests fail because of the lack of test.
But seems legit. I'm going to give it a shoot to naveenvalecha he review very well each patch xD. But seems correct to apply to me when RTBC I'll commit this patch.
Comment #36
chishah92 CreditAttribution: chishah92 at Blisstering Solutions commentedThanks !! waiting for naveenvalecha to review
Comment #37
naveenvalechaunrelated change. Do it in a followup.
I'm not sure why the invalidation we are doing here. we can get review probably someone from cache system maintainers
Comment #38
Wim LeersThis is very very wrong.
#13 already has the full explanation. You should not explicitly invalidate the cache. You just need to make sure that each of this module's cache entries has the cache tag of the configuration. That's enough.
Finally, I think
alludes to something else: why does this module even maintain its own cache?This caching appears to happen in
eu_cookie_compliance_page_attachments()
. The code is pretty awful. It's doing its own rendering and then caching multiple rendered things along with other metadata. And then it sends it to the page inside ofdrupalSettings
.This is abuse of
drupalSettings
. And it comes with a significant cost to front-end performance, because you're now potentially sending entire chunks of HTML very early in the page, which means the browser can do less useful work since it has to receive and parse a bunch of HTML encoded into JSON instead.You probably should create a new issue to fix this, because IMO that should be a blocker to shipping a final (non-beta) release.
Comment #39
killua99 CreditAttribution: killua99 commentedWell thanks for the nice review.
Comment #40
borisson_I just ran into this as well. I checked the queue for a cache related issue and found this.
I came here to say the exact same thing, why is there an internal cache? The easiest thing to do here should be to remove the internal cache and just rely on drupal's cacheing system.
Comment #41
mr.baileysNote that #2798829: Cannot add relative internal link also contains the changes described here (and provides test coverage).
Comment #42
killua99 CreditAttribution: killua99 commentedFixed with the other issue.