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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mr.baileys created an issue. See original summary.

mr.baileys’s picture

Status: Active » Needs review
FileSize
1.51 KB

Patch 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.

killua99’s picture

I'll accept this patch. But could be good add some test for it later.

Hard to find free time to write those test tho.

  • killua99 committed deea05a on 8.x-1.x authored by mr.baileys
    Issue #2669028 by mr.baileys: No cache invalidation when settings change
    
killua99’s picture

Status: Needs review » Fixed
Issue tags: +ecc_tag-beta5
mr.baileys’s picture

Thanks!

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.

Status: Fixed » Closed (fixed)

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

nicobot’s picture

I'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.

Status: Needs review » Needs work

The last submitted patch, 8: no_cache_invalidation-2669028-8.patch, failed testing.

The last submitted patch, 8: no_cache_invalidation-2669028-8.patch, failed testing.

The last submitted patch, 8: no_cache_invalidation-2669028-8.patch, failed testing.

The last submitted patch, 8: no_cache_invalidation-2669028-8.patch, failed testing.

mr.baileys’s picture

Thanks @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:

 \Drupal::cache()->set('eu_cookie_compliance_client_settings.' . $language->getId(), $data, Cache::PERMANENT, ['config:eu_cookie_compliance.settings']);
killua99’s picture

Plus also this feature could have some sort of test?

It will be nice to have if make sense to have any here.

mr.baileys’s picture

@killua99 => #2662090: Privacy Policy link no longer accepts external urls provides test-coverage for this scenario.

nicobot’s picture

Right, 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.

mr.baileys’s picture

Yes, 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?

killua99’s picture

Should 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.

mr.baileys’s picture

Might 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.

naveenvalecha’s picture

Adding parent issue.

killua99’s picture

Ok the patch had been committed now we need the patch for explicit cache invalidation.

Status: Needs review » Needs work

The last submitted patch, 22: no_cache_invalidation-2669028-22.patch, failed testing.

The last submitted patch, 22: no_cache_invalidation-2669028-22.patch, failed testing.

The last submitted patch, 22: no_cache_invalidation-2669028-22.patch, failed testing.

The last submitted patch, 22: no_cache_invalidation-2669028-22.patch, failed testing.

killua99’s picture

Status: Needs work » Needs review
naveenvalecha’s picture

Status: Needs review » Needs work
Issue tags: +Novice, +Needs reroll

This needs a reroll as this went in #2656830: Export default setttings in config/install

chishah92’s picture

Assigned: Unassigned » chishah92
chishah92’s picture

Status: Needs review » Needs work

The last submitted patch, 30: no_cache_invalidation-2669028-30.patch, failed testing.

The last submitted patch, 30: no_cache_invalidation-2669028-30.patch, failed testing.

The last submitted patch, 30: no_cache_invalidation-2669028-30.patch, failed testing.

chishah92’s picture

Assigned: chishah92 » Unassigned
killua99’s picture

Nice 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.

chishah92’s picture

Thanks !! waiting for naveenvalecha to review

naveenvalecha’s picture

+++ b/eu_cookie_compliance.module
@@ -6,16 +6,7 @@
-use Drupal\Core\Path\AliasManager;
-use Drupal\Core\Path\AliasStorage;
-use Drupal\Core\Path\AliasWhitelist;
-use Drupal\Core\Extension\ModuleHandler;
-use Drupal\Core\Database\Database;
 use Drupal\Core\Cache\Cache;
-use Drupal\Core\Cache\DatabaseBackend;
-use Drupal\Core\Language\LanguageManager;
-use Drupal\Core\Url;
-use Drupal\Component\Utility\SafeMarkup;

unrelated 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

Wim Leers’s picture

+++ b/src/Form/EuCookieComplianceAdminForm.php
@@ -91,6 +117,20 @@ class EuCookieComplianceAdminForm extends ConfigFormBase {
   /**
+   * Invalidates the cache of the client settings
+   */
+  protected function invalidateCaches() {
+    $languages = $this->languageManager->getLanguages();
+
+    $cids = [];
+    foreach($languages as $lang) {
+      $cids []= 'eu_cookie_compliance_client_settings.' . $lang->getId();
+    }
+
+    $this->cacheService->invalidateMultiple($cids);
+  }

This 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 however I missed the fact that EUCC also maintains it's own internal cache 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 of drupalSettings.

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.

killua99’s picture

Well thanks for the nice review.

borisson_’s picture

I just ran into this as well. I checked the queue for a cache related issue and found this.

Finally, I think however I missed the fact that EUCC also maintains it's own internal cache alludes to something else: why does this module even maintain its own cache?

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.

mr.baileys’s picture

Note that #2798829: Cannot add relative internal link also contains the changes described here (and provides test coverage).

killua99’s picture

Status: Needs work » Fixed

Fixed with the other issue.

Status: Fixed » Closed (fixed)

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