#stable blocker

I think we should express the uploaded videos as follows in schema.json:

{
  "video": {
    "title": "video",
    "type": "object",
    "required": ["src"],
    "properties": {
      "src": {
        "title": "Video URL",
        "type": "string",
        "format": "uri"
      },
      "poster": {
        "title": "Poster image URL",
        "$ref": "json-schema-definitions://experience_builder.module/image-uri"
      },
      "width": {
        "title": "Video width",
        "type": "integer"
      },
      "height": {
        "title": "Video height",
        "type": "integer"
      },
      "autoplay": {
        "title": "Autoplay",
        "type": "boolean"
      },
      "loop": {
        "title": "Loop",
        "type": "boolean"
      },
      "muted": {
        "title": "Muted",
        "type": "boolean"
      },
      "controls": {
        "title": "Show controls",
        "type": "boolean"
      }
    }
  }
}

For oEmbed, I'm not sure we need the max width / max height here. It seems something that the component itself should handle? We may need to expose the height / width of the resource, as well as title which would make this shape as follows in schema.json:

{
  "oembed_video": {
    "title": "oembed_video",
    "type": "object",
    "required": ["url"],
    "properties": {
      "url": {
        "title": "oEmbed URL",
        "type": "string",
        "format": "uri",
        "contentMediaType": "application/oembed+json",
        "x-oembed-type": "video"
      },
      "width": {
        "title": "Video width",
        "type": "integer"
      },
      "height": {
        "title": "Video height",
        "type": "integer"
      },
      "title": {
        "title": "Video title",
        "type": "string"
      }
    }
  }
}

For documents, to display a link, we'd need a bit more than just the URI in the schema.json:

{
  "document": {
    "title": "document",
    "type": "object",
    "required": ["src"],
    "properties": {
      "src": {
        "title": "Document URL",
        "$ref": "json-schema-definitions://experience_builder.module/document-uri"
      },
      "title": {
        "title": "Document title",
        "type": "string"
      },
      "description": {
        "title": "Document description",
        "type": "string"
      },
      "filename": {
        "title": "Filename",
        "type": "string"
      },
      "filesize": {
        "title": "File size",
        "type": "integer",
        "description": "File size in bytes"
      },
      "mimetype": {
        "title": "MIME type",
        "type": "string"
      }
    },
    "document-uri": {
      "title": "Document URL",
      "type": "string",
      "format": "uri-reference",
      "pattern": "^(/|https?://)?.*\\.(txt|rtf|doc|docx|ppt|pptx|xls|xlsx|pdf|odf|odg|odp|ods|odt|fodt|fods|fodp|fodg|key|numbers|pages)(\\?.*)?(#.*)?$"
    }
  },
}

but the actual update in gitlab file goes to this issue - https://www.drupal.org/project/experience_builder/issues/3518292 ✨ Allow searching for content in the editor navigation Active , where pipeline failing for 1 and passes for another , i'll update the gitlab file for that. Thanks

and

The failing pipeline in cypress test is not part of the changes done in this MR.

But … this one surely isn't introducing transliteration, so why is this one failing? 🤔 Anyway, because you really want this merge order, I tried to land ✨ Allow searching for content in the editor navigation Active , but couldn't: #3518292-28: Allow searching for content in the navigator → .

It needs to fix a complex conflict.

As beta blocker tag is added, I am removing stable blocker tag

This is now good for review. The failing pipeline in cypress test is not part of the changes done in this MR.

Identified in review for 📌 PersonalizationSegment config entity Active .

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

Thanks!

It failed due to type -> component_id in the tree

The pipeline passed - https://git.drupalcode.org/issue/experience_builder-3516432/-/pipelines/..., when the work was done. but failed after a rebase done from another person and i may know why because of the major changes introduced in ApiLayoutController.

I'll update the test case for this once.

