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

User interface changes

Command icon 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

longwave created an issue. See original summary.

longwave’s picture

wim leers’s picture

Title: Handle update and delete of Block component config entities » Handle update and delete of Block `Component`s
Issue summary: View changes
Issue tags: +Needs tests

👍

wim leers’s picture

Component: Config management » Component sources
wim leers’s picture

Title: Handle update and delete of Block `Component`s » Handle update and delete of Block `Component`s, plus missing config dependencies
Issue summary: View changes
Issue tags: +stable blocker, +missing config dependencies

This should add \Drupal\Tests\experience_builder\Kernel\Plugin\ExperienceBuilder\ComponentSource\BlockComponentTest and test Delete block component config entities once the plugin (or derivative) no longer exists. — by for example deleting a Menu config entity, which will result in \Drupal\system\Plugin\Derivative\SystemMenuBlock deriving 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.admin XB Component:

uuid: 784eab71-dbfa-49da-b932-dc703a5de2f8
langcode: en
status: true
dependencies: {  }
label: 'Administration block'
id: block.system_menu_block.admin
provider: system
source: block
category: Menus
settings:
  plugin_id: 'system_menu_block:admin'
  default_settings:
    id: 'system_menu_block:admin'
    label: Administration
    label_display: ''
    provider: system
    level: 1
    depth: 0
    expand_all_items: true

😱

That

dependencies: {  }

should be

dependencies:
  config:
    - system.menu.admin

That is an oversight/bug when Block support was first added in #3475584: Add support for Blocks as Components.

wim leers’s picture

Assigned: Unassigned » longwave
Issue tags: +sprint

longwave’s picture

Status: Active » Needs work

Adding dependencies fixes the menus case, but there are other cases where a block might change or disappear which needs additional work and test coverage.

longwave’s picture

Assigned: longwave » Unassigned
Status: Needs work » Needs review

Added 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.

wim leers’s picture

Assigned: Unassigned » longwave
Status: Needs review » Needs work
Issue tags: -Needs tests

Yay 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? 🙈

longwave’s picture

Once 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?

wim leers’s picture

Right. 🤔

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.

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?

I don't think we need to prevent it. But we need to ensure that we do not cascade it down: if a ContentTemplate depends on a Component that depends on a Menu, the current code is basically saying that Component and ContentTemplate will disappear on you! That's bad 😅

I think we should implement \Drupal\Core\Config\Entity\ConfigEntityInterface::onDependencyRemoval(), to prevent that? 😇

longwave’s picture

Yep was thinking whatever we implement here has to be done in a base class or trait and cascaded down to all our config entities.

wim leers’s picture

IDK about all of XB's config entities — perhaps only Component::onDependencyRemoval() is sufficient to start. Because … how do we remove a component instance in a Pattern, PageRegion or ContentTemplate that itself contains other component instances?! 😅

I guess we could do it automatically if the component instance is provided by a ComponentSource that does not implement \Drupal\experience_builder\ComponentSource\ComponentSourceWithSlotsInterface?

larowlan’s picture

FYI 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.

wim leers’s picture

f.mazeikis’s picture

Assigned: longwave » f.mazeikis
larowlan’s picture

@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!

f.mazeikis’s picture

Status: Needs work » Postponed

@larowlan Yes, there's not just an overlap - this issue is pretty much blocked on #3519168.

wim leers’s picture

Title: Handle update and delete of Block `Component`s, plus missing config dependencies » [PP-1] Handle update and delete of Block `Component`s, plus missing config dependencies
Assigned: f.mazeikis » Unassigned
Issue tags: -sprint
wim leers’s picture

Title: [PP-1] Handle update and delete of Block `Component`s, plus missing config dependencies » Handle update and delete of Block `Component`s, plus missing config dependencies
Assigned: Unassigned » f.mazeikis
Status: Postponed » Needs work
wim leers’s picture

f.mazeikis’s picture

Assigned: f.mazeikis » wim leers
Status: Needs work » Needs review

Responded to feedback, added additional test cases.

wim leers’s picture

Assigned: wim leers » Unassigned
Status: Needs review » Reviewed & tested by the community

Very nice to see this solved so elegantly now! 😊

wim leers’s picture

Title: Handle update and delete of Block `Component`s, plus missing config dependencies » Handle update and delete of Block `Component`s, plus react to dependency removal

  • wim leers committed 41457d6b on 0.x authored by longwave
    Issue #3484682 by f.mazeikis, longwave, wim leers, larowlan: Handle...
wim leers’s picture

Status: Reviewed & tested by the community » Fixed
wim leers’s picture

Issue summary: View changes

Status: Fixed » Closed (fixed)

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