Overview

    Create image component: 
    const Image = ({
  image
}) => {
  return (
    <img {...image} />
  );
};


export default Image;

    With these props

  1. Add this to library
  2. Try to add this to page
  3. Fatal error 💥

    4. Proposed resolution

    User interface changes

CommentFileSizeAuthor
#2 Screenshot 2025-04-15 at 12.39.10 AM.png39.46 KBlarowlan

Issue fork experience_builder-3519179

Command icon 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:

About issue forks

Comments

larowlan created an issue. See original summary.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Issue summary: View changes
StatusFileSize
new Screenshot 2025-04-15 at 12.39.10 AM.png39.46 KB
larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Assigned: Unassigned » larowlan

larowlan opened merge request !902

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Assigned: larowlan » Unassigned
Status: Active » Needs review
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Assigned: Unassigned » wim leers
Status: Needs review » Needs work
Issue tags: +Needs tests
Related issues: +#3501290: Introduce unit test coverage for both ComponentSource plugins (Block + SDC)

I'd like to ensure we never again run into this problem. We've only last week started to get really serious about ComponentSource plugin test coverage (landed on Friday: #3501290: Introduce unit test coverage for both ComponentSource plugins (Block + SDC)). I created #3518833: [PP-2] Update JsComponentTest to subclass ComponentSourceTestBase for consistent test coverage specifically to make "in-browser code component" test coverage catch up to other ComponentSource plugins' test coverage.

So … I'd like to see this MR update \Drupal\Tests\experience_builder\Kernel\Plugin\ExperienceBuilder\ComponentSource\JsComponentTest.

There's also some config schema changes I need to dig deeper into to understand properly. Assigning to myself for that.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Priority: Major » Critical
Status: Needs work » Needs review

We should merge this ASAP so I'd recommend we move adding tests to a follow-up unless something that can be done very easily. There are quite a few people building on top of XB even though it's alpha and this is a pretty disruptive regression so we should try to roll a new release with this as soon as we can.

wim leers credited longwave.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Assigned: wim leers » larowlan
Status: Needs review » Needs work
Issue tags: -Needs tests +ddd2025

Discussed with @longwave at Drupal Dev Days, he agrees that adding type: experience_builder.json_schema.object.json-schema-definitions://experience_builder.module/image is a no-go.

Added the missing tests. But can't get them to fail.

Neither of us understands why we can't reproduce the problem neither through that test, nor through the manual STR in the issue summary. My changes/the solution I believe should work does appear to work, but also appears to somehow trigger the e2e tests to fail.

I'm hoping @larowlan can figure this out 🤞

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Status: Needs work » Needs review

So the difference is strict schema checking in tests or rather SchemaCheckTrait also checks each individual value has a schema.
So whilst validation is checking that 'this data validates against the schema we have', strict config schema checking as seen in tests (and in FunctionalTestSetupTrait) fails if there is no schema at all. Which is what was happening. I've expanded the test to mimic what SchemaCheckTrait does.
The test did indeed fail with the missing schema. So then I reinstated the new schema and updated the test to validate invalid props against the new schema.
Then I removed it again and added a new schema class that could auto derive the mapping from the $ref.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Assigned: larowlan » wim leers
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

As I started skimming the changes, I uttered "WOW" and went to get more coffee 😜☕️

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

#10: 🤯 Wow, I totally forgot that in some ways, SchemaCheckTrait is the more complete validation of config schema. The thing that it does and \Drupal\Core\Config\Development\ConfigSchemaChecker (which XB's tests use) does not is the completeness of the schema.

So: while \Drupal\Core\Config\Development\ConfigSchemaChecker does use SchemaCheckTrait, and so it does run for the entire XB codebase, it only runs upon saving. And the XB config entity validation test coverage tests validation *prior* to saving. We could gain extra confidence by also running the relevant subset of ConfigSchemaChecker on unsaved entities while asserting XB config entity validation errors, and only surfacing those schema incompleteness errors for which no validation errors occur.

Given that @larowlan has succeeded in undermining my confidence in the completeness of XB's config schema and validation 😬 😱, I'm generalizing what you did here, @larowlan! 🙏

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Assigned: wim leers » Unassigned
Status: Needs review » Reviewed & tested by the community

Will merge when this passes tests.

I took @larowlan's impressive work and generalized it from that single config entity subtree in a single test method, to all test methods of all config entity types. Two tiny tweaks were necessary in ComponentValidationTest, but I'm relieved to report that no other XB config entity types surfaced similar additional problems! 🥵

Zero changes to the awesome expanded infrastructure: virtually no remarks on it. All I did was generalize the specific test coverage @larowlan added to all of XB 👍

  • wim leers committed f52249a9 on 0.x authored by larowlan 
    Issue #3519179 by wim leers, larowlan, longwave: Cannot place a code...
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Reviewed & tested by the community » Fixed
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Related issues: +#2544708: The `label_display` setting in `type: block_settings` has the wrong config schema type

Status: Fixed » Closed (fixed)

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