Create validation constraint for ComponentTreeStructure

Postponing on 📌 FieldType: Support storing component *trees* instead of *lists* Fixed

tedbow → credited Wim Leers → .

I see this now, after I already wrote #3455728-9: FieldType: Support storing component *trees* instead of *lists* → …

Actually … why don't we merge this with 📌 Lift most logic out of ComponentTreeItem::preSave() and into a new validation constraint Needs work — i.e. why don't we expand the scope of that? It's exactly the same problem space!

Per #3456024-15: Lift most logic out of ComponentTreeItem::preSave() and into a new validation constraint → , keeping this issue around, not marking it as a duplicate. This is blocked on #3456024.

This is the next most important issue, because it is in the critical path of 📌 Introduce an example set of representative SDC components; transition from "component list" to "component tree" Fixed , which is itself getting in the critical path of next steps on the UI side.

Pre-emptively assigning to @tedbow — now that #3456024 is nearly done, this should go a lot faster. 🤞

Blocker is in.

This is going in the right direction 👍

1. ComponentTreeStructureTest looks good but is not yet testing all edge cases. I see at least two missing important edge cases:
   1. What if there is a tree of 3 levels deep, and the first level is valid (i.e. all UUIDs under the root also have top-level keys), and the third level is valid, but the second is not? (i.e. a child of the first level does not have its UUID does not exist as a top-level key).
   2. "dangling" aka leftover top-level keys, for components that have been removed. These would effectively be dead weight that continues to exist, taking up storage space while not being visible!
2. Can you add a failing test case to \Drupal\Tests\experience_builder\Kernel\Config\DefaultFieldValueTest that triggers a failure for this new constraint for a config-defined component tree?
3. I think the test cases would be much simpler to read if they weren't defined as JSON strings, but as json_encode([ … stuff … ]) instead.

I just realized an important validation piece is missing if this is to unblock 📌 Introduce an example set of representative SDC components; transition from "component list" to "component tree" Fixed .

re #12

1. 1. What if there is a tree of 3 levels deep, and the first level is valid (i.e. all UUIDs under the root also have top-level keys), and the third level is valid, but the second is not? (i.e. a child of the first level does not have its UUID does not exist as a top-level key).

   and

   👆 these are edge cases that I described succinctly in the issue summary as any top-level UUID appears as a UUID in a _parent branch/tree_(except its own branch)

   I don't think this first case is actually what is described in the summary but rather the inverse(unless we are still not talking about the same of constructing the tree from the storage)

   (i.e. all UUIDs under the root also have top-level keys)

   UUIDs under the root should not need to have top-level keys, some will but components without slots should not need any other information in the tree. I confirmed with Jesse Baker that Components are not required to have slots.

   (i.e. a child of the first level does not have its UUID does not exist as a top-level key).

   There is a typo here I think but again I think a child of the first level should not be required to exist as top level key because the child of the first level might not have a slot that needs representing in the tree.

   So I think this tree would valid but it does pass your suggested validation

   [
     "ROOT_UUID" => [
         [ "uuid" => "UUID-IN-ROOT-NOT-TOP-LEVEL" , "component" => "component-with-no-slots" ] , 
         [ "uuid" => "UUID-IN-ROOT-AT-TOP-LEVEL" , "component" => "component-with-slots" ] , 
     ],
     "UUID-IN-ROOT-AT-TOP-LEVEL" => [
       'firstCol' => [
           [ "uuid" => "UUID-UNDER-1ST-LEVEL-NOT-AT-TOP-LEVEL" , "component" => "component-with-no-slots" ] , 
           [ "uuid" => "UUID-UNDER-1ST-LEVEL-ALSO-TOP-LEVEL" , "component" => "component-with-slots" ] , 
       ]
        
     ],
     "UUID-UNDER-1ST-LEVEL-ALSO-TOP-LEVEL"  => [
         'firstCol' => [
              [ "uuid" => "UUID-UNDER-2ND-LEVEL" , "component" => "component-with-no-slots" ] , 
              [ "uuid" => "UUID-UNDER-2ND-LEVEL" , "component" => "component-with-no-slots" ] , 
        ]
       
     ]
   ]
   

   Sorry for the weird naming basically I thought "component-with-no-slots" will be dead-end in the tree and that is by design.

   I have updated the test cases to use either "component" => "component-no-slots" or "component" => "component-slots" to make this clearer.

