- Issue created by @tedbow
- Status changed to Postponed
5 months ago 4:12pm 11 July 2024 - πΊπΈUnited States tedbow Ithaca, NY, USA
Postponing on π FieldType: Support storing component *trees* instead of *lists* Fixed
- π§πͺBelgium wim leers Ghent π§πͺπͺπΊ
tedbow β credited Wim Leers β .
- π§πͺBelgium wim leers Ghent π§πͺπͺπΊ
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!
- Assigned to tedbow
- π§πͺBelgium wim leers Ghent π§πͺπͺπΊ
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. π€
- Status changed to Active
5 months ago 6:41pm 17 July 2024 - Status changed to Needs work
5 months ago 7:47pm 17 July 2024 - π§πͺBelgium wim leers Ghent π§πͺπͺπΊ
This is going in the right direction π
ComponentTreeStructureTest
looks good but is not yet testing all edge cases. I see at least two missing important edge cases:- 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).
- "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!
- 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? - 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.
- π§πͺBelgium wim leers Ghent π§πͺπͺπΊ
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 .
- πΊπΈUnited States tedbow Ithaca, NY, USA
re #12
-
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. -
"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.'],
], - I fixed some of the test cases that were not nested correctly.
- 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.
-
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 -
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)
-
- πΊπΈUnited States tedbow Ithaca, NY, USA
@Wim Leers I am working on this part now
slots of every component: the stored slot names (
firstColumn
andsecondColumn
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. - Assigned to wim leers
- Status changed to Needs review
5 months ago 7:13pm 18 July 2024 - πΊπΈUnited States tedbow Ithaca, NY, USA
- Assigned to tedbow
- Status changed to Needs work
5 months ago 11:10am 19 July 2024 - π§πͺBelgium wim leers Ghent π§πͺπͺπΊ
#14:
- 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 treeIndeed π
So I think this tree would valid but it does pass your suggested validation
Yes π (And β I think it's excellent naming π)
-
I think that is what this existing case is catching
You're right β my bad! π
- π
- 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.
- Yes, that would be great.
#15: you can load a component from an ID using
\Drupal\experience_builder\Entity\Component::loadByComponentMachineName()
. - Sorry for the confusion on this one β my wording was a bit too succinct it seems π
- πΊπΈUnited States tedbow Ithaca, NY, USA
@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
- π§πͺBelgium wim leers Ghent π§πͺπͺπΊ
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?
- π§πͺBelgium wim leers Ghent π§πͺπͺπΊ
- 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 usingplugin.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.
- 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.
- Assigned to wim leers
- Status changed to Needs review
5 months ago 10:38pm 22 July 2024 - Assigned to tedbow
- Status changed to Needs work
5 months ago 1:42pm 23 July 2024 - π§πͺBelgium wim leers Ghent π§πͺπͺπΊ
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.
- Assigned to wim leers
- Status changed to Needs review
5 months ago 7:39pm 25 July 2024 - πΊπΈUnited States tedbow Ithaca, NY, USA
@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
- Assigned to tedbow
- Status changed to Needs work
5 months ago 8:03am 26 July 2024 - π§πͺBelgium wim leers Ghent π§πͺπͺπΊ
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 π€―
- π§πͺBelgium wim leers Ghent π§πͺπͺπΊ
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! π
- Assigned to wim leers
- Status changed to Needs review
5 months ago 9:23pm 29 July 2024 - Issue was unassigned.
- Status changed to RTBC
5 months ago 4:34pm 30 July 2024 - π§πͺBelgium wim leers Ghent π§πͺπͺπΊ
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 useTranslatableMarkup
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 π
-
Wim Leers β
committed 7047b8a8 on 0.x authored by
tedbow β
Issue #3460856 by tedbow, Wim Leers: Create validation constraint for...
-
Wim Leers β
committed 7047b8a8 on 0.x authored by
tedbow β
- Status changed to Fixed
5 months ago 4:35pm 30 July 2024 - π§πͺBelgium wim leers Ghent π§πͺπͺπΊ
π’
Unblocked:
- π Support propless SDCs Needs review
- π Create validation constraint for ComponentTreeStructure Needs work
Automatically closed - issue fixed for 2 weeks with no activity.