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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

andypost’s picture

Issue tags: +Needs tests

taggin

tim.plunkett’s picture

Issue tags: +VDC

The current usage of $this->baseDiscovery is clever, and that should be moved into PluginManagerBase for all managers.

yched’s picture

expanding on #2: yeah, so what WidgetPluginManager currently does is :

protected $cache_bin = 'field';
protected $cache_id = 'field_widget_types';

public function __construct() {
    $this->baseDiscovery = new WidgetLegacyDiscoveryDecorator(new AnnotatedClassDiscovery('field', 'widget'));
    $this->discovery = new CacheDecorator($this->baseDiscovery, $this->cache_id, $this->cache_bin);

    $this->factory = new WidgetFactory($this);
  }

  /**
   * Clears cached definitions.
   *
   * @todo Remove when drupal.org/node/1764232 is fixed.
   */
  public function clearDefinitions() {
    // Clear 'static' data by creating a new object.
    $this->discovery = new CacheDecorator($this->baseDiscovery, $this->cache_id, $this->cache_bin);
    cache($this->cache_bin)->delete($this->cache_id);
  }

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

yched’s picture

Possible 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 :

public function __construct() {
  $this->discovery = new CacheDecorator($this->baseDiscovery, $this->cache_id, $this->cache_bin);
}
public function clearDefinitions() {
  $this->discovery->clearDefinitions();
  $this->discovery = new CacheDecorator($this->baseDiscovery, $this->cache_id, $this->cache_bin);
}
tim.plunkett’s picture

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

tim.plunkett’s picture

Assigned: Unassigned » yched

Assigning to @yched for some insight.

yched’s picture

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

yched’s picture

Hm, would that be because annotation discovery finds plugins exposed in disabled modules ?

neclimdul’s picture

It does find plugins for disabled modules because simpletest loads all namespaces to find tests for disabled modules. something we need to fix.

tim.plunkett’s picture

yched’s picture

Status: Needs review » Active
FileSize
14.36 KB

Attached 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

public function clearCachedDefinitions() {
  if (method_exists($this->discovery, 'clearCachedDefinitions')) {
    this->discovery->clearCachedDefinitions()
  }
}

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

yched’s picture

Status: Active » Needs review
yched’s picture

Status: Active » Needs review

Or possibly better : rather than adding PluginManagerCachedBase,
- add CachedDiscoveryInterface extends DiscoveryInterface with ClearCachedDefinition()
- in PluginManager, add :

public function clearCachedDefinitions() {
  if ($this->discovery instanceof CachedDiscoveryInterface)) {
    this->discovery->clearCachedDefinitions();
  }
}

Thoughts ?

tstoeckler’s picture

#13 makes total sense to me. :-) ++

Crell’s picture

Where would the method in #13 go? That determines if it's awesome or an awful hack. :-)

yched’s picture

@Crell : in PluginManagerBase - see attached patch, implements the proposal in #13.
(interdiff attached FWIW, but this probably easier to review as a fresh patch...)

Status: Needs review » Needs work

The last submitted patch, plugins-cache-decorator-1764232-16.patch, failed testing.

yched’s picture

Status: Needs work » Needs review
FileSize
1.08 KB
11.65 KB

Fixes phpdoc glitches.

Status: Needs review » Needs work

The last submitted patch, plugins-cache-decorator-1764232-17.patch, failed testing.

yched’s picture

Status: Needs work » Needs review
FileSize
518 bytes
11.65 KB

And a missing 'use' statement.

tstoeckler’s picture

Issue tags: -Needs tests, -VDC
+++ b/core/lib/Drupal/Core/Plugin/Discovery/CacheDecorator.php
@@ -110,6 +111,16 @@ protected function setCachedDefinitions($definitions) {
+    if (isset($this->cacheKey)) {
+      cache($this->cacheBin)->flush();
+    }

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

tstoeckler’s picture

Issue tags: +VDC

Restoring tags although "Needs tests" is no longer accurate.

yched’s picture

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

lilou’s picture

Typo in /core/modules/system/lib/Drupal/system/Tests/Plugin/CacheDecoratorTest.php :

+ // Poluplate sample definitions.

... // Populate

tstoeckler’s picture

Ahh, that makes sense @yched, thanks!

yched’s picture

Fixed typo pointed in #25.

sdboyer’s picture

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

+++ b/core/modules/field/lib/Drupal/field/Plugin/Type/Widget/WidgetPluginManager.php
@@ -28,41 +28,16 @@ class WidgetPluginManager extends PluginManagerBase {
+    $this->discovery = new AlterDecorator(new WidgetLegacyDiscoveryDecorator(new AnnotatedClassDiscovery('field', 'widget')), 'field_widget_info');
+    $this->discovery = new CacheDecorator($this->discovery, 'field_widget_types',  'field');

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

yched’s picture

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

sdboyer’s picture

ok, then...

+++ b/core/modules/field/lib/Drupal/field/Plugin/Type/Widget/WidgetPluginManager.php
@@ -28,41 +28,16 @@ class WidgetPluginManager extends PluginManagerBase {
+    $this->discovery = new AlterDecorator(new WidgetLegacyDiscoveryDecorator(new AnnotatedClassDiscovery('field', 'widget')), 'field_widget_info');
+    $this->discovery = new CacheDecorator($this->discovery, 'field_widget_types',  'field');

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.

sdboyer’s picture

Status: Needs review » Reviewed & tested by the community

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

tim.plunkett’s picture

FileSize
5.01 KB
11.84 KB

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

tim.plunkett’s picture

Priority: Normal » Major

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

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to 8.x. Thanks.

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

Anonymous’s picture

Issue summary: View changes

reword