[PP-1] Add support for Blocks as Components

Is it correct to state that 📌 Prepare for multiple component types: ComponentTreeStructure should contain Component config entity IDs, not SDC IDs Fixed is a blocker for this?

f.mazeikis → credited larowlan → .

I wasn't marking it as blocking as it looked as if the issue would be merged anyway and we could have started this issue while the work continued on #3469609 📌 Prepare for multiple component types: ComponentTreeStructure should contain Component config entity IDs, not SDC IDs Fixed . And it is merged now.
I've moved most of the code from MR68 (closed) by @larowlan and reached out to him on slack as I couldn't retain his commit signature.
This is not a working commit and will be used as starting point.

Did an initial high-level review. Looks to be on the right track! 😄 Thanks for getting this going! 🙏 I bet @larowlan will be pleased 😁

Marking needs work for the following high-level bugs:

1. mixed up BlockInterface (which is for Block config entities) versus BlockPluginInterface (which is for block plugins). We need only the latter.
2. unclear what the purpose of expose_settings is, and AFAICT it cannot result in validatable XB Component config entities
3. related to the above: the existing defaults should be moved into the SDC source-specific settings

📌 Prepare for multiple component types: ComponentTreeStructure should contain Component config entity IDs, not SDC IDs Fixed landed ~2 weeks ago. Updating title.

@f.mazeikis just discussed this issue for an ~hour. This comment captures the essence of that meeting.

Docs

Tagging because besides an initial implementation, the most crucial part here is that @f.mazeikis documents how he envisions how this will all work. IOW: an equally imporant goal for this issue is to update docs/components.md and docs/config-management.md, which were introduced by 📌 Document the current component discovery + SDC criteria, and describe in an ADR Active .

Implementation

The initial implementation can be very limited — we shouldn't try to solve everything all at once here. I just discussed this with @f.mazeikis and we agreed on the following high-level points, with more detail/nuance to be defined by @f.mazeikis in the coming days:

1. XB will auto-generate Component config entities for block plugins just like it does for SDCs
2. … but only for those block plugins that meet certain requirements (just like for SDCs)
3. The requirement that immediately comes to mind: fully validatable block settings (possible since 📌 Make Block config entities fully validatable Fixed ). Because without that, it's too easy for blocks used in XB to just have arbitrary data blobs associated with them. Validation allows for confidence, reliability, better UX. (And will likely be crucial for upgrade paths for block plugins, too.)
4. For this MR, a test that verifies a single block plugin gets an auto-generated Component config entity is sufficient. The simplest one: \Drupal\system\Plugin\Block\SystemPoweredByBlock (which has zero settings). Ideally, there would be one more: \Drupal\search\Plugin\Block\SearchBlock (which has a single setting that was made fully validatable in #3379725).

A special case is \Drupal\block_content\Plugin\Block\BlockContentBlock, because it is a (derived) block plugin that depends on content entities. It's crucial in Layout Builder, for which XB eventually must provide an upgrade/migration path. We think we see a way forward for that special case too. Details TBD.

Also changing the parent issue to get this listed at https://contribkanban.com/board/experience_builder, and hence increase visibility.

Discussed with @longwave, he'll be pushing this forward right after he finishes 📌 [later phase] [PP-2] Prepare for multiple component types: prefix Component config entity IDs with `sdc` Postponed , which blocks this anyway.

@f.mazeikis did a walkthrough/hand-over with me last Friday, before he went on vacation, so I'll be able to provide reviews 👍

📌 [later phase] [PP-2] Prepare for multiple component types: prefix Component config entity IDs with `sdc` Postponed landed!

🏓 AFAICT we're on the same page, @longwave :)

📌 Add PHP test coverage of layout controller Active is in, which AFAIK should help with getting this MR ready :)

The MR is green. "Search form" is available as a component and the preview works (ish) but after dragging it to the canvas the preview fails with an error; going to continue working on this while looking for reviews of the rest of the changes.

#16 🥳

Reviewing in-depth! Docs definitely have been updated, removing tag 👍

First complete review pass posted!

Thanks for the review! I've run out of time now to work on this, so unassigning; I've addressed some of the feedback and added a bunch of @todos but there are still some things to be done - replied to some of the MR comments on those points. Tagging to add test coverage as well as we should add some here to prove that blocks work to some extent.

📌 Refactor XB internals to not assume /xb-components returns only SDC-powered XB Components Active is at minimum a soft blocker.

In DM, @f.mazeikis asked what the point is of keeping the source property around separately, if it's already part of the ID prefix.

Having source separate makes it easy to validate, plus it's relevant information inside the config entity.

The ID's naming structure might still change!

Fortunately, over at 📌 [PP-1] Convert field_storage_config and field_config's form validation logic to validation constraints Postponed , I already worked on a StringParts constraint that simplifies that. Pushing commits to transplant that into XB — because that core issue has not moved forward in a very long time, but that constraint definitely is solid.

After working on this with Felix for some time, we don't think this is necessarily the final implementation - there are a lot of unknowns and new @todos - but it does decouple SDCs from Components, introduce the capability of placing at least some Blocks, and allows some other issues to be worked on in parallel.

