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 :

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

Files: 
CommentFileSizeAuthor
#32 drupal-1764232-32.patch11.84 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 48,739 pass(es).
[ View ]
#32 interdiff.txt5.01 KBtim.plunkett
#27 plugins-cache-decorator-1764232-27.patch11.67 KByched
PASSED: [[SimpleTest]]: [MySQL] 48,069 pass(es).
[ View ]
#24 plugins-cache-decorator-1764232-23.patch11.67 KByched
PASSED: [[SimpleTest]]: [MySQL] 48,004 pass(es).
[ View ]
#24 interdiff.txt500 bytesyched
#20 plugins-cache-decorator-1764232-19.patch11.65 KByched
PASSED: [[SimpleTest]]: [MySQL] 47,870 pass(es).
[ View ]
#20 interdiff.txt518 bytesyched
#18 plugins-cache-decorator-1764232-17.patch11.65 KByched
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#18 interdiff.txt1.08 KByched
#16 plugins-cache-decorator-1764232-16.patch11.58 KByched
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#16 interdiff.txt7.36 KByched
#11 plugins-cache-decorator-1764232-11.patch14.36 KByched
PASSED: [[SimpleTest]]: [MySQL] 47,887 pass(es).
[ View ]

Comments

Issue tags:+Needs tests

taggin

Issue tags:+VDC

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

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

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

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 :

<?php
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);
}
?>

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.

Assigned:Unassigned» yched

Assigning to @yched for some insight.

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

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

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

Status:Needs review» Active
StatusFileSize
new14.36 KB
PASSED: [[SimpleTest]]: [MySQL] 47,887 pass(es).
[ View ]

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

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

Status:Active» Needs review

Status:Active» Needs review

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

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

Thoughts ?

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

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

StatusFileSize
new7.36 KB
new11.58 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

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

Status:Needs work» Needs review
StatusFileSize
new1.08 KB
new11.65 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

Fixes phpdoc glitches.

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new518 bytes
new11.65 KB
PASSED: [[SimpleTest]]: [MySQL] 47,870 pass(es).
[ View ]

And a missing 'use' statement.

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.

Issue tags:+VDC

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

StatusFileSize
new500 bytes
new11.67 KB
PASSED: [[SimpleTest]]: [MySQL] 48,004 pass(es).
[ View ]

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

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

+    // Poluplate sample definitions.

... // Populate

Ahh, that makes sense @yched, thanks!

StatusFileSize
new11.67 KB
PASSED: [[SimpleTest]]: [MySQL] 48,069 pass(es).
[ View ]

Fixed typo pointed in #25.

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

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.

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.

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.

StatusFileSize
new5.01 KB
new11.84 KB
PASSED: [[SimpleTest]]: [MySQL] 48,739 pass(es).
[ View ]

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.

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.

Status:Reviewed & tested by the community» Fixed

Committed to 8.x. Thanks.

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

Issue summary:View changes

reword