Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Spin-off from #1683644: Use Annotations for plugin discovery:
Problem
- CacheDecorator does not care for @Translation at all, which means that the identical plugin definitions are reloaded from cache, regardless of the current language. In short, the cache was primed in German, and I also saw the German plugin title when switching back to English.
- CacheDecorator does not specify cache tags for the entries it creates.
- CacheDecorator::__construct() defaults to a bogus
$cache_bin = 'default'
, which does not exist and has to be changed to$cache_bin = 'cache'
to make it cache anything in the first place.
Steps to reproduce
- Re-implant the CacheDecorator from #1683644-107: Use Annotations for plugin discovery
- Install Language and Locale modules
- Enable an additional language
- Translate "Default fetcher" (whatever that means :P) into the additional language
- Hack aggregator.admin.inc to also output the aggregator fetcher configuration widget in case there is only one
- Visit
/admin/config/services/aggregator/settings
in one language, then in the other. - Actual result: Default fetcher appears in wrong language.
- Expected result: Default fetcher appears in current language.
Notes
- CacheDecorator actually has to create proper cache IDs and cache tags for every single annotation that may appear in the @Plugin declaration.
Screenie:
Comment | File | Size | Author |
---|---|---|---|
#40 | cache-decorator-clear-1722882-40-test-only.patch | 1.49 KB | Berdir |
#40 | cache-decorator-clear-1722882-40.patch | 2.38 KB | Berdir |
#29 | 1722882-28-test-only.patch | 10.51 KB | EclipseGc |
#28 | 1722882-28.patch | 14 KB | EclipseGc |
#24 | 1722882-24.patch | 12.5 KB | EclipseGc |
Comments
Comment #1
effulgentsia CreditAttribution: effulgentsia commentedCacheDecorator::__construct() receives a $cache_key argument, so the plugin manager class that chooses to use this decorator can pass a key that includes a language code. That's modeled on image_effect_definitions(), but perhaps this issue can be used to add better built-in support.
Comment #2
aspilicious CreditAttribution: aspilicious commentedTested the cache decorator with views. For non translated views the following patch works.
It is important to notice that currently "getDefinition($plugin_id)" doesn't set the cache.
That leaded to *serious* performance problems on full cache clear and rebuild.
Views seems to go smoothly now...
(Yes I know this needs work for the translation and cache tags part)
Comment #3
dawehnerThis really improves the working with views.
I guess the intention behind the code was to save some function calls. If this is really a problem we could improve this later.
It would be much easier to get this change in first and then later fix the actual problems outlined in #0
Comment #4
aspilicious CreditAttribution: aspilicious commentedJust wanted to note that I copied that approach from the annotationDiscovery
Comment #5
catchI've committed this for now to unblock views. Leaving open for translation, cache tags and test coverage.
Comment #6
yched CreditAttribution: yched commentedRelated : I just opened #1764232: CacheDecorator provides no way to clear cached definitions.
Dunno if that should be dealt with within the larger set of cache logic changes mentioned here ? Sounds like it could be adressed separately.
Comment #7
moshe weitzman CreditAttribution: moshe weitzman commentedAnother important cache issue that folks might want to help on - #1774332: Better handling of invalid/expired cache entries
Comment #8
andypostThis is not critical anymore.
Comment #9
tim.plunkettWe need cache tags for #1763974: Convert entity type info into plugins.
Comment #10
neclimdulSince "CacheBackendInterface::CACHE_PERMANENT" is being added, should it be considered as a possible argument? Follow up?
Comment #11
fubhy CreditAttribution: fubhy commentedYes, I think so too. Also, the new @param docs were missing for __construct.
Comment #12
EclipseGc CreditAttribution: EclipseGc commentedI'm running this code for myself locally already and am very ++
Comment #13
fubhy CreditAttribution: fubhy commentedFYI: I opened another CacheDecorator issue a week ago with a minor tweak over here: #1853226: Improve performance of CacheDecorator.
Comment #14
catchIsn't this still a global cache entry despite the translations? I'm not sure exactly what the problem is but it's in the issue summary and not in the patch.
Also there's no tests added despite this being a critical bug previously.
Comment #15
EclipseGc CreditAttribution: EclipseGc commentedOK, this doesn't include tests yet, but it does include an implementation that does work for sun's reproduction steps in the OP. Lets see how testbot feels about this.
Eclipse
Comment #16
xjmComment #17
EclipseGc CreditAttribution: EclipseGc commentedComment #18
xjmThis is blocking #1535868: Convert all blocks into plugins, so bumping to critical. Next we need tests for it.
Comment #19
YesCT CreditAttribution: YesCT commentedComment #20
xjmComment #21
EclipseGc CreditAttribution: EclipseGc commentedOK, the test on this is failing for a couple of reasons.
1.) I assume my language negotiation setup is wrong.
2.) Even once that's working, no strings are being added to the locales_source table for translating, which is very puzzling.
Once this is squared away, I think this will be committable.
Eclipse
Comment #23
BerdirThe attached patch passes for me, using the locale_custom_strings shortcut, which is imho a valid simplification (other tests are using it too) that *considerably* simplifies the test code.
You probably want to add another language and make it a loop over the languages and also use the API to create the language(s).
Comment #24
EclipseGc CreditAttribution: EclipseGc commentedOk, this is a modified version of what berdir provided above utilizing the api a bit better, and moving the setup code around a bit. I think I got the proper docs added as well. Let's see if this passes, I think it should.
Eclipse
Comment #25
aspilicious CreditAttribution: aspilicious commentedNeeds newline
Comment #26
BerdirHere's a mostly nitpicky review. Feel free to ignore it to be able to unblock the blocks as plugins issue and open follow-up issues, but most of it should be trivial to fix, although the argument order might need some more discussion and might require to change other managers.
As discussed already in IRC, the test currently only adds test coverage for the existing arguments of the cache decorator, @EclipseGc is already working on adding test coverage for the new arguments.
The use of a dynamic cache key with eg. the language makes the use of cache tags more or less mandatory if you want to clear the cache of all possible combinations without clearing the whole cache bin.
So I'm wondering if we want to change the order of arguments here. My suggestion would be $cache_key, $cache_tags, $cache_bin, $cache_expire, as I expect that this is the order in which they actually need to be customized. I have no idea for what you'd need to use a non-permanent cache expiration here :)
Maybe also document this fact in the @param documentation (get() on the cache bin probably needs something like that too if it does not already exist)
Also nitpick: missing . on the $cache_tags documentation.
We also might want to change $cache_bin to CacheBackendInterface $cacheBin at some point, but that's a separate issue and is a bit ugly because it needs to be passed through the manager :)
If we want/need a way to clear the cache of just this information, we'd need a cache tag as well here, although that is probably not necessary...
Contains instead of Definition of.
Should use \Exception inline instead of the use.
\Drupal\...
I've seen that other tests in this group have similar names (e.g. CacheDecorator) but that is not how we usually name tests, not sure...
@sun has even been arguing in another class that we should drop name completely and just rely on the class name, so I don't really care :)
It's just test code but __construct() might still need a docblock...
This might be trivial enough to use the new router already, then we don't have to convert it later on? :) Also plugin_definition_test vs plugin_test_definitions, might not hurt to use the same name for the route and the function :)
This on the other hand should use use ;)
Comment #27
EclipseGc CreditAttribution: EclipseGc commentedI think my only push back on what you've got here is the ordering of the parameters. I agree with what you've said, but I actually wrote almost identical code to the improvements that were already on this issue (before beginning to work in this issue) and I did the exact same approach to constructor parameters because of the existing order of parameters in the API. While I agree this weights tags more heavily, we should discuss further this notion of moving the parameters around and whether that has implications for the cache api itself. Probably a follow up. I think I'll leave the constructor alone right now unless we just really have consensus that what you've suggested should be done, in which case I have NO complaints with the suggestion at all.
Eclipse
Comment #28
EclipseGc CreditAttribution: EclipseGc commentedOk,
Added tests for clearing the tag and also expiration setting. Tried to fix most of what Berdir laid out above.
Eclipse
Comment #29
EclipseGc CreditAttribution: EclipseGc commentedand a test only patch as well. sorry.
Comment #31
xjmExcellent!
Comment #32
yched CreditAttribution: yched commentedWhy can't we bake the ":$language" suffix within CacheDecorator itself (possibly controlled by a boolean $cache_by_language flag on the controller) ?
Then CacheDecorator::clearCachedDefinitions() can take care of clearing entries for all languages without forcing the use of a tag to do so.
Comment #33
EclipseGc CreditAttribution: EclipseGc commentedI considered doing this. It seemed a lot of extra scaffolding for fairly limited improvement. I also ran the current implementation by chx, and if I can speak for him, he didn't seem to have a problem with the invocation of the decorator that this code suggests, so I didn't try to push it further. I'm not necessarily opposed to what your suggesting (others might be, I dunno) but I think what we currently have is probably good enough, and I'd like to get this in now since it's blocking blocks as plugins. Then if your suggestion is largely agreed upon, a follow up issue seems totally legitimate to me.
Eclipse
Comment #34
tim.plunkettI think that'd be a step too far. But, a follow-up for discussion would probably be a good idea.
I think this is good to go as-is.
Comment #35
yched CreditAttribution: yched commentedRight, fine by me.
Comment #36
catchI think it's fine with leaving the cache tags optional. It's possible that someone would use plugins with no translatable strings. Also if you wanted to avoid the overhead of cache tags you could loop over languages when clearing caches (as long the cids are otherwise predictable).
Committed/pushed to 8.x, could use with change notice (or an updated one) to explain how to use this.
Comment #37
xjmComment #38
BerdirCreated a change notice: http://drupal.org/node/1887798
While doing so, I noticed two things:
a) The last sentence is a lie :) ("This causes the cache decorator to add the tag to all created caches (one for each language he is invoked in) and will use the tags to clear the caches instead of just the current cache identifier."). That does not actually happen yet but must: cache()->deleteTags($this->cacheTags) must be used if it's not empty instead of $this->cacheKey.
b) BlockManager does not use a cache tag. That is a bug, calling clearCachedDefinitions will only clear it for the currently active language. A tag *must* be used if you want clearCachedDefinitions to work. We should document that :)
Comment #39
BerdirThis here should not happen with a direct cache() call but through clearCachedDefinitions(). Changing that should provide test coverage for a) and will fail right now.
Then we can fix the BlockManager and use a cache tag there to fix b).
Sorry for not catching this before.
Comment #40
BerdirTest only patch and fixed attached.
Will open a separate follow-up for b) (the broken block caching, as test coverage won't be quite so easy there)
Comment #41
yched CreditAttribution: yched commentedSo one CacheDecorator instance receives and statically + persistently caches the definitions in one language.
This means that we don't have anyone to ask for "clear definitions for all languages".
Clearing cached definitions :
- can be done from the outside for the persistent cache entries, by $cache_bin->deleteTags($tag) - which is ugly because both $cache_bin and $tag are supposed to be internal to the CacheDecorator and the PluginManager.
- cannot be done for the static caches.
I have a feeling we haven't cracked the code on this yet :-/
Comment #42
Berdir@yched: Was that a response to my patch?
AFAIK, there can only be one plugin manager active at the same time (at least for plugin managers in the container, are there any other examples? and the interface language can not change, so I think my patch is fine?
Comment #43
xjmSo berdir's fix in #40 looks like a pretty unambiguous bugfix to me. Let's get that in and then open a followup to explore (b) and for #41.
Comment #44
EclipseGc CreditAttribution: EclipseGc commentedI agree
Comment #45
webchickOk, for now committed and pushed #40 to 8.x. Thanks!
Given the change notice in #38 it seems like this is now fixed. However, looks like we need follow-up(s) for yched's concerns in #41 and b) in #40.
Comment #46
BerdirFollow-up for #40: #1893818: Plugin discovery caches of blocks are not cleared for all languages
Comment #47
sunComment #49
xjmUntagging. Please remove the "Needs change notification" tag when the change notice task is complete.
Comment #49.0
xjmUpdated issue summary.