[1.x][2.0.x] Better caching for cookie performance popup

Great idea. I'd love to see some better caching.

Just to understand the implications of the slowdown. Did you check whether the caching situation improves when the Internal Dynamic Page Cache or Internal Page Cache are enabled?

In D7, we're storing a cache item with all the settings, including the rendered templates. Do you think that's a good approach, or does D8 have something better to offer?

Take a look at the D7 codebase. Around line 135 we have some caching that seems to do the job. Maybe something similar can be achieved in D8? I didn't do the initial port, so I don't know why the caching was removed.

Thanks for the patch. Sorry that we don't have tests :)

The cid needs to also factor in the theme and the the active domain if the domain module is enabled.

The patch also needs a re-roll against latest -dev.

I have re-rolled this patch to work with the latest dev. I have also added the active domain and theme in the cache tag string. I did not test the domain portion of this so we would definitely want to test that or remove that before patching dev.

Thanks. I'll have a look.

The proposed patch didn't apply cleanly. Here's an updated patch.

When the domain module is available, we also need to check if there are domains configured, or else the call to
\Drupal::service('domain.negotiator')->getActiveId() fails. This can be resolved by checking count(Domain::loadMultiple()).

Here's an updated patch that resolves that issue and also re-rolls the rest of the changes against the latest -dev.

Would be nice if somebody can review the patch.

@svenryen patch is not getting applied. Needs a re-roll against the latest dev.

  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 29843  100 29843    0     0  36349      0 --:--:-- --:--:-- --:--:-- 36349
error: patch failed: eu_cookie_compliance.module:134
error: eu_cookie_compliance.module: patch does not apply

Re-rolled patch please review!

The patch for 8.x-1.9 release is the copy of #13 patch adapted to renamed configuration variables (#2969161: Form technical names mixed up for labels?).

@maximpodorov, the patch in #20 won't apply. Would you be able to make a patch against the latest -dev?

Re-rolled patch, please review

We can't use the patch from #22. It misses the fix from #14, where \Drupal::service('domain.negotiator')->getActiveId() was added to fix a fatal crash. I see the code was dropped in #20

Can you please make sure you don't leave out any code from the previous patches when you re-roll?

Also, there are code styles to fix. See the diff in #23.

The patch also doesn't pick up on the many changes that's been made to the code base since #13, I'm fixing that now.

Here's a patch that resolves all the issues with #22

Here's an interdiff for information.

#27 looks good to me. Moving to RTBC.

We can't use render cache, but we might be able to just put everything into a single regular cache entry and fetch from that instead of having to build it over and over again on every page?

Why can't we use render cache? The three existing parts of the $cid (language ID, domain ID, theme name) are already covered by the render cache. In fact, having to add info from another module, the domain module, is a code smell that perhaps this isn't the best approach. The domain module's documentation suggests users add the url.site cache context so that the cache can vary based on the domain. So the EU Cookie Compliance module shouldn't need to worry about the domain. However, because it micromanages this part of the renderable content, it needs to account for all the ways it may vary.

In working on the #3480626: DNT (Do Not Track) header detection not working when caching enabled bug, I traced the issue to the custom cache entry's $cid not varying based on the DNT header. If a custom cache entry was not deployed, that bug would easily be fixed with one line of code adding the DNT header to the cache context. Instead, we need yet another variance of the $cid.

So to summarize, why can't we let the render cache do its job? Micromanaging a separate cache entry causes additional complexity, makes it easier to cause bugs (DNT header issue), and causes this module to care about unrelated modules (domain module).

Reopening this issue after comment from @pianomansam.

hi @pianomansam

How much of this change are we talking about undo'ing?

@atowl the merge request on #3480626: DNT (Do Not Track) header detection not working when caching enabled does what I propose (removing the custom cache entry). The only line of code that adds the DNT header is the cache context addition.

+  $variables['#cache']['contexts'][] = 'headers:DNT';

Hi @pianomansam,

So given that line is in the request of #3480626, we could close this issue and merge that instead?

@atwol yes, you absolutely could. However, I wanted to point out that #3480626 undoes much of the work done in this issue.

OK thanks for the information @pianomansam,

I'll (re) close this as fixed, and i'll look at merging #3480626, and any further disucussion in that thread.