but the actual update in gitlab file goes to this issue - https://www.drupal.org/project/experience_builder/issues/3518292 ✨ Allow searching for content in the editor navigation Active , where pipeline failing for 1 and passes for another , i'll update the gitlab file for that. Thanks

Based on meeting just now, can we get:

• the failing tests to explicitly skipped on SQLite, and have a comment pointing to the relevant core issue?
• an update to .gitlabci.yml to use MariaDB by default instead of SQLite? (Or MySQLite, whichever is fastest.)

The only 2 e2e tests that are failing now are `entity-form-field-types-test.cy.js` which does pass for me locally and `media-library.cy.js` which did fail locally but I didn't see an 409 request errors in the log. So maybe these are unrelated random test errors? Not sure, retesting

This is not yet passing tests. 😇 Left a review with pointers.

Will try to push a solution for that, too.

Done:

(I'm aware of the clean-up potential. I'll let @larowlan refactor from this — AFAICT — working state to his liking 😊)

To enable @larowlan to focus on the actual tricky bits, and to avoid him having to spend time fixing the remaining test failures I left in #23 (8 days ago), I got that out of the way. I also addressed the @todo for adding validation.

All PHPUnit tests are passing, except for one. That one actually reveals a severe flaw in what I did 😇😬

The last failure:

Drupal\Tests\experience_builder\Kernel\Config\JavascriptComponentStorageTest::testComponentEntityCreation
Drupal\Core\Config\Schema\SchemaIncompleteException: Schema errors for experience_builder.component.js.bxnge13p with the following errors: 0 [versioned_properties.46d5d8c16712f873.settings.prop_field_definitions] 'noodles' is a required key.

… this is because the way Component config entities are currently validated per the current config schema, it is required for all past versions of a Component to have valid source-specific settings.

However … in the case of code components, it's possible that new props/slots are added! 😅 (More to come on that front in 🌱 Plan to allow editing props and slots for exposed code components Active .) Which then fails due to

…
    prop_field_definitions:
      constraints:
        # There must be a key for every `prop` in the corresponding \Drupal\experience_builder\Entity\JavaScriptComponent::$props.
        SequenceKeysMustMatch:
          configPrefix: experience_builder.js_component.
          configName: '%parent.%parent.%parent.%parent.source_local_id'
          propertyPathToSequence: props

And that is the big flaw: that actually only the active version MUST be valid, older versions MAY be valid. It's impossible to perfectly validate them, given the old implementation is no longer around. Will try to push a solution for that, too.

I brought in the changes from 📌 Add rudimentary conflict prevention to the preview end-point Active because I made progress getting the e2e test to pass, so I thought I would the current state

Hi @wim leers to test my code I followed the below steps:

1. edit the existing component and remove the default text from the required prop
2. saved the component

Let me know if this is correct or have I missed anything?Thanks.

I've implemented a new validation constraint NotNullValueForEveryRequiredSdcProp to ensure that all required props defined in a component's schema have non-null entries in the default_value mapping. This is based on the required array from the component plugin's metadata schema. The logic closely follows the pattern used in KeyForEverySdcProp.

I've tried to achieve the requirement as described—please let me know if anything is missed or needs adjustment.

I will give it a try!

Cool, then maybe this issue will be just about test coverage :)

It feels like it should be simple enough - the main impacts here are on page regions and patterns. Add content templates if that ends up being in beta.

Tagging as such

✨ Content templates, part 3b: store exposed slot subtrees on individual entities Active adds an array_filter to ComponentTreeItem::getValue that would also have resolved this

One final issue with the composite_field_type approach is that storing prop field definitions in a separate entity that is only written when the component is saved means we lost the ability to have draft JS code components drawn from autosave.

larowlan → closed merge request !1070

Kinda relieved to read my concerns about third-party settings are (seemingly) confirmed by you.

The static prop source defaults work can be combined with Wim's versioned config entity and we can add a version column to the field item that will apply to all components not just specific sources. That will give us the consolidation of the inputs column we're chasing here

