Store the ComponentTreeStructure field property one row per component instance

Thoughts:

• I agree a JSON blob is overkill and a table is in principle sufficient.
• … but Drupal's Entity Field system assumes every field prop corresponds to a data type (true here too), and that data type must be a single column in the database (not true here), and is always a primitive (the data type class must implement \Drupal\Core\TypedData\PrimitiveInterface if that data type is used for a field property — AFAICT)
• So then IMHO the question becomes: how do we store such a table in a single DB column?

The only exception I can think of (right now, in the little time I have this morning while writing this up to ensure we follow through!): the PathItem field type, which essentially is a computed field type that stores its data in another database table and does the necessary additional DB queries.

Maybe that can work? That maybe actually gets closer to @effulgentsia's original proposal at #3440578: [PP-2] JSON-based data storage proposal for component-based page building, where he proposed to do some deduplication stuff that most of us wanted to defer to later?

We could then literally have a single DB table for all XB uses, by expanding the columns from

parent,slot,delta,uuid,component

(@longwave's comment)
to:

entity_type_id,entity_id,field_name,parent,slot,delta,uuid,component

… which would not violate the 3.2.4 Facilitating `component props` changes section in docs/data-model.md.

#5++

#6++

#7: an XB field must store a tree of components + sources for their props values. This issue proposes to store that tree as a list, so not as a single column in a single row, but as multiple columns in multiple rows. That's what I was trying to convey in #4, and is also why I pointed to @effulgentsia's "deduplication" proposal (we could implement this by storing a single value for each XB field revision's tree, with that value pointing to a foreign key in another table, where @longwave's proposed columns could then be used).

👏🙏

Complication not yet discussed here: #3519352: Content templates, part 3b: store exposed slot subtrees on individual entities. But perhaps it's worth first doing this issue without complicating it with that aspect?

Note: we could choose to tackle #3495625: Remove ComponentTreeItemList::ROOT_UUID from hydration and client-to-server conversion as part of this, by equating \Drupal\experience_builder\Plugin\DataType\ComponentTreeStructure::ROOT_UUID with NULL for the parent column.

I defer to @larowlan.

This would also unblock #3520517: [PP-1] ComponentTreeItem should automatically purge subtrees for slots that are no longer exposed for ContentTemplate support.

I intend to get to the bottom of that one tomorrow, but it is reassuring to read you do not anticipate problems with this implementation order 😊

Just caught up on #3519352 — high-level responses at #3519352-38: Content templates, part 3b: store exposed slot subtrees on individual entities.

Makes me want to #22+++++++++++++ even more than I could've anticipated 😄 — as in: "do this first, but keep #3519352 in mind".

Awesome! Thank you for summarizing the current status as well 🙏😊 Excited to review this!

I'm also curious, how does having one row per instance help with prop translation? Might be worth explaining the change in more detail in the issue summary because I feel like I'm missing something.

This bit is specifically what I wanted @larowlan and @effulgentsia to discuss. Per #28, they did discuss that, and @larowlan got the (a)symmetrical translation support working as he mentions in #28.1.

I'd be fine with explaining that in the updated docs/data-model.md rather than the issue summary. I think @larowlan just hasn't gotten to that point yet 😊

Linking right parent.

The big bet we made with XB >1 year ago was that #3343634: Add "json" as core data type to Schema and Database API would get fixed, which with the last core minor release (11.2) prior to XB 1.0 in the final stages now, is guaranteed to not happen.

So the primary goal of this MR: decomposing the 2 big JSON blobs per revision (tree and inputs) that we've been discussing for >1 year in many places, most recently in #3457504: XB field type: calculate all dependencies, store them, surface in new Component "Audit" operation. This MR must do it in a manner where:

• everything that already works today continues to work
• all technical debt and missing features listed in #3520449: [META] Production-ready data storage are either solved, become simpler, or at minimum, remain equally achievable

The approach @larowlan went with is option 1 that @catch listed in #22.

Reviewed this entire 71 files, +1908, -1680 MR 😄

Posted 67 remarks, of which 19 were trivial comment nits that I already applied. The remainder are either observations to help the next reviewer (ℹ️), excitement/admiration/applause (🤩) or questions 😄

Light bulb moments

While reviewing, these were the most important "aha!" moments:

1. The (a)symmetrical translatability support concerns in #31 are solved by core's require_all_groups_for_translation, which is used by the image field type already 👍
2. This MR's new ValidParentAndSlotConstraintValidator allows trivially adding support for ContentTemplates: there's already a @todo, the remaining work to make it fully validatable is trivial, and makes #3519352: Content templates, part 3b: store exposed slot subtrees on individual entities completely obsolete AFAICT! This MR's introduction of storing every component instance separately, with an optional parent_uuid and optional slot is what makes this easy (instead of very hard in HEAD) 👍

   IOW: this does address the meta (#3520449)'s feature: content entities' XB component trees must be able to populate the exposed slots provided by the corresponding ContentTemplate.

3. A single additional normalizer (for the ComponentInputs data type) that would be trivial to implement would be sufficient to achieve JSON:API read/write support for content entities with XB fields! Which means XB would be able to leap ahead of LB on that front already, because #2942975: [PP-1] Expose Layout Builder data to REST and JSON:API remains unsolved. The difference is that XB started with strict schema + validation, so the big "ensure the normalization can be interpreted by front-end developers" problem is already solved. (It'd even comply with the recommended best practice at #2926507: Discourage @FieldType-level normalizers, encourage @DataType-level normalizers, to strengthen the API-First ecosystem.)
   EDIT: that's what @larowlan independently concluded in #28.2! 👍

Issues we could close if this lands

AFAICT this allows us to close, @larowlan should confirm:

1. #3495625: Remove ComponentTreeItemList::ROOT_UUID from hydration and client-to-server conversion
2. #3462235: [PP-1] Add typed value-objects for the component tree structure
3. #3519352: Content templates, part 3b: store exposed slot subtrees on individual entities

Questions

1. While I know it was not a goal of this issue, his does not yet address the meta (#3520449)'s feature: asymmetric translation will need some explicit inputs to be locked, because some explicit inputs may be considered to be not translatable — for example an image input of some component.

   AFAICT this MR does not make that neither more difficult nor easier to achieve. In HEAD, it would have required some extra per-component instance metadata. Here, it would, too. In HEAD, it'd probably have had to live in the data contained in the ComponentInputs data type. With this MR in, that would still have to be the case.

   So I think this MR is neutral for that.

   EDIT: that seems to match what @larowlan independently wrote in #28.3 👍

Conclusion

A very strong "+1, yes please! 😄" to @larowlan's Assigning to Wim to get a +1 keep going before going any further..

@catch in #33:

Even if there are more rows, writing additional rows in a transaction shouldn't make a lot of difference

Good reassurance, this I'm less familiar with 🙏👍

storage requirements, it should help compatibility with […] because instead of a single large field that could have a single character change

Exactly!

ot removing the last remaining instance of an 'interned' value, so that the full history of changes to individual fields can be preserved, just thought about this now and not sure if it will hold up, but it would be a nice bonus if so.

Wow, great insight — that would be amazing! 🤩

Any kind of auditing or bulk operation on components should be easier - e.g. to find all entities using a specific component, you can query the component column (including from custom views admin listings or similar

Exactly! IOW: this would basically solve make #3522953: [later phase] Make Component `audit` operation performant/scalable simpler, and would allow us omitting config: experience_builder.component.* dependencies from #3521202: Store XB field type's "deps_*" columns in separate table to allow efficient querying's component_tree_depencies table. Because that's essentially duplicating that same bit of information now.

For DX, if someone is looking around the database tables, it will be a more familiar structure than a JSON blob + separate denormalized dependency lookup table. Apart from looking in the database or working on XB itself, it should be transparent anyway.

💯 — this MR single-handedly makes XB much easier to wrap your head around. Much becomes deducable.

However, it will be a simpler JSON structure to query than with the current schema, so can only make that better rather than worse.

Indeed! To be more precise:

• from 1 massive JSON blob per XB field revision
• to 1 small JSON blob (with one less level of nesting too) per component instance per XB field revision

However when it's combined with […]

That's what I'm starting work on right now! 🤓🥳

This is a long list of benefits or neutral effects, with no actual downsides, but I think that's right.

💯

@catch in #35:

You're right that that choice is not finalized in this MR. But this bit in ValidParentAndSlotConstraintValidator definitely lean towards option #22.1 aka storing an exposed slot name in the slot field property/column of an XB field item:

    // @todo 👉️ For 'bonsai' trees, the parent may not be in the corresponding
    //   parent list but in a ContentTemplate for the parent entity.

Which at least I interpreted as @larowlan intending to implement it this way in this MR, which would make Or am I misunderstanding and this already implements #1 because slot === exposed slot in this context? If so the confusion is all mine. almost true: it's not quite implemented yet, but AFAICT @larowlan intends it to be.

This is why I tried to deduce how @larowlan intends to implement this and the required code would indeed be trivial.

Perhaps we're both missing something? :D

The spike has proven itself, as I already said in my review in #34, but especially now that it's green!

Excited to review this 85 files, +3644, -3221 diff in depth tomorrow! 😄

@larowlan indicated he'd be doing a self-review first thing tomorrow, so leaving assigned to him for that. 👍

🥳🤓

I think the true test of a well-designed system (besides adoption, vibrant ecosystem, etc. — all of which by definition cannot yet exist) is:

Well of course it works like that, that all looks like it makes total sense.

IOW: when it seems so evident that the data and logic is structured in this particular way, that it's hard to imagine any other way.

I think XB is now entering that territory with this MR. 🤓🤩

It's amazing to see how this single (massive!) MR completely transforms the most essential part of XB (the way it stores component trees in both content entities as fields and in config entities), and we can be confident that it'll A) work (thanks to tests), B) scale to many different uses (thanks to the 3 different config entity types that use it, and kinda 4).

If it were so evident that this was the right approach, we would've written it like this at the very start. But we didn't. It required iteration. And the uses, the tests, the validation logic all needed to be structured in a sufficiently sensible manner for this massive refactor (by a single (very capable) person over the course of a few days!!!) to be achievable.

IMHO this is the biggest achievement yet of the team working on XB's back end. And it's thanks to literally all of those people having contributed — including all the front end work that uses that back end and that end-to-end tests are exercising the back end through the front end.

See the remaining open threads — they highlight some of the crucial improvements.

Bonus delight: @larowlan's fine test string choices:

• And we laugh like soft, mad children
• Smug in the wooly cotton brains of infancy

🤣

It also allowed me to:

1. close #3471026-13: Harden UUID validation in ComponentTreeStructureConstraintValidator
2. reduce scope and stable blocker tag for #3495625-14: Remove ComponentTreeItemList::ROOT_UUID from hydration and client-to-server conversion
3. clarify #3499632-13: Remove `ClientServerConversionTrait` (C→S) and `ComponentTreeItemList::getClientSideRepresentation()` (S→C) in favor of (de)normalizers
4. reduced scope and precise remaining steps for #3519352-41: Content templates, part 3b: store exposed slot subtrees on individual entities
5. create a nicely scoped follow-up for the last crucial DX improvement for config management: #3526127: Ensure deterministic config export order of config-defined component trees

Finally, this is what a single simple component tree on a content entity now looks like in the DB:

… and that component_id column will evolve in the next issue @larowlan is working on: #3523841: Versioned Component config entities (SDC, JS: prop_field_definitions, block: default_setting, all: slots for fallback) + component instances refer to versions ⇒ less data to store per XB field row.

Thanks for the posthumous review/confirmation of the tweaks I made to your epic MR, and merged the follow-up 👍 😊

This somehow introduced a regression for default_content's export functionality: #3526814: Default content exports of component trees are invalid and hence are not correct after importing

Which would've been far less painful for @justafish to run into if the validation this MR introduced caught the problematic bits, created #3526818: Tighten validation of `parent_uuid` and `slot` on XB fields to match the strictness of config for that.