Handle components provided by ComponentSources EXPLICITLY disappearing — enables deleting JS components that are in use

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 react to dependency removal, 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 #3517941: [META] Robust component instance error handling, 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 #3457504: XB field type: calculate all dependencies, store them, surface in new Component "Audit" operation 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 #3484682: Handle update and delete of Block `Component`s, plus react to dependency removal

Done for today, but next step is to confirm #3484682: Handle update and delete of Block `Component`s, plus react to dependency removal fixes the test

Plucked in the fix from #3484682: Handle update and delete of Block `Component`s, plus react to dependency removal this is ready for review now

Updated the issue summary with the current proposed resolution, after getting the 10,000 foot view from @larowlan.

I've reviewed and approved the MR. Afterwards, I've had a go at testing this on 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 if they were used. I don't think this is correct behaviour, as this would lead to polluting DB with unused components every time one enables and disables modules. I would expect only components that were used to create layouts, templates, patterns, etc to be retained in fallback mode.
   
3. Clicking on a "fallback" component in layout tree results in UI error message with "Try again" prompt and the following API response error. I don't expect to be able to edit the component, but I would expect to not see errors and perhaps some sort of boilerplate information about component - previous label, type, information that it is currently in fallback mode.
   
   Response error:

   {
       "message": "Drupal\\experience_builder\\Plugin\\ExperienceBuilder\\ComponentSource\\Fallback::clientModelToInput(): Argument #3 ($client_model) must be of type array, null given, called in \/web\/modules\/contrib\/experience_builder\/src\/Form\/ComponentInputsForm.php on line 95"
   }
   

   Drupal watchdog error:

   TypeError: Drupal\experience_builder\Plugin\ExperienceBuilder\ComponentSource\Fallback::clientModelToInput(): Argument #3 ($client_model) must be of type array, null given, called in /Users/felix.mazeikis/work/pbx-poc/web/modules/contrib/experience_builder/src/Form/ComponentInputsForm.php on line 95 in Drupal\experience_builder\Plugin\ExperienceBuilder\ComponentSource\Fallback->clientModelToInput() (line 82 of /Users/felix.mazeikis/work/pbx-poc/web/modules/contrib/experience_builder/src/Plugin/ExperienceBuilder/ComponentSource/Fallback.php)
   

4. If I enable module again, all the "fallback" Components are correctly reverted back to their original sources. They do however stay as status: false, requiring user to manually enable them. Not quite sure if we should automatically enable the Component, but I am leaning towards that approach.
5. Using SDC component with slots and placing other components inside of it, then disabling the module that provides top level SDC results in errors when loading XB.
   
   browser console error log 1
   browser console error log 2

I most of the above be addressed in BE part of XB for initial UX improvement and maybe UI icon could be added to identify problematic fallback components at a later date.

Great pickups @f.mazeikis - the naming I can fix easily enough
I think we'll need #3457504: XB field type: calculate all dependencies, store them, surface in new Component "Audit" operation 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: #3457504: XB field type: calculate all dependencies, store them, surface in new Component "Audit" operation is on the verge of landing, so +1 for waiting for that.

#3457504: XB field type: calculate all dependencies, store them, surface in new Component "Audit" operation 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 #3457504: XB field type: calculate all dependencies, store them, surface in new Component "Audit" operation 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.

#31:

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:), this issue does not address this.

Testing this MR would involve uninstalling modules providing Block Plugins/SDCs or 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 to me seems to be in a direct conflict with the issue title and #31. 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 → the JS component config entity disappearing 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 #3470422: Handle components provided by ComponentSources IMPLICITLY disappearing: gracefulness when developing SDCs: SDCs may appear/disappear from one request to the next and #3522164: Handle update and delete of SingleDirectoryComponent `Component`s, plus missing config dependencies 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 #3470422: Handle components provided by ComponentSources IMPLICITLY disappearing: gracefulness when developing SDCs: SDCs may appear/disappear from one request to the next
- 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 #3470422: Handle components provided by ComponentSources IMPLICITLY disappearing: gracefulness when developing SDCs: SDCs may appear/disappear from one request to the next 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 #3523505: [PP-1] [Needs design] Implement design treatment for fallback component 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 😊

~3 weeks of progress since I last looked at this in #20 and raised some major concerns. Curious to see what I'll find :D

Very nice work here, @larowlan and @f.mazeikis!