I read that as "versions for all Component config entities, not just some, and stored as a new component_version field property for content entities and a same-name key-value pair for config-defined component instances" — which is exactly where I thought this was going last week 😄👍

Can't wait to see what you cook up for me by your EOD/my start! 🤓

🕺

Will reply in detail shortly
Been thinking about this overnight

I think the third party settings and the component source specific aspects (eg the need to apply defaults in client conversion) make this approach non tenable

The third party settings show that while the schemas are close, they don't align

The composite field work we can copy into a branch in the issue for adding generic object shape support, so it isn't wasted work

The static prop source defaults work can be combined with Wim's versioned config entity and we can add a version column to the field item that will apply to all components not just specific sources. That will give us the consolidation of the inputs column we're chasing here

I'll change course to that today

Reviewed this 34 files, +2899, -166 diff at a high level.

1. My go-to test for making changes to the Component config entity is unsurprisingly ComponentValidationTest. But unfortunately that currently fails hard 😅
2. ComponentTreeItemListTest nicely illustrates the simplification for the stored data 👍
3. I have concerns about ::applyComponentTreeItemDefaults(), and I'm especially concerned about how it's used by ClientServerConversionTrait::clientModelToInput(). Left a lengthy comment there.
4. 👏 ComponentInputsEvolutionTest is fantastic! But … see the aforementioned comment for why that actually doesn't cover the full gamut of possible input evolutions.

AFAICT there are more remaining tasks:

1. multiple-cardinality support
2. explain the rationale behind the (auto-generated) naming scheme of the new config entities
3. provide an overview of which files are >=90% copy-pasted-and-renamed from field_union 🙏 I bet that >2K of the new lines are coming from field_union?

good point to stop and decide if we should keep going with this approach

I still am optimistic! So, I'd say: start with my main concerns (explained in detail on the MR), because if you can overcome/explain away those, then I think we're on track here 🤓

Per #3526818-5: Tighten validation of `parent_uuid` and `slot` on XB fields to match the strictness of config → .

@phenaproxima: I suspect that the NULL values stored in the DB for parent_uuid and slot are actually cast to string at some point (either the DB layer, or SQL content entity storage, or Entity Field API, or …), and that's how they end up being exported wrong?

Note that I actually can get validation errors to occur, for example doing

diff --git a/tests/fixtures/xb-page-with-data.yml b/tests/fixtures/xb-page-with-data.yml
index 97b35785c..216b7041e 100644
--- a/tests/fixtures/xb-page-with-data.yml
+++ b/tests/fixtures/xb-page-with-data.yml
@@ -19,7 +19,7 @@ default:
       inputs:
         width:
           sourceType: 'static:field_item:list_integer'
-          value: 50
+          value: 40
           expression: ℹ︎list_integer␟value
           sourceTypeSettings:
             storage:

results in:

1) Drupal\Tests\experience_builder\Functional\DefaultContentImportTest::testImportDefaultContentWithXbData
Drupal\Core\DefaultContent\InvalidEntityException: /Users/wim.leers/core/modules/contrib/experience_builder/tests/src/Functional/../../fixtures/xb-page-with-data.yml: components.0.inputs.16176e0b-8197-40e3-ad49-48f1b6e9a7f9.width=Does not have a value in the enumeration [25,33,50,66,75]. The provided value is: "40".

So that means

Worse: the Recipes default content import functionality apparently did not report a validation error?! 😱

is a different problem, and it's a beta-blocking one.

Extracted that to 📌 Tighten validation of `parent_uuid` and `slot` on XB fields to match the strictness of config Active .

Ironically I surfaced something closely related in reviewing that Recipes MR a ~year ago: #3439923-31: Add recipes api as experimental API to core → + @phenaproxima's response at -32, where he says:

it's exported by the default_content module, which is what generates these YAML files. IMHO they should not be changed at this time, but if (which, once this is committed, is really when) core adds the ability to export content entities as YAML […]

