🇧🇪Belgium @wim leers

Just caught up on #3519352 — high-level responses at #3519352-38: Content templates, part 3b: store exposed slot subtrees on individual entities → .

Makes me want to #22+++++++++++++ even more than I could've anticipated 😄 — as in: "do this first, but keep #3519352 in mind".

@larowlan in #21:

one about an edge case of a slot named '0'

Another reason to do 📌 Constraint slot names allowed by XB in its component tree Active ! 😄

I'm glad to see @lauriii already said as much in #29 😊

WRT deleting component subtrees (for this issue: of unused slots' subtrees): ✨ [later phase] Provide API for finding and UI for surfacing dangling/dead component subtrees — aka garbage clean-up Postponed — but also of defunct components, which is necessary because of 🐛 Cannot delete JS components due to component depending on them Active .

@phenaproxima in #32:

the frontend has absolutely no concept of "this tree will fit into a slot".

1. That's intentional: docs/adr/0005-Keep-the-front-end-simple.md
2. That's for later: ✨ Component restrictions Active
3. Yes, the client side will need a new "node type" for its client-side data model — see 3.4 UI Data Model: communicating a `component tree` to the front end in docs/data-model.md — currently only 3 node types exist: component, slot and region. I do agree a new one will be needed for this. That's what 🌱 [META] Production-ready client-side data model + internal HTTP API Active exists for.

P.S.: I'm very tempted to make "bonsai trees" an official term in XB, so can you elaborate on why you used that term and your rationale? 😄

@catch in #33:

RE: information disclosure risks Yep, I raised exactly that concern. 💯

The reason we have no choice but to allow this is that we know from the XB team members who've worked on https://github.com/acquia/cohesion (aka "Acquia Site Studio") for years that site builders will accidentally delete a slot, confirm through all the warnings, and then later realize that actually they do want to undo it after all.

RE: Wow — glad you caught that! Had not crossed my mind yet!

@larowlan in #37: see the validation+config schema added in ✨ Content templates, part 2: store a component tree in the template, and allow individual entities to fill in specific slots in that tree Active — exposed slots are intentionally only allowed for the full/canonical view mode. Because otherwise you get different component trees per view mode, which would be an authoring/usability nightmare. I first identified this in #3511366-3: [META] Introduce a `ContentTypeTemplate` config entity + related infrastructure → , @lauriii responded at #3511366-7: [META] Introduce a `ContentTypeTemplate` config entity + related infrastructure → .2.

Apologies, @phenaproxima, for having sent you down this path 😬 Turns out that this "incremental step" was fraught with more challenges than I anticipated. I expected it to need refactoring later on for sure, but I tried to unblock you on next steps for content templates. Turns out that was not quite realistic.

Per discussion with @effulgentsia, we think this is best tabled until after 📌 [PP-1] Consider not storing the ComponentTreeStructure data type as a JSON blob Postponed . @larowlan is actively working on spikes to help us make the best choice possible.

Agreed that an ADR is needed and appropriate, but during this read-through/catch-up, I did not end up with the impression there's only 2 possible choices as @lauriii indicates in #35 — I think there's many possible choices.

Added two missing things for the client-side data model:

1. Missing "node type" in docs/data-model.md's 3.4 UI Data Model: communicating a `component tree` to the front end: a node type for exposed ContentTemplate slots: #3519352-32: Content templates, part 3b: store exposed slot subtrees on individual entities →
2. Missing "node type" in docs/data-model.md's 3.4 UI Data Model: communicating a `component tree` to the front end: a node type for overrides/personalization variants

Fix issue link.

FYI: 🐛 Component trees with dynamic prop sources that introspect field values don't add the relevant fields to their dependencies Active has been fixed 🥳

Is this still relevant?

📌 Compile Tailwind CSS globally for code components Active recently landed.

Is this still relevant?

I've not seen/heard reports of people running into this for a long time. Tentatively deprioritizing.

🐛 The placement of the component preview is sometimes inconsistent Active landed yesterday and is kinda related.

I'd swear we fixed this a very long time ago. Is this still relevant? I bet @hooroomoo will know this off the top of their head! 🤞😄

Tagging per @lauriii in #4.

This one would be best reviewed by somebody intimately familiar with both React, XB's use of React, Drupal behaviors and the AJAX system. Besides @bnjmnm that's only one person: @larowlan 😅

🥳 — thanks!

@tedbow, thoughts? :D

📌 [UI update] Lock left and right panels to the sides of the screen so they no longer render "over" the canvas. Active is in! 🤩

Alright, unpostponing, but reflecting the lower urgency for this by reducing priority to .

Thanks, @lauriii!

The main thing I'm wondering is, once that lands, would we still need to define the format in the front end? If that's the case there should be explicit explanations of when that's necessary vs […]

💯 — yes, totally, because this is really a non-trivial work-around to something that a combination of upstream dependencies (Drupal core, and even Composer IIRC!) is forcing upon us! 🤪

Alas, @larowlan indicated in #23 that that updated upstream dependency won't change all that much for us, except for the provider 🤷‍♀️

Signaling that @bnjmnm should feel free to merge this when he considers it ready: RTBC! 😄

Thanks, all!

Nice catch, thanks!

Fair point. @effulgentsia, thoughts?

The List with search test case of ApiContentControllersList has been failing.

Also: I found a pretty massive bug which indicates the current test coverage is not adequate.

This MR is not yet updating \Drupal\experience_builder\Extension\XbWrapperNode and friends, but I'd be happy to land the current MR first — it just needs the test expectations to be updated now 🤓

An SDC with the following in its *.component.yml:

props:
  type: object
  required:
    - heading
  properties:
    heading:
      type: string
      title: Heading
      examples:
        - blabla

… yet the explicit input it receives is not

heading: "something"

but

heading: "something"
foo: :"bar"

IOW: some key-value pairs that are not expected by the SDC!

I can think of one mechanism: for ComponentTreeHydrated to detect the presence of #lazy_builder or not, and if it exists, for XB to decorate the component source's lazy builder with its own, so that it's XB's "decorating lazy builder" that adds the prefix & suffix.

P.S.: I doubt my #4 was as precise/accurate as my #12 😅

@larowlan in #7:

both

Hah! I should've known 😅

@longwave in #8:

I agree that slimming down the inputs to remove a lot of duplicated data is worthwhile, although this feels like it adds a huge amount of complexity

+1 to both those statements.

would this be better off as some kind of new core API?

🤔 I'm not sure I follow. Are you thinking of ✨ Add way to "intern" large field item values to reduce database size by 10x to 100x for sites with many entity revisions and/or languages Needs review ? If so: I don't see how that applies here, because this is not about "interning", but about compacting? If not that: can you elaborate? 🙏

@catch in #9: I don't think there will be a big number of "versions" of a particular component. So storing all versions in a single Component config entity seems reasonable. Especially because we aim to allow instances using the older version to be able to transition/update to the newer versions (until none of the old version are left, and the old version can be deleted).

That being said … making the version part of the Component config entity ID would make what I summarily described → in 🌱 [META] Production-ready data storage Active trivial:

Expand on what 📌 Calculate field and component dependencies on save and store them in an easy to retrieve format Active did: omit Component config entity dependencies from deps_config, and introduce a separate component_versions field prop, to store <component config entity ID>:<version> <component config entity ID>:<version> … → allows finding which revisions are using not the latest version of a component

So that is definitely worth considering, too.

I think @larowlan has a good (implied) point: if it's okay for us to rely on "external data" for DynamicPropSources (aka the FieldConfig entity, the host entity an instance lives in, etc.), then why is it not for StaticPropSources?

So: let's get a working PoC and then re-assess 👍

@larowlan Given that this will touch the Component config entity implementation, its config schema, and (Static|Dynamic)PropSource, I think it probably makes sense for me to take this one on. Then you can tackle 📌 [PP-1] Consider not storing the ComponentTreeStructure data type as a JSON blob Postponed at the same time? 🤓

I think you're thinking of ✨ Add way to "intern" large field item values to reduce database size by 10x to 100x for sites with many entity revisions and/or languages Needs review ?

Not opposed to #3469082, nor to #7.2. But I think especially #7.2 is something that could happen later, and most certainly should not be XB-specific.

The only reason for me to prefer @larowlan tackling it here instead of landing #3469082: time (well, and potential scope creep). #3469082 will not land before 11.2, which means it won't be in time for XB's 1.0-goal-by-DrupalCon-Vienna-in-October.

The fallback component will not, I assume, have any props -- but it will render child slots, presumably to keep as much of the layout intact as possible.

was also still wrong in the issue summary — see #69 and earlier for why that's not the case.

🐛 Cannot delete JS components due to component depending on them Active is in!

🐛 Cannot delete JS components due to component depending on them Active is in!

Next up:

• 📌 Handle update and delete of Block component config entities Active
• 💬 ComponentNotFoundException: Unable to find component Active

Cross-posted with @lauriii's #66.

You're right, @lauriii, that behavior was changed 👍 But the docs still said that was the behavior until a few seconds ago. And @penyaskito also got confused by the code — so docs in the relevant code have been clarified too 👍

Yep, +1 for a follow-up on that. And +1 for postponing that until we have made decisions around what the data model is going to be.

Done: ✨ [later phase] Provide API for finding and UI for surfacing dangling/dead component subtrees — aka garbage clean-up Postponed .

Confirmed by @lauriii that we want this at 📌 `BlockComponent::validateComponentInput()` allows garbage to pile up Active .

Plus, this is at minimum postponed on 📌 [PP-1] Consider not storing the ComponentTreeStructure data type as a JSON blob Postponed and 📌 [PP-1] Spike: Explore storing a hash lookup of component inputs Postponed .

wim leers → created an issue.

This MR elevated the importance of 📌 Constraint slot names allowed by XB in its component tree Active .

@penyaskito seems to have struggled to understand the same bits of the MR as me — good reassurance — so I got his +1s for some of the comment/docs additions 😊🙏

The subtler/trickier bits of the Fallback plugin made me discover a bug in the block component source: 📌 `BlockComponent::validateComponentInput()` allows garbage to pile up Active .

Epic work here, everyone — especially @larowlan & @f.mazeikis!

I think the essence of this issue is best described by

• docs/components.md#3.4 Fallback for architecture and end-user impact
• the ComponentInterface::setSource() docblock for details wrt when "changing the source" is appropriate

wim leers → created an issue.

wim leers → created an issue.

This seems like a stable blocker? 😇

Thanks!

@larowlan in #52: very nice work! 👏🤩 Glad to see #48.1 was easy, #48.2 has a stronger reason than I realized to be the way it is (thanks for the follow-up #3524052) and #48.3 was solved subsequently 👍

@lauriii in #54: The fallback rendering actually was rendering the subtrees on both the live site and in the preview. That's what the Fallback source was literally doing.

You can run into this without even doing anything too crazy because if you add a required field to an existing content type with content, it makes all of the content invalid. We have no safe guards against that today.

This is not a fair comparison IMHO. If an SDC/code component adds a new required prop, the component will refuse/fail to render if it is not populated. Content entities in Drupal that don't have a required field just won't render that field, and subsequent edits will require a value to be specified.
Note: supporting new required props on SDCs/code components is actually the easiest problem to solve. Explanation at #3509115-6: Plan to allow editing props and slots for exposed code components → .

document how one could delete all instances of a component when you really want to get rid of all of the content.

So, you're proposing a new issue for identifying and surfacing dangling/dead component subtrees? If so: +1, and no work should happen on it until after 📌 [PP-1] Consider not storing the ComponentTreeStructure data type as a JSON blob Postponed + 📌 [PP-1] Spike: Explore storing a hash lookup of component inputs Postponed are done, because they would significantly simplify the logic for finding+listing those dead subtrees, and for bulk-deleting them.

Tagging for follow-up.

@larowlan in #61 Woah, 📌 [PP-1] Autosave data should store converted server-side representation of model Postponed would be a massive change indeed.

Re-reviewing in detail. Hopeful to merge 🤓🤞

@effulgentsia: please prioritize this ASAP, because the decisions here need to be properly documented, given that proposals are starting to surface to change it: [#3524298.

This would be a massive change in the auto-save architecture.

Unfortunately, the ADR for that still hasn't landed: 📌 Document the reasons for not using Workspaces for saving in 0.2.0 Active . 😬

Important for usability of Code Component creators.

Critical for XB's data integrity when we add support for changing props/slots.

Note: a new required prop would actually be trivial to support. Because XB requires required props to have an example value!

Especially since 🐛 Component trees with dynamic prop sources that introspect field values don't add the relevant fields to their dependencies Active just added ::getDefaultExplicitInput(), this would be easy to support.

Conceptually speaking, the needed changes would be:

1. ComponentTreeHydrated would have to change from
   

   $source->renderComponent($stored_explicit_input)
   

   to

   $source->renderComponent($stored_explicit_input + $source->getDefaultExplicitInput())
   </li>
   <li>EITHER <code>ValidComponentTreeConstraintValidator

   would need to be updated to relax from

   $component_source->validateComponentInput(
             inputValues: $stored_explicit_input,
             component_instance_uuid: $component_instance_uuid,
             entity: $host_entity,
           ),
   

   to also do + $source->getDefaultExplicitInput()

   OR

   \Drupal\experience_builder\Controller\ClientServerConversionTrait::convertClientToServer() should do + $source->getDefaultExplicitInput(), which would ensure that any edit to any component tree would automatically add new required props' default values.

Related: ✨ Usage info for code components with unpublished changes Postponed . Because … it'd be 100% safe to change props and slots whenever as long as there are no instances yet. And thanks to 📌 Calculate field and component dependencies on save and store them in an easy to retrieve format Active , that's now actually possible.

IOW: the conversation here needs to be forked along two paths:

1. 0 instances exist: anything is allowed
2. >=1 instances exist: care is needed

Can you reproduce this, @mayur-sose? 🙏

~3 weeks of silence, so I have to assume #2 was accurate.

Feel free to reopen if not!

@lauriii: has this been decided by now? 🤞

Per @tedbow's testing in #3, plus the fact that 2 months later this still hasn't been reopened, considering this done.

The screenshot shows this particular PHP notice/deprecation came from the field_group module. I'm 95% confident that 📌 Improve server-side error handling Active indeed fixed it.

If not, feel free to reopen, @heyyo! 😊

Thanks, @meghasharma!

Never got the STR, but 📌 Improve server-side error handling Active landed in the meantime 👍

AFAICT this has been (largely?) solved elsewhere?

@balintbrews, can you give us an update? 😇🙏

Marked 🐛 Error Active as a duplicate. This is very low-hanging fruit.

This is a duplicate of ✨ Allow XB to be gradually adopted: make XB's viewing (formatter) and editing (`ApiLayoutController::get()`) work with an empty XB field Active .

I think this is a duplicate of 🐛 SDC prop of `type: string` with empty string listed in its `enum` results in broken input UX Active .

The part was actually solved in 📌 Calculate field and component dependencies on save and store them in an easy to retrieve format Active .

Cross-posted with @longwave in #43 — obviously we agree 😁

Updating title, because thanks to ::testRemoveFieldTypeProviderModule(), @phenaproxima actually solved more while at it 🥳

Was failing tests. Wanted to see this through, so debugged, and found that reverting a single line was sufficient to make it pass. Added a clarifying comment.

Glad to have @longwave's +1 for the new GeneratedFieldExplicitInputUxComponentSourceBase::getDefaultExplicitInput().

Agreed with @longwave's suggestion to improve it + adopt it more widely in a follow-up. Tagging for that. (That will be squarely in the issue queue component, rather than issue's . 👍)

@phenaproxima's additions of ComponentInputs::getPropSourcesWithDependency() and ComponentTreeItem:: updatePropSourcesOnDependencyRemoval() look fantastic (plus, the test coverage of course). The latter new method will undoubtedly come in handy when updating the Pattern and PageRegion config entities. Created ✨ Implement `::onDependencyRemoval()` for `Pattern` and `PageRegion` config entities: when a module providing a field type is uninstalled, replace it with the default StaticPropSource Active for that.

Note that this is far less likely to occur, hence marking this for a later phase.

wim leers → created an issue. See original summary → .

Thanks, @brandonlira!

Specifically, we need to know which database (SQLite, MariaDB …) and which version.

Realization while running, will verify in code tomorrow: this might be caused by the global asset library (auto-saved or not) not being an asset library dependency _yet_ of every code component’s asset library.

I intend to get to the bottom of that one tomorrow, but it is reassuring to read you do not anticipate problems with this implementation order 😊

@mglaman and @penyaskito pointed something out that I missed: #3518185 forgot to update the cache tag logic of the JS component config entity.

This blocks #3523130 per #3523130-15: [PP-1] Auto-saved code component changes are not reflected in content preview and component library preview thumbnail → .

Even if #13 says 🐛 Components sourced from JsComponent are missing source component cacheable metadata Active didn't fix it (and perhaps that's due to a bug I spotted in the current MR, perhaps not), #3522217 should land first because otherwise it wouldn't be clear how to debug/reason about this issue.

Thanks, @bnjmnm!

Removed

debt: we know we'll need to support implicit inputs (e.g. block plugins can declare required + optional "contexts" that allow block plugins behave differently not only to component instance author's explicit wishes ("explicit inputs"), but also based on the component developer's logic ("implicit inputs")

because that doesn't affect the data storage.

Added 📌 Spike: Explore storing component inputs in separate columns (aka field union) Active .

we would have one table per component version (set of fields)

Try to do it in a storage layer that supports one table per component (version), not one per component version per entity type (as is the case with fields in core)

🤯 That could easily be hundreds of DB tables: a site can easily have a 100 components (the issue summary assumes 50), and for each of those multiple versions. (Note that "version" here is a massively overloaded term — there can be very different reasons. See #3523841-6: Version component prop definitions for SDC and Code components → .)

⚠️ Concern: this would make 📌 [later phase] When the field type for a PropShape changes, the Content Creator must be able to upgrade Postponed much harder. What if a site with a million existing revisions decides to implement hook_storage_prop_shape_alter() to change the field type for a prop of an SDC that is present in all of them (to improve the authoring experience, or to switch from plain images to Media Library or $REASON).
This architecture would require rows to be removed from one table and moved into another!

Although I think it could be argued that that would be much clearer. It'd also allow dropping tables for older "component versions" that don't have any remaining rows anymore, and would also allow removing the corresponding entries in the Component config entity that 📌 Version component prop definitions for SDC and Code components Active would've added.

🤔 Not sure yet, but for sure interesting 😄

Explore what views integration would look like

I don't see yet how that'd be meaningful. Views lists things of the same type in a single list/grid/table/…. But here those same things (instances of the same component version) are spread across many entities and bear no relation to one another. Unless you're thinking about listing all the different component instances of a single entity? Or something else still? But listing the first or fifth or Nth instance of some component still is not meaningful?

I struggle to follow your thinking here 😇

Explore what this would like for e.g. Block settings that whilst modeled using config schema (and therefore typed data) are arbitrary in shape and would traditionally be stored in a serialized column

I'm really curious about this part 🧐

So for example if a block changes its settings, we have to loop over every revision and search for components and then update the whole blob.

Indeed. And for that, we have 📌 [SPIKE] Prove that it's possible to apply block settings update paths to stored XB component trees Active .

Thanks to 📌 Calculate field and component dependencies on save and store them in an easy to retrieve format Active + 📌 [PP-1] Evaluate storing XB field type's "deps_*" columns in separate table Active , it's now trivial to find all the content entity revisions in which a certain block plugin is used.

But what's still missing is fundamental core support for generically updating the inputs expected by plugins that may be used in multiple contexts, and not just e.g. Block config entities. The core issue for that is 🐛 Block plugins need to be able to update their settings in multiple different storage implementations Active .

Thanks, @catch, for opening that, and stating it as starkly as you did: — I don't either. 😬

Self-assigned in #10, forgot to link here to the write-up I posted at #3520484-22: [META] Production-ready ComponentSource plugins → as promised. Unassigning.

This potential direction is why I prioritized 📌 [later phase] Support matching `{type: array, …}` prop shapes Postponed and got it finished. Because I have a hard time seeing how this work with multi-value fields. Especially because of #3052670: Support multi-valued "field union"s → .

So, to avoid us adopting this and potentially losing multi-value support, I made sure 📌 [later phase] Support matching `{type: array, …}` prop shapes Postponed was working, and proves that multi-value scalars (type: array, items: { type: integer } — see the sparkline test SDC) and multi-value object shapes (see the image-gallery test SDC) can work in the current architecture.

(I'm not fundamentally opposed to this — just concerned we'd forget about that, and now we can't! 👍)

Related: I tried to push #3467890 forward and assigned it to you at #3467890-13: [later phase] Support `{type: object, …}` prop shapes with single level that require *multiple* field types: use `field_union`? — OUT OF SCOPE: nested components/component reuse → for feedback, @larowlan 😄

Basically only a naming remark left — other than that, this is is IMHO good to go!

Please assign this to @longwave for final review after you've addressed the naming nit.

@longwave, I think the new ::getDefaultExplicitInput() should be used in multiple places in GeneratedFieldExplicitInputUxComponentSourceBase — curious if you agree. That'd allow us to be more confident about it IMHO.

Added @larowlan's 📌 [PP-1] Consider not storing the ComponentTreeStructure data type as a JSON blob Postponed and 📌 [PP-1] Spike: Explore storing a hash lookup of component inputs Postponed .

Obsolete now, see #3518336-32: When a field used by a content template is deleted, any inputs in that template which refer to that field should be replaced with static default values → and #3518336-33: When a field used by a content template is deleted, any inputs in that template which refer to that field should be replaced with static default values → , thanks to 📌 Calculate field and component dependencies on save and store them in an easy to retrieve format Active :)

This would also unblock ✨ ComponentTreeItem should automatically purge subtrees for slots that are no longer exposed Active for ContentTemplate support.