Cannot delete JS components due to component depending on them

mglaman → credited larowlan → .

mglaman → credited laurii → .

laurii, larowlan were part of the debugging crew

Ah the curse of a unit test with mocking :) https://git.drupalcode.org/project/experience_builder/-/merge_requests/7...

I've written a test here
And I think this behavior might be by design.

If we delete the JS component then the component is deleted too.
And we don't want that if the component has usages, so that opens a can of worms about finding usages.
Do we want to tackle that here too or more likely we'll have to create issues to handle those pieces and postpone this.

I've pushed up a test, but because of the 'are we using this component question - I don't think this one is a simple fix .

Linking a related entity usage system - but we could also implement something like layout builder does for inline block usage.

Assigning to Wim for direction on how we should go here.
I think trying to calculate the usage in real-time in a deletion check is arduous.
I think something like layout builder's inline block usage where we store usage in a table that we write when a component tree is saved AND that can be re-calculated (ala entity usage module) is probably more robust.

And we don't want that if the component has usages, so that opens a can of worms about finding usages.
Do we want to tackle that here too or more likely we'll have to create issues to handle those pieces and postpone this.

I think trying to calculate the usage in real-time in a deletion check is arduous.

💯 to all this! 🤓

🏓 Detailed thoughts: https://git.drupalcode.org/project/experience_builder/-/merge_requests/9... — tl;dr: implement Component::onDependencyRemoval(). See #3484682-12: Handle update and delete of Block `Component`s, plus missing config dependencies → , where @longwave is hitting on the exact same problem with a different ComponentSource plugin!

Added a fallback source plugin that can step in and continue to render children when a JS component is removed if any instance exist.

Lots of cleverness here, but also some huge long-term impact decisions!

This is sort of a counterpart to 🌱 [META] Robust component instance error handling Active , at least if I interpreted everything correctly.

I'd like @longwave to take a very critical look at this, because this has just shifted to a critical data integrity issue. 😅

I think this is the right way forward instead of trying to figure do usage tracking as historically we have never succeeded at proper usage tracking, and it also will help with other cases where e.g. SDCs or block plugins suddenly vanish because a module or theme update dropped them.

Moving to NW for the fundamental question: does this need to be a separate interface, or should it just be something that all component source plugins are forced to support?

e.g. SDCs or block plugins suddenly vanish because a module or theme update dropped them.

This.

@longwave: I'm curious about your thoughts on this review remark by me. Also, AFAICT this can't work for vanishing SDCs — what am I missing? 🙈

@longwave in #14:

I think this is the right way forward instead of trying to figure do usage tracking as historically we have never succeeded at proper usage tracking,

Two things:

1. At DDD, I talked to Alex Pott, and he's been making https://www.drupal.org/project/entity_usage → drastically better. We've never succeeded in core, but that's now starting to look better :)
2. For XB purposes, the usage tracking problem is much more narrowly defined, and 📌 Calculate field and component dependencies on save and store them in an easy to retrieve format Active is tackling it. Imagine/assume for a second that component usage tracking is a solved problem. Then … what would change here in your perspective?

Moving to NW for the fundamental question: does this need to be a separate interface, or should it just be something that all component source plugins are forced to support?

Good question, I guess an SDC component could depend on a theme/module.

thoughts @wim leers?

Fixed tests

I guess an SDC component could depend on a theme/module.

WDYM? The Component config entity's provider already gets set to the the module/theme that provided the SDC, see \Drupal\experience_builder\Plugin\ExperienceBuilder\ComponentSource\SingleDirectoryComponent::createConfigEntity().

The challenge is: what if a module/theme is updated and the SDC is gone? How then would we figure out the slots it had?

Will poke at #20 today

Pushed changes as follows:

* Store slot definitions for SDC in the component config so we can handle fallback for them
* Remove the new interface and add the method to the base interface
* Make SDC and blocks implement it
* Move the test to the base component test so we have coverage for all.

Blocks are currently failing which I suspect is 📌 Handle update and delete of Block component config entities Active

Done for today, but next step is to confirm 📌 Handle update and delete of Block component config entities Active fixes the test

Plucked in the fix from 📌 Handle update and delete of Block component config entities Active this is ready for review now

Tested in my local, a few things stood out:

1. Upon deletion, fallback component label reverts to generic `Component` in the sidebar. I wonder if we should add something explicit, to let user know it's a "fallback"/"broken" component?
   
   
2. Disabling sdc test module resulted in XB retaining ALL of the Component entities associated with SDC's provided by the module, regardless of whether they were in use anywhere. Not sure if this is correct behaviour, as this would lead to polluting DB with unused components. I would expect only components that were used to create layouts in some way to be retained.
   

Great pickups @f.mazeikis - the naming I can fix easily enough
I think we'll need 📌 Calculate field and component dependencies on save and store them in an easy to retrieve format Active to be able to handle straight up deleting - which I agree is a better approach.
@wim leers - should we postpone this issue on that or should we add a todo pointing to it?

The `Component` rendered in the side bar is because once we disable this component (which we do here) it no longer shows in the list controller.

Disabling it prevents it from being reused again, but it does prevent it from showing the old label in the layers.

Not sure what the best approach is here - Component is the fallback the frontend uses https://git.drupalcode.org/project/experience_builder/-/blob/0.x/ui/src/... - defering to @laurii on that one.

#27: 🌱 [META] Production-ready data storage Active is on the verge of landing, so +1 for waiting for that.

📌 Calculate field and component dependencies on save and store them in an easy to retrieve format Active is in!

@lauriii

In situation when a Component is used in a Layout on a Node and then the JS Component, Block Plugin or SDC is removed from the Drupal, what is the expected behaviour for such "removed" Components?
Currently this MR assigns "Fallback" plugin as source for such components, allowing us to have graceful error handling when rendering such pages and interacting with tree structures. We should be able to "recover" data completely, if JS Component, Plugin or SDC is re-introduced into Drupal.
There are some questions about UI/UX side of things, specifically:

1. Is it correct to assume that it is removed from the library listing of available Components?
2. Is it correct to assume that it should be rendered as empty div with a placeholder UUID and identifiers for preview purposes?
3. Should such component has slots with other Component instances inside of it - should those components still be rendered in preview and in live versions of the page?
4. Should "removed" Component still be visible in the Layers section of the sidebar?
5. If so, is it ok to use generic "Component" label for all such components?
6. If "removed" Components are visible in the Layers section of the sidebar, should it be possible for users to interact with them? What kind of interactions user is allowed to do? (Moving them around in Layers? Editing contents of Slots? Deleting them from Layout?)
7. If such components are visible - do we need designs for icons/messages/labels that inform users of "removed"/"fallback" status of such Components?

For ease of review for @laurii, here's how the current patch responds to @f.mazeikis's questions 1-7 above

1. Currently it is
2. Currently it is
3. Currently it renders any child slots in order each as a div
4. Currently they are visible
5. Currently they are named 'Component' only but we could easily make that 'Disabled Component'
6. Currently all of these interactions are possible
7. Excellent question 🙏 currently there is no affordance

Rebased on top of 0.x now that 📌 Calculate field and component dependencies on save and store them in an easy to retrieve format Active is in, made a couple of improvements to the audit service while I was touching the code.

Works real nice 😎 - we only do the on dependency removal dance if the item is in use and just let stuff get deleted if its not in use.

Added test coverage that shows that unused things just get deleted.

Still needs product feedback from @laurii but is ready for another review

Re-tested it and this feels much closer. Components are deleted based on usage, there's even warning during module uninstall via UI - awesome.

During testing this locally I've still encountered the problem when removing module that provided Component with slots and this Component was used on a Node with slot populated by other Component.
This leads causes XB UI crash with error message "An unexpected error has occurred while rendering preview." prompting to "Try again", which re-triggers same error.

I guess this is a FE side thing and we probably can spin that into separate follow up issue.

1. Yes 👍
2. Yes, I'll work with Callum on a design and we can then define based on that what would be needed 👍
3. They should not be visible in the preview or in the live version of the page. However, they should be visible in the layers. It is okay to implement this in a follow-up issue.
4. Yes 👍
5. Yes, I think we would want to somehow indicate that those components are missing / broken. I'll ask Callum to design this.
6. The primary use case that we'd want to enable is being able to drag components from the slots outside of them. I'd imagine them working largely the same as other layers with the exception that they are not visible in the preview?
7. Callum will design these and provide for you

