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 via config import.

Created MR 13171 with a new approach that is mostly based on the MR for #3326447: Deleting a Media view mode should not result in the deletion of an entire text format, combined with some ideas from the patch in #70:

1. Add new interface Drupal\Core\Plugin\RemovableDependentPluginInterface with method onCollectionDependencyRemoval()
2. Add code in ConfigEntityBase::onDependencyRemoval() that tests if the entity implements EntityWithPluginCollectionInterface. If so, iterate through collections and if there are plugin that implement RemovableDependentPluginInterfacedependencies::onCollectionDependencyRemoval(), invoke the method on that plugin so it can react by changing its configuration or removing itself from the collection
3. If onCollectionDependencyRemoval() returns TRUE, then ConfigEntityBase::onDependencyRemoval() will return TRUE, so the config entity's dependencies can be recalculated
4. Implement two use cases: 1) prevent image styles from being deleted if a module that provides any of its image effects is uninstalled and 2) prevent filter formats from being deleted if a media entity view mode used in a media embed filter is deleted
5. Add tests for two use cases

This approach kind of comes at the problem from the other way of the patch. The handling of the dependency removal is done completely in the plugin class, other than the entity invoking the plugin's onCollectionDependencyRemoval() method. The plugin collection and index ID of the plugin instance in the collection are provided as arguments along with the dependencies, so that the plugin instance can be removed from the collection as needed. This approach is kind of based on how Drupal\views\Entity\View\onDependencyRemoval() invokes DependentWithRemovalPluginInterface::onDependencyRemoval() for handler plugins, as well as FieldItemInterface::onDependencyRemoval(), which has the field definition object as well as the dependencies passed in.

There may need to be tests added to test the dependency removal handling generically. The tests in the MR are a kernel test and functional test specific to the the image style and media view mode use cases. The unit tests in #70 no longer apply to the solution.

Also bumping this to critical, because #3326447: Deleting a Media view mode should not result in the deletion of an entire text format combined with #3537624: Deleting filter formats may result in data loss can lead to content data loss. There is a case of this actually happening brought up in slack: https://drupal.slack.com/archives/C1BB308HH/p1755719629686569

Also, with this #3326447: Deleting a Media view mode should not result in the deletion of an entire text format is effectively a duplicate, so it perhaps can be closed with credits transferred here.

Changed the functional test for media to a kernel test, and added a unit test for more generic coverage.

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Rebased. Also tagging for Needs CR to document new interface if we go that way.

I like the new approach from #75. This is looking good. I left one question on the MR. I also like that we've got two use-cases in core addressed in the MR and there are likely to be more in core and contrib.

Addressed MR comments. Also added the CR: https://www.drupal.org/node/3549101

Test coverage looks good. The only thing I didn't like at first was the name, but it follows the established patterns set so not much to suggest beyond that?

The default setting gave me pause because cleaning up new dependency chains can be complex, but contrib can calculate those. I think the only open question is what happens if a method doesn't properly reset dependencies. I assume just what happens now which is it will remove everything. I don't think it's worth holding up over that since it's a much better situation and fixes a data loss bug.

Read through the CR and it seems clear too.

I also read through the IS and it seems up to date and the history, but focused more on the newer comments since the change in direction.

I've added an API change to consider before we merge this.

Made changes per #87 to the MR and updated IS and CR. This is ready again.

Just wanted to make sure to note #3326447: Deleting a Media view mode should not result in the deletion of an entire text format is a duplicate at this point and should be closed with credits transferred here.

I had a look, I think the feedback has been addressed. Name makes sense, left a comment on the MR about a discussion I had with @godotislate. I think this is ready again!

Committed 7c964ee and pushed to 11.x. Thanks!

Given we have new interfaces and an API I've only put this in 11.x so it'll be part of 11.3.0

Thanks! I took a pass at credit since I don't think it saved properly.

Ok try to look at the related issues soon.