Opened several followups for @todos tagged here.

📌 Add support for block derivatives Active
📌 Display disabled Block components in /admin/structure/component/status Active
📌 Clean up Component::calculateDependencies() Active
📌 Improve or remove ComponentSourceInterface::getClientSideInfo() Active
📌 Handle update and delete of Block component config entities Active

Also two followups to discuss handling block settings:

📌 Define `props` in the context of Block components Active
📌 Define how block settings should be presented in the UI Active

Even though we have two source plugins now it's still hard to be 100% confident that we have fully decoupled things in the right way. I feel like it would be helpful to have a third source plugin to really solidify the abstractions here and make sure we have done the right thing. But I don't think that third plugin is known yet so this will have to do for now, we are still in the early phases anyway and landing this means the followups and numerous other issues can be worked on in parallel.

Even though we have two source plugins now it's still hard to be 100% confident that we have fully decoupled things in the right way.

💯

We do not have designs/UX for all XB component types. The designs do not cover the unknown territory of "abstract additional component types", and hence we cannot possibly know what is shared (across component types) vs unique (per component type).

There's no way around this IMHO, but to just build what we do know and acknowledge it will need to evolve.

But I don't think that third plugin is known yet so this will have to do for now, we are still in the early phases anyway and landing this means the followups and numerous other issues can be worked on in parallel.

Well, we do know that we'll need to support layout plugins for sure. See 🌱 [META] Support component types other than SDC Needs work .

I think this good enough considering the number of follow-ups we have. There are number of places we assume "props" but I think handling that in 📌 Define `props` in the context of Block components Active is good enough to get things moving

Looks good! Thanks everyone!

Given this has not been merged yet and sitting at RTBC for 2 hours, I'd like to do one final review pass because it's such a critical leap forward.

Reviewing in the next few hours. If I fail to post my review in the next 3.5 hours, go ahead and merge 🤓

Finished doing a thorough review. 🥵

This looks great compared to when I last reviewed this >10 days ago! 👍

3 things stand out to me:

1. The 'settings_tray' => FALSE, criterion is AFAICT at odds with what the PageTemplate functionality (introduced in 📌 Introduce an XB `PageTemplate` config entity Active ) must able to provide. See https://git.drupalcode.org/project/experience_builder/-/merge_requests/3... for details.
2. the whole "plugin ID" handling situation, where it is assumed that every Component Source Plugin will have a plugin_id key-value pair in its settings seems wrong. Details: https://git.drupalcode.org/project/experience_builder/-/merge_requests/3.... I think we can do better. I'll push a commit for that tomorrow.
3. The props term that SDC uses and XB adopted too … kinda gets in the way when abstracting to multiple component types. I proposed "explicit inputs" (as the generalized version of "SDC props" and "block plugin settings") and "implicit inputs" (as the generalized version of "block contexts"; SDCs have no such concept). See #3484666-5: Define `props` in the context of Block components → for more detail.

… plus a bunch of things lacking clarity, some of which @longwave already addressed.

I think only the first point is commit-blocking, plus the simple requests for clarification on the MR beyond those 3 big points.

While reviewing the cacheability test coverage remark I posted in detail, I discovered that there was an actual performance (cacheability) regression. Details at https://git.drupalcode.org/project/experience_builder/-/merge_requests/3.... Fixed.

I'll deal with #31.2 WRT plugin ID handling in a follow-up — because I'm not entirely sure yet my idea will work.

See y'all in the follow-ups!

Finally, about this bit of the issue summary:

User interface changes

For now, block plugins appear under the main "Default components" section in the UI, but in a future issue they will be moved to a separate "Dynamic components" section.

→ the infrastructure for that is being added in ✨ Provide the frontend a way to distinguish module and theme components from one another Active .

Apologize for coming late, I had some time looking at this.

When I disable a block component via /admin/structure/component, I can't enable it back again with an error message

Drupal\Component\Plugin\Exception\PluginNotFoundException: The "local_tasks_block" plugin does not exist.

.

Is this something to worry about? Or should this be a follow-up issue?

Oops sorry, accidentally changed status

@el7cosmos I think that's in scope for 📌 Display disabled Block components in /admin/structure/component/status Active . Will repeat #36 there 👍😊

FYI: In descoping things from this issue/MR, one crucial data integrity aspect for the overall XB architecture was also descoped; I started pushing that forward in 📌 `ComponentTreeMeetRequirements` constraint validator must be updated now that Blocks-as-Components are supported Active .

looks like 0.x is now failing since this commit https://git.drupalcode.org/project/experience_builder/-/pipelines?page=1...

I can't get it to fail locally

So setting to needs work to alert anyone who is following this issue and might have ideas. We should probably fix in another issue

It could have been a drupal core change but I have pull the latest changes and can't get it to fail

Created 🐛 Test failing on 0.x Active

FYI: follow-up 📌 Display disabled Block components in /admin/structure/component/status Active was merged into 📌 Capture all reasons why particular SDC is incompatible Active .