🇧🇪Belgium @wim leers

Next steps:

1. review by @larowlan
2. update Pattern::calculateDependencies() etc to leverage this new infrastructure
3. update FieldTypeUninstallValidator(Test) to leverage this new infrastructure
4. add missing low-level test coverage
5. …

I just pushed a sequence of commits:

1. 100% of prop expressions now implement calculateDependencies()
2. Then I started using that to implement DynamicPropSource::calculateDependencies() with essentially zero effort, with test coverage to prove it works correctly.
3. … at which point it was trivial to impose ::calculateDependencies() for all 4 of XB's "prop sources", by adding it to abstract class PropSourceBase, implementing it took only a few dozen LoC
4. The above enabled me to drastically simplify ComponentInputs::calculateDependencies(): it now just forwards the responsibility of calculating dependencies to each prop source, and the (Static|Dynamic)PropSources largely forward that responsibility again to the low-level "prop expressions

The result (assuming the test coverage is expanded): simple code, and the responsibility for ensuring correct dependencies is pushed down to the individual code that is actually interacting with those dependencies!

Working PoC live, with plenty of @todos.

Thoughts, @catch? 😊

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

Let's do this as a stepping stone towards 📌 Refactor the XB field type to be multi-valued, to de-jsonify the tree, and to reference the field_union type of the prop values Active .

I'm still collecting my thoughts, but there's at least 3 use cases already identified for adding more field properties (i.e. expanding ComponentTreeItem::::propertyDefinitions()) and hence DB columns:

1. the dependencies implied by a component tree and its inputs
   👉 (this issue)
2. multi-valued XB field with a unique exposed_slot property per delta, to target a specific exposed slot in a ContentTemplate config entity, i.e. for the per-content entity unique creative freedom while the overall component tree is guaranteed to be consistent by the ContentTemplate
   👉 being worked on at ✨ Content templates, part 3b: store exposed slot subtrees on individual entities Active , as part of 🌱 [META] 7. Content type templates — aka "default layouts" — clarify the tree+props data model Active
3. (surfaced yesterday by @lauriii at DDD and discussed at length with @pdureau and others at DDD) the fact that some SDC props may need to be considered either translatable or not (see 📌 Allow Experience Builder fields to support Asymmetric and Symmetric translations Needs review ) — for example an image in some SDC might be considered translatable or not — details TBD
   👉 issue to still follow

(Not linking any of those issues nor crediting those people, because that's what that follow-up will be for. I'm just writing it down as context, and as a reminder for myself when I do write that next week.)

I will implement basically what I wrote in #15, but @catch was concerned in #16 that would still require a flat JSON structure.
AFAICT he thought that because it'd store both config and module dependencies:

  config:
    - a
    - b
  module:
   - x

But that's easily transformed to:

config:a config:b module:x

… at which point it's a flat list that is trivially queryable :)

Given what @catch wrote in #18, I think that my thinking has arrived at the same point: we basically want for each row for an XB field revision a flat list of dependencies, that is functionally equivalent to what a config entity's dependencies stores. IOW: we want the equivalent of \Drupal\Core\Config\Entity\ConfigEntityInterface::calculateDependencies(), but for the XB field, to allow efficiently (and with 100% confidence — unlike file_usage) determining which XB fields (and even which revisions of XB fields) are impacted by a potential change in available modules/themes/config.

IOW:

so I think this issue would be a small step forwards if those are not being prioritised yet.

💯

P.S.: I'm no DB nor SQL expert, so perhaps it seems super naïve. That's fine! I just want to restart this conversation, @catch! 😄

I agree with @catch in #23.

Added ✨ Support other format for string and integer: color and range Active .

You're off to a great start here! :D

Asked a few questions, and asked @larowlan to provide you with a crucial pointer, because while this may be what is a reasonable expectation from a HTML POV, neither "color" nor "range" are part of the JSON Schema spec. That makes the SDC integration nontrivial.

(Added this to 📌 [SPIKE] Comprehensive plan for integrating with SDC Active . Tangentially related: ✨ Enable contrib to register `pattern` and `$ref` shape matches for prop shapes Active .)

This definitely still needs test coverage.

Great find! 🤩

The best part: this should not require any update path, nor is it a BC break; this is just tightening SDC's existing JSON Schema! 👏

Given https://git.drupalcode.org/project/experience_builder/-/merge_requests/9..., I'm hoping @larowlan can point us in the right direction — I'm currently flailing to find what I'd swear is a subclass of \JsonSchema\Constraints\FormatConstraint being added by him to XB!