2. "dangling" aka leftover top-level keys, for components that have been removed. These would effectively be dead weight that continues to exist, taking up storage space while not being visible!

   I think that is what this existing case is catching

   'INVALID: valid tree, with unknown top level' => [
           [
             ComponentTreeStructure::ROOT_UUID => [["uuid" => "uuid-in-root-only", "component" => "component-no-slots"]],
             "uuid-unknown-top-level" => [["uuid" => "uuid-under-top-level", "component" => "component-no-slots"]],
           ],
           ['The UUID <em class="placeholder">uuid-unknown-top-level</em> is not in the tree.'],
         ],
   

   but I have expanded the tree here
   'INVALID: valid tree, with unknown top level' => [
   [
   ComponentTreeStructure::ROOT_UUID => [
   ["uuid" => "uuid-in-root-only", "component" => "component-no-slots"],
   ["uuid" => "uuid-in-root-and-top-level", "component" => "component-slots"],
   ],
   "uuid-in-root-and-top-level" => [
   "slot1" => [
   ["uuid" => "uuid-1st-tier-top-level", "component" => "component-slots"],
   ],
   ],
   "uuid-1st-tier-top-level" => [
   "slot1" => [
   ["uuid" => "uuid-under-1st-tier", "component" => "component-no-slots"],
   ],
   ],
   "uuid-unknown-top-level" => [
   "slot1" => [
   ["uuid" => "uuid-under-top-level", "component" => "component-no-slots"],
   ],
   ],
   ],
   ['The UUID uuid-unknown-top-level is not in the tree.'],
   ],

3. I fixed some of the test cases that were not nested correctly.
4. RE: Do we want to add validation that if component has slots it should have top level key? My guess is not because that would require use to load the actual component config and also maybe some have optional slots but just putting this out there we don't validate this.

5. Can you add a failing test case to \Drupal\Tests\experience_builder\Kernel\Config\DefaultFieldValueTest that triggers a failure for this new constraint for a config-defined component tree?

   Yes, will do this, but not done yet.

   Should we also add the test cases to ComponentTreeItemTest::testInvalidField() to make it sure it triggers for field instances? Right now they use the same helper function, ComponentTreeTestTrait::getComponentTreeTestCases() to provide test cases so we test the same cases in both places

6. I think the test cases would be much simpler to read if they weren't defined as JSON strings, but as json_encode([ … stuff … ]) instead.

   Agreed, fixed

Leaving assigned to myself to do the remaining items but I need confirmation from @Wim Leers that I am right about 1)

@Wim Leers I am working on this part now

slots of every component: the stored slot names (firstColumn and secondColumn in the example at 📌 FieldType: Support storing component *trees* instead of *lists* Fixed ) MUST be actually existing slot names for this particular component instance.

I haven't actually dealt with this part of the module at all. Any tips here. How do I load a component from an id? Is this always config component as defined by \Drupal\experience_builder\Entity\Component? there is no class comment so hard to tell. Or could this any sdc component. I have loaded a Component config entity but also there I can't see slot info there.

@Wim Leers assigning back to you for questions on #14. If you have pointers for #15 that would be great(otherwise when I get back this issue I will keep looking)

#14:

1. Sorry for the confusion on this one — my wording was a bit too succinct it seems 🙈
   

   some will but components without slots should not need any other information in the tree
   …
   There is a typo here I think but again I think a child of the first level should not be required to exist as top level key because the child of the first level might not have a slot that needs representing in the tree

   Indeed 👍

   So I think this tree would valid but it does pass your suggested validation

   Yes 👍 (And → I think it's excellent naming 😄)

2. I think that is what this existing case is catching

   You're right — my bad! 🙈

3. 👍
4. Regardless of what choice we make here: we should have explicit test coverage for it. Either explicitly test the absence of a validation error, or the presence. AFAICT we don't have such a test case yet.
   • I'm not sure yet which way to go. I think (but it's subjective currently, not yet objective) it'd actually be clearer if we did require that.
   • An objective benefit of that would be that the completeness of the tree can be validated.
   • An objective downside is that this means more JSON to store.
   • Your wording seems to imply that loading the actual component config entity is a downside. I disagree with that. Because \Drupal\experience_builder\Plugin\Validation\Constraint\ValidComponentTreeConstraintValidator::validate() already needs to do that anyway — so either way, the caches would be warm.

   Overall, I'd rather err on the side of more detailed validation for now, to allow us to not have to ever think about this again after this issue — because this issue will immediately catch/flag any mistake we'll make on our way to the full XB functionality scope.

