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

@larowlan in #21:

one about an edge case of a slot named '0'

Another reason to do 📌 Constraint slot names allowed by XB in its component tree Active ! 😄

I'm glad to see @lauriii already said as much in #29 😊

WRT deleting component subtrees (for this issue: of unused slots' subtrees): ✨ [later phase] Provide API for finding and UI for surfacing dangling/dead component subtrees — aka garbage clean-up Postponed — but also of defunct components, which is necessary because of 🐛 Cannot delete JS components due to component depending on them Active .

@phenaproxima in #32:

the frontend has absolutely no concept of "this tree will fit into a slot".

1. That's intentional: docs/adr/0005-Keep-the-front-end-simple.md
2. That's for later: ✨ Component restrictions Active
3. Yes, the client side will need a new "node type" for its client-side data model — see 3.4 UI Data Model: communicating a `component tree` to the front end in docs/data-model.md — currently only 3 node types exist: component, slot and region. I do agree a new one will be needed for this. That's what 🌱 [META] Production-ready client-side data model + internal HTTP API Active exists for.

P.S.: I'm very tempted to make "bonsai trees" an official term in XB, so can you elaborate on why you used that term and your rationale? 😄

@catch in #33:

RE: information disclosure risks Yep, I raised exactly that concern. 💯

The reason we have no choice but to allow this is that we know from the XB team members who've worked on https://github.com/acquia/cohesion (aka "Acquia Site Studio") for years that site builders will accidentally delete a slot, confirm through all the warnings, and then later realize that actually they do want to undo it after all.

RE: Wow — glad you caught that! Had not crossed my mind yet!

@larowlan in #37: see the validation+config schema added in ✨ Content templates, part 2: store a component tree in the template, and allow individual entities to fill in specific slots in that tree Active — exposed slots are intentionally only allowed for the full/canonical view mode. Because otherwise you get different component trees per view mode, which would be an authoring/usability nightmare. I first identified this in #3511366-3: [META] Introduce a `ContentTypeTemplate` config entity + related infrastructure → , @lauriii responded at #3511366-7: [META] Introduce a `ContentTypeTemplate` config entity + related infrastructure → .2.

Apologies, @phenaproxima, for having sent you down this path 😬 Turns out that this "incremental step" was fraught with more challenges than I anticipated. I expected it to need refactoring later on for sure, but I tried to unblock you on next steps for content templates. Turns out that was not quite realistic.

Per discussion with @effulgentsia, we think this is best tabled until after 📌 [PP-1] Consider not storing the ComponentTreeStructure data type as a JSON blob Postponed . @larowlan is actively working on spikes to help us make the best choice possible.

Agreed that an ADR is needed and appropriate, but during this read-through/catch-up, I did not end up with the impression there's only 2 possible choices as @lauriii indicates in #35 — I think there's many possible choices.

I'm very tempted to make "bonsai trees" an official term in XB, so can you elaborate on why you used that term and your rationale?

As I understand it, a bonsai tree is, physiologically speaking, a regular tree. The only reason it doesn't reach full size is because it's planted in a small pot and its growth is there constrained.

The data in an exposed slot is very similar. It's a completely normal XB tree, just rooted in a small pot (a single slot) of a larger garden (the content template).

"Subtree" is also a reasonable way to describe it, but that's more technical and less pleasing than "bonsai tree". 🌳

#39: thanks for that lovely imagery :) I think we should make that an official term! 😄 Issue created: 📌 Coin 2 new terms that are both precise and fun! Active .

As described at #3468272-34: Spike: Explore storing the ComponentTreeStructure field property one row per component instance → , I believe that MR will allow us to close this MR. Still, in case that's not true, I want to explicitly acknowledge that this should block 🌱 Milestone 1.0.0-beta1: Start creating non-throwaway sites Active , because without this supported, the data storage/model may need to change significantly later, which would be very disruptive.

Reviewing 📌 [PP-1] Consider not storing the ComponentTreeStructure data type as a JSON blob Postponed right now, it 100% certainly addresses this! 🥳

All that this issue will need to do is refine the ComponentTreeStructureConstraintValidator validation constraint, for which @larowlan even left a very nice @todo in the right place:

    // @todo For 'bonsai' trees, the parent may not be in the corresponding
    //   parent list but in a ContentTemplate for the parent entity. Add UUIDs
    //   from any relevant ContentTemplates for this entity -
    //   https://drupal.org/i/3519352

Hence removing the beta blocker tag, because this won't require data storage changes anymore; it will only require refining validation logic (essentially to allow more: allow a component instance to refer to a parent component instance that lives outside its own component tree). 🥳

All that this issue will need to do is refine the ComponentTreeStructureConstraintValidator validation constraint, for which @larowlan even left a very nice @todo in the right place:

Is that definitely all this issue needs to do if there needs to be data cleanup and access/rendering changes when exposed slot subtrees are added or removed?

#42: for that, we have ✨ [later phase] Provide API for finding and UI for surfacing dangling/dead component subtrees — aka garbage clean-up Postponed .

@wim leers that issue wouldn't be necessary if the exposed subtrees were stored as individual fields as opposed to within one field, which I assumed was something to sort out here?

that issue wouldn't be necessary if the exposed subtrees were stored as individual fields as opposed to within one field, which I assumed was something to sort out here?

The downside of this approach would be that we'd have to read from multiple table to construct the merged tree. It would also add overhead for every exposed slot in the template (in a linear fashion).

I'm not saying its a bad idea, I think it warrants a spike so we have points of comparison for both approaches. I'll open an issue for that.

One benefit it would provide is that it might solve #37 (note that this isn't in the product requirements, which limit exposed slots to the full view mode).

When I was talking about cleanup with Alex we wondered whether we could make use of the deleted column in the default dedicated field storage schema for flagging slots that no longer existed.

This could probably use a consolidated comparison in an updated issue summary.

After reading through this I'm strongly in favor of one per field.

The benefits seem to be it matches closer to expected drupal norms, let's you order them, organize them by display and leverage other apps making it better for outside devs trying to interact. Nevermind the security and access issues mentioned.

The only downside of per field seems to be more complex queries (which can be cached right?) And a concern that users will delete them unexpectedly.

We have the trash module, I wonder if we could probably leverage a place holder that delays that deletion temporarily or something to solve that issue rather than an architectural decision that compromises security and makes other standard use cases far harder.

Opened 📌 Spike: explore merits of one XB field per exposed slot in content templates Active for #44

Reanswering the questions from comment #28 now that 📌 [PP-1] Consider not storing the ComponentTreeStructure data type as a JSON blob Postponed is in

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

It doesn't matter anymore - yes we have a delta in the field values (because of unlimited cardinality) but it is only important for elements that are siblings in the tree - ie. have the same value for parent_uuid and slot. So re-ordering a whole slot is transparent. Its only ordering within a slot that is important.

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

Now that slot is a column, we can use an entity query to detect any content in this slot and warn. In theory we could also set the 'deleted' column to 1 on those rows to avoid the overhead of loading and manipulating them in memory/queries if they do nothing. Or failing that we could have something similar to field_purge (but driven by queues) that cleaned up these rows during cron runs.

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

Yeah still would need to use an entity query to find items with a matching slot now that slot is a column of its own. Then load those items and call $entity->get('component_tree')->setValue(\array_filter($entity->get('component_tree')->getValue(), static fn (array $item) => $item['slot'] === $dead_slot))->save()

So that's back to bulk updating entities - so having a queue purge process to do this automatically on cron would probably be best, but yeah VBO is another option.

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

This is another argument for trying to reuse the deleted column, because we could just toggle that back to 0 for any remaining data and the old values would re-appear. But yeah, needs input from @lauriii about the intended behaviour in this case

To further validate 📌 [PP-1] Consider not storing the ComponentTreeStructure data type as a JSON blob Postponed I merged this with 0.x and updated the `::mergeSubTrees` logic

Before

  public function mergeSubTrees(array $exposed_slot_info, iterable $subtrees): self {
    $subtrees = array_intersect_key(
      self::keySubtreesBySlot($subtrees),
      $exposed_slot_info,
    );

    $our_tree = Json::decode($this->get('tree')->getValue());
    $our_inputs = Json::decode($this->get('inputs')->getValue());

    foreach ($subtrees as $slot_name => $subtree) {
      assert($subtree instanceof self);
      $component_uuid = $exposed_slot_info[$slot_name]['component_uuid'] ?? NULL;
      $component_slot_name = $exposed_slot_info[$slot_name]['slot_name'] ?? NULL;

      if (empty($component_uuid)) {
        throw new SubtreeMergeException("Cannot merge subtree because we don't know the UUID of the component instance to target.");
      }
      if (is_null($component_slot_name)) {
        throw new SubtreeMergeException("Cannot merge subtree because we don't know the name of the component slot to target.");
      }

      $subtree_tree = Json::decode($subtree->get('tree')->getValue());

      if (!array_key_exists(ComponentTreeStructure::ROOT_UUID, $subtree_tree)) {
        throw new SubtreeMergeException("Cannot merge subtree because the subtree has no root UUID.");
      }

      // The target slot needs to be empty.
      if (!empty($our_tree[$component_uuid][$component_slot_name])) {
        throw new SubtreeMergeException("Cannot merge subtree because the targeted slot is not empty.");
      }

      // The stuff at the root of the subtree goes directly into the exposed
      // slot in our tree. Config schema validation guarantees that the slot is
      // empty, so we're not going to be overwriting anything.
      $our_tree[$component_uuid][$component_slot_name] = $subtree_tree[ComponentTreeStructure::ROOT_UUID];

      // Everything else in the subtree is just added to our tree as-is.
      unset($subtree_tree[ComponentTreeStructure::ROOT_UUID]);
      if (array_intersect_key($subtree_tree, $our_tree)) {
        throw new SubtreeMergeException("Cannot merge subtree because some of its components are already in the final tree.");
      }
      $our_tree += $subtree_tree;

      // Add the subtree's inputs to the final set of inputs.
      $subtree_inputs = Json::decode($subtree->get('inputs')->getValue());
      // If the final tree already has any of these inputs, that's a problem
      // because we can't be sure which ones we should actually use.
      if (array_intersect_key($subtree_inputs, $our_inputs)) {
        throw new SubtreeMergeException("Cannot merge subtree because some of its inputs are already in the final tree.");
      }
      $our_inputs += $subtree_inputs;
    }

    $our_tree = Json::encode($our_tree);
    $our_inputs = Json::encode($our_inputs);

    $this->get('tree')->setValue($our_tree);
    $this->get('inputs')->setValue($our_inputs);

    return $this;
  }

  private static function keySubtreesBySlot(iterable $subtrees): array {
    $keyed = [];
    foreach ($subtrees as $subtree) {
      assert($subtree instanceof self);
      $slot = $subtree->get('slot')->getValue();
      $keyed[$slot] = $subtree;
    }
    return $keyed;
  }

After

public function mergeSubTreeItemList(array $exposed_slot_info, ComponentTreeItemList $subTreeItemList): self {
    foreach ($exposed_slot_info as $slot_detail) {
      $parent_uuid = $slot_detail['component_uuid'];
      $slot = $slot_detail['slot_name'];
      if ($parent_uuid === NULL) {
        throw new SubtreeMergeException("Cannot merge subtree because we don't know the UUID of the component instance to target.");
      }
      if ($slot === NULL) {
        throw new SubtreeMergeException("Cannot merge subtree because we don't know the name of the component slot to target.");
      }
      $existing = \count(\iterator_to_array($this->componentTreeItemsIterator(self::isChildOfComponentTreeItemSlot($parent_uuid, $slot))));
      // The target slot needs to be empty.
      if ($existing !== 0) {
        throw new SubtreeMergeException("Cannot merge subtree because the targeted slot is not empty.");
      }
      foreach ($subTreeItemList->componentTreeItemsIterator(self::isChildOfComponentTreeItemSlot($parent_uuid, $slot)) as $item) {
        \assert($item instanceof ComponentTreeItem);
        if ($this->getComponentTreeItemByUuid($item->getUuid()) !== NULL) {
          throw new SubtreeMergeException("Cannot merge subtree because some of its components are already in the final tree.");
        }
        $this->appendItem($item->getValue());
      }
    }
    return $this;
  }

That feels like a pretty big endorsement to me. 80% of the new code is validation, the rest is ->appenditem

This is green on top of the new data storage model

It doesn't matter anymore - yes we have a delta in the field values (because of unlimited cardinality) but it is only important for elements that are siblings in the tree - ie. have the same value for parent_uuid and slot. So re-ordering a whole slot is transparent. Its only ordering within a slot that is important.

In one of the early conversations with @lauriii he was very concerned that the data model should reflect how things appear on the page (e.g. ordering not getting out of sync). I get the principle that only ordering within a slot would actually affect the rendering though.

Now that slot is a column, we can use an entity query to detect any content in this slot and warn. In theory we could also set the 'deleted' column to 1 on those rows to avoid the overhead of loading and manipulating them in memory/queries if they do nothing. Or failing that we could have something similar to field_purge (but driven by queues) that cleaned up these rows during cron runs.

Using the deleted column within a field is an interesting idea. I personally don't think loading and saving every revision of every entity with a deleted slot should be considered for data cleanup because it could be a huge undertaking with lots of content, there can be other problems loading and saving very old revisions with different data integrity problems etc.

So either actually using field purge (because field-per-slot), or expanding 'field delta purge by component' would avoid that. Although field delta purge by component obviously doesn't exist as a concept so it might not be a small undertaking to implement.

So that's back to bulk updating entities - so having a queue purge process to do this automatically on cron would probably be best

Per above I think that's something to avoid, it could be millions of entity/revision saves and crash due to other issues.

In one of the early conversations with @lauriii he was very concerned that the data model should reflect how things appear on the page (e.g. ordering not getting out of sync). I get the principle that only ordering within a slot would actually affect the rendering though.

This is probably one of the conversations at dev days but I have no recollection whatsoever on what the specific concern was. What's important is that components within a slot are ordered correctly. I don't have strong opinions on whether that's done with field delta or not.

What's also important is that there's a simple way to construct the tree of components because we may have decoupled clients like Lupus building alternate rendering engines for the data model.

If I'm understanding correctly, a content template could have 12 exposed slots. Which means 12 fields will be programmatically attached to that bundle, and all the implications of dedicated field storage. Right?

@mglaman yes exactly, if there are a lot of exposed slots, then field-per-slot means a lot of fields - but also 12 fields on an entity is not that many in the scheme of things, I regularly see bundles with 20-30 configurable fields. It's also nothing like paragraphs where you could have three different tables or more per-paragraph (equivalent to component instance/field delta).

So if it does simplify data clean-up when slots are unexposed (no loading and saving unbounded numbers of entities), and potentially other things like access/hiding of slots (because field access becomes available), then I think it would be a good trade off. If it's not actually a simplification, then it wouldn't be worth doing for its own sake, but at the moment we're talking about having to do ✨ [later phase] Provide API for finding and UI for surfacing dangling/dead component subtrees — aka garbage clean-up Postponed vs probably not having to do it at all.

But also 12 different exposed slots would imply non-exposed slots in between, I can't see why there would be adjacent exposed slots, that could just be one slot. So we're talking about well over 20 distinct slots at that point? To me this seems like it would be an extreme upper bound, and 1-3 is more likely unless I'm missing something with the use case.

Bumping this to because of the security concerns raised by @catch. It's important that we land on how to handle the storage needs of content templates before 🌱 Milestone 1.0.0-beta1: Start creating non-throwaway sites Active , even if the end-user facing functionality does not exist yet.

Discussion

@catch in #44: no, ✨ [later phase] Provide API for finding and UI for surfacing dangling/dead component subtrees — aka garbage clean-up Postponed will be necessary even if we do that. Check it out — there's other ways to end up with dangling subtrees 😬

@larowlan in #45: 👏 Yes!

@larowlan in #48:

1. This is the part that this issue could make simple 👍 That would keep the scope of ✨ [later phase] Provide API for finding and UI for surfacing dangling/dead component subtrees — aka garbage clean-up Postponed smaller, but #3524406 would still be necessary.
2. 💯 — that's why I was excited about that insight by you and @effulgentsia! 😄

@catch in #51:
Where you wrote

So either actually using field purge (because field-per-slot), or expanding 'field delta purge by component' would avoid that. Although field delta purge by component obviously doesn't exist as a concept so it might not be a small undertaking to implement.

I do understand .

But I don't understand the other. We'd have to purge all component instances within an exposed slot that was deleted. So it'd be basically:

1. UPDATE {field_table} SET deleted=1 WHERE parent_uuid IS NULL AND slot LIKE 'my_deleted_exposed_slot'; for the content entity type+bundle whose ContentTemplate had an exposed slot deleted
2. the same for the {field_revision_table} revision table
3. … then gradually purging just those rows, not all fields. The challenge there is AFAICT the need to call \Drupal\file\Plugin\Field\FieldType\FileFieldItemList::deleteRevision() and friends.

@catch in #54:

But also 12 different exposed slots would imply non-exposed slots in between, I can't see why there would be adjacent exposed slots, that could just be one slot. So we're talking about well over 20 distinct slots at that point? To me this seems like it would be an extreme upper bound, and 1-3 is more likely unless I'm missing something with the use case.

How do you go from "12 exposed slots" to "20 distinct slots"? 🤔

MR review + issue/MR scope

👏 @larowlan for #49 and #50 🤩

The issue summary is now wildly out-of-date, given the new 📌 [PP-1] Consider not storing the ComponentTreeStructure data type as a JSON blob Postponed -powered reality, so let's fix that.

The current data model (specifically the allowed values for the parent_uuid and slot field properties in \Drupal\experience_builder\Plugin\Field\FieldType\ComponentTreeItem::propertyDefinitions()) clearly are sufficient to make this functionality work — because the post-3468272-updated MR is passing tests.

The current MR chooses to not store one field per exposed slot. If we were to do one-field-per-subtree-for-an-exposed-slot, we'd need to do the following on top of this MR:

1. Add a exposed_slot storage setting to ComponentTreeItem.
2. Add a validation constraint that enforces that storage setting: it must disallow both parent_uuid and slot being NULL, because those would be component instances targeting the root of a component tree.
3. Update \Drupal\experience_builder\Entity\ContentTemplate::build() and the ComponentTreeLoader service not load the sole XB field, but all of the XB fields on the given content entity type + bundle, and call ::mergeSubTreeItemList() on all of them.
4. Tests

I don't see any reason why we cannot do that in a follow-up, nor do I think that that introduces any additional risk at this time.

That would allow @phenaproxima to get started on next steps (such as ✨ Content templates, the boss battle: create a UI for editing templates Active ).

Thoughts? 😊

Update issue summary, much less to talk about now because this is dramatically simpler since 📌 [PP-1] Consider not storing the ComponentTreeStructure data type as a JSON blob Postponed

I think we should move the multiple fields idea to a follow up spike. The main benefit is to allow identification and removal of dangling sub-trees but I think the cost (in terms of complexity) is too high. Implementing a purge technique (either by reusing deleted column from core or something custom) feels like a more long term approach in terms of managing complexity. We could do a spike of both (multiple fields vs purge approaches) to decided which is the better approach.

Agree with getting this in and unblocking the UI elements for the FE team.

@catch in #44: no, #3524406: [later phase] [Needs design] [PP-1] Provide API for finding and UI for surfacing dangling/dead component subtrees — aka garbage clean-up will be necessary even if we do that. Check it out — there's other ways to end up with dangling subtrees 😬

hmm, that issue seems like it's dealing with a data-integrity bug as a result of code going AWOL (similar to Views invalid plugin handling). For me that is very different to being forced to deal with a data integrity issue because of the way that fully valid and allowed configuration changes are reflected in the schema.

But I don't understand the other. We'd have to purge all component instances within an exposed slot that was deleted. So it'd be basically:

UPDATE {field_table} SET deleted=1 WHERE parent_uuid IS NULL AND slot LIKE 'my_deleted_exposed_slot'; for the content entity type+bundle whose ContentTemplate had an exposed slot deleted
the same for the {field_revision_table} revision table
… then gradually purging just those rows, not all fields. The challenge there is AFAICT the need to call \Drupal\file\Plugin\Field\FieldType\FileFieldItemList::deleteRevision() and friends.

This is about as far as I got trying to think what purging only components would look like, or well, it's a lot further than I got.

How do you go from "12 exposed slots" to "20 distinct slots"?

Hmm maybe I am missing something.

My understanding of ✨ Content templates, part 2: store a component tree in the template, and allow individual entities to fill in specific slots in that tree Active was that it allows for the following use case:

1. Parts of the template where the content author can add different components of their choosing.
2. Parts of the template which are 'fixed' and will be filled in via field content, or hard-coded to a specific block or something.

and that there could be multiple of #1 and #2.

If that is correct, then I would assume that when you have #2, you also have #1, hence going from 12 to 20, or at least, that you would have 12 exposed slots and 0 non-exposed slots. But do I have the wrong end of the stick?

But either way, the point is that I can't see people adding more than 12 of them, it seems like the sort of thing you'd add 1 or 2 of at most? And if so, then field-per-slot is a couple of fields, not dozens.

I commented in #3526189-3: Spike: explore merits of one XB field per exposed slot in content templates → with a concern about the "different field for each exposed slot" idea.

@larowlan Thanks for #56 Thanks for the issue summary update!

@catch in #57: RE 12 vs 20: There could easily be 100 slots in a given component tree. Most of them would indeed be "fixed" in the ContentTemplate: the site builder populates them as they see fit. The ones that are exposed by the site builder to content creators for them to populate are required to be empty (see ValidExposedSlotConstraintValidator). So, yes, 12 exposed slots already seems pretty much the upper bound in real-world terms. (In fact, we could even limit it — for either or both content author UX and performance/scalability reasons!)

So: AFAICT you were just talking about how many actual component instance slots exist in total in a content template ("distinct", 20) vs which ones are exposed (12). 👍

I see that both @larowlan in #56 and @effulgentsia in #58 basically agreed with what I wrote in #55, so tightening scope, and landing this first.

So: AFAICT you were just talking about how many actual component instance slots exist in total in a content template ("distinct", 20) vs which ones are exposed (12).

Yes or more specifically, I couldn't think of a use case for consecutive exposed slots, so am assuming there'd always be at least one 'fixed' slot in-between. But the main point is that 12 would be at the top end, not an average.

👍Agreed on both counts!

Although, actually … I can think of a use case for consecutive exposed slots. Imagine you always want 2 columns. But you want content authors to populate them freely. That’s 2 slots in the same component instance that you’re both exposing.