Overview
In @longwave's review of #3461490: Document the current JSON-based data model, and describe in an ADR, he pointed out:
If the structure was something like this it reduces special-casing of the root and allows for possible future expansion:
{ "ROOT_UUID": { "component": "root", "slots": { "root": { "uuid-root-1", "uuid-root-2", "uuid-root-3", } } }, "uuid-root-1": { "component": "provider:two-col", "slots": { "firstColumn": [ "uuid4-author1", "uuid2-submitted", ], "secondColumn": [ "uuid5-author2", ] } }, "uuid-root-2": { "component": "provider:marquee", "slots": { "content": [ "uuid4-author3", ] } }, "uuid-root-3": { "component" : "provider:no-slots" }, "uuid4-author1": { "component" : "provider:no-slots" }, "uuid2-submitted": { "component" : "provider:no-slots" }, "uuid5-author2": { "component" : "provider:no-slots" }, "uuid4-author3" : { "component": "provider:no-slots" } }ie. we treat
rootjust as another component which is really just a list of other components, and movecomponentinside its own component UUID instead of keeping it with the slot definition? Then we can treat the slot definitions as a simple array of UUIDs.This seems both flatter and simpler than the proposed structure but not sure if it has any disadvantages?
If it turns out we need to store something else in here we can do it at the third level alongsidecomponentandslots?
Proposed resolution
I agree with @longwave that this simplification makes sense — it'd also simplify various other pieces:
- the validation logic introduced in #3460856: Create validation constraint for ComponentTreeStructure
- the tree reconstruction logic for the hydration logic introduced in #3463986: Refactor SdcController::preview() to use ComponentTreeHydrated, and update it to support nested components
… and thanks to the test coverage that was introduced there, this would actually be very simple to evolve the structure towards! 🥳
User interface changes
None.
Issue fork experience_builder-3468269
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
Comment #3
wim leersComment #4
wim leersAnd after this lands, an intriguing potential next step is possible: #3468272: Store the ComponentTreeStructure field property one row per component instance
Comment #5
wim leersLOL, changed issue metadata on the wrong issue 🤪 — that was intended for #3463988-6: HTTP API: new /xb/api/entity-form/{entity_type}/{entity}/{entity_form_mode} route to load form for editing entity fields (meta + non-meta).
Comment #6
tedbowThis seems like a good idea but I would like to look at how it simplifies, hopefully, the code we already have.
I have updated the sample structure in the issue summary because I think it was missing component information for component that were at the tip of the branches. At the end of each tree branch that will have components that either have no slots, only properties, or have slots that don't components in them.
For example
uuid-root-3anduuid4-author3had no component specifiedComment #7
wim leersComment #8
tedbowI think we should do #3469253: Convert test cases to use PHP arrays instead of JSON strings where possible first otherwise the reviewer will be having to review a lot of 1 line long json strings
Comment #9
wim leers#3469253: Convert test cases to use PHP arrays instead of JSON strings where possible is in.
Comment #10
wim leersComment #12
wim leersComment #13
wim leersThis is still very much worth doing. #3469609: Prepare for multiple component types: ComponentTreeStructure should contain Component config entity IDs, not SDC IDs landed last week, and this would be another step in the right direction on the data model front 🙏
Comment #14
wim leersComment #15
kristen polFixing tag
Comment #16
stefdewa commentedHaving a go at the reroll and fixing the pipeline errors, at Drupalcon Barcelona.
Comment #17
stefdewa commentedUpdated MR with latest changes and fixed phpcs violations. Getting the tests green was a bit of a stretch for me. I'm leaving that for anyone with more knowledge of the expierence builder itself.
Comment #18
wim leersThanks, Stef! 😊
Next steps here:
3.2.2 The `field prop` storing the props valuesofdocs/data-model.mdas described in the issue summary.propsJSON blob in/tests/modules/xb_test_config_node_article/config/install/field.field.node.article.field_xb_test.yml, that will fix a bunch of test failures.ComponentTreeStructureTest::testValidation().Comment #20
abhisekmazumdar@stefdewa I hope you don't mind that I tried to spend some time moving this forward with @wim leers' input.
I also made an oopsie and added an unwanted commit to it, which I undid and fixed.
Added a new commit for the
docs/data-model.mdchanges. I hope thats correct changes.Now I want to understand how can I create a correct format props JSON blob for
/tests/modules/xb_test_config_node_article/config/install/field.field.node.article.field_xb_test.ymlThen maybe eventually I can fix the other broken test cases.
Comment #21
wim leersYep, looks good, keep going! 😄
Comment #22
wim leersComment #23
wim leers@longwave, a lot of time has passed since this was created. Assuming that we end up deciding the final direction in #3477428: [PP-1] Refactor (or decide not to) the XB field type to be multi-valued, to de-jsonify the tree, and to reference the field_union type of the prop values, I think it makes sense to close this? 😇
Comment #24
wim leers… but I think #3495625: Remove ComponentTreeItemList::ROOT_UUID from hydration and client-to-server conversion is still worth doing independently of #3477428: [PP-1] Refactor (or decide not to) the XB field type to be multi-valued, to de-jsonify the tree, and to reference the field_union type of the prop values.
Comment #25
longwaveYeah, we haven't needed to do this, it doesn't make much difference either way; if/when we start using JSON database extensions this might make some difference but it's hard to know up front so let's close for now.