Interesting find! Not sure yet if this is a client-side or server-side bug. But since the server side renders the preview for the component tree it receives, I suspect it's the client side. If it's the server side, then I suspect it's in the auto-save functionality.

Idea: a test that sets up a representative use of XB, and then asserts the config dependency tree using ASCII art.

This is not being worked on right now.

Listed on https://drupal.org/project/experience_builder now 👍

Listed on https://drupal.org/project/experience_builder now 👍

Per #3518248-11: [PP-1] Content templates, part 4 (boss battle): create a UI for editing templates → , this should block that.

🐛 Component trees with dynamic prop sources that introspect field values don't add the relevant fields to their dependencies Active should happen before we allow users creating ContentTemplates in the UI 🙏

📌 Junit output for cypress Active changed that to:

[DX: CI] @wimleers @bnjmnm @larowlan @longwave

Still relevant!

#4++ — and we now have a precedent for XB-owned locked config: 📌 `StringSemanticsConstraint::MARKUP`: agree how SDC prop JSON schema can convey it should be markup, and Needs review did the same!

Bump, thoughts, @jessebaker?

Text area props

I always have been concerned about that, and now even more so.

json-schema-definitions://experience_builder.module/textarea is what @lauriii advocated for, but it really should be json-schema-definitions://experience_builder.module/multiline_string.

"textarea" is the only prop shape where the shape is pointing to the input UX instead of the shape/semantics of the data.

Make this new shape the default for text areas — we can assume all of them want a WYSIWYG editor;

IMHO we should just remove the "textarea" stuff and name this "HTML block" in the code component editor UI, which then closely matches the semantics:

      type: string
      contentMediaType: text/html
      x-formatting-context: block

Probably we don't even want users to focus on global regions (double-click). TODO: Confirm and create issue

They won't be able to, because the regions will simply not appear in the information the server sends to the client; the client won't even know about these regions! 👍 So: no follow-up needed.

wim leers → created an issue.

Thanks!

✨ Content templates, part 2: store a component tree in the template, and allow individual entities to fill in specific slots in that tree Active is in!

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

🚢

THANK YOU!

Looks fantastic!

The only thing that was inaccurate was the distinction between

1. the machine name of the component slot that is being exposed (e.g. the "two column" SDC's column_one)
2. the machine name of the exposed slot itself, i.e. the name being picked by the Content Template author

which per the example in the config schema are two different things:

          # exposed_slots:
          #  profile_bio:
          #    label: 'Profile Bio!'
          #    component_uuid: 28bcab26-e434-4ad4-9eaf-0520bdb32fcc
          #    slot_name: column_two

👆 profile_bio is the machine name of the exposed slot, column_two is the name of the component slot.

I clarified that.

What added to that confusion is that XB is not yet sufficiently restricting this across ComponentSource plugins, so created 📌 Constraint slot names allowed by XB in its component tree Active for that!

Thanks, @phenaproxima at https://git.drupalcode.org/project/experience_builder/-/merge_requests/8..., for making me realize this!

wim leers → created an issue.

Thanks to @penyaskito, I was able to open 🐛 ContentCreatorVisibleXbConfigEntityAccessControlHandler's `view` access must refuse access to disabled config entities Active .

Added 🐛 ContentCreatorVisibleXbConfigEntityAccessControlHandler's `view` access must refuse access to disabled config entities Active — great find, @penyaskito!

Tests still needed.

wim leers → created an issue.

Yes, please!

wim leers → made their first commit to this issue’s fork.

It will be so nice to have this in! :D

And given it's a long weekend for many, I'm prioritizing this landing today to minimize overall disruption.

Yay! Most important next step: 📌 Support props that can use wysiwyg widgets Active .

📌 CKeditor not loading on formatted text fields in the content entity + component instance forms Active is in!

balintbrews → credited wim leers → .

Awesome! Can’t wait to start using this 😄

Not passing tests.

By the way, what does "Live" part of testRenderComponentLive() refers to? Don't think I've seen this term used before in XB Components context.

It refers to it using isPreview: FALSE. I didn't like the name, but needed to unblock this 😅 Suggestions VERY welcome! 🙏

At the moment this causes failures of Functional tests for a bunch of endpoints a

That's because xb_test_sdc is installed by a bunch of tests; we shouldn't default to crashing — that immediately fixes those widespread failures!

I am not sure if that was what @wim-leers expected?

This is definitely looking like the direction I asked for!

But this was not yet proving used as a component instance in an XB component tree does NOT make the rest of the component tree unusable. See review feedback, and partial implementation.

Surfaced again at 🐛 Cannot place a code component with an image prop Active , kudos to @longwave for connecting it to this issue!

I think this is in the final stretch now! This needs a change record, plus 2 clarifications, plus 1 code clarity nit by Lee that I +1'd.

FYI: Ran into @pdureau at DDD yesterday — he +1'd the implementation plan I added to the issue summary in #11 😊

This last bit now is perhaps genuinely nitpicky, but I find ::getRegionComponents() genuinely confusing 🙈

I'd prefer to see https://git.drupalcode.org/project/experience_builder/-/merge_requests/8... get fixed, but if you disagree: I've approved the MR and marked this RTBC :)