I wasn't able to test this manually though. I tested this by deleting the experience_builder:two_column when I had that component placed on a page. I got following error:
The \u0022experience_builder:two_column\u0022 plugin does not exist. Valid plugin IDs for Drupal\\experience_builder\\Plugin\\ComponentPluginManager are: olivero:teaser, experience_builder:my-section, experience_builder:image, experience_builder:shoe_button, experience_builder:shoe_badge, experience_builder:shoe_icon, experience_builder:shoe_details, experience_builder:video, experience_builder:heading, experience_builder:shoe_tab_panel, experience_builder:shoe_tab, experience_builder:shoe_tab_group, experience_builder:one_column, experience_builder:druplicon, experience_builder:my-hero, navigation:title, navigation:badge, navigation:toolbar-button

@lauriii If I understand correctly, you manually (via Drush?) attempted to delete a Component entity and hit the error? If so, this is not what this MR is addressing. This MR addresses situations where due to module uninstall, code change or JS Component removal we encounter Component with missing/invalid Component Sources.
While the problem you're describing is valid and we should introduce some sort of safeguard against deleting Components that are in-use (made possible by #3457504: 📌 Calculate field and component dependencies on save and store them in an easy to retrieve format Active ), this issue does not address this.
Testing this MR would involve uninstalling modules providing Block Plugins/SDCs, deleting Code component entities (js_component) that are in-use on Nodes by Component entities. Deleting Component entities directly is outside of scope here.

Will review this tomorrow 👍 Meanwhile, it'd be great to have @f.mazeikis' question answered — funny enough I was forced to ask the same at #3517941-12: [META] Robust component instance error handling → , independently of this! 😅

I tried to delete a SDC that was placed on a page (the experience_builder:two_column component). So I placed the component on a page, deleted the component from the code base, cleared caches, and reloaded XB.

I have no idea how to read this issue because #37 🐛 Cannot delete JS components due to component depending on them Active to me seems to be in a direct conflict with the issue title and #31 🐛 Cannot delete JS components due to component depending on them Active . I'm probably missing some nuance which is why I've completely misinterpreted this issue. 😅

It would be great if we could update the issue summary but besides that, I'm always open to jumping on a quick call to get us on the same page again.

"components disappearing" ==

• block → the block plugin disappearing from a module
• sdc → the SDC disappearing from a module
• js → should not be possible thanks to config dependencies

So I placed the component on a page, deleted the component from the code base, cleared caches, and reloaded XB.

Hm, that seems fine then. That's equivalent to the SDC disappearing.

BTW, this might end up fixing both 💬 ComponentNotFoundException: Unable to find component Active and 📌 Handle update and delete of SingleDirectoryComponent `Component`s, plus missing config dependencies Active too!

From description of this I think we need "on the fly" swap Component Source with Fallback if we detect that Component Source is no longer `valid`. I'll take a stab at it.

Didn't get very far. I've pushed what I was thinking off, but it's not a complete solution yet. Preview rendering breaks, something to do with HidratedTrees and mismatch between saved inputs and Falllback, it seems. Block and JsComponent source plugins also miss logic in their respective ::valid() methods. We would need tests for this as well.

I was thinking that @lauriii testing revealed, that sometimes Source Plugins can become invalid without triggering any of our Logic, so maybe we should try to catch that at the time of instantiating/accessing Source Plugins and use Fallback source plugin in such situations.

Not even sure if this is of any use - feel free to undo my commit entirely if you think this is wrong approach, @larowlan.

I think we should defer #40 through #44 to 💬 ComponentNotFoundException: Unable to find component Active
- the scope here is already large enough

This issue was specifically about dealing with config dependency removals (the original STR were creating a Code Component, then adding it to the library but _not_ being able to delete it because it was *in use*)

I've pushed @f.mazeikis's commits to a branch in 💬 ComponentNotFoundException: Unable to find component Active with all of the commits from here squashed so rebasing will be easier once this is in.

I've then rewound this branch to the commits before @f.mazeikis started addressing #40 onwards.

Added 📌 [PP-1] [Needs design] Implement design treatment for fallback component Active to implement the design changes (once ready) eluded to in #36

Putting this back to needs review in its current state.

Exciting! Finally reviewing this 😊