The code below, clearly works for different languages.
$data = array();
if($cache = cache_get('eu_cookie_compliance_client_settings_' . $language->language, 'cache')) {
$data = $cache->data;
}
else {
... theme-dependent rendering...
}
The problem comes, when there are different themes, that can define different templates for the pop-up. Themes can be switched in many ways:
- Per domain with some domain_access sub-module.
- Per site-builder's condition with themekey module.
- Others...
The end result is that the popup is rendered in one theme, but it's markup can be displayed in another from cache, causing UI breakages.
I can think of 2 options for fixing this (non mutually exclusive):
1. Harder - Expose a hook/API to manipulate the used cache key.
2. Add the currently active theme to the key's generation.
Any other ideas?
Patch, review, rtbc, commit.
None.
Addition / None - depending on the solution that is taken.
None.
| Comment | File | Size | Author |
|---|---|---|---|
| #7 | eu_cookie_compliance-generic-cache-key-2705743-D7-7.patch | 2.6 KB | ndobromirov |
| #2 | eu_cookie_compliance-generic-cache-key-2705743-D7-2.patch | 2.71 KB | ndobromirov |
Comments
Comment #2
ndobromirov commentedUpdated title to reflect the issue more correctly.
Here is a patch that implements option 2 + some re-formatting.
Comment #3
svenryen commentedThank you for the patch. I will have a look and commit if it passes testing.
Comment #4
ndobromirov commentedSomething that I have missed initially, as it was part of the original code, so it's maybe a separate issue...
1. Is this Really needed to be temporary?
2. Can we set it to something more greedy (hour / day / CACHE_PERMANENT / custom TTL config)?
The rendered values in the pop-up fully depend on the config form (in terms of content), so on re-save it will clear it either way. As it is at the moment I do not see reason for the TEMPORARY vs PERMANENT or at least some cache TTL.
BR, Nick.
Comment #5
dakku commentedoption 2 seems ok to me. Thank for the patch
Comment #6
svenryen commented@ndobromirov, thanks you again for the patch. I tried applying it and it doesn't apply to the latest beta release. Do you think you could re-roll it, and I'll take a look an review it?
Comment #7
ndobromirov commentedAs you can see this issue was against 7.x-2.x version of the module. There were no commits there for the last ~2 years. What's the status for it?
Re-rolling the patch against version 1. Changes include:
1. Fix of the cache key name-spacing.
2. More consistent cache clear, as the existing one was clearly wrong / not working.
3. As per the discussion in #4 and #5 changed the cache TTL from temporary to permanent.
BR, Nick.
Comment #8
svenryen commented@ndobromirov/Nick, we don't plan to use the 2.x version unless we introduce something that breaks backwards compatibility.
In the 1.x code base we already had the key
eu_cookie_compliance_client_settings_' . $theme . '_' . $language->language, what are your benefits of changing this toeu_cookie_compliance_client_settings:' . $language->language . ':' . $theme?Also, how do I test that your code actually works as intended. I'm able to verify it has no side-effects, but what should I expect to see when I set up a site that has two themes available?
Comment #10
ndobromirov commentedThe issue was in the cache clear scenario - the key was prefix:thme:lang when setting, but it was removing single key in the prefix:lang convention. This way the cache will never be cleared, unless you wipe the whole thing out.
If both themes are active at the same time (based on a rule or something), rendered stuff from one should not show on the other.
This was fixed, as I only changed the cache clear on a config change part and the cache TTL.
Apparently the issue in 2.x was not present (that much) in 1.x, or this was duplicated.
I am marking this as fixed.
Comment #11
svenryen commentedThanks for marking it as fixed. I won't pull out your code, so it will be in the next release.
Comment #12
ndobromirov commented