Config entities implementing EntityWithPluginCollectionInterface should ask the plugins to react when their dependencies are removed

1. Add code in ConfigEntityBase::onDependencyRemoval() that tests if the entity implements EntityWithPluginCollectionInterface. If so, iterate through collections and if there are specific plugin dependencies being removed, call the plugin ::onDependencyRemoval() method to allow the plugin to react.

Right, and most of the code currently in EntityDisplayBase::onDependencyRemoval() & the ::getPluginRemovedDependencies() helper can be moved straight up to ConfigEntityBase::onDependencyRemoval() for that.

But the tricky part is then how each specific ConfigEntity class can react to the plugin::onDependencyRemoval() result (whether "OK, I adapted, ask me for my new settings" or "screw it, I'm not changing myself"). AFAICT that logic is specific to the business logic and implementation of each "config entity with a plugin collection" : EntityDisplays do something, others will need something else.

So it seems ConfigEntityBase::onDependencyRemoval() can provide the general mechnism, but will need to defer a separate submethod to handle the result of plugin::onDependencyRemoval(). And that submethod will likely need to be defined in EntityWithPluginCollectionInterface (it only makes sense if the ConfigEntity is a EntityWithPluginCollectionInterface).

Then, same problem as below with adding a method with existing direct implementations, we'll likely need a new subinterface for that...

2. Add a new method onDependencyRemoval() in \Drupal\Component\Plugin\DependentPluginInterface.
3. Add empty onDependencyRemoval() implementations in base classes implementing DependentPluginInterface with a simple return FALSE;.

Problem is, there are no such base classes ? Most implementations of that interface implement it directly, and we can be sure that that is also the case in contrib/custom code out there, and adding that method would break them.

So I fear this means adding a new subinterface of DependentPluginInterface to receive that new method :-/

This has come up again in #2830581: Fix ContentModeration workflow type to calculate correct dependencies.

Bumping to major because this is really confusing and it's hard to realize you need to add such a method to your specific entity type interface, and screwing up your entity's dependency management can cause data integrity issues or data loss, so it would be good to find some solution or mitigation for this.

This would help resolve #1764380: Merge PluginSettingsInterface into ConfigurableInterface, as we need a replacement for \Drupal\Core\Field\PluginSettingsInterface::onDependencyRemoval()

Here is an initial version of this. I think the BC concerns are valid, so here is an interface which extends DependentPluginInterface.

I think the meaty tests will come from the implementations of onDependencyRemoval, which are up to the plugins to provide and test. The current test only verifies plugins that implement the interface have a chance to act. Most of the docs on the interface are copied from ConfigEntityInterface::onDependencyRemoval, but should still apply.

Great start!
First, due to #2851574: DependentPluginDefinitionInterface and DependentPluginDefinitionTrait should be in Drupal\Core not Drupal\Component we shouldn't repeat the mistakes of core, let's have this live in Drupal\Core.