This is essentially a sibling to #3517941: [META] Robust component instance error handling — but taking care of a different scenario: not broken logic inside a component, nor invalid inputs passed into a component instance, but a component disappearing from a source and XB getting informed about it. (By contrast, #3470422: Handle components provided by ComponentSources IMPLICITLY disappearing: gracefulness when developing SDCs: SDCs may appear/disappear from one request to the next is about a component disappearing from a source without XB getting informed about it.)

Been sick, so didn't get to do in #38 what I said I'd do 🙈

I see that @larowlan opened a follow-up for conveying through the UI which component instances are actually fallbacks (thanks @mglaman for asking critical questions!): #3523505: [PP-1] [Needs design] Implement design treatment for fallback component — linking.

🐛 Only fixes the canonical route; editing in XB still broken 😅

On a fresh install, I can still reproduce exactly the failure that @f.mazeikis reported on #35, which means that despite the test coverage of this MR proving it works, it only does so when viewing the Node or Page at its canonical page; it still crashes when editing it through XB.

This crash appears to be happening in the client's ComponentOverlay and/or SlotOverlay.

@f.mazeikis proposed to do that in a FE follow-up, but that doesn't yet exist?

🤔 Deep review: 3 concerns

I still have three remaining concerns:

1. The first is tying back to #20. The current MR does solve it for the sdc ComponentSource (👍 — technically addresses #20), but does so in a way that is SDC-specific. What about other \Drupal\experience_builder\ComponentSource\ComponentSourceWithSlotsInterface component source plugins in the future? 😅
   The way it's currently implemented will require significant duplication (and likely inconsistency) across component sources.

   I think it's possible to solve generically: why not move slot_definitions out of type: experience_builder.component_source_settings.(sdc|fallback) and into a new fallback_metadata.slot_definitions under type: experience_builder.component.* itself? That could then contain all the "best-effort fallback" metadata.

2. The second is about how the fallback rendering is implemented — for future maintainability and cohesion with #3470422, I think that ideally this would be part of \Drupal\experience_builder\Element\RenderSafeComponentContainer and the way that is used by ComponentTreeHydrated::renderify(). IF there's a reason not to do that, then I think at minimum we need docs explaining where the boundaries lie.

   Because this MR is de facto introducing a new kind of robustness: the robustness of ensuring that child trees of components with slots that were properly uninstalled do not result in their contained subtrees disappearing on content authors, to avoid perceived data loss. By contrast, #3470422 was about ensuring invalid inputs and/or broken logic in components not resulting in crashes.

3. Currently, component instances from the sdc and js component sources can have their underlying components (SDC plugins or JavaScriptComponent config entities) uninstalled and they'll then automatically switch to the fallback plugin. They'll continue to render partially (i.e. subtrees in slots will be visible, nothing else will be, nor could be).

   But AFAICT (although I cannot test this due to the bug reported by @f.mazeikis in #35), editing the component tree will lose the stored explicit inputs for the component instance (aka data loss 👻), because:

   • \Drupal\experience_builder\Plugin\ExperienceBuilder\ComponentSource\Fallback::requiresExplicitInput() returns FALSE
   • \Drupal\experience_builder\Plugin\ExperienceBuilder\ComponentSource\Fallback::clientModelToInput() returns []
   • … et cetera

   … which means that the Now perform an action that causes the component to recover from the fallback. test coverage at the end of ComponentSourceTestBase::testFallback() is essentially an illusion: it only works because the test assumes nothing has happened to the component tree while the Fallback source was active.

Sorry for being a party pooper 😔

If we're to proceed with a fallback source, I'd expect it to be explicitly documented in docs/components.md — both which sources are eligible, but also what can be expected of the fallback in terms of:

• rendering component instances relying on it
• data loss impact on the component instance's props & slots if edits happen once it's using the fallback source
• etc.

BTW, updated the title here to contrast it more clearly with #3470422: Handle components provided by ComponentSources IMPLICITLY disappearing: gracefulness when developing SDCs: SDCs may appear/disappear from one request to the next's title & scope.

#35 (the front end editing issue) is #3524047: Component/Slot overlay is brittle if HTML comments are missing - I've refactored how we render the fallback component to make sure the Twig visitor outputs the comments the FE needs. I've also added a test for it. So that fixes the issues we were seeing but highlights that this is particularly rigid w.r.t. how component slots are rendered. We can't know that _every_ source plugin will print the slots by name in twig (ie {{ some_slot_name }} in order for the twig node visitor to wrap that with the comments. So that follow up is more about making the FE more robust rather than fixing a new issue added here.

#48.1 I've made this adjustment. Was pretty clean/simple in the end.

#48.2

I think that ideally this would be part of \Drupal\experience_builder\Element\RenderSafeComponentContainer

We are already getting wrapped in one of those for free - thanks to how ::renderify works - this is just another component. The changes I had to make to resolve #35 also make it further evident that keeping the two separate are probably best. If we added generic slot printing fallback to the RenderSafeComponentContainer it would have to mimic the same stuff we're doing here with explicitly printing slots by name, but also in try-catch because that component cannot fail to render. I think it is worth exploring that in a new issue - I've opened #3524052: [PP-1] Explore adding support for fallback rendering of slots to RenderSafeComponentContainer to see if the changes I've made to Fallback::renderComponent and ::setSlots could move to a generic render element. I am proposing doing it in a followup because the scope of this MR is already large. If you feel strongly about it I can close that and do it here.

#48.3In theory we can also copy over fallback metadata on prop definitions to the fallback plugin but that assumes we are only dealing with SDC/Code components. What do we do for blocks where the form isn't based off the generated UX base class? I think we should get some input from @laurii here - what is the intended behavior. I don't think we can continue to show the same component inputs form (because how we handle SDC vs Blocks isn't compatible) - we could probably just store the values in hidden inputs so at least the values are retained. But, is it safe to keep the values around in case the scenario that caused the component to be remove is reversed? What happens if the deleted JS component is added back but with different props? I think we should confirm the intended behavior before jumping to an implementation. As a result assigning to @laurii

Added docs too

i.e. subtrees in slots will be visible, nothing else will be, nor could be

Per #36, subtrees should not be visible in the preview or in the live site when the parent component is missing. Maybe you're describing that they are visible in the layers?

I don't think we can continue to show the same component inputs form (because how we handle SDC vs Blocks isn't compatible) - we could probably just store the values in hidden inputs so at least the values are retained.

