Lift most logic out of ComponentTreeItem::preSave() and into a new validation constraint

Working on this

Assigning back to @tedbow to address feedback.

@larowlan, thanks for the review. Addressed your feedback

@larowlan on 2nd thought I am going to unassign this because I think your feedback was pretty straight forward. Welcome your review though

This part of the issue summary is not yet complete:

1. allow that same logic to be reused in multiple things of 🌱 [META] Configuration management: define needed config entity types Active , starting with the "component" config entity that 📌 "Developer-created components": mark which SDCs should be exposed in XB Fixed introduced

… but I laid the groundwork for you to make it easy:

1. https://git.drupalcode.org/project/experience_builder/-/merge_requests/6... → this introduced the failure 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 BLA
2. https://git.drupalcode.org/project/experience_builder/-/merge_requests/6... → that opts in this particular test to strict config schema checking, to actually see the aforementioned fake validation error
3. https://git.drupalcode.org/project/experience_builder/-/merge_requests/6... → allows reusing the existing validation logic you wrote :)

This 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.

📌 FieldType: Support storing component *trees* instead of *lists* Fixed 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: [PP-1] Create validation constraint for ComponentTreeStructure → .

@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.

This was kinda heading in the right direction! 👍

Changes I made:

• I made the constraint violation test coverage use the same test infrastructure as Drupal core.
• Logic was added to ValidComponentTreeConstraintValidator::validate() that re-ran the same validation logic — removed.
• Due to Typed Data magic, it was impossible to get to the values actually passed to the field instance — fixed that 👍

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.props line from config/optional/field.field.node.article.field_xb_demo.yml, and you'll get this test failure:

1) Drupal\Tests\experience_builder\Functional\EndToEndDemoIntegrationTest::test
Drupal\Core\Config\Schema\SchemaIncompleteException: Schema errors for field.field.node.article.field_xb_demo with the following errors: 0 [default_value.0] The array must contain a "props" key.

Now 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.

Added 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

I was going to RTBC this, after self-fixing my review feedback (and making the connection to 🌱 [later phase] [META] 7. Content type templates — aka "default layouts" — affects the tree+props data model Active 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 📌 Clarify "components" vs "elements" vs "patterns" Active ).

This is more complex than I hoped, so let's once #14 is addressed, let's land this, and let's not fold 📌 Create validation constraint for ComponentTreeStructure Needs work into this. Instead, this is now definitely the blocker for #3460856.

RE

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.

Isn't this covered in

\Drupal\Tests\experience_builder\Kernel\Plugin\Field\FieldType\ComponentTreeItemTest::testDefaultFieldValue

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?

Ah … 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.php instead? 🙏 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 😊🙏

Did #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 for testCalculateDependencies() which I think was confusing and made changes that were not needed for the new test method to pass. See MR comments

Your own remark https://git.drupalcode.org/project/experience_builder/-/merge_requests/6... is not yet addressed, and there is a suggested solution for it.

Please move the 2 commits you added to comply with an upstream change to 📌 CI: phpcs job failing due to upstream changes in Drupal core: unused variables in catch statements now disallowed Fixed , I just created that after @bnjmnm asked for help to figure out why 📌 End-to-end test that tests both the client (UI) and server Active was mysteriously failing. 😅

📌 CI: phpcs job failing due to upstream changes in Drupal core: unused variables in catch statements now disallowed Fixed is in 🚢

re #19 Created the follow-up 📌 Create a new Exception type for a dynamic prop missing a host entity Active and fixed 📌 CI: phpcs job failing due to upstream changes in Drupal core: unused variables in catch statements now disallowed Fixed

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