Lots of cleverness here, but also some huge long-term impact decisions!

This is sort of a counterpart to 🌱 [META] Robust component instance error handling Active , at least if I interpreted everything correctly.

I'd like @longwave to take a very critical look at this, because this has just shifted to a critical data integrity issue. 😅

#9 Thanks so much for writing up your research in detail! 👏🙏

• experience_builder.content_template.*.*.full cannot resolve to a type unless it was also based on some kind of meta-type like experience_builder.content_template.*.*.[view_mode]. There's precedent in core for something like this (editor.schema.yml), but not for config entities.

But Editor is a config entity too, so that does seem fine?

• This is a config entity, which means there's always an exposed_slots property, even if unused. If it's present (which it will be) in content templates in other view modes, those are automatically config schema errors.

That's a great point! So given that, the best we can do anyway is to restricted exposed_slots on view modes other than full to the empty array. And we need a validation constraint anyway. So then doing what you did here seems reasonable.

FYI this is inspired by what I proposed to @effulgentsia while discussing 📌 [PoC] Introduce a `ContentTypeTemplate` config entity Active , and which he encouraged to explore:

Nice work here!

This is now on the verge of RTBC, with only one crucial comment missing — I proposed a sample comment for making this sufficiently clear for the next person/reader/contributor to understand how ContentTemplateAwareViewBuilder then actually renders those content entities that it has a ContentTemplate for! It's quite indirect, so not at all obvious, took me ~10 mins to understand 😅

Nothing to do here: one-line change, expanded test coverage by @larowlan ⇒ 🚢

Seems like it'd be a common place for anyone starting fresh with XB and just turned on their global regions. Great catch

Only when 0 Block config entities existed! See PageRegion::createFromBlockLayout() + experience_builder_form_system_theme_settings_submit()

I suspect 🐛 Cannot place a code component with an image prop Active fixed this, but it'd be good to confirm. 🤞

🤪🤯 Wow wow wow @ ClientDataToEntityConverter::setEntityFields(), but given the detailed (and essential!) comments left by @larowlan on there, I feel zero need to step through all that again to verify it. This looks ready to me.

Unfortunately the widget-multivalue.cy.js e2e test failed with

      cy:command ✘  assert	expected **<input#edit-field-xbt-unlimited-text-0-value--5Ex2yiBamp4.rt-reset.rt-TextFieldInput.js-text-full.text-full.form-text>** to have value **item five**, but the value was **item one**

Retesting… 🤞

@tedbow I agree we should avoid duplicate efforts, or even temporary efforts. Can you propose the (issue) implementation order? 🙏

For #14 :)

#11++

Not yet passing tests 😇

Also: did you want to keep or revert your changes to ::renderComponent() wrt #access or not? (As discussed yesterday in person during DDD.)

At first sight, we should obviously do this.

However, on second though: what if the description is targeted at other front end developers, and not humans?

🙏 Could you review Lee’s additions here, Ben?

Woohoo, thanks so much! 😊

Crediting Ted for the idea of combining of (decorating) controller and event subscriber in a single class. One cohesive unit 👍

::getRegion() is still wrong, and the openapi.yml file was updated incorrectly.

I recommend putting a breakpoint on \Drupal\Tests\experience_builder\Kernel\ApiLayoutControllerPostTest::testMissingSlot() and stepping through it — that's what I just did and it's how I observed the modified docs for ::getRegion() to be wrong in multiple ways 😅

Let's spend the 10–20 mins it takes to get this right now, otherwise this will continue to cause confusion! 🙏

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! 🥵

#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! 🙏