Second, since PluginSettingsBase already provides implementations of these methods (due to PluginSettingsInterface as mentioned in #10), let's use this new interface there.

Third, the docs on the new RemovableDependentPluginInterface shouldn't mention entities at all. I think this is just because they were copy/pasted as you mentioned.

Finally, we need to untangle \Drupal\Core\Entity\EntityDisplayBase::onDependencyRemoval(). However that may prove trickier than expected, since neither EntityFormDisplay or EntityViewDisplay use EntityWithPluginCollectionInterface correctly...

Here's a patch addressing my first two points.

It seemed to make sense to mention their relationship to config entities and how removing dependencies will preserve those entities, but I guess that connection isn't explicit based on the interface.

Fixed the docs and resolved a @todo pointing to this issue. EntityDisplayBase looks like a hairy one, is that in scope for this issue?

Thanks for summarising that, I think step 1 and 3 are already done in #13.

From what I can tell, getPluginRemovedDependencies is used to detect if a plugin couldn't handle removing it's dependencies. This happens so that the display can just disable the component. I think it's pretty unique for a config entity to understand how to disable some part of itself based on a plugin having unresolved dependencies, it seems like the most most complex use case to the point where I don't know if it's worth using getPluginRemovedDependencies elsewhere? All removed dependencies can be passed to all plugins, I don't see a need for them to be filtered in advance.

Re: suggestion of untangling EntityDisplayBase::onDependencyRemoval in #12, not sure how to go about that. I don't know if it makes sense for a plugin to manage and remove dependent config, because each one isn't responsible for it's own isolated tree of config that it can deal with. A plugin can't move itself from 'content' to 'hidden'. Keen to here if @tim.plunkett has any thoughts on how to approach this.

NR for random fail.

Is there a concrete use case for onUnresolvedPluginDependencyRemoval? It seems like something unique to entity displays, based on their ability to disable fields.

Also, I think it makes sense to actually use the interface outside of a test when it's introduced. The deletions show an example of what the interface gives you free.

#2830581: Fix ContentModeration workflow type to calculate correct dependencies is now in, so we should address that introduction of onDependencyRemoval() as well.

+++ b/core/modules/image/src/Entity/ImageStyle.php
@@ -407,8 +407,16 @@ public function onPluginDependencyRemoval($collection_group, PluginInspectionInt
+        $this->getEffects()->setInstanceConfiguration($plugin->getPluginId(), $plugin->getConfiguration());

This is handled in ConfigEntityBase::preSave(), do we really need it again here?

I think we should switch to a TDD approach here, it will help avoid adding extra code. Let's define what the goals are, write test coverage for it, and then adjust the internals as needed.

This all looks pretty good, but I think the onPluginDependencyRemoval method needs to be removed from the picture. I don't think it does anything in the example in the patch and I don't see why anything it could conceivably do couldn't be part of the method for the individual plugin. The "let the config entity react" comment isn't really needed if the point was to refresh the config entity, as @tim.plunkett suggested that's the job of the collection.

NW for that + @todos.

1. +++ b/core/modules/image/src/Entity/ImageStyle.php
   @@ -399,6 +400,40 @@ public function setName($name) {
   +      if (in_array($plugin->getPluginDefinition()['provider'], $dependencies['module'])) {
   +        // If the module providing the effect is disabled, remove the effect
   +        // from this image style and exit here.
   +        $this->deleteImageEffect($plugin);
   +        return TRUE;
   +      }
   

   Wouldn't this already happen because the image style fails to remove the module as a dependency and onUnresolvedPluginDependencyRemoval removes it?

Hi claudiu.cristea, are you still actively working on this?

I've tried to jump start this up again by rerolling and implementing some of #45.

I don't really understand the test cases or the fails, mocking them for specific return values on a specific number of calls is hard to follow but will find some time to have a look at it all again.

+++ b/core/lib/Drupal/Core/Entity/EntityWithPluginCollectionInterface.php
@@ -7,9 +7,12 @@
+ * @deprecated Scheduled for removal before  Drupal 9.0.0. Use
+ *   \Drupal\Core\Entity\EntityWithDependentPluginCollectionInterface instead.

I think we also need to check the BC policy, I think interface additions when there is a suitable base class are allowed.

#44 doesn't have an explanation.

I don't think the pattern of the config entity having specific awareness of the plugins private configuration (and thus changes when the plugin does) is a pattern that should be encouraged by the interfaces, no matter what is in EntityDisplayBase. Generally speaking, the plugin configuration should be self-contained (you know, pluggable...) and there shouldn't typically be much reason for a config entity to change when a plugin cleanly resolves a dependency removal. The plugin config should be encapsulated and irrelevant to the config entity.

Perhaps the patch should have a concrete example of where this method makes sense instead of shoehorning it into ImageStyle. You could remove that whole method from that entity and it would behave the same.

Maybe step 7 should be resolved before core APIs are written to cater for it:

#44.7 Follow-up to refactor EntityDisplayBase for a proper usage of plugin collections.

Is there an issue for fixing EntityDisplayBase?

Wonder if someone could provide an updated issue summary? Been about 5 years so a lot has changed.

I bet it's about to kinda "behavior.detach()" to allow plugins manage destruction for example

it still very handy to have this reaction, it moves forward action plugins at least

+++ b/core/lib/Drupal/Core/Plugin/RemovableDependentPluginInterface.php
@@ -0,0 +1,42 @@
+ * Provides an interface for plugins that react when dependencies are removed.
...
+interface RemovableDependentPluginInterface extends DependentPluginInterface {
...
+   * If this method returns TRUE then the entity the plugin is attached to needs
+   * to be re-saved by the caller for the changes to take effect.
...
+   * @param array $dependencies
+   *   An array of dependencies that will be deleted keyed by dependency type.
+   *   Dependency types are content, config, module and theme.
...
+   * @return bool
+   *   TRUE if the entity has been changed as a result, FALSE if not.
...
+  public function onDependencyRemoval(array $dependencies);

It still makes sense as separate interface

Added reroll of patch #49 on Drupal 10.1.x. still needs work for comment #69.

Related, with 162 followers: #3056633: It is not possible to uninstall a module that provides a filter plugin - Drupal\Component\Plugin\Exception\PluginNotFoundException: The "{filter}" plugin does not exist.