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 root just as another component which is really just a list of other components, and move component inside 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 alongside component and slots?

Proposed resolution

I agree with @longwave that this simplification makes sense — it'd also simplify various other pieces:

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

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

wim leers’s picture

Priority: Major » Critical
Issue summary: View changes
Issue tags: +Needs title update
Parent issue: » #3450586: [META] Back-end Kanban issue tracker

And after this lands, an intriguing potential next step is possible: #3468272: Store the ComponentTreeStructure field property one row per component instance

wim leers’s picture

Priority: Critical » Major
Issue tags: -Needs title update
tedbow’s picture

Issue summary: View changes

This 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-3 and uuid4-author3 had no component specified

wim leers’s picture

Assigned: Unassigned » tedbow
tedbow’s picture

Title: `ComponentTreeStructure` date type: simplify the stored structure » [PP-1] `ComponentTreeStructure` date type: simplify the stored structure
Status: Active » Postponed
Related issues: +#3469253: Convert test cases to use PHP arrays instead of JSON strings where possible

I 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

wim leers’s picture

Title: [PP-1] `ComponentTreeStructure` date type: simplify the stored structure » `ComponentTreeStructure` date type: simplify the stored structure
Status: Postponed » Active
wim leers’s picture

Title: `ComponentTreeStructure` date type: simplify the stored structure » `ComponentTreeStructure` data type: simplify the stored structure

wim leers’s picture

wim leers’s picture

Assigned: tedbow » Unassigned
Issue tags: +Needs reroll, +Novice

This 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 🙏

wim leers’s picture

Issue tags: +DrupalCon Barcelona 2024
kristen pol’s picture

Version: » 0.x-dev
Issue tags: -DrupalCon Barcelona 2024 +Barcelona2024

Fixing tag

stefdewa’s picture

Having a go at the reroll and fixing the pipeline errors, at Drupalcon Barcelona.

stefdewa’s picture

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

wim leers’s picture

Status: Active » Needs work
Issue tags: -Needs documentation updates, -Needs reroll, -Novice +Needs documentation updates Novice

Thanks, Stef! 😊

Next steps here:

  1. Update the sample JSON blob in section 3.2.2 The `field prop` storing the props values of docs/data-model.md as described in the issue summary.
  2. Update the props JSON 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.
  3. Then, the lowest-level tests that fails (and hence should be fixed first) is ComponentTreeStructureTest::testValidation().

abhisekmazumdar made their first commit to this issue’s fork.

abhisekmazumdar’s picture

@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.md changes. 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.yml

Then maybe eventually I can fix the other broken test cases.

wim leers’s picture

Issue tags: -Needs documentation updates Novice +Novice

Yep, looks good, keep going! 😄

wim leers’s picture

Issue tags: -Novice +stable blocker
wim leers’s picture

Assigned: Unassigned » longwave
Status: Needs work » Reviewed & tested by the community
Issue tags: -stable blocker

@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? 😇

longwave’s picture

Assigned: longwave » Unassigned
Status: Reviewed & tested by the community » Closed (won't fix)

Yeah, 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.