Content templates, part 3b: store exposed slot subtrees on individual entities

wim leers → credited effulgentsia → .

FYI this is inspired by what I proposed to @effulgentsia while discussing 📌 [PoC] Introduce a `ContentTypeTemplate` config entity Active , and which he encouraged to explore:

Been contemplating this for a couple of days.

I think what each item in a multi-cardinality XB field needs to look like is...a full tree. With a root UUID and everything.

The reason I need a root UUID is so that the content template, which assembles the final renderable tree, knows which part of the tree goes directly into the exposed slot, and which parts of the tree are just NestedArray::mergeDeep()'ed into the main tree (since it seems that the tree structure is never more than three levels deep).

In other words, given a single mini-tree loaded from an entity and targeting a particular slot in the template, the root of the mini-tree goes directly into the exposed slot, and the rest of the tree is just added to the main tree.

✨ Content templates, part 2: store a component tree in the template, and allow individual entities to fill in specific slots in that tree Active is in!

Okay -- needs test coverage (quite a bit of it, I'd think), and validation, but I've at least implemented my first stab at the render-time logic. Could use a quick review to confirm I'm not way off-base here.

Okay, this has a test now, so removing the tag. There are still some data integrity questions here, though, which surely means this is a stable blocker.

wim leers → credited larowlan → .

wim leers → credited mglaman → .

There's still unaddressed feedback by @larowlan. I don't see nearly enough test coverage in here yet — there's many more possible edge cases.

I added some low-level test coverage for this, as well as exceptions for as many possible edge cases as I could see. If there are others, can you please explicitly lay them out so I can write tests for them? 🙏

Whoops! Definitely not an Umami stable blocker. :)

Mostly applauding, but also some questions. A key part here should be reviewed by multiple people.

Couple of comments
- one about a constraint I think we're missing
- one about an edge case of a slot named '0'

Good thing that @larowlan asked for that slot name validation, because it turned up a bug -- if you set cardinality on a field type's plugin definition, that cardinality will be used on all configurable field storages for that field type! It's right there in black and white: https://git.drupalcode.org/project/drupal/-/blob/11.x/core/modules/field....

Luckily, the tests caught this. I've removed the enforced cardinality and everything seems to work as intended now.

I ended up reverting that, and just fixing the broken test, because I realized that we probably always want XB fields to have infinite cardinality, except for on xb_page entities. Infinite cardinality makes the most sense where content templates are concerned, because the template may expose any number of slots, but if the cardinality is limited on the entities which are expected to fill in those slots, well...they won't be able to fill in those slots because they won't have enough cardinality.

Obviously it is possible to increase cardinality on extant fields, but that would just put a roadblock in front of site builders. Why bother with that all? Let's just enforce infinite cardinality, and therefore make sure that templates are free to expose as many slots as they want, and entities which use those templates will be able to fill in those slots without any additional friction.

Assigning to Wim for a final review, on balance I don't think splitting the field type is worth it unless we foresee future uses where it will make things easier. We could also always add additional testing, but I think what we've got here is enough to move forward with this feature.

Actually, NW for test failures first!

The failures are very tricky to solve. They are due to @larowlan's request for us to validate that component trees attached to most entities have a slot name. That is NOT sent from the client side, currently, or handled by the ClientDataToEntityConverter service, so tests break and I'm not sure how to fix them.

Just seen this, there some things that as I was typing seemed fine, however there are some that don't and I don't see discussion of these in this issue yet. I tried to answer my own questions as much as possible.

1. How will this handle re-ordering of slots?

Storing the slot as a column means that if a content template re-orders slots, this should be OK - the slot determines where things fit in, not the field delta. However it does mean that content, and not just content, individual revisions, will have different deltas corresponding to different slots depending on when it's created.

e.g.:

entity 1 | revision 1 | delta 0 | slot 1
entity1 | revision 1 | delta 1 | slot 2

entity 1 | revision 2 | delta 0 | slot 2
entity 1 | revision 2 | delta 1 | slot 1

If we always request data based on the slot and never the delta, then that's probably fine, even for revision restores and diffs, so it might just be an extra artefact in the database. However this goes onto the next questions:

2. If a content template removes a slot, what should happen?

Should there be validation that informs the site builder that content is going to be hidden/orphaned?

It's possible to add validation or messaging based on querying the field column but I'm not sure what that implies for either the UX or the storage.

3. If I want to delete all field data associated with a slot programmatically is there a way to do this?

I don't think there is because it's field data in a specific entity, it would require bulk updating entities via VBO or a custom update.

4. If a content template re-adds a slot with the same name after previously deleting it, what should happen?

e.g. if content was previously hidden, then should it re-appear? Or... should adding a slot have validation that no field data with the new slot name already exists and prevent it, or add a confirmation step?

--

I think all of these questions would be easier to answer if instead of a multiple cardinality field, this used multiple instances of the field.

e.g. answering the questions above with multiple field instances:

1. this is fine - can still be a column on a single-cardinality field, but no delta.

2. The field data would persist if nothing explicit is done, messaging would be the same except:

3. You could delete the field in this case to remove the data from the database, separately from step #2. This could use standard field deletion mechanisms, no need for custom bulk updates.

4. This problem does not really go way conceptually, but because it would be possible to delete the field, or not, there would be more options for handling it.

The downside to multiple field instances is that the UI for template building would need to create the new fields under the hood when slots are added, but it does feel like that's one problem to solve in one place, rather than multiple problems to solve across the lifecycle of an entity type.