😭😬 — no related issues linked from #3439923 that are doing that, and search engines aren't finding such an issue either…

OMG, AFAICT \Drupal\Core\DefaultContent\Importer exists but not \Drupal\Core\DefaultContent\Exporter?! CR: https://www.drupal.org/node/3445169 → .

I'm hoping that @phenaproxima can push this forward 😇

wim leers → credited justafish → .

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

Sooooooooooo very close! 😄

Tests are now all passing with the update approach. Moving to needs review.

@wimleers thanks, yes I'm unblocked on this.

Here there is not, and perhaps we should introduce it.

Here it is indeed the sequence order (which implicitly has "deltas" as keys: a PHP "list" array, so 0, 1 etc.) that ensures the correct ordering.

I'm contemplating whether this should be a beta blocker or not. It feels like it'd be safer to make this a beta blocker. Do you agree?

Was the component already placed within experience builder and saved?

My bet too :)

@thoward216 Just confirming … you are unblocked, right? 🤞

wim leers → changed the visibility of the branch 0.x to hidden.

Update for #29:

• 🐛 Component trees with dynamic prop sources that introspect field values don't add the relevant fields to their dependencies Active is fixed
• #3519352 is unblocked, and is in need of review by @phenaproxima at #3519352-55: Content templates, part 3b: store exposed slot subtrees on individual entities →

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

@wimleers - thanks for the context and suggested path forward in #22 🐛 SDC prop of `type: string` with empty string listed in its `enum` results in broken input UX Active will take a look into this.

@tirupati_singh - I've pulled in the latest commit locally and tested your above component and it appears to work as expected, it does not show in the component library in experience builder and appears within the "disabled" section under "/admin/appearance/component/status" with the reasons - see screenshot.

Today's progress:

• was able to collapse done the values stored in inputs to #32 and #34
• added a new column to ComponentTreeItem to store a reference to the composite field type
• starting on some failing tests
• at this point I can see \Drupal\Tests\experience_builder\Kernel\ApiLayoutControllerPostTest::testWithDraftCodeComponent is failing because previously we were relying on prop field definitions being stored in autosave, will see what the options are for that tomorrow

I think its probably a good point to stop and decide if we should keep going with this approach

Remaining tasks:

• Fix any tests
• New validation for the third party settings

1) yes I hope we can add typed-data integration to easily set/get inputs by prop name on the inputs column in ComponentTreeItem so folks dont need to learn the ins and outs of it being JSON encoded, see comment #30 for the type of DX I'm thinking about
2) my hope is that it will, I've got one eye on that as I'm working on this. The code I touched in \Drupal\experience_builder\Plugin\ExperienceBuilder\ComponentSource\GeneratedFieldExplicitInputUxComponentSourceBase::getPropsForComponentPlugin I hope to be able to refactor further into a service (or perhaps method on the existing matcher service) that takes an array of props rather than a component plugin. Because then we could also use it for object shapes and nest them.
3) if we store this in the source plugin settings, we need to be able to version it still. Either way I'm still not sure about that and the interdependency between the two entities, I may end up moving it back and adopting the versioning logic you have in your MR. I should know today/tomorrow if the current approach will work or not and will pivot as needed.
4) At present this is also looking ahead to object shapes and more generic (structured data) uses of the composite field type where we have a fixed table schema and those changes can't be made if a dedicated field table has been created. I don't think it matters when we're talking about JSON or config mappings. But yes, its very rudimentary at this point and could be expanded to consider the dependencies data if we need it to. The structured data approach would require a separate module for adding these via the UI and would likely live at https://drupal.org/project/composite_field - a namespace we grabbed a while back after discussions amongst Drupal CMS team in relation to a possible requirement for 'field_union' in the long term in Drupal CMS but with a better name. If XB has the config entity and field type, then that module would just be the UI. I've left a few todos in the code to date referencing that end goal.

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

