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.

This blocks #3446722-14: Introduce an example set of representative SDC components; transition from "component list" to "component tree".

Steps to reproduce

N/A

Proposed resolution

  • Update \Drupal\experience_builder\Plugin\DataType\ComponentTreeStructure::$tree to 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\TwoTerribleTextAreasWidget accordingly, to ensure editing remains possible.
  • Update config/optional/field.field.node.article.field_xb_demo.yml and EndToEndDemoIntegrationTest accordingly, 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

Command icon 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

Wim Leers created an issue. See original summary.

wim leers’s picture

Issue summary: View changes
tedbow’s picture

Assigned: Unassigned » tedbow
Issue summary: View changes

I am taking a look at this

tedbow’s picture

Assigned: tedbow » Unassigned
Status: Active » Needs review
wim leers’s picture

Issue summary: View changes
Status: Needs review » Needs work
Issue tags: +validation, +data integrity

There 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....

wim leers’s picture

Issue summary: View changes

Add missing comma to proposed resolution.

bhuvaneshwar’s picture

Assigned: Unassigned » bhuvaneshwar
wim leers’s picture

  1. 🥳 It works! 😄
  2. // @todo This is a temporary checks until we have a constraint. Just to make
        //   tests fail if we don't have good test values.
        if (!isset($this->tree[ComponentTreeStructure::ROOT_UUID])) {
          throw new \UnexpectedValueException('Temp exception replace with constraint. Root UUID is missing.');
        }
    

    On 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... 🤓)

wim leers’s picture

wim leers’s picture

I 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.

tedbow’s picture

Assigned: tedbow » wim leers
Status: Needs work » Needs review

The 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😢

wim leers’s picture

I have no idea how the pages CI 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 Blocked status as the summary for the overall CI pipeline. That's just GitLab CI being silly/dumb: it's because the pages CI 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 manual rule — then it just won't show up for MRs — and that's indeed working: https://git.drupalcode.org/project/experience_builder/-/merge_requests/6... 👍

wim leers’s picture

Assigned: wim leers » Unassigned
Status: Needs review » Reviewed & tested by the community

  • Wim Leers committed a1410b6f on 0.x authored by tedbow
    Issue #3455728 by tedbow, Wim Leers: FieldType: Support storing...
wim leers’s picture

Status: Reviewed & tested by the community » Fixed

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.