All of these questions were discussed as part of the design process but in fairness haven't been documented in the issue queue so I'm glad you're asking these questions. I don't know how close we're having all of these capabilities available so someone else will have to respond to this from that perspective.

1. How will this handle re-ordering of slots?

The slots should have an unique id. User should be able to change their placement in the template without it impacting the contents of that slot or other slots.

→

2. If a content template removes a slot, what should happen?

User should receive a warning that content inside the slot will be deleted. However, the content would not be deleted immediately; it would actually get deleted when the content entities with the slot contents get re-saved.

→

3. If I want to delete all field data associated with a slot programmatically is there a way to do this?

We discussed implementing garbage collection process running on cron. That would require having APIs for this.

4. If a content template re-adds a slot with the same name after previously deleting it, what should happen?

In this scenario, in case that the garbage collection had not happened yet, it would be fine to bring back the old slot content. But this would require the slot having the same unique id.

We discussed implementing garbage collection process running on cron. That would require having APIs for this.

This seems like a lot of potentially complex and expensive logic that would have to go through both entities and their revisions, loading and saving them, vs. being able to use the field deletion API which already exists if this was doing field-per-slot.

All of the other design considerations seem like they'd be either neutral or easier with field-per-slot to me.

All of these questions were discussed as part of the design process but in fairness haven't been documented in the issue queue so I'm glad you're asking these questions.

It would really help if requirements and design process was documented in the issue queue from the outset so I didn't have to though, it is extremely difficult to contribute to experience builder when all of these assumptions are implicit and hidden from the implementation issues. And it's not just the initial implementation issues but also people trying to figure out why things were done when they discover bugs in the future.

I did not expect this issue to be so dragon-shaped.

I discussed the problem with @larowlan. The conundrum is that you (currently) kinda have two ways for an XB field to exist (obviously this could be subject to change, but this is the idea being implemented in this MR thus far):

1. On the xb_page content entity type, which is a single-value field that contains an entire tree which is used for rendering.
2. On any other content entity type, which is an infinite-cardinality field where each item is a bonsai tree that is injected into an exposed slot in a content template

In the first situation, "which slot is this tree in?" doesn't matter. The tree is self-contained and complete.

But in the second situation, "which slot" is critical! The subtree must must be associated with a specific slot. Since the slot is defined and exposed by the content template, it is inextricably linked with that at the data layer.

@larowlan feels (as @longwave alluded to in his review) that this boils down to being, effectively, two different field types -- the only differences between them are:

• the usage of a slot property
• the enforced cardinality (either 1, or infinite)

Forgive my idiomatic metaphor here, but this is a conversation that needs to be had amongst the grand poobahs of Experience Builder (Lauri, Wim, etc.) and the decision probably needs to be documented with an ADR. It is very out of scope for this issue.

Exacerbating the problem is that the frontend has absolutely no concept of "this tree will fit into a slot". The backend doesn't tell the frontend what slot in a template we're working on, and the frontend therefore doesn't tell the backend. That's what's breaking the tests right now in this MR.

So...where does that leave this issue? Lee felt that we should do this -- in order:

1. Greatly reduce the scope of this issue to only add the ComponentTreeItem::mergeSubTrees() method and its test coverage. We're gonna need that in any event, since that's key to combining a template's tree with bonsai trees stored elsewhere.
2. Make the frontend recognize the concept of "I'm working on a particular slot". That might not necessitate any user-facing change, but the backend needs to transmit the name of a slot (or NULL) to the frontend, and the frontend needs to transmit that information to the backend.
3. Make an architectural decision about how Drupal will store trees, regardless of whether the target slot is relevant. Two different field types? Some other kind of magic? Removing the xb_page entity type and allowing nodes to contain complete trees (something Lee floated in our call)?

Whew.

Further questions on top of #28.

User should receive a warning that content inside the slot will be deleted. However, the content would not be deleted immediately; it would actually get deleted when the content entities with the slot contents get re-saved.

I think there are several information disclosure risks with this approach:

Let's say a site deletes a slot from a template, and they are told that content will be deleted.

1. Can field data in the slot still be search indexed? 📌 Create a configurable search api processor for XB data Active is planning to add an XB-specific search API integration, is that going to have to implement hiding of the data from search API separately?

2. What will prevent 'deleted' slots from being available in the field data to JSON:API / REST? Or conversely, it might be a problem if they aren't available in certain cases? I'm not sure at all what the behaviour should be there.

Additionally, and related to #2, having this as field deltas rather than fields, will make it impossible to implement field access for slots, and hence incompatible with modules like field permissions. Maybe Experience Builder is going to have a completely independent 'slot access' system but given it's using a field for storage, it feels like that decision should be explicit and documented, not an accidental byproduct of the implementation here.

It sounds like we could benefit from an ADR for this because we are trying to weigh tradeoffs between two options. Should we do that here or should we open a separate issue for that?

+1 for ADR

There is currently no way for individual entities to provide the subtrees that should fill in those slots, to be assembled for final rendering. Let's add that in this issue.

I think we also have to allow for multiple view modes.

What happens if Full exposes 3 slots and Teaser exposes an entirely different slot.

Is delta + slot enough to distinguish - do we also need a view mode column?

What does the UI look like when editing a piece of content with multiple view mode templates? is there a view mode switcher? Do we prevent saving if a piece of content is not valid for a given view mode?

Adding 📌 Refactor the XB field type to be multi-valued, to de-jsonify the tree, and to reference the field_union type of the prop values Active as related