Problem/Motivation
\Drupal\experience_builder\Plugin\DataType\ComponentTreeStructure::$tree (used by \Drupal\experience_builder\Plugin\Field\FieldType\ComponentTreeItem) can currently only store lists of components:
/**
* The parsed data value.
*
* @var array<int, array{'uuid': string, 'type': string}>
*/
protected array $tree = [];
its values look like this:
[
{"uuid":"dynamic-image-udf7d","type":"experience_builder:image"},
{"uuid":"static-static-card1ab","type":"sdc_test:my-cta"},
{"uuid":"dynamic-static-card2df","type":"sdc_test:my-cta"},
{"uuid":"dynamic-dynamic-card3rr","type":"sdc_test:my-cta"},
{"uuid":"dynamic-image-static-imageStyle-something7d","type":"experience_builder:image"}
]
— citing config/optional/field.field.node.article.field_xb_demo.yml
This means it is currently unable to record which components are placed in which slots — all components are effectively placed in the virtual <root> slot! Which is why … it's not a tree, but a list.
Steps to reproduce
N/A
Proposed resolution
- Update
\Drupal\experience_builder\Plugin\DataType\ComponentTreeStructure::$treeto support storing a tree, like this:
{ "<root>": [ {uuid: <uuid1>, component: "provider:two-col"}, {uuid: <uuid3-standfirst>, component: "provider:marquee"}, ], "<uuid1>": { firstColumn: [ {uuid: <uuid4-author>, component: "provider:person-card"}, {uuid: <uuid2-submitted>, component: "provider:elegant-date"} ], secondColumn: [ {uuid: <uuid5-main-image>, component: "provider:image-hero"} ], } }with
<root>being a UUID too, but one that is hardcoded as a constant:class ComponentTreeStructure extends TypedData { const ROOT_UUID = '…'; } - No update path.
- Update
\Drupal\experience_builder\Plugin\Field\FieldWidget\TwoTerribleTextAreasWidgetaccordingly, to ensure editing remains possible. - Update
config/optional/field.field.node.article.field_xb_demo.ymlandEndToEndDemoIntegrationTestaccordingly, to ensure that upon installation in the Standard install profile, XB continues to provide the exact same demo experience. - Validation constraint for
ComponentTreeStructure, see https://git.drupalcode.org/project/experience_builder/-/merge_requests/6...
Remaining tasks
All of the above.
User interface changes
Yes: TwoTerribleTextAreasWidget supports more, but it can stay equally terrible.
API changes
N/A
Data model changes
N/A
Issue fork experience_builder-3455728
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 #2
wim leersComment #3
tedbowI am taking a look at this
Comment #5
tedbowComment #6
wim leersThere is a pretty obvious inconsistency in the test data, which is why the scope needs to be expanded to also add test coverage: https://git.drupalcode.org/project/experience_builder/-/merge_requests/6....
Comment #7
wim leersAdd missing comma to proposed resolution.
Comment #8
bhuvaneshwar commentedComment #9
wim leersOn a meeting just now I said I was fine with this, but … I now see that the constraint was part of this issue's scope, as mentioned in the issue summary.
Having actually reviewed the MR, I am now convinced by that even more, because since #3452397: Allow specifying default props values when opting an SDC in for XB landed, we now have strict config schema checking for all tests.
Having explicit test coverage for this will also allow us to validate the default value in
config/optional/field.field.node.article.field_xb_demo.yml— similar to what #3456024: Lift most logic out of ComponentTreeItem::preSave() and into a new validation constraint is doing. (I made that part working — see https://git.drupalcode.org/project/experience_builder/-/blob/f0048333ab1... 🤓)Comment #10
wim leersComment #11
wim leersI wrote #9 before I saw #3460856: Create validation constraint for ComponentTreeStructure.
Why don't we merge#3460856 with #3456024: Lift most logic out of ComponentTreeItem::preSave() and into a new validation constraint — i.e. why don't we expand the scope of that? It's exactly the same problem space!
IOW: I'm still fine with merging this first.
Comment #12
tedbowThe pipeline https://git.drupalcode.org/project/experience_builder/-/pipelines/222097 says it is "blocked" but I am not sure what is blocked on
I ran all the jobs and it passed
UPDATE: I ran all the jobs manually and now it is marked as failed because "pages" failed😢
Comment #13
wim leersI have no idea how the
pagesCI job is getting triggered for you 😅🤪 Never had that happen!This is obviously actually passing all tests.
I just fixed a few nits, and I see the same status as the summary for the overall CI pipeline. That's just GitLab CI being silly/dumb: it's because the
pagesCI job is the last CI job (in the last stage), and hence that becomes the status/summary.I think we can get rid of it by removing the
manualrule — then it just won't show up for MRs — and that's indeed working: https://git.drupalcode.org/project/experience_builder/-/merge_requests/6... 👍Comment #14
wim leersComment #16
wim leers