Component::onDependencyRemoval() should be an uninstall validator

It's @larowlan who led us in this direction — see 🐛 Cannot delete JS components due to component depending on them Active for the history.

I don't see any discussion of an uninstall validator in that issue. On a quick look, it looks like it's half dealing with components/plugins going missing (arbitrarily removed during development or via a module update) and half with module uninstall - but those are two very different scenarios because one is developer error and one is a site builder operation.

You're right that plugins going missing is a developer error. This is also what @longwave called out in #3519168-14: Handle components provided by ComponentSources EXPLICITLY disappearing — enables deleting JS components that are in use → .

So, how did we go from that to seemingly conflating it with site builder operations?

I think because that issue was originally scoped only to disappearing JavaScriptComponent config entities (original title: ). See #3519168-12: Handle components provided by ComponentSources EXPLICITLY disappearing — enables deleting JS components that are in use → , in which I retitled, and the MR comments I posted just before that.

From your POV, that's more of a "site builder operation", because no modules were updated, installed or uninstalled ("zero composer" if you will).

From quickly skimming that issue and some of its links, I pushed back against either a js Component Source-specific solution and against an sdc-specific solution (in #3519168-48: Handle components provided by ComponentSources EXPLICITLY disappearing — enables deleting JS components that are in use → ).

That's how we eventually ended up with the current \Drupal\experience_builder\Entity\Component::onDependencyRemoval(). I'm still not entirely convinced that what we have in 0.x is wrong though. I suspect a crystal-clear concrete example would help most? 🤓

I think we probably need both, because as you pointed out there's a site builder operation and a module operation.

Thinking of the site builder operation, we've had lots of reports over the years of 'I removed this field and my (view|some other thing) was removed' so onDependencyRemoval is for that. Lauri made the original report in that he wanted to remove a Code component but was prevented from doing so because the code component was depended on by the component config entity.

But you're right, an uninstall validator should hard prevent someone from uninstalling a module and footgunning their content into the fallback plugin.

So I think that means we should limit Component::onDependencyRemoval to config dependencies and not module and theme ones - and cover those two with an uninstall validator.

I can take that on.

I think

limit Component::onDependencyRemoval to config dependencies and not module and theme ones - and cover those two with an uninstall validator.

sounds reasonable. But I'll only know for sure once the full impact of that change is visible.

AFAICT this does not need to be a beta blocker, because it won't impact data storage aka 🌱 [META] Production-ready data storage Active .

WRT full impact — there's a few major consequences I expect:

1. ContentTemplateDependencyTest::testRemoveFieldTypeProviderModule() will change drastically ⇒ it'll be much more painful
2. ✨ Implement `::onDependencyRemoval()` for `Pattern` and `PageRegion` config entities: when a module providing a field type is uninstalled, replace it with the default StaticPropSource Active to get marked
3. etc.

I think the commonality between those is: it's hard to find the specific component instance that depends on some dependency going missing (whether it's a field type or something else). IIRC that's why @lauriii advocated for this approach. But, I wish we had documented the rationale better 😞 That's on me. Sorry.
(The best I can find is @longwave's RTBC at #3518336-43: When a field instance used by a `ContenTemplate` is deleted, or a field-type providing module is uninstalled, replace affected `inputs` with default `StaticPropSource` → after @lauriii's direction at #3518336-11: When a field instance used by a `ContenTemplate` is deleted, or a field-type providing module is uninstalled, replace affected `inputs` with default `StaticPropSource` → — but AFAICT @phenaproxima interpreted @lauriii's request more broadly.)

Got an MR up showing the hybrid approach, uninstall validator for modules/themes
Config dependency for config dependencies, during testing found that we weren't adding static prop source dependencies to the config entity so added that - this gave me a nice way to test fallback with media types.

Will fix any fails tomorrow but I think the approach is ready for review

Reviewed this together with @catch.

The behavior in HEAD is that a code component (JavaScriptComponent) that is in use in e.g. 1 article and 1 page can still be deleted. It requires a little bit of shenanigans though: you have to open a content entity in XB that does not contain an instance of that code component!

If such an instance is present, you do get a nice error message/confirmation dialog:

On all other content entities (i.e. any with an XB component tree but without this particular code component), the deletion will proceed unceremoniously 😅 ⇒ It's far too easy to shoot yourself in the foot right now!

The changes in this MR remove the "shoot yourself in the foot" case for:

• uninstalling modules providing block plugins
• uninstalling themes/modules providing SDCs

👍

BUT concerns remain regarding code components — because this MR doesn't change anything (intentionally!) for those.

@catch and I agreed that:

1. A confirmation dialog like the above should ALWAYS appear
2. It should inform the user of how many entities/revisions would be impacted
3. It should inform the user that if they proceed all existing content will look broken.

IOW: allow it only for developers who really know what they're doing, but present the consequences as explicitly as possible.

This strikes the middle ground of desire for reliability+consistency (@catch and I) vs pragmaticness when developing code components (@lauriii's concern).

In the future, if Drupal core gets a "developer mode", we could adjust the behavior based on that — see ✨ Add a Production/Development Toggle To Core Needs work .

After some more brainstorming with @catch, we think we thought of an alternative approach that actually is better still:

1. add a soft_deleted: true|false config entity property to JavaScriptComponent, which defaults to false
2. make the delete operation either:
   1. : actually delete (as is the case today)
   2. : set soft_deleted: true (🆕)
3. make XB in all its API responses pretend this config entity does not exist
4. … then just respect all config dependency management as per usual

P.S.: an alternative/more refined approach could be that we make JavaScriptComponent implement VersionedConfigEntityInterface (new since ~2 weeks) to keep the underlying config entity around, and only allow 2 versions: whichever one is the active one, plus a special deleted/trashed version that gets treated everywhere in XB as if it doesn't exist. That way, we would not have to delete

After #10 + #11, I also reviewed this.

This looks great! 😄

Pushed a single commit to try to clarify the thing I found most confusing, plus identified 2 renames that IMHO would clarify the situation a lot 🤞

Thanks will look at review feedback and #11 later today

Clarification: #10 and #11 are out of scope here, just conclusions @catch and I reached while reviewing 😊

Ok, addressed the review feedback, this is ready for another review.

Follow-up created for #10 + #11: 📌 Only allow deleting Code Components if there's 0 usages Active .

Placed the block in 1 content entity and in a region. If I then try to install the Views module (which provides that block):

and then if you click the :

Note that no message appears for the PageRegion config entity that also contains an instance of this component — because that's handled by config dependencies. 👍

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