Prepare for multiple component types: ComponentTreeStructure should contain Component config entity IDs, not SDC IDs

Created on 22 August 2024, 26 days ago
Updated 16 September 2024, about 3 hours ago

Overview

Per ðŸŒą [META] Support component types other than SDC Needs work , we'll need to support additional component types eventually.

But currently, ComponentTreeStructure (the tree prop on the ComponentTreeItem field type) is storing SDC plugin IDs.

Which means that the data model is currently tightly coupled to SDCs.

Proposed resolution

Change those to Component config entity IDs.

IOW, this issue must make the following documentation change a reality:

diff --git a/docs/data-model.md b/docs/data-model.md
index c5eea807..77eca2bd 100644
--- a/docs/data-model.md
+++ b/docs/data-model.md
@@ -243,22 +243,22 @@ Example:
 ```json
 {
   "ROOT_UUID": [
-    {"uuid": "uuid-root-1", "component": "provider:two-col"},
-    {"uuid": "uuid-root-2", "component": "provider:marquee"},
-    {"uuid": "uuid-root-3", "component": "provider:marquee"}
+    {"uuid": "uuid-root-1", "component": "provider+two-col"},
+    {"uuid": "uuid-root-2", "component": "provider+marquee"},
+    {"uuid": "uuid-root-3", "component": "provider+marquee"}
   ],
   "uuid-root-1": {
     "firstColumn": [
-      {"uuid": "uuid4-author1", "component": "provider:person-card"},
-      {"uuid": "uuid2-submitted", "component": "provider:elegant-date"}
+      {"uuid": "uuid4-author1", "component": "provider+person-card"},
+      {"uuid": "uuid2-submitted", "component": "provider+elegant-date"}
     ],
     "secondColumn": [
-      {"uuid": "uuid5-author2", "component": "provider:person-card"}
+      {"uuid": "uuid5-author2", "component": "provider+person-card"}
     ]
   },
   "uuid-root-2": {
     "content": [
-      {"uuid": "uuid4-author3", "component": "provider:person-card"}
+      {"uuid": "uuid4-author3", "component": "provider+person-card"}
     ]
   }
 }

User interface changes

Zero changes.

📌 Task
Status

Needs work

Component

Data model

Created by

🇧🇊Belgium Wim Leers Ghent 🇧🇊🇊🇚

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @Wim Leers
  • 🇧🇊Belgium Wim Leers Ghent 🇧🇊🇊🇚
  • 🇧🇊Belgium Wim Leers Ghent 🇧🇊🇊🇚
  • 🇧🇊Belgium Wim Leers Ghent 🇧🇊🇊🇚
  • ðŸ‡ŪðŸ‡ģIndia abhisekmazumdar India

    I want to pick this up but before that, can I get some doubts:

    1. Do we need to make changes to ComponentTreeStructure so that the said format of config are generated?
    2. All of these changes are only resticted to the field ComponentTreeItem's tree prop?
    3. To make it work in the existing system locally, I will just need to create a layout using these said components?
  • 🇧🇊Belgium Wim Leers Ghent 🇧🇊🇊🇚

    Yay, thank you, @abhisekmazumdar! 😊

    1. No: zero changes are necessary to that. Only docs changes, like this:
      diff --git a/src/Plugin/DataType/ComponentTreeStructure.php b/src/Plugin/DataType/ComponentTreeStructure.php
      index afbc656d..7685c241 100644
      --- a/src/Plugin/DataType/ComponentTreeStructure.php
      +++ b/src/Plugin/DataType/ComponentTreeStructure.php
      @@ -56,6 +56,8 @@ use Drupal\Core\TypedData\TypedData;
        * @see \Drupal\experience_builder\Plugin\Validation\Constraint\ComponentTreeStructureConstraintValidator
        * @see \Drupal\Tests\experience_builder\Kernel\DataType\ComponentTreeStructureTest
        *
      + * @phpstan-type ComponentConfigEntityId string
      + *
        * @todo Implement ListInterface because it conceptually fits, but â€Ķ what does it get us?
        */
       #[DataType(
      @@ -243,7 +245,10 @@ class ComponentTreeStructure extends TypedData {
          * @param string $component_instance_uuid
          *   The UUID of a placed component instance.
          *
      -   * @return string
      +   * @return ComponentConfigEntityId
      +   *   A Component config entity ID.
      +   *
      +   * @see \Drupal\experience_builder\Entity\Component
          */
         public function getComponentId(string $component_instance_uuid): string {
           if (!in_array($component_instance_uuid, $this->getComponentInstanceUuids(), TRUE)) {
      @@ -257,7 +262,7 @@ class ComponentTreeStructure extends TypedData {
         }
       
         /**
      -   * @return array<string>
      +   * @return array<ComponentConfigEntityId>
          */
         public function getComponentIdList(): array {
           return array_values(array_unique(array_column($this->getComponents(), 'component')));
      
      
    2. No, it also affects config, in particular: config/optional/field.field.node.article.field_xb_demo.yml. But it's the exact same change, because both content and config are validated by \Drupal\experience_builder\Plugin\Validation\Constraint\ComponentTreeStructureConstraintValidator(). That is the file you'll have to make logic changes in. See the first paragraph of https://wimleers.com/xb-week-12 for context.
    3. No, "just" doing that using the UI is impossible until all the code has been updated first 😅 I recommend starting with updating the expectations in ComponentTreeStructureTest to match what I've described in the issue summary.
  • 🇧🇊Belgium Wim Leers Ghent 🇧🇊🇊🇚

    D'oh, I guess this issue interestingly sits at the intersection of and : it's an update to the data model to correctly use future config management stuff.

    Undoing what I did in #4 🙈

  • Assigned to abhisekmazumdar
  • ðŸ‡ŪðŸ‡ģIndia abhisekmazumdar India

    This makes sense to me. I will make these changes.

  • 🇧🇊Belgium Wim Leers Ghent 🇧🇊🇊🇚

    🎉 Thank you! :)

  • 🇧🇊Belgium Wim Leers Ghent 🇧🇊🇊🇚
  • ðŸ‡ŪðŸ‡ģIndia abhisekmazumdar India

    abhisekmazumdar → changed the visibility of the branch 3469609-component-tree-component-config-entity-ids to hidden.

  • ðŸ‡ŪðŸ‡ģIndia abhisekmazumdar India

    abhisekmazumdar → changed the visibility of the branch 3469609-component-tree-component-config-entity-ids to active.

  • Pipeline finished with Failed
    19 days ago
    Total: 415s
    #267287
  • Issue was unassigned.
  • ðŸ‡ŪðŸ‡ģIndia abhisekmazumdar India

    I will need some more help here. This is what I understand so far:

    1. I'm able to stop the debugger for ComponentTreeStructure and see the different structure for the components which now don't have the sdc names.
    2. In the WIP MR, I have made the changes suggested in #6(Point 1) and description.
    3. I export config and compare the newly created YAML for field.field.node.article.field_xb_demo, which doesn't have much of a difference in the config nor for the default content.
    4. I can run phpunit -c core modules/contrib/experience_builder/tests/src/Kernel/DataType/ComponentTreeStructureTest.php and see it all green.
    5. I understand that I still need to make changes to Constraint\ComponentTreeStructureConstraintValidator but I'm not sure how and where.

    This is what I need to understand:

    1. How can I ensure that what I'm doing for the ComponentTreeStructureTest is correct? I don't fully understand the tests.
    2. Again, what changes will be needed for ComponentTreeStructureConstraintValidator, and how can I debug it?

    I also understand this is a critical issue, and unassigning this from me. If someone already has the experience to do it quickly, please take it over.
    I will pick it up if I get my answer or figure it out.

  • 🇧🇊Belgium Wim Leers Ghent 🇧🇊🇊🇚

    #14:

    1. That config is not in the MR? You can't just export it though, you'd need to manually modify that default config right now.
    2. The only reason that test passes is because you haven't updated the validator yet (next point). 😄
    3. This is the crucial next step. 😊

    Besides updating the validator, the other crucial next step is to update the logic in the XB field type and the hydrated computed property's logic due to the tree field property now containing component config entity IDs instead of SDC plugin IDs. Did that for you in https://git.drupalcode.org/project/experience_builder/-/merge_requests/2... 👈 Thanks to this commit, as soon as you update the default config (#14.3), everything should render once again. But you probably need to disable the validator temporarily, until you've updated its logic, because that will (should) complain about invalid config.

    WRT understanding:

    1. These are tightly scoped tests. They test only a single validator. I'm confident you can make sense of them 😊 Tip: the names (keys) of the test cases returned by the provider describe what the test case is testing.
    2. I've made one example update to the test cases in https://git.drupalcode.org/project/experience_builder/-/merge_requests/2... — all other test cases must be updated similarly. You can see that this is exactly like the changes to docs/data-model.md. You'll see that now the test starts to fail, so it points to how to validator logic will need to be updated :)

    🏓 Back to you — you can do it! 😊

  • Assigned to abhisekmazumdar
  • Pipeline finished with Failed
    14 days ago
    Total: 544s
    #271654
  • Pipeline finished with Failed
    14 days ago
    Total: 345s
    #271773
  • Issue was unassigned.
  • Status changed to Needs review 14 days ago
  • ðŸ‡ŪðŸ‡ģIndia abhisekmazumdar India

    Thank you, @Wim Leers, for the detailed input & believing in me 😁

    🏓 The MR is still a work in progress, so it is not completely ready for review. However, I seek some answers to the questions I have asked over the MR. Really appreciate your help.

  • Status changed to Needs work 11 days ago
  • 🇧🇊Belgium Wim Leers Ghent 🇧🇊🇊🇚

    I do totally believe in you! 😄

    Also, I very much messed up the sample commits and issue summary 🙈

    Issue summary fixed, and mistake rectified on the MR: https://git.drupalcode.org/project/experience_builder/-/merge_requests/2....

  • Assigned to abhisekmazumdar
  • Pipeline finished with Failed
    7 days ago
    Total: 533s
    #278181
  • Pipeline finished with Failed
    7 days ago
    Total: 556s
    #278290
  • Issue was unassigned.
  • Status changed to Needs review 7 days ago
  • ðŸ‡ŪðŸ‡ģIndia abhisekmazumdar India

    Done:

    • Made all the suggested changes to the best of my knowledge.

    Todo:

    1. The ComponentPropsForm is not working for the above said reasons.
    2. I see the CI is failing for phpcs which are unrelated to this MR.
    3. The Cypress tests are also failing for which I'm not sure about.

    Please review and give feedback.

  • ðŸ‡ŪðŸ‡ģIndia abhisekmazumdar India

    I will check and rebuild the XB with a fresh setup. Some of the outstanding TODO have been fixed:

    Remaining TODO:

    1. I see the CI is failing for phpcs which are unrelated to this MR.
    2. The Cypress tests are also failing for which I'm not sure about.

    For these, I still need feedback.

  • Status changed to Needs work 4 days ago
  • 🇧🇊Belgium Wim Leers Ghent 🇧🇊🇊🇚

    Wow those phpcs failures are super weird! ðŸĪŠ

    The first next step: making the phpunit tests pass — they have many failures at the moment: https://git.drupalcode.org/issue/experience_builder-3469609/-/jobs/2697530

    Those need to pass first, before the UI will be able to work and before the Cypress E2E tests can possibly pass 😊

  • Assigned to abhisekmazumdar
  • Pipeline finished with Failed
    about 6 hours ago
    #284447
  • Pipeline finished with Failed
    about 5 hours ago
    Total: 561s
    #284468
  • Pipeline finished with Failed
    about 5 hours ago
    #284475
  • ðŸ‡ŪðŸ‡ģIndia abhisekmazumdar India

    I tried setting up the xdebugger on my local machine to make it work with the current local setup I have. Setting up the xdebugger correctly will give me a much clearer idea of what is breaking during the test.
    Yet I was unable to make Xdebugger work for the unit test cases.

  • ðŸ‡ŪðŸ‡ģIndia abhisekmazumdar India

    Okay, I was successfully able to make the debugger work. It works out of the box, but I need to click the continue button one more time to stop it at the required mark.

    @Wim Leers

    I see mostly the \Drupal\Tests\experience_builder\Kernel\DataType\ComponentTreeStructureTest::testValidation is creating problem. For our case should we just update providerValidation data to match it with what its actually trying to assert?

    Or should I be looking at why the assertion is breaking ?

Production build 0.71.5 2024