Come together with the global Drupal community in Rotterdam, 28 Sept – 1 Oct 2026. Sessions, contribution, connection, and Early Bird savings until 8 June.Problem/Motivation
This logic:
// Validate that each prop source resolves into a value that is considered
// valid by the destination SDC prop.
// @todo Move to validation constraint.
foreach ($component_instance_uuids as $component_instance_uuid) {
$component_id = $tree->getComponentId($component_instance_uuid);
$props_values = $this->resolveComponentProps($component_instance_uuid);
try {
$component = $this->getComponentPluginManager()->find($component_id);
$this->getComponentValidator()->validateProps($props_values, $component);
}
catch (ComponentNotFoundException $e) {
throw new \LogicException(sprintf('The component instance with UUID %s uses component %s but does not exist! Put a breakpoint here and figure out why.', $component_instance_uuid, $component_id));
}
catch (InvalidComponentException $e) {
throw new \LogicException(sprintf('The component instance with UUID %s uses component %s and receives some invalid props! Put a breakpoint here and figure out why.', $component_instance_uuid, $component_id));
}
}
… belongs in a validation constraint.
It'll:
- simplify the field type logic
- allow that same logic to be reused in multiple things of #3444424: [META] Configuration management: define needed config entity types, starting with the "component" config entity that #3444417: "Developer-created components": mark which SDCs should be exposed in XB introduced
Steps to reproduce
Proposed resolution
Do the above.
If #3452397: Allow specifying default props values when opting an SDC in for XB lands before this, its config schema should be updated to use this new validation constraint too.
Remaining tasks
User interface changes
API changes
Data model changes
Issue fork experience_builder-3456024
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
tedbowWorking on this
Comment #4
tedbowComment #5
lauriiiAssigning back to @tedbow to address feedback.
Comment #6
tedbow@larowlan, thanks for the review. Addressed your feedback
Comment #7
tedbow@larowlan on 2nd thought I am going to unassign this because I think your feedback was pretty straight forward. Welcome your review though
Comment #8
wim leersThis part of the issue summary is not yet complete:
… but I laid the groundwork for you to make it easy:
Drupal\Core\Config\Schema\SchemaIncompleteException: Schema errors for field.field.node.article.field_xb_demo with the following errors: 0 [default_value.0] BLA BLA BLAThis now really still needs tests, that verify invalid values are caught, both for component tree field instances on actual nodes, as well as in default field values in config.
Comment #9
wim leers#3455728: FieldType: Support storing component *trees* instead of *lists* is in — let's get this done next! First let's get this to green, and then I think merging #3460856 into this will be the least amount of effort, as described over at #3460856-5: Create validation constraint for ComponentTreeStructure.
Comment #10
tedbow@Wim Leers I am still working on this(but finished for the day) but feel to take a look to see if totally on the wrong track anywhere.
Comment #11
wim leersThis was kinda heading in the right direction! 👍
Changes I made:
ValidComponentTreeConstraintValidator::validate()that re-ran the same validation logic — removed.Definitely tricky stuff!
The piece still missing from this MR: explicit test coverage for
type: type: field.value.component_tree— i.e. when there is a component tree defined in configuration.Manually triggering this is easy: delete the
default_value.0.propsline fromconfig/optional/field.field.node.article.field_xb_demo.yml, and you'll get this test failure:Comment #12
wim leersNow it's green.
What remains is the test coverage mentioned above. And I think it'd be good to tighten the test slightly: no extraneous keys should be allowed either.
Comment #13
tedbowAdded more test cases. I think this is ready for review.
I have 1 question/comment on the MR but I think that can be handled in follow-up, if needed
Comment #14
wim leersI was going to RTBC this, after self-fixing my review feedback (and making the connection to #3455629: [PP-1] [META] 7. Content Templates — aka "default layouts" — affects the tree+props data model explicit) and implement my proposed solution to the question/comment on the MR you referred to in #13.
But then I realized that the config test coverage I requested at the bottom of #11 is not yet present. That's a crucial piece, and it helps pave the path for #3455629 (as well as "patterns", whose definition is currently still vague, see #3455036: Clarify "components" vs "elements" vs "patterns").
Comment #15
wim leersThis is more complex than I hoped, so let's once #14 is addressed, let's land this, and let's not fold #3460856: Create validation constraint for ComponentTreeStructure into this. Instead, this is now definitely the blocker for #3460856.
Comment #16
tedbowRE
Isn't this covered in
where it expects the error
Schema errors for field.field.node.article.field_xb_test with the following errors: 0 [default_value.0] The array must contain a "tree" key.. This almost the same error you pointed out just for different field. This is save configuration or did you mean explicitly in config .yml file?Comment #17
wim leersAh … that's fair! 😅 I didn't realize that because it is in
ComponentTreeItemTest. Sorry! 🙈Can you please move that into a new
tests/src/Kernel/Config/DefaultFieldValueTest.phpinstead? 🙏 I think that'd be clearer, and make it easier to find this test, and expand it in the future.Once you've done that, definitely remove the and re-assign to me for review 😊🙏
Comment #18
tedbowDid #17, had to make new test trait for this. Also made some change to `ComponentTreeItemTest` because in refactoring to the new test class I realized
setUp()and the class constants where needed fortestCalculateDependencies()which I think was confusing and made changes that were not needed for the new test method to pass. See MR commentsComment #19
wim leersYour own remark https://git.drupalcode.org/project/experience_builder/-/merge_requests/6... is not yet addressed, and there is a suggested solution for it.
Because that needs to be addressed anyway: please also move the 2 commits you added to comply with an upstream change to #3462143: CI: phpcs job failing due to upstream changes in Drupal core: unused variables in catch statements now disallowed, I just created that after @bnjmnm asked for help to figure out why #3461435: End-to-end test that tests both the client (UI) and server was mysteriously failing. 😅
Comment #20
wim leers#3462143: CI: phpcs job failing due to upstream changes in Drupal core: unused variables in catch statements now disallowed is in 🚢
Comment #21
tedbowre #19 Created the follow-up #3462160: Create a new Exception type for a dynamic prop missing a host entity and fixed #3462143: CI: phpcs job failing due to upstream changes in Drupal core: unused variables in catch statements now disallowed
Comment #22
tedbowComment #23
wim leersComment #25
wim leersComment #26
wim leers