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

  1. Re-implant the CacheDecorator from #1683644-107: Use Annotations for plugin discovery
  2. Install Language and Locale modules
  3. Enable an additional language
  4. Translate "Default fetcher" (whatever that means :P) into the additional language
  5. Hack aggregator.admin.inc to also output the aggregator fetcher configuration widget in case there is only one
  6. Visit /admin/config/services/aggregator/settings in one language, then in the other.
  7. Actual result: Default fetcher appears in wrong language.
  8. 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:

plugin-cachedecorator-language.png

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

effulgentsia’s picture

CacheDecorator::__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.

aspilicious’s picture

Status: Active » Needs review
FileSize
1.72 KB

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

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

This really improves the working with views.

+++ b/core/lib/Drupal/Core/Plugin/Discovery/CacheDecorator.phpundefined
@@ -64,14 +64,8 @@ class CacheDecorator implements DiscoveryInterface {
-    $definitions = $this->getCachedDefinitions();
-    if (isset($definitions)) {
-      $definition = isset($definitions[$plugin_id]) ? $definitions[$plugin_id] : NULL;
-    }
-    else {
-      $definition = $this->decorated->getDefinition($plugin_id);
-    }

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

aspilicious’s picture

Just wanted to note that I copied that approach from the annotationDiscovery

catch’s picture

Status: Reviewed & tested by the community » Active

I've committed this for now to unblock views. Leaving open for translation, cache tags and test coverage.

yched’s picture

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

moshe weitzman’s picture

Another important cache issue that folks might want to help on - #1774332: Better handling of invalid/expired cache entries

andypost’s picture

Priority: Critical » Normal
Issue tags: +Needs tests

This is not critical anymore.

tim.plunkett’s picture

Status: Active » Needs review
FileSize
1.98 KB
neclimdul’s picture

+++ b/core/lib/Drupal/Core/Plugin/Discovery/CacheDecorator.phpundefined
@@ -104,7 +113,7 @@ protected function getCachedDefinitions() {
+      cache($this->cacheBin)->set($this->cacheKey, $definitions, CacheBackendInterface::CACHE_PERMANENT, $this->cacheTags);

Since "CacheBackendInterface::CACHE_PERMANENT" is being added, should it be considered as a possible argument? Follow up?

fubhy’s picture

FileSize
2.51 KB

Since "CacheBackendInterface::CACHE_PERMANENT" is being added, should it be considered as a possible argument?

Yes, I think so too. Also, the new @param docs were missing for __construct.

EclipseGc’s picture

Status: Needs review » Reviewed & tested by the community

I'm running this code for myself locally already and am very ++

fubhy’s picture

FYI: I opened another CacheDecorator issue a week ago with a minor tweak over here: #1853226: Improve performance of CacheDecorator.

catch’s picture

Status: Reviewed & tested by the community » Needs work

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

EclipseGc’s picture

FileSize
3.49 KB

OK, 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

xjm’s picture

Status: Needs work » Needs review
EclipseGc’s picture

xjm’s picture

Priority: Normal » Critical

This is blocking #1535868: Convert all blocks into plugins, so bumping to critical. Next we need tests for it.

YesCT’s picture

Issue tags: +D8MI
xjm’s picture

Issue tags: +VDC, +Blocks-Layouts
EclipseGc’s picture

FileSize
13.38 KB

OK, 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

Status: Needs review » Needs work

The last submitted patch, 1722882-21.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
12.72 KB

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

EclipseGc’s picture

FileSize
12.5 KB

Ok, 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

aspilicious’s picture

+++ b/core/modules/system/tests/modules/plugin_test/plugin_test.moduleundefined
@@ -14,3 +14,33 @@ function plugin_test_plugin_test_alter(&$definitions) {
\ No newline at end of file

Needs newline

Berdir’s picture

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

+++ b/core/lib/Drupal/Core/Plugin/Discovery/CacheDecorator.phpundefined
@@ -54,11 +69,18 @@ class CacheDecorator implements CachedDiscoveryInterface {
+   * @param array $cache_tags
+   *   The cache tags associated with the definition list
    */
-  public function __construct(DiscoveryInterface $decorated, $cache_key, $cache_bin = 'cache') {
+  public function __construct(DiscoveryInterface $decorated, $cache_key, $cache_bin = 'cache', $cache_expire = CacheBackendInterface::CACHE_PERMANENT, array $cache_tags = array()) {
     $this->decorated = $decorated;
     $this->cacheKey = $cache_key;
     $this->cacheBin = $cache_bin;
+    $this->cacheExpire = $cache_expire;
+    $this->cacheTags = $cache_tags;

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

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Plugin/FetcherManager.phpundefined
@@ -18,6 +19,7 @@ class FetcherManager extends PluginManagerBase {
     $this->discovery = new AnnotatedClassDiscovery('aggregator', 'fetcher');
+    $this->discovery = new CacheDecorator($this->discovery, 'aggregator_fetcher:' . language(LANGUAGE_TYPE_INTERFACE)->langcode);
     $this->factory = new DefaultFactory($this->discovery);

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

+++ b/core/modules/system/lib/Drupal/system/Tests/Plugin/CacheDecoratorLanguageTest.phpundefined
@@ -0,0 +1,104 @@
+ * Definition of Drupal\system\Tests\Plugin\CacheDecoratorLanguageTest.

+++ b/core/modules/system/tests/modules/plugin_test/lib/Drupal/plugin_test/Plugin/CachedMockBlockManager.phpundefined
@@ -0,0 +1,23 @@
+ * Definition of Drupal\plugin_test\Plugin\CachedMockBlockManager.

Contains instead of Definition of.

+++ b/core/modules/system/lib/Drupal/system/Tests/Plugin/CacheDecoratorLanguageTest.phpundefined
@@ -0,0 +1,104 @@
+use Exception;

Should use \Exception inline instead of the use.

+++ b/core/modules/system/lib/Drupal/system/Tests/Plugin/CacheDecoratorLanguageTest.phpundefined
@@ -0,0 +1,104 @@
+   * @var Drupal\plugin_test\Plugin\AlterDecoratorTestPluginManager;

\Drupal\...

+++ b/core/modules/system/lib/Drupal/system/Tests/Plugin/CacheDecoratorLanguageTest.phpundefined
@@ -0,0 +1,104 @@
+      'name' => 'CacheDecoratorLanguage',

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

+++ b/core/modules/system/tests/modules/plugin_test/lib/Drupal/plugin_test/Plugin/CachedMockBlockManager.phpundefined
@@ -0,0 +1,23 @@
+class CachedMockBlockManager extends MockBlockManager {
+  public function __construct() {

It's just test code but __construct() might still need a docblock...

+++ b/core/modules/system/tests/modules/plugin_test/plugin_test.moduleundefined
@@ -14,3 +14,33 @@ function plugin_test_plugin_test_alter(&$definitions) {
+function plugin_test_menu() {
+  $items = array();
+  $items['plugin_definition_test'] = array(
+    'access callback' => TRUE,
+    'page callback' => 'plugin_test_definitions',

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

+++ b/core/modules/system/tests/modules/plugin_test/plugin_test.moduleundefined
@@ -14,3 +14,33 @@ function plugin_test_plugin_test_alter(&$definitions) {
+  $manager = new Drupal\plugin_test\Plugin\CachedMockBlockManager();

This on the other hand should use use ;)

EclipseGc’s picture

I 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

EclipseGc’s picture

FileSize
14 KB

Ok,

Added tests for clearing the tag and also expiration setting. Tried to fix most of what Berdir laid out above.

Eclipse

EclipseGc’s picture

FileSize
10.51 KB

and a test only patch as well. sorry.

Status: Needs review » Needs work

The last submitted patch, 1722882-28-test-only.patch, failed testing.

xjm’s picture

Status: Needs work » Needs review

Excellent!

yched’s picture

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

EclipseGc’s picture

I 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

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

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

yched’s picture

Right, fine by me.

catch’s picture

Title: Plugin CacheDecorator caches globally, ignores request context, and does not specify tags for cache items » Change notice: Plugin CacheDecorator caches globally, ignores request context, and does not specify tags for cache items
Category: bug » task
Status: Reviewed & tested by the community » Active

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

xjm’s picture

Berdir’s picture

Created 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 :)

Berdir’s picture

+++ b/core/modules/system/lib/Drupal/system/Tests/Plugin/CacheDecoratorLanguageTest.phpundefined
@@ -0,0 +1,121 @@
+    // Delete cached items tagged with "plugin_test".
+    cache()->deleteTags(array('plugin_test'));

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

Berdir’s picture

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

yched’s picture

So 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 :-/

Berdir’s picture

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

xjm’s picture

Status: Needs review » Reviewed & tested by the community

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

EclipseGc’s picture

I agree

webchick’s picture

Status: Reviewed & tested by the community » Fixed

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

Berdir’s picture

Title: Change notice: Plugin CacheDecorator caches globally, ignores request context, and does not specify tags for cache items » Plugin CacheDecorator caches globally, ignores request context, and does not specify tags for cache items
Category: task » bug
sun’s picture

Component: base system » plugin system

Status: Fixed » Closed (fixed)

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

xjm’s picture

Issue tags: -Needs change record

Untagging. Please remove the "Needs change notification" tag when the change notice task is complete.

xjm’s picture

Issue summary: View changes

Updated issue summary.