Updated: Comment #N

Problem/Motivation

Foo module provides a plugin type, FooPlugin.
MyModule provides one FooPlugin.
During the install of MyModule, the FooPlugin is instantiated.

Expected:
The plugin is instantiated.
Actual:
Uncaught PHP Exception Drupal\Component\Plugin\Exception\PluginException: "The plugin (foo_plugin) did not specify an instance class." at /www/d8/core/lib/Drupal/Component/Plugin/Factory/DefaultFactory.php

There are 3 known workarounds in core:

  • Entity types have entity_info_cache_clear() called directly in ModuleHandler::install()
  • Field types are cleared by forum_module_preinstall()
  • Views has code just commented out in ViewsHandlerManager::getHandler()

It is also holding up the ActionManager conversion and new search config entity patch, see related issues.

Proposed resolution

Provide a new service tag that plugin managers can use. Any manager tagged will have "clearCachedDefintions()" called on it during module install.
This could also prove useful for a drush command or other places anyone wants to clear plugin manager caches.

Remaining tasks

Agree on proposal
Write code
Fix the 3 workarounds listed above.

User interface changes

N/A

API changes

No changes, but a new service tag is added.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tim.plunkett’s picture

Clarified existing workarounds.

dawehner’s picture

Status: Active » Needs review
FileSize
11.4 KB

What about something like this?

Status: Needs review » Needs work

The last submitted patch, 2: cache_clearer-2155635.patch, failed testing.

tim.plunkett’s picture

  1. +++ b/core/core.services.yml
    @@ -182,27 +182,45 @@ services:
         parent: default_plugin_manager
    +    tags:
    +      - { name: plugin_manager_cache_reset }
    

    Berdir suggested that this be added to the parent: default_plugin_manager definition.

  2. +++ b/core/core.services.yml
    @@ -182,27 +182,45 @@ services:
    +      - { name: plugin_manager_cache_reset }
    

    We need a better tag name, but clear is better than reset.

We should add a call to \Drupal::service('plugin.cache_clearer')->clearCachedDefinitions(); :)

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
15.06 KB
14.66 KB
13.75 KB

Turns out the tags don't work with abstract/parents. Or at least it wouldn't install locally.

I've renamed the tag, and tried moving some more stuff around. Uploading 2 patches to see what I can move out of ModuleHandler::install().

The last submitted patch, 5: plugin-managers-2155635-5A.patch, failed testing.

The last submitted patch, 5: plugin-managers-2155635-5B.patch, failed testing.

tim.plunkett’s picture

FileSize
15.09 KB
2.5 KB

Yay for finding bugs in core!

InstallUninstallTest and ModuleApiTest both blindly call module_uninstall on comment, which used to be fine, but is not fine now that comment provides fields.

Now because we're clearing the views plugin cache a bit more, field_views_data tries to look up the storage controller for comment_field right after comment is uninstalled. Whoops!

Adding field_module_preuninstall for now, but we'll need to fix that exception.

Also found some weirdness with language_list(), and had to remove the views fix for now.

The last submitted patch, 8: plugin-managers-2155635-8.patch, failed testing.

dawehner’s picture

@tim
Should we try to keep the changes in ModuleHandler at least for now as small as possible and iterate in follow ups?

tim.plunkett’s picture

Yes, we can move that call back. It won't help with the bug though...
I plan to work on this tonight, but if someone wants to jump in before then, feel free.

tim.plunkett’s picture

FileSize
16.72 KB
2.43 KB

I don't think forum module is removing its comment field properly. It tries to in forum_uninstall, but doesn't seem to take. Here are some hacks to show how this works.

I don't know why this is only a problem with comment...

I also removed the change to ModuleHandler, that didn't make the interdiff.

The last submitted patch, 12: plugin-managers-2155635-12.patch, failed testing.

aspilicious’s picture

Smells like crappy copy paste dx. Isn't there a way to have this on by default?

pwolanin’s picture

Since all plugins are provided by modules, and hence may have new plugins available when a module installed, I guess I also don't see how to make this simple unless each module needs to declare somehow which plugins it provides?

