Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
CacheDecorator currently only writes stuff to (persistent & static) cache, but offers no way to clear cached data.
To circumvent that currently in #1742734: [META] Widgets as Plugins, my WidgetPluginManager class does :
public function __construct() {
$this->discovery = new CacheDecorator(new HookDiscovery($this->hook), $this->cache_key, $this->cache_bin);
$this->factory = new ReflectionFactory($this);
}
/**
* Clear cached definitions.
* @todo A cleaner way should be baked within CacheDecorator.
*/
public function clearDefinitions() {
// Clear 'static' data by creating a new object.
$this->discovery = new CacheDecorator(new HookDiscovery($this->hook), $this->cache_key, $this->cache_bin);
cache($this->cache_bin)->delete($this->cache_key);
}
Obviously not ideal (having to re-instantiate a whole discovery object(s), and manually clearing a cache entry that should be encapsulated within the decorator).
Comment | File | Size | Author |
---|---|---|---|
#32 | drupal-1764232-32.patch | 11.84 KB | tim.plunkett |
#32 | interdiff.txt | 5.01 KB | tim.plunkett |
#27 | plugins-cache-decorator-1764232-27.patch | 11.67 KB | yched |
#24 | plugins-cache-decorator-1764232-23.patch | 11.67 KB | yched |
#24 | interdiff.txt | 500 bytes | yched |
Comments
Comment #1
andyposttaggin
Comment #2
tim.plunkettThe current usage of
$this->baseDiscovery
is clever, and that should be moved into PluginManagerBase for all managers.Comment #3
yched CreditAttribution: yched commentedexpanding on #2: yeah, so what WidgetPluginManager currently does is :
$this->baseDiscovery contains the discovery decorators stack without CacheDecorator, so that we don't reinstanciate the full stack in clearDefinitions().
But that still leaves this clearDefinitions() method that currently needs to be re-implemented by every PluginManager that happens to use CacheDecorator.
Also, that method includes a direct call to cache($bin)->delete($cid), which is not proper separation of concerns, dealing with the cache bins should be internalized in CacheDecorator.
In short, CacheDecorator is currently not self-included and needs to be helped from the outside.
Comment #4
yched CreditAttribution: yched commentedPossible fix:
- Add a PluginManagerCachedBase base class (extends PluginManagerBase), to be used by plugin types that use CacheDecorator.
That base class includes the clearDefinitions() method, as well as the $cache_bin, $cache_id, $baseDiscovery members.
- Add a clearDefinitions() method in CacheDecorator, that takes care of hitting the cache bin.
Then in PluginManagerCachedBase :
Comment #5
tim.plunkettI was working on this, but clearDefinitions() is never directly called in core, so I'm at a loss for how that's actually working right now.
Comment #6
tim.plunkettAssigning to @yched for some insight.
Comment #7
yched CreditAttribution: yched commentedAw. Er, indeed... WidgetPluginManager/FormatterPluginManager::clearDefinitions() should be called within field_info_cache_clear(), but I apparently failed to do so :(. [edit: Yeah, despite making a fuss about having to build a dedicated method myself :-p]
Which is indeed surprising as to why/how the thing currently works and tests currently pass... The cache of available widgets/formatters is never cleared, yet when enabling a module new widgets/formatters are found.
I'm not where I can easily run code to check what happens, unfortunately...
Comment #8
yched CreditAttribution: yched commentedHm, would that be because annotation discovery finds plugins exposed in disabled modules ?
Comment #9
neclimdulIt does find plugins for disabled modules because simpletest loads all namespaces to find tests for disabled modules. something we need to fix.
Comment #10
tim.plunkettSee #1780396: Namespaces of disabled modules are registered during test runs, it's only during simpletest.
Comment #11
yched CreditAttribution: yched commentedAttached patch implements #4.
- Adds new ClearCachedDefinition() method in CacheDecorator, clears both persistent and "in-memory" caches.
- Adds a new PluginManagerCachedBase class, extending PluginManagerBase with a clearCachedDefinitions() method that proxies to the factory. Since the factory is private, external code that has to clear caches needs a method on the Manager.
I left this class in Drupal/Components, next to PluginManagerBase, which is better. CacheDecorator is in Drupal\Core, but PluginManagerCachedBase has no strict dependency on it, the clearCachedDefinitions() method could apply to other discoveries, that only do static caching for example)
Possible alternative to avoid adding this new base class : add this directly in PluginManagerBase
- Uses PluginManagerCachedBase instead of PluginManagerBase on all existing Manager classes that use CacheDecorator, and removes the workaround code in WidgetPluginManager & FormatterPluginManager.
- Adds tests for CacheDecorator (including basic behavior, which is currently untested)
Note : patch doesn't yet add the missing clearCachedDefinitions() calls for widget and formatter plugins. Field data cleanup is actually a bit messy, I'll open a separate issue for this specific cleanup.
Comment #12
yched CreditAttribution: yched commentedComment #13
yched CreditAttribution: yched commentedOr possibly better : rather than adding PluginManagerCachedBase,
- add CachedDiscoveryInterface extends DiscoveryInterface with ClearCachedDefinition()
- in PluginManager, add :
Thoughts ?
Comment #14
tstoeckler#13 makes total sense to me. :-) ++
Comment #15
Crell CreditAttribution: Crell commentedWhere would the method in #13 go? That determines if it's awesome or an awful hack. :-)
Comment #16
yched CreditAttribution: yched commented@Crell : in PluginManagerBase - see attached patch, implements the proposal in #13.
(interdiff attached FWIW, but this probably easier to review as a fresh patch...)
Comment #18
yched CreditAttribution: yched commentedFixes phpdoc glitches.
Comment #20
yched CreditAttribution: yched commentedAnd a missing 'use' statement.
Comment #21
tstoecklerCan you clarify in which situation
$this->cacheKey
is not set? I haven't dived too deep into this, but looking at the constructor it seems that shouldn't happen.Comment #22
tstoecklerRestoring tags although "Needs tests" is no longer accurate.
Comment #24
yched CreditAttribution: yched commented@tstoeckler: well, setCachedDefinitions() and getCachedDefinitions() both have this isset() check. Agreed that its doesn't seem to make too much sense, except that the current code acts like a non-documented feature of "if the constructor receives a NULL $cache_key, the object behaves as a static cache with no persistent cache".
Therefore, I'd rather leave that as is, and thus use the same isset() check in clearCachedDefinition().
This being said, your remark makes me notice I was totally on crack when writing #20 - we don't want to flush the whole bin, just delete a specific entry. Updated patch attached.
Comment #25
lilou CreditAttribution: lilou commentedTypo in /core/modules/system/lib/Drupal/system/Tests/Plugin/CacheDecoratorTest.php :
+ // Poluplate sample definitions.
... // Populate
Comment #26
tstoecklerAhh, that makes sense @yched, thanks!
Comment #27
yched CreditAttribution: yched commentedFixed typo pointed in #25.
Comment #28
sdboyer CreditAttribution: sdboyer commentedthis is one of those lovely issues where i read through comments, have a thought, then see it addressed by the next comment :)
so yeah, this looks pretty good to me.
this seems a bit odd, i liked the baseDiscovery pattern when i first saw it. why did we get rid of it?
is it because baseDiscovery is being set somehow in the base class? i don't see that anywhere, though...
Comment #29
yched CreditAttribution: yched commentedThe baseDiscovery trick was only so that I didn't recreate the whole discovery stack when hackingly "clearing caches" from the outside - in WidgetPluginManager::clearDefinitions() by manually calling cache()->delete() and deleting the CacheDecorator object to get rif of static cache.
But now that the discovery has a clearDefinitions() method that takes care of everything, separating out the CacheDecorator and what's below is not needed anymore.
Comment #30
sdboyer CreditAttribution: sdboyer commentedok, then...
why are these on two lines, instead of a single stack of object constructions?
if there's no reason, then either let's put them on a single line or save them in a local var that just evaporates later. saving them in an object property, then immediately overwriting it, looks like we were doing something then changed our minds halfway through.
i think i'm good to RTBC this, after that.
Comment #31
sdboyer CreditAttribution: sdboyer commentedand i see there's been some deliberate discussion in another direction about that syntax over in #1764278: Run PluginManagerBase::processDefinition() in a ProcessDecorator, and while i disagree, it's a stupid thing to hold this up over.
this seems good enough to move forward with, at least within our current "decorate everything" paradigm. per some of the discussions in that other issue, i think i'd like to see us take a different approach to that - maybe enhancing the
PluginManagerBase
, or somesuch - but until such time as some larger shift has been undertaken there, this need not block on it.Comment #32
tim.plunkettWhile reviewing #1764278: Run PluginManagerBase::processDefinition() in a ProcessDecorator, I noticed that clearDefinitions() was never actually called, leading me back here.
Since these two issues will conflict, one has to go in first, and it might as well be this one.
I agree with @sdboyer that the combination of inlining and splitting $this->discovery is weird, and since the other issue splits them up, it will be easier to reroll.
The main reason I rerolled this was to fix a couple docblock oddities, like mising () and double ..
Leaving RTBC.
Comment #33
tim.plunkettAlso, since this is replacing completely non-functional code (clearDefinitions() was never actually called anywhere), and since it effectively clashes with another major, I'm bumping this.
Comment #34
Dries CreditAttribution: Dries commentedCommitted to 8.x. Thanks.
Comment #35.0
(not verified) CreditAttribution: commentedreword