Thanks @thoward216 for the quick implementation of the requested changes. I’ve been reviewing the updated merge request with the feedback incorporated, but I’m encountering an issue while testing it with the same component props mentioned in comment #23.

When I use the component props as follows (from the earlier example):

name: CTA Button
description: This is component for adding CTA.
status: stable
props:
  type: object
  properties:
    cta_url:
      type: string
      title: Link URL
      description: Add the url to be used.
      format: uri
      examples:
        - 'https://www.drupal.org'
    cta_title:
      type: string
      title: Link Title
      description: Add the title to be shown on hovering
      examples:
        - 'Learn more'
    cta_icon:
      type: string
      title: CTA icon
      description: Choose the icon to be display for cta
      examples: ['PDF', 'Video', 'Arrow', 'External Icon']
      default: Video
      enum:
        - ''
        - PDF
        - Video
        - External link
        - Arrow icon
    cta_icon1:
      type: string
      title: CTA icon 1
      description: Choose the icon to be display for cta
      examples: ['PDF', 'Video', 'Arrow', 'External Icon']
      default: Video
      enum:
        - PDF
        - Video
        - ''
        - External link
        - Arrow icon
    cta_icon2:
      type: string
      title: CTA icon 2
      description: Choose the icon to be display for cta
      examples: ['PDF', 'Video', 'Arrow', 'External Icon']
      default: Video
      enum:
        - PDF
        - Video
        - External link
        - Arrow icon
        - ''

I did not encounter the specific error message ('Prop "%s" has an empty enum value.', $prop_name) in the ComponentMetadataRequirementsChecker class, after the feedback was incorporated and the changes were made in the MR. However, I am now encountering a different issue in the Experience Builder UI. The error message displayed is: "An unexpected error has occurred while rendering the component's form." I’ve attached a screenshot for reference.
Could you please suggest any specific steps or checks I should follow when creating or testing the component? If there’s anything I might have missed or overlooked in the process, I’d really appreciate it.

Thanks!

@wim leers, thank you for the detailed feedback and insights — much appreciated!

You took a different approach than what's in the issue summary, @thoward216, and you had me first thinking it was genius, simple and elegant, but … I think I spotted some negative consequences: do you agree?

Yes, I do agree with your assessment. You're absolutely right — I took a different approach by omitting "" from the enum instead of explicitly disallowing it. At first glance, it seemed like a clean and unobtrusive way to sidestep the HTML limitation, but I can now see the downside — particularly in cases where "" is explicitly included in a required field's enum. This results in an unusable form element, which defeats the purpose of validating the SDC in the first place.

After re-reviewing the changes in my merge request, I realized that the current fix does not fully resolve the issue, especially for props like cta_icon1 and cta_icon2.

While the intention was to handle enums with empty strings more gracefully, the approach I took (silently omitting "") doesn't correctly address all problematic cases. For instance:

• cta_icon1 includes "" in the middle of the enum array.
• cta_icon2 includes "" at the end.

Both of these cases still result in invalid for below component props

cta_icon:
      type: string
      title: CTA icon
      description: Choose the icon to be display for cta
      examples: ['PDF', 'Video', 'Arrow', 'External Icon']
      default: Video
      enum:
        - ''
        - PDF
        - Video
        - External link
        - Arrow icon
    cta_icon1:
      type: string
      title: CTA icon 1
      description: Choose the icon to be display for cta
      examples: ['PDF', 'Video', 'Arrow', 'External Icon']
      default: Video
      enum:
        - PDF
        - Video
        - ''
        - External link
        - Arrow icon
    cta_icon2:
      type: string
      title: CTA icon 2
      description: Choose the icon to be display for cta
      examples: ['PDF', 'Video', 'Arrow', 'External Icon']
      default: Video
      enum:
        - PDF
        - Video
        - External link
        - Arrow icon
        - ''