Would it be possible to show the values in disabled fields? This way it would be possible to copy and paste the content elsewhere. I understand this may not be rendered exactly like the real component form would be but that seems fine since this is a fallback which is not really intended for authoring, but just to prevent you from losing data.

But, is it safe to keep the values around in case the scenario that caused the component to be remove is reversed? What happens if the deleted JS component is added back but with different props?

Wouldn't this fall under the scenario where props have been changed (i.e. #3509115: [META] Plan to allow editing props and slots for in-use code components)? I think in the absence of that, it would be fine if we cannot render the component until the shape is in a valid state, i.e. no missing required props. When content with that component is being edited, it would have to be brought to validity.

This is not all that different from how things work today in Drupal. You can run into this without even doing anything too crazy because if you add a required field to an existing content type with content, it makes all of the content invalid. We have no safe guards against that today.

We may need a sibling issue to #3501708: Prove that it *will* be possible to apply block settings update paths (assuming #3521221 in core) to stored XB component trees in config/content to document how one could delete all instances of a component when you really want to get rid of all of the content.

Would it be possible to show the values in disabled fields?

Without the Component Source Plugin, we lose the metadata about the props/block settings. We would need to rely on data stored in XB field on a node and do best our attempt go generate input fields matching value, expression and source type.
This is certainly possible, but it feels like somewhat a separate deep dive that warrants it's own issue.

With that being said, I've tried testing Wim's described scenario of data loss:

1. Enable xb_test_sdc module
2. Place couple of components on a node - Heading and XB test SDC with props and slots in laytout
3. Edit heading of XB test SDC with props and slots
4. Publish the changes
5. field_xb_demo_inpus store the following:
   {"6c280bf1-cc58-4c34-af73-7ea36d6dc1eb": {"heading": {"value": "There goes my test", "expression": "ℹ︎string␟value", "sourceType": "static:field_item:string"}}, "8cb6fa57-08df-4508-b439-cc88d9301999": {"text": {"value": "A heading element", "expression": "ℹ︎string␟value", "sourceType": "static:field_item:string"}, "style": {"value": "primary", "expression": "ℹ︎list_string␟value", "sourceType": "static:field_item:list_string", "sourceTypeSettings": {"storage": {"allowed_values": [{"label": "primary", "value": "primary"}, {"label": "secondary", "value": "secondary"}]}}}, "element": {"value": "h1", "expression": "ℹ︎list_string␟value", "sourceType": "static:field_item:list_string", "sourceTypeSettings": {"storage": {"allowed_values": [{"label": "div", "value": "div"}, {"label": "h1", "value": "h1"}, {"label": "h2", "value": "h2"}, {"label": "h3", "value": "h3"}, {"label": "h4", "value": "h4"}, {"label": "h5", "value": "h5"}, {"label": "h6", "value": "h6"}]}}}}}
