Closed (fixed)
Project:
Experience Builder
Component:
Data model
Priority:
Critical
Category:
Task
Assigned:
Unassigned
Reporter:
Created:
22 Jul 2024 at 19:49 UTC
Updated:
25 Sep 2024 at 15:49 UTC
Jump to comment: Most recent
Overview
found in #3460856: Create validation constraint for ComponentTreeStructure
$component_id = $tree->getComponentId($component_instance_uuid);
try {
$props_values = $value->resolveComponentProps($component_instance_uuid);
$component = $this->componentPluginManager->find($component_id);
$this->componentValidator->validateProps($props_values, $component);
}
$props_values = $value->resolveComponentProps($component_instance_uuid); will throw an exception if
the component does not exist or it does exist but there are no props for the component in the props value. But not all components will have props. It could just have slots or I guess could have neither if props or slots.
#3460856 we have to fix the problem of the component not existing at all but here we should set up a test case where the component has no props
Proposed resolution
Check if the component has props defined before trying resolving them.
User interface changes
Issue fork experience_builder-3463188
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:
changes
Comments
Comment #2
wim leersSee
core/modules/system/tests/modules/sdc_test/components/no-props/no-props.component.ymlfor an example of a propless SDC.So yes, this is intended to work. And we should have an explicit test for it. 👍
Comment #4
wim leersGetting a test going.
Comment #5
wim leersTest exists now. And it fails as expected.
Note that the proposed resolution presents
as the only possible choice.
But there's another possible choice: storing an empty list of props even for propless component instances. It keeps the structure simpler and non-conditional. I think that would make future component upgrade paths simpler (e.g. for adding props to a component that is already in use — which #3452756: Ensure querying JSON nested values when parent keys are unknown is possible in all supported databases is laying the groundwork for).
The cost of that simplicity is slightly more storage space being consumed, because there's additional JSON to be stored.
Comment #6
wim leersThanks to the draft MR I pushed, with clear
@todos, I think this is now actionable by novice contributors.Comment #7
wim leersThis is now blocking #3460856: Create validation constraint for ComponentTreeStructure: https://git.drupalcode.org/project/experience_builder/-/merge_requests/1...
Comment #8
tedbowComment #9
wim leers#3460856: Create validation constraint for ComponentTreeStructure is in, can you rebase this? 🙏
Comment #10
wim leersComment #11
wim leersDoesn't need to be @Utkarsh_33 who works on this :)
Comment #12
cosmicdreams commentedIt sounds like what is left for this issue is to get it to pass the tests.
Comment #13
wim leersWith ~1 month of changes, I think a rebase is necessary.
Comment #14
wim leersMerged in upstream, looks like more tests than before are failing.
Comment #16
wim leersComment #17
deepakkm commentedComment #18
deepakkm commentedComment #19
wim leers@deepakkm: Only two nits for you to address, then it's up to @tedbow to review :)
Comment #20
deepakkm commentedComment #21
tedbowComment #22
deepakkm commentedFeedback changes done.
Comment #23
wim leersThe code that was added is inappropriately expanding the API surface, and it's too open-ended. Please tighten it 🙏
Comment #24
tedbowComment #25
wim leersI cannot approve this MR because I started this MR.
Up to you to decide whether this is ready to be merged, @tedbow :)
Comment #26
tedbow@wim leers thanks
I merged in 0.x. Will merge MR if tests pass
Comment #27
tedbowComment #28
wim leersNeeds Felix' sign-off for
… but the changes there are trivial.
So: bypassing review and merging 🚢
Comment #30
wim leers