5. Yes, that would be great.

#15: you can load a component from an ID using \Drupal\experience_builder\Entity\Component::loadByComponentMachineName().

@Wim Leers sorry for further confusion

but originally in #14 I typed

So I think this tree would valid but it does pass your suggested validation

But meant this.

So I think this tree would valid but it does NOT pass your suggested validation

You responded to this in #17 so wanted to called that out.

I edited #14

1. uuid-unknown-top-level is danging/leftover and is the expected validation error message … That's why I replied affirmatively. I don't see how that tree would be valid?

re #19

The correction in #18 I was referring to was actually for #14.1

not #14.2

1. Yes, like I already wrote in #17.1 while specifically quoting what you wrote about slotless components. Which means I already confirmed that the list of things to validate was missing that important nuance.

   I agree that the example you provided in #14.1 MUST be considered valid.

   In fact, if UUID-IN-ROOT-NOT-TOP-LEVEL did appear as a top-level key, then THAT would be a violation.

I see I didn't respond to this bit in #15:

I have loaded a Component config entity but also there I can't see slot info there.

• First get the component plugin ID (i.e. \Drupal\Core\Plugin\Component::$machineName) for a Component config entity, by using \Drupal\experience_builder\Entity\Component::convertIdToMachineName().
• Then load the Component plugin instance using plugin.manager.sdc
• Finally: given the plugin instance, use \Drupal\Core\Plugin\Component::$metadata to determine what the slots are.

… adding a helper method to the config entity would be fine, of course.

This is making a pretty drastic change that is not captured in the issue summary: https://git.drupalcode.org/project/experience_builder/-/merge_requests/1... — in that comment I explain why we should revert that and leave such a change for 🌱 [META] Support component types other than SDC Needs work to make.

I also found 2 critical gaps in the test coverage:

• https://git.drupalcode.org/project/experience_builder/-/merge_requests/1...
• https://git.drupalcode.org/project/experience_builder/-/merge_requests/1...

… and a number of additional questions.

@Wim Leers I changed this to use symfony/validator constraints. I think this is better.

I still need to add some test cases for missing keys and type errors but this logic should already be covered by the constraints.

So I have still have work on it. If you want to just wait until I am done, I can just continue working on it. But if you want to look at the approach feel free

Wow, that looks much better! 👏

I'd even use the word impressive — it's nice to see that when we use Symfony's Validation component "as intended", that it is so much clearer than if there's Drupal's layer of abstraction (either config schema or Entity/Field/Typed Data) in between 🤯

I know this issue is not yet marked as (especially because you wrote at https://git.drupalcode.org/project/experience_builder/-/merge_requests/1... that you need more test cases before addressing that), but given its importance, pre-emptively reviewed to make this go faster 😊

I think this is now in its final stretch — the Symfony-based validation logic makes this much easier to understand! 👍

@Wim Leers see MR comments

This looked great, but given its foundational importance, I subjected it to a lot of scrutiny. Some bits I found hard to understand, so I worked to make that easier for the next person. And since there will be many next persons … I spent the entire day on this 😳

Summary of changes I made:

• add detailed documentation about the data structure, including establishing terminology
• use that terminology consistently in validation error messages
• make the validation error messages point to more precise property paths
• properly integrate the "subvalidator"-triggered violations with the Drupal validator — no longer losing property path information
• use the BasicRecursiveValidatorFactory to ensure validation error messages use TranslatableMarkup just like other validators — see https://www.drupal.org/node/3396203 →
• made the (kernel) tests much faster — they were painfully slow 😅

90% of what @tedbow wrote is still there. I just smoothened out the rough edges.

So glad to have this done 😊

🚢

Unblocked:

1. 🐛 Support propless SDCs Needs review
2. 📌 Create validation constraint for ComponentTreeStructure Needs work

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