6. Disable xb_test_sdc module and clear cache
7. Edit the node in question - update Heading component
8. Publish the changes
9. field_xb_demo_inpus value is updated to the following:
   {"8cb6fa57-08df-4508-b439-cc88d9301999": {"text": {"value": "A heading test", "expression": "ℹ︎string␟value", "sourceType": "static:field_item:string"}, "style": {"value": "primary", "expression": "ℹ︎list_string␟value", "sourceType": "static:field_item:list_string", "sourceTypeSettings": {"storage": {"allowed_values": [{"label": "primary", "value": "primary"}, {"label": "secondary", "value": "secondary"}]}}}, "element": {"value": "h1", "expression": "ℹ︎list_string␟value", "sourceType": "static:field_item:list_string", "sourceTypeSettings": {"storage": {"allowed_values": [{"label": "div", "value": "div"}, {"label": "h1", "value": "h1"}, {"label": "h2", "value": "h2"}, {"label": "h3", "value": "h3"}, {"label": "h4", "value": "h4"}, {"label": "h5", "value": "h5"}, {"label": "h6", "value": "h6"}]}}}}}
   Notice how "6c280bf1-cc58-4c34-af73-7ea36d6dc1eb": {"heading": {"value": "There goes my test", "expression": "ℹ︎string␟value", "sourceType": "static:field_item:string"}} is removed from the beginning of JSON blob.
10. Enable xb_test_sdc module
11. Attempt opening XB editor for node in testing
12. Observe the following exception when trying to view the node:
    Drupal\experience_builder\MissingComponentInputsException: No props sources stored for 6c280bf1-cc58-4c34-af73-7ea36d6dc1eb. Caused by either incorrect logic or `inputs` being out of sync with `tree`. in Drupal\experience_builder\Plugin\DataType\ComponentInputs->getValues() (line 174 of /Users/felix.mazeikis/work/pbx-poc/web/modules/contrib/experience_builder/src/Plugin/DataType/ComponentInputs.php).
13. Attempting to open same node in XB Editor results in a crash that prevents XB UI from fully loading, storing same kind of exception in log reports:
    Drupal\experience_builder\MissingComponentInputsException: No props sources stored for 6c280bf1-cc58-4c34-af73-7ea36d6dc1eb. Caused by either incorrect logic or `inputs` being out of sync with `tree`. in Drupal\experience_builder\Plugin\DataType\ComponentInputs->getValues() (line 174 of /Users/felix.mazeikis/work/pbx-poc/web/modules/contrib/experience_builder/src/Plugin/DataType/ComponentInputs.php).

I was about to write a suggestion that we should move "data loss and recovery" into a separate issue, but after managing to brick my XB/Drupal so easily I am no longer sure it's a good idea. Although one could argue that this is not the fault of this MR, but rather that rendering side of things should handle exception thrown by
Drupal\experience_builder\Plugin\DataType\ComponentInputs more gracefully.

Without the Component Source Plugin, we lose the metadata about the props/block settings. We would need to rely on data stored in XB field on a node and do best our attempt go generate input fields matching value, expression and source type.
This is certainly possible, but it feels like somewhat a separate deep dive that warrants it's own issue.

I'm totally fine if we want to move this to a follow-up. Tagging just so that it doesn't get forgotten!

During todays XB team scrum there was agreement that we should try to split the part of this issue MR that can be merged. Changes related to permissions and audit can be merged separately from the rest of the Fallback Source Plugin mechanism related work.
I've tried to do that in 3519168-subset-mr.

Thanks for #55 @f.mazeikis and #54 @laurii

I think there's perhaps an incremental step here that prevents the bricked state.

I'll timebox an hour working on that.

I have a solution

here's the component edit form when in fallback state

And here's the recovered state

Will add some tests

#60 works for published content because we do client to server transformations during the entity conversion. This means any client-side representations of the model have been removed and we're back to the raw server side version.
But it doesn't work if the data is in the autosave store - opened #3524298: Auto-save data should update itself when the Fallback plugin is (de)activated for that because the scope here is already big enough and that is a big change on its own. Added a skipped test for this case and a passing test for the published case. In the auto save case, we're working with client data that is source plugin specific and needs access to the prop definitions. If we do the client to server transformation before writing to auto save, we don't need the prop field definitions around in order to re-do it.