Thanks again for the clear explanation — it’s really helped clarify the situation. I'll adjust the implementation to explicitly disallow "" in enums and ensure proper test coverage for these cases.

Fix HTML 🙈

Explain relation to SDC and code components.

Just did a video call with @isholgueras to unblock him on next steps and clarify the unclear bits in my outline at #15 (🙈 Sorry!).

Updated issue title + summary to convey the intended scope.

✨ Move zoom controls, show only one viewport Active landed.

Did an early peek at @larowlan's in-progress branch — observations:

1. That proxy functionality+architecture is super interesting — looking forward to reviewing that in detail when there's an MR up :)
2. experience_builder.composite_field.*:fields.<key>.field_type won't itself allow referencing a composite field (type), right? Related: 📌 [PP-1] Support `{type: object, …}` prop shapes that require *multiple* field types Postponed .
3. I have doubts about type: experience_builder.composite_field:.*.third_party.experience_builder. Why not keep that information on type: experience_builder.generated_field_explicit_input_ux? 🤔 The new CompositeField config entity type must by definition not have a reference back to the Component config entity that will use it, which means I don't see any way for being able to validate (aka ensure data integrity). Whereas it'd be trivial to validate if these key-value pairs remained on type: experience_builder.generated_field_explicit_input_ux!
4. CompositeFieldType::isInUse() will only work for content entity types, so it won't work for e.g. an XB ContentTemplate config entity — won't this be problematic if this component (and its associated FieldComposite config entity) happen to only be instantiated in config entities? I see that ::preSave() explicitly relies on ::inUse(), so that seems like a date integrity problem? Or … I bet you're going to address this using a pre_save hook implementation that makes it also respect XB's (atypical!) use of fields in config entities? 🤓

This looks great!

I wanted to RTBC & merge, but:

• it's not yet passing CI
• I think the validation probably should be tightened slightly, do you agree?

#21:

XB started early on with only SDC support. For pragmatic reasons, and to avoid painting ourselves into a corner decorated with our own assumptions, we relied on test SDCs in Drupal core. That includes core/modules/system/tests/modules/sdc_test/components/my-cta/my-cta.component.yml, which now turns out to be a bit of a weird one.

I see that there are 37 failing tests relating to sdc_test:my-cta after the (necessary!) change this MR makes.

The clearest possible failure is

ComponentValidationTest::testEntityIsValid
Drupal\Core\Config\Schema\SchemaIncompleteException: Schema errors for experience_builder.component.sdc.sdc_test.my-cta with the following errors: 0 [status] The component '<em class="placeholder">sdc.sdc_test.my-cta</em>' cannot be enabled because it does not meet the requirements of Experience Builder., 1 [status] Prop "target" has an empty enum value.

… which is exactly right: this is saying that the Component config entity has status: true but it can't/shouldn't be because of the empty string enum value! 👍

I suggest the following path forward:

1. in places where the use of sdc_test:my_cta is meaningless (i.e. "just some SDC"), simply pick another SDC. For example, experience_builder:heading or experience_builder:my-hero (which both also have a few props) or even experience_builder:druplicon (which has zero props).
2. in places where some aspects of the props of sdc_test:my_cta are tested/used for tests: replace it with another SDC that has similar props. For example ContentTemplateDependencyTest used "my CTA" but could easily switch to "heading" and retain the same test coverage, even despite testing specific props (explicit inputs). Because the input that the test is centered around is a type: string, which is a prop shape that "heading" also contains 👍
3. in other places, switch to a different SDC.
4. … if and only if you run into a place where there truly is a need for semantically exactly what my-cta offered (essentially, a link: URL + text + target attribute), then just add a "cta" or "link" SDC to XB itself.

Thinking about this some more, the minimal dataset in #32 and #34 omits the sourceType and expression. I have an idea about how we might do that for static prop sources here. We'll need to retain that for non static props (Dynamic, adapted) but I think that's ok as they are less common.