Overview
Block components have config entities for each plugin created automatically on cache clear. However, block plugins that cease to exist do not have components deleted, nor are existing block component config entities updated once they have been created.
Proposed resolution
- Delete block component config entities once the plugin (or derivative) no longer exists.
- Decide when and how to update block component config entities when changes occur.
- Avoid the
Componentconfig entity for a block plugin that is getting uninstalled from being deleted when it has >0 component instances already — i.e. ensure the fallback mechanism kicks in that #3519168: Handle components provided by ComponentSources EXPLICITLY disappearing — enables deleting JS components that are in use introduced.
User interface changes
Issue fork experience_builder-3484682
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
Comments
Comment #2
longwaveComment #3
wim leers👍
Comment #4
wim leersComment #5
wim leersThis should add
\Drupal\Tests\experience_builder\Kernel\Plugin\ExperienceBuilder\ComponentSource\BlockComponentTestand test — by for example deleting aMenuconfig entity, which will result in\Drupal\system\Plugin\Derivative\SystemMenuBlockderiving one fewer block.I bet there will be config dependency-related challenges 😇 In fact … I see that the necessary config dependencies are missing currently. For example the
experience_builder.component.block.system_menu_block.adminXB Component:😱
That
should be
That is an oversight/bug when Block support was first added in #3475584: Add support for Blocks as Components.
Comment #6
wim leersComment #8
longwaveAdding dependencies fixes the menus case, but there are other cases where a block might change or disappear which needs additional work and test coverage.
Comment #9
longwaveAdded additional test coverage for uninstalling a module that provides a block, and updating a block label.
There is the possibility that a block that previously met requirements (ie. it must be fully validatable) stops meeting those requirements, but is this realistically a case we need to handle? I added protection for it by deleting the block component if a reason exists that it is not valid, but unsure if it is worth contriving test coverage for this case.
Comment #10
wim leersYay for #3501290: Introduce unit test coverage for both ComponentSource plugins (Block + SDC) providing the appropriate place for this new test coverage!
I found a few problems in the MR, but I'm wondering if that's my brain still readjusting from the weekend? 🙈
Comment #11
longwaveOnce config entities get dependencies, they automatically get deleted if the dependent entity is deleted - ie. if we depend on a menu and that menu ceases to exist, then the component will be deleted too. But we explicitly wanted to add the dependencies here, so what should we do? Prevent the menu from being deleted, or only have a soft dependency on the menu existing?
Comment #12
wim leersRight. 🤔
What I'd like to see in the test coverage is — somehow — verifying that there's a confirmation dialog when humans try to delete a menu, which would then inform the user that they're making this component obsolete.
I don't think we need to prevent it. But we need to ensure that we do not cascade it down: if a
ContentTemplatedepends on aComponentthat depends on aMenu, the current code is basically saying thatComponentandContentTemplatewill disappear on you! That's bad 😅I think we should implement
\Drupal\Core\Config\Entity\ConfigEntityInterface::onDependencyRemoval(), to prevent that? 😇Comment #13
wim leersSame exact challenge appeared over at #3519168-10: Handle components provided by ComponentSources EXPLICITLY disappearing — enables deleting JS components that are in use overnight! 😅
Comment #14
longwaveYep was thinking whatever we implement here has to be done in a base class or trait and cascaded down to all our config entities.
Comment #15
wim leersIDK about all of XB's config entities — perhaps only
Component::onDependencyRemoval()is sufficient to start. Because … how do we remove a component instance in aPattern,PageRegionorContentTemplatethat itself contains other component instances?! 😅I guess we could do it automatically if the component instance is provided by a
ComponentSourcethat does not implement\Drupal\experience_builder\ComponentSource\ComponentSourceWithSlotsInterface?Comment #16
larowlanFYI In #3519168: Handle components provided by ComponentSources EXPLICITLY disappearing — enables deleting JS components that are in use I've added a fallback plugin that can step in and continue to render children when a JS component is removed if any instance exist.
Comment #17
wim leers@longwave: I think there's still things here that make sense to land independently of #3519168: Handle components provided by ComponentSources EXPLICITLY disappearing — enables deleting JS components that are in use?
Comment #18
f.mazeikis commentedComment #19
larowlan@f.mazeikis fyi there's some overlap with #3519168: Handle components provided by ComponentSources EXPLICITLY disappearing — enables deleting JS components that are in use here that it would be good to be across, would appreciate your review there too - thanks!
Comment #20
f.mazeikis commented@larowlan Yes, there's not just an overlap - this issue is pretty much blocked on #3519168.
Comment #21
wim leersComment #22
wim leers#3519168: Handle components provided by ComponentSources EXPLICITLY disappearing — enables deleting JS components that are in use is in!
Comment #23
wim leersComment #24
f.mazeikis commentedResponded to feedback, added additional test cases.
Comment #25
wim leersVery nice to see this solved so elegantly now! 😊
Comment #26
wim leersRetitling given the scope changed post-#3519168: Handle components provided by ComponentSources EXPLICITLY disappearing — enables deleting JS components that are in use.
Comment #28
wim leersComment #29
wim leers