tim.plunkett’s picture

Plugins can also be provided by Drupal\Core and Drupal\Component.

sun’s picture

  1. +++ b/core/lib/Drupal/Core/Plugin/PluginCacheClearer.php
    @@ -0,0 +1,51 @@
    +  public function addPluginManager(PluginManagerInterface $plugin_manager) {
    +    if (!$plugin_manager instanceof CachedDiscoveryInterface) {
    +      throw new PluginException('The plugin manager does not implement the CachedDiscovery interface.');
    +    }
    +    $this->pluginManagers[] = $plugin_manager;
    +  }
    ...
    +  public function clearCachedDefinitions() {
    +    foreach ($this->pluginManagers as $plugin_manager) {
    +      $plugin_manager->clearCachedDefinitions();
    +    }
    +  }
    

    Instead of throwing an exception here, can't we move the CachedDiscoveryInterface condition into the clearCachedDefinitions() loop?

  2. +++ b/core/lib/Drupal/Core/Plugin/PluginManagerPass.php
    @@ -0,0 +1,29 @@
    +    foreach ($container->findTaggedServiceIds('plugin_manager_cache_clear') as $service_id => $tags) {
    

    Can't we simply grep all defined services for the prefix "plugin.manager." in this compiler pass, instead of manually tagging them?

    Combined with aforementioned suggestion, that would simplify things and improve DX.

  3. +++ b/core/modules/field/field.module
    @@ -275,6 +275,27 @@ function field_modules_installed($modules) {
    +  $entity_info = \Drupal::entityManager()->getDefinitions();
    +  foreach ($entity_info as $type => $info) {
    +    if ($info['provider'] == $module) {
    +      $types[$type] = $type;
    

    Looks like a ::getDefinitionsByProvider() method would be handy?

  4. +++ b/core/modules/system/lib/Drupal/system/Tests/Module/DependencyTest.php
    @@ -174,6 +174,7 @@ function testUninstallDependents() {
         // Uninstall comment module.
    +    entity_load('field_instance', 'comment.node__comment_forum.comment_body')->delete();
         $edit = array('uninstall[comment]' => 'comment');
         $this->drupalPostForm('admin/modules/uninstall', $edit, t('Uninstall'));
    

    IIRC, there is an existing separate issue that intends to make destruction of all module data part of the module uninstallation process (through the existing confirmation form), which should eliminate the need for having to manually delete these comment body field instances in tests?

  5. +++ b/core/modules/system/system.module
    @@ -2863,3 +2863,15 @@ function system_admin_paths() {
    +function system_module_preinstall($module) {
    +  // @todo \Drupal\Core\Plugin\DefaultPluginManager calls language_list() while
    +  //   language.module is in the module list but before its entities have been
    +  //   installed, which will cause language_list() to store an empty list.
    +  if ($module != 'language') {
    +    \Drupal::getContainer()->get('plugin.cache_clearer')->clearCachedDefinitions();
    +  }
    +}
    

    I've the impression that this code should rather be part of the ModuleHandler::install()?

    The reason for why a hook_modules_uninstalled() implementation is not required here is that ModuleHandler::uninstall() calls into drupal_flush_all_caches() already (which is undesired/unwanted on its own).

    Both the install() and uninstall() operations should clear cached plugin manager definitions automatically at the right point in time.

    (FWIW, I did not understand the @todo, as it talks about priming a storage, whereas the code here is about pruning/flushing a primed storage → why is the condition necessary?)

Berdir’s picture

1. No because that method is part of that interface, if the plugin manager doesn't implement it, so we can't call it. Not sure if the exception is necessary or if we want to silently ignore it.

2. The entity manager is just entity_manager now, without the prefix.

5. The objectify the language system issue might fix this, as the core language manager is going to be very simple and return a fixed set of languages (english + special languages like not specified).

tim.plunkett’s picture

Assigned: tim.plunkett » Unassigned

I don't have really strong opinions on *how* we solve this, just that we need to. So unassigning for myself.

dawehner’s picture

Instead of throwing an exception here, can't we move the CachedDiscoveryInterface condition into the clearCachedDefinitions() loop?

I am quite a fan of telling people when they are doing something wrong, which would here happen directly in the container rebuild and not on runtime as the clearCachedDefinitions loop and yeah
also berdir's point is quite good.

Can't we simply grep all defined services for the prefix "plugin.manager." in this compiler pass, instead of manually tagging them?

Combined with aforementioned suggestion, that would simplify things and improve DX.

Sadly we really cannot load all the services and check there interface directly. We could auto tag every service with "plugin.manager" it front and require it for the other ones?

Looks like a ::getDefinitionsByProvider() method would be handy?

Not sure whether there are really many usecase for this kind of functionality on a generic plugin manager, though at least for entities this is kind of helpful.

I've the impression that this code should rather be part of the ModuleHandler::install(

I kind of have the feeling that the module handler should know the least possible amount of information, like not care about the other subsystems like fields/entities/plugins/whatever.

Status: Needs review » Needs work

The last submitted patch, 20: plugin-managers-2155635-20.patch, failed testing.

Berdir’s picture

+++ b/core/modules/system/system.module
@@ -2890,3 +2890,15 @@ function system_admin_paths() {
+/**
+ * Implements hook_module_preinstall().
+ */
+function system_module_preinstall($module) {

While I do see your point, I don't really see how it's more OK if it's system.module. If the plugin component would be able to re-act to a module handler change yes, but this just seems to hide something important in a rather hidden location?

sun’s picture

the module handler should know the least possible amount of information, like not care about the other subsystems like fields/entities/plugins/whatever.

While that is a noble goal, fact (and sad truth) is that ModuleHandler::install() (and implicitly also ::uninstall()) calls into system_list_reset().

That function clears the majority of all extension related caches. The remainder is handled by $kernel->updateModules() and entity_info_cache_clear(), whereas the latter is currently guilty for flushing all entity info + field info caches, which are all plugins today.

From a conceptual perspective, we introduced hook_rebuild() for this purpose (in drupal_flush_all_caches()) two years ago. To make a clear differentiation between cache flushing vs. data rebuilding.

Unfortunately, the introduction of hook_rebuild() in D8 went mostly unnoticed and wasn't advanced on yet. A whole bunch of hook_cache_flush() implementations (in particular, relating to entity/field data) should actually be hook_rebuild() implementations.

Semantically, the "cached" plugin info/definitions are much rather state than cache, because they're statically defined in code and persistently cached (forever) until they need to be refreshed.

That differentiation applies to almost all of the former _info hooks, and even more so today, as we've started to take the differentiation much more seriously in D8, by disallowing context/request specific values in all declarations that were _info hooks previously.

Long story short:

  1. Unlike former versions of Drupal, we're looking at a major improvement of module_install() today, because it no longer calls into drupal_flush_all_caches(). (Unlike module_uninstall(), which still calls it, for whatever reason.)
  2. Next to clearing all caches, drupal_flush_all_caches() also happened to rebuild all data structures. But in D7+, we started to bake a "base system knowledge" into module_install() — i.e., it knows that entity info caches need to be rebuilt as well.
  3. Why did we do that? → Because entity_get_info() & Co are defined in /includes/entity.inc; i.e., the service is not provided by a module, and thus, there is no originating module that is responsible for performing the operation. (Gentle reminder: We want to get rid of System module ASAP.)
  4. In D8, and in this issue here, we're looking at a vast multiplication of such services. Whether they are plugin managers or not is irrelevant. And System module still has nothing in common with them and should still die ASAP.
  5. In light of D8's architecture, the proper answer is to inject the EventDispatcher into ModuleHandler (+ ThemeHandler) and trigger an event that allows all plugin managers to listen to it.

Thoughts?

EclipseGc’s picture

Is there a reason we want to check the names instead of the tag? Caching has some implicit developer knowledge in order to get it working and while true, plugins have attempted to side step that as much as possible, I'd much rather just tag a service as needing to be part of the cache invalidation paradigm. This is one of the "hard" programming problems, I don't think asking people to put a tag on a service is really all that large of a DX request/regression.

Eclipse

dawehner’s picture

Semantically, the "cached" plugin info/definitions are much rather state than cache, because they're statically defined in code and persistently cached (forever) until they need to be refreshed.

Well, there are cases of plugins which are certainly dynamic it the sense that they have to be recreated based on user actions. The block plugins provided by views is one example.

That differentiation applies to almost all of the former _info hooks, and even more so today, as we've started to take the differentiation much more seriously in D8, by disallowing context/request specific values in all declarations that were _info hooks previously.

This also just worked on some cases, which wasn't cached. The plugin system in ctools for example (obviously) had a similar behavior, so I wonder whether that system ever had dynamic usecases.

In light of D8's architecture, the proper answer is to inject the EventDispatcher into ModuleHandler (+ ThemeHandler) and trigger an event that allows all plugin managers to listen to it.

I am glad that someone suggested that, but I wonder how we deal with the problem of having both events and hooks. We would have to decide an order for that or just get rid of hook_modules_preinstall

Berdir’s picture

Status: Needs work » Needs review
FileSize
16.36 KB

Straight untested re-roll, will check afterwards if we can improve things now.

Status: Needs review » Needs work

The last submitted patch, 26: plugin-managers-2155635-26.patch, failed testing.

The last submitted patch, 26: plugin-managers-2155635-26.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
16.9 KB
3.22 KB

Ok, updated the code and calling the clearer directly for now, replacing the entity_info_clear_cache() call, otherwise we do that twice.

Status: Needs review » Needs work

The last submitted patch, 29: plugin-managers-2155635-29.patch, failed testing.

sun’s picture

It looks like most of the failures are because the field.info service defined by field.module is no longer cleared?

Can we make the FieldInfo class implement the CachedDiscoveryInterface, simply rename the flush() method into clearCachedDefinitions(), and tag the service?


PS: Closely related, partially blocked on this: #2201785: Remove drupal_flush_all_caches() from installer

Berdir’s picture

Status: Needs work » Needs review
FileSize
14.87 KB
4.01 KB

Ok, forum needs to delete the comment body field too. This is a bit weird :(

sun’s picture

Status: Needs review » Needs work

The last submitted patch, 32: plugin-managers-2155635-31.patch, failed testing.

sun’s picture

Exception: If you don't eat yer meat, you can't have any pudding

Seriously?

And… some people are actively shooting for a beta...?

(Disregard the example, it's just a testament of how large the real distance between HEAD and a hypothetical beta actually is.)

tim.plunkett’s picture

I added that as a placeholder because I don't know what we should do in that case. This patch has never passed tests, and was in no danger of being committed to core.

If you would like to help work toward a beta and resolve this issue, please feel free.

Berdir’s picture

Status: Needs work » Needs review
FileSize
18 KB
3.96 KB

@sun: This is not related to FieldInfo, those caches are only about actual existing fields and the caches are transparently cleared when there are changes. This is about field types and fields that reference field types that no longer exist. Which is no longer supporter with the removal of the active flag.

This should fix those test fails.
- Rewritten FieldInfoTest::testInstanceDisabledEntityType() to use entity_test, I'm a bit confused why this is even supported, maybe #2198429: Make deleted fields work with config synch will remove it if it will happen.
- Re-added the manual deletion to CommentFieldsTest but I think what should happen here is that comment removes it's bundles in uninstall which will also remove all fields. We could even standardize this now.
- The installer is an ugly one. clearCachedDefinitions() calls out to the language manager. In case of locale installation, because rebuild the container before we install the module/schema call preinstall (why?!), the locale translation service already exists which then tries to translate the default language labels but the schema has not yet been installed. The order of calls in install() looks very random to me, I think we should d something like this: call preinstall, install schema, clear caches, call enable, rebuild container? As a quickfix, clearing the plugin caches after installing the schema makes the test pass.

Status: Needs review » Needs work

The last submitted patch, 37: plugin-managers-2155635-37.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review

Patch passed, testbot was just too lazy to update the issue status.

Berdir’s picture

Instead of having to manually clean up the fields, can we do something similar to entity_module_preuninstall() and drop all fields that belong to a field type of the module that is being uninstalled?

tim.plunkett’s picture

  1. +++ b/core/lib/Drupal/Core/Entity/EntityManager.php
    @@ -176,6 +176,17 @@ public function getDefinition($entity_type_id, $exception_on_invalid = FALSE) {
    +  public function getDefinitionsByProvider() {
    +    $grouped = array();
    +    foreach ($this->getDefinitions() as $entity_type_id => $entity_type) {
    +      $grouped[$entity_type->getProvider()][$entity_type_id] = $entity_type_id;
    +    }
    +    return $grouped;
    +  }
    
    +++ b/core/lib/Drupal/Core/Entity/EntityManagerInterface.php
    @@ -245,4 +245,12 @@ public function getDefinition($entity_type_id, $exception_on_invalid = FALSE);
    +  public function getDefinitionsByProvider();
    

    This is a nice enough addition, but it's not used and I don't think its in scope...

  2. +++ b/core/lib/Drupal/Core/Plugin/PluginCacheClearer.php
    @@ -0,0 +1,51 @@
    +class PluginCacheClearer {
    ...
    +  public function addPluginManager(PluginManagerInterface $plugin_manager) {
    

    I think this name is fine, but considering the interface we check and the method we call, CachedDiscoveryClearer might be a better name. And then we don't have need for the PluginManagerInterface typehint...

  3. +++ b/core/modules/field/field.module
    @@ -279,6 +279,27 @@ function field_modules_installed($modules) {
    + * Implements hook_module_preuninstall().
    + */
    +function field_module_preuninstall($module) {
    +  $entity_type_ids = array();
    +  $entity_types = \Drupal::entityManager()->getDefinitions();
    +  foreach ($entity_types as $entity_type) {
    +    if ($entity_type->getProvider() == $module) {
    +      $entity_type_ids[] = $entity_type->id();
    +    }
    +  }
    +  if ($entity_type_ids) {
    +    $fields = \Drupal::entityQuery('field_instance_config')
    +      ->condition('entity_type', $entity_type_ids)
    +      ->execute();
    +    if ($fields) {
    +      throw new \Exception("If you don't eat yer meat, you can't have any pudding");
    +    }
    +  }
    +}
    +
    

    I used this to find tests that didn't clean up fields properly, so this whole hook can be remove AFAICS?

  4. +++ b/core/modules/views/views.services.yml
    @@ -2,60 +2,98 @@ services:
       plugin.manager.views.access:
         class: Drupal\views\Plugin\ViewsPluginManager
         arguments: [access, '@container.namespaces', '@cache.views_info', '@language_manager', '@module_handler']
    +    tags:
    +      - { name: plugin_manager_cache_clear }
    

    Do we want to tag all of these even though they match the 'plugin.manager.' pattern? It doesn't seem like other services were updated

Berdir’s picture

1. Yeah, removed.

2. Renamed, let's see if I got all references right. I did not rename the compiler pass as that still has plugin manager specific assumptions. I also didn't rename the service name, I think that one still works, it's still a part of the plugin system.

3. Yes, removed. Maybe a follow-up to discuss to automatically remove such fields? We already automatically delete bundles, which results in deleting fields attached to these bundles. So doing it for field types should complete the picture there. And there might be an issue with comment, as it should probably re-act on field deletion and delete it's own corresponding bundles? If you look at comment_field_instance_config_delete(), it deletes the content (this should probably happen as part of field data deletion, as this does not scale), but not bundles and once the field instance is gone, the bundles silently vanish and are not deleted. I think that's why most fails that we had here are related to the comment body field. Or we do it here, then we can drop the manual deletions.

4. Yeah, I think that depends on whether we want to do the magic name thing or not, I don't think there was an agreement there. If we do, then we can remove it.

Berdir’s picture

Oh, I found the problem, there was a delete call in comment_field_config_delete(), but it used the wrong name for the bundle. This fixes the CommentFieldsTest without the need to manually delete the field.

tim.plunkett’s picture

+++ b/core/lib/Drupal/Core/Plugin/CachedDiscoveryClearer.php
@@ -0,0 +1,52 @@
+   * @throws \Drupal\Component\Plugin\Exception\PluginException
+   *   Thrown when the passed object does not implement the required
+   *   interface.
...
+  public function addCachedDiscovery(CachedDiscoveryInterface $cached_discovery) {
+    if (!$cached_discovery instanceof CachedDiscoveryInterface) {

I think this is enforced by PHP :)

Nice find with that last field deletion.

Berdir’s picture

Yeah, my brain does not work reliably before 9am :)

larowlan’s picture

+++ b/core/modules/comment/comment.module
@@ -284,7 +284,7 @@ function comment_field_instance_config_update(FieldInstanceConfigInterface $inst
+    entity_invoke_bundle_hook('delete', 'comment', $field->entity_type . '__' . $field->getName());

Good catch, berdir++

Berdir’s picture

Status: Needs review » Needs work

The last submitted patch, 45: plugin-managers-2155635-45.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
15.6 KB

Re-roll.

Berdir’s picture

Status: Needs review » Needs work

The last submitted patch, 49: plugin-managers-2155635-49.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
10.8 KB

Re-roll, lots of conflicts in views.services.inc, so here's a patch without the explicit tags there for now.

dawehner’s picture

+++ b/core/lib/Drupal/Core/Plugin/CachedDiscoveryClearer.php
@@ -0,0 +1,45 @@
+  /**
+   * Clear the cache on all cached discoveries.
+   */
+  public function clearCachedDefinitions() {
+    foreach ($this->cachedDiscoveries as $cached_discovery) {
+      $cached_discovery->clearCachedDefinitions();
+    }
+  }
+

Given how many potential plugin managers we have I would vote to make them loaded lazy here, which means that we store the service names and just load the ones which are already instanciated.

Berdir’s picture

That won't help with clearing persistent caches?

I was thinking about a flag to avoid clearing persistent caches more than once unless they have been rebuilt again, but we rebuild the container anyway, so that will drop those objects?

sun’s picture

Aside from the remark below, the latest patch looks good to me - would be great to move forward here.

+++ b/core/modules/forum/forum.install
@@ -113,6 +101,10 @@ function forum_uninstall() {
+  if ($field = field_info_field('comment', 'comment_body')) {
+    $field->delete();
+  }

Won't that delete the entire comment body field wherever it is used...?

When uninstalling Forum module, comments may still be enabled on my blog and elsewhere?

Berdir’s picture

Yes, that is wrong. Might not even be needed anymore now, with the field instance deletion above, worth a try.

Berdir’s picture

sun’s picture

Status: Needs review » Reviewed & tested by the community

Yay, thanks!

This is a blocker for #2201785: Remove drupal_flush_all_caches() from installer, which will be a major performance improvement for the installer + core test suite, so let's get this in ASAP :-)

tim.plunkett’s picture

Awesome work @Berdir, thanks for finishing this off!
I went to write a draft change record for the API addition, and noticed some weird docblocks. No functional changes, still RTBC.
Draft change record coming soon, will appear in the sidebar.

tim.plunkett’s picture

Hmm, I just noticed that if a plugin manager service ID starts with plugin.manager., it is assumed to implement CachedDiscoveryInterface.
There is no way to use that service ID pattern without implementing that interface, because of CachedDiscoveryClearer::addCachedDiscovery().

Now, both PluginManagerBase and DefaultPluginManager implement this, which covers every manager in core. Just thought I'd point out the assumption.

Added https://drupal.org/node/2218621

catch’s picture

Status: Reviewed & tested by the community » Fixed

Hmm #60 is interesting but I think it can be a follow-up, patch looks like a very nice improvement otherwise.

Committed/pushed to 8.x, thanks!

Status: Fixed » Closed (fixed)

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

andypost’s picture

Filed follow-up to remove @todo that left #2324945: clean-up @todo after 2155635