Also added #3524299: [PP-1] Improve the UX of the fallback component input form to improve the initial version of the inputs form

Removing the follow up tag

New changes are green

#60 is a great workaround to this problem 👏 Definitely remove some urgency from #3524299: [PP-1] Improve the UX of the fallback component input form.

Could we update the text to: "Component has been deleted. Copy values to new component." to be a bit more concise?

Could we update the text to: "Component has been deleted. Copy values to new component." to be a bit more concise?

Done 👌

@larowlan in #52: very nice work! 👏🤩 Glad to see #48.1 was easy, #48.2 has a stronger reason than I realized to be the way it is (thanks for the follow-up #3524052) and #48.3 was solved subsequently 👍

@lauriii in #54: The fallback rendering actually was rendering the subtrees on both the live site and in the preview. That's what the Fallback source was literally doing.

You can run into this without even doing anything too crazy because if you add a required field to an existing content type with content, it makes all of the content invalid. We have no safe guards against that today.

This is not a fair comparison IMHO. If an SDC/code component adds a new required prop, the component will refuse/fail to render if it is not populated. Content entities in Drupal that don't have a required field just won't render that field, and subsequent edits will require a value to be specified.
Note: supporting new required props on SDCs/code components is actually the easiest problem to solve. Explanation at #3509115-6: [META] Plan to allow editing props and slots for in-use code components.

document how one could delete all instances of a component when you really want to get rid of all of the content.

So, you're proposing a new issue for identifying and surfacing dangling/dead component subtrees? If so: +1, and no work should happen on it until after #3468272: Store the ComponentTreeStructure field property one row per component instance + #3523842: Spike: Explore storing a hash lookup of the ComponentInputs field property: one hash per component instance are done, because they would significantly simplify the logic for finding+listing those dead subtrees, and for bulk-deleting them.

Tagging for follow-up.

@larowlan in #61 Woah, #3524298: Auto-save data should update itself when the Fallback plugin is (de)activated would be a massive change indeed.

Re-reviewing in detail. Hopeful to merge 🤓🤞

The fallback rendering actually was rendering the subtrees on both the live site and in the preview. That's what the Fallback source was literally doing.

Is this still the behavior? If it is, we need at least a follow-up to revisit that.

This is not a fair comparison IMHO. If an SDC/code component adds a new required prop, the component will refuse/fail to render if it is not populated. Content entities in Drupal that don't have a required field just won't render that field, and subsequent edits will require a value to be specified.

The point was that core is not protecting the data from becoming invalid; instead it deals with it on the runtime. This is probably something we will have to do as well when it comes to changing props.

FWIW, you will run into that exact same problem if you use SDCs for rendering the content. Because of this, using required isn't common for components. For example, UI Suite themes are not using required fields at all.

So, you're proposing a new issue for identifying and surfacing dangling/dead component subtrees?

Yep, +1 for a follow-up on that. And +1 for postponing that until we have made decisions around what the data model is going to be.

This MR elevated the importance of #3519891: Constrain slot names allowed by XB in Components (and in its component tree).

@penyaskito seems to have struggled to understand the same bits of the MR as me — good reassurance — so I got his +1s for some of the comment/docs additions 😊🙏

The subtler/trickier bits of the Fallback plugin made me discover a bug in the block component source: #3524399: `BlockComponent::validateComponentInput()` allows garbage to pile up.

Epic work here, everyone — especially @larowlan & @f.mazeikis!

I think the essence of this issue is best described by

• docs/components.md#3.4 Fallback for architecture and end-user impact
• the ComponentInterface::setSource() docblock for details wrt when "changing the source" is appropriate

Cross-posted with @lauriii's #66.

You're right, @lauriii, that behavior was changed 👍 But the docs still said that was the behavior until a few seconds ago. And @penyaskito also got confused by the code — so docs in the relevant code have been clarified too 👍

Yep, +1 for a follow-up on that. And +1 for postponing that until we have made decisions around what the data model is going to be.

Done: #3524406: [later phase] [Needs design] [PP-1] Provide API for finding and UI for surfacing dangling/dead component subtrees — aka garbage clean-up.

Next up:

• #3484682: Handle update and delete of Block `Component`s, plus react to dependency removal
• #3470422: Handle components provided by ComponentSources IMPLICITLY disappearing: gracefulness when developing SDCs: SDCs may appear/disappear from one request to the next

The fallback component will not, I assume, have any props -- but it will render child slots, presumably to keep as much of the layout intact as possible.

was also still wrong in the issue summary — see #69 and earlier for why that's not the case.