🇦🇺Australia @larowlan

greggles → credited larowlan → .

greggles → credited larowlan → .

greggles → credited larowlan → .

alexpott → credited larowlan → .

Could we update the text to: "Component has been deleted. Copy values to new component." to be a bit more concise?

Done 👌

yep, name looks better, 🚲️🎨🏚️

HookDocumentation?

https://www.youtube.com/watch?v=5sSp-t2fXXg

New changes are green

#60 works for published content because we do client to server transformations during the entity conversion. This means any client-side representations of the model have been removed and we're back to the raw server side version.
But it doesn't work if the data is in the autosave store - opened 📌 [PP-1] Autosave data should store converted server-side representation of model Postponed for that because the scope here is already big enough and that is a big change on its own. Added a skipped test for this case and a passing test for the published case. In the auto save case, we're working with client data that is source plugin specific and needs access to the prop definitions. If we do the client to server transformation before writing to auto save, we don't need the prop field definitions around in order to re-do it.

Also added 📌 [PP-1] Improve the UX of the fallback component input form Postponed to improve the initial version of the inputs form

Removing the follow up tag

larowlan → created an issue.

larowlan → created an issue.

I think this should be closed as won't fix for the following reasons

• It doesn't meet the needs of the 80% use case
• It can easily be achieved with existing APIs for sites or themes that need it
• It causes a lot of cache fragmentation because we're now varying the page cache in every site by each unique combination of roles the user can have. Making all sites pay this cost for the low fraction of sites that need it is the main reason to close this in my opinion (note th current patch doesn't add the user.roles context to the #cache array, but should)

Thank you for taking the time to open this issue, for sharing your solution and for creating an MR for consideration

I have a solution

here's the component edit form when in fallback state

And here's the recovered state

Will add some tests

Thanks for #55 @f.mazeikis and #54 @laurii

I think there's perhaps an incremental step here that prevents the bricked state.

I'll timebox an hour working on that.

I've used the import from Ajv, much cleaner - and added some more inline comments to clarify why it is needed per #20

Thanks for the review!

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 the established approach of using the schema from the BE-returned component prop definition. The BE was previously the source of schema truth and I'd like to know when/why that should be deviated from. It's possible this is info is implicit in the docs already in the MR, but I couldn't parse with certainty.

No, that won't change this. The move to v6 will allow us to drop all of the service provider shenanigans because v6 correctly sees entity:node/1 as a valid URI whilst v5 does not.

In terms of the FE, validation runs before transforms so we need a way to tell ajv that `This is a node title (3)` is valid before then transforming it to entity:node/3

Will fix that export/import

cross post

The goal is both. At present we store all of this in the prop source (and in the inputs column) but the repetition of e.g. storing all of the instance settings each time we use it is very verbose. Consider this

'element' => [
          'sourceType' => 'static:field_item:list_string',
          'value' => 'h1',
          'expression' => 'ℹ︎list_string␟value',
          'sourceTypeSettings' => [
            'storage' => [
              'allowed_values' => [
                [
                  'value' => 'div',
                  'label' => 'div',
                ],
                [
                  'value' => 'h1',
                  'label' => 'h1',
                ],
                [
                  'value' => 'h2',
                  'label' => 'h2',
                ],
                [
                  'value' => 'h3',
                  'label' => 'h3',
                ],
                [
                  'value' => 'h4',
                  'label' => 'h4',
                ],
                [
                  'value' => 'h5',
                  'label' => 'h5',
                ],
                [
                  'value' => 'h6',
                  'label' => 'h6',
                ],
              ],
            ],
          ],
        ],

That's a lot of data to store for every instance of the element prop in the heading component. All that is unique to this instance is the value , expression and the source type.

When we have a dynamic prop source we're only storing this

'image' => [
          'sourceType' => 'dynamic',
          'expression' => 'ℹ︎␜entity:node:article␝field_hero␞␟{src↝entity␜␜entity:file␝uri␞␟url,alt↠alt,width↠width,height↠height}',
        ]

which is much more succinct.

The intention here is to try to hoist some of the repeated elements (e.g source type settings) to somewhere we can rely on them being fixed. The idea is the versioning gives us a mutable place to store them. And in doing so it also opens up some more flexibility with regards to recovery for stored data in previous iterations.

Next step is approval from Ben

I think ✨ [PP-1] Make link widget autocomplete work (for uri and uri-reference props) Postponed might support this

hooroomoo → credited larowlan → .

nod_ → credited larowlan → .

wim leers → credited larowlan → .

wim leers → credited larowlan → .

Nope still failed on CI - might be because of the subdir setup on CI, pushed something else

P.S.: autocomplete-props.cy.js was failing, re-tested: https://git.drupalcode.org/project/experience_builder/-/jobs/5229137

saw that happen, did some stability changes, did some repeat runs locally and it looks ok 🤞

Added docs too

#35 (the front end editing issue) is 🐛 Component/Slot overlay is brittle if HTML comments are missing Active - I've refactored how we render the fallback component to make sure the Twig visitor outputs the comments the FE needs. I've also added a test for it. So that fixes the issues we were seeing but highlights that this is particularly rigid w.r.t. how component slots are rendered. We can't know that _every_ source plugin will print the slots by name in twig (ie {{ some_slot_name }} in order for the twig node visitor to wrap that with the comments. So that follow up is more about making the FE more robust rather than fixing a new issue added here.

#48.1 I've made this adjustment. Was pretty clean/simple in the end.

#48.2

I think that ideally this would be part of \Drupal\experience_builder\Element\RenderSafeComponentContainer

We are already getting wrapped in one of those for free - thanks to how ::renderify works - this is just another component. The changes I had to make to resolve #35 also make it further evident that keeping the two separate are probably best. If we added generic slot printing fallback to the RenderSafeComponentContainer it would have to mimic the same stuff we're doing here with explicitly printing slots by name, but also in try-catch because that component cannot fail to render. I think it is worth exploring that in a new issue - I've opened 📌 [PP-1] Explore adding support for fallback rendering of slots to RenderSafeComponentContainer Postponed to see if the changes I've made to Fallback::renderComponent and ::setSlots could move to a generic render element. I am proposing doing it in a followup because the scope of this MR is already large. If you feel strongly about it I can close that and do it here.

#48.3In theory we can also copy over fallback metadata on prop definitions to the fallback plugin but that assumes we are only dealing with SDC/Code components. What do we do for blocks where the form isn't based off the generated UX base class? I think we should get some input from @laurii here - what is the intended behavior. I don't think we can continue to show the same component inputs form (because how we handle SDC vs Blocks isn't compatible) - we could probably just store the values in hidden inputs so at least the values are retained. But, is it safe to keep the values around in case the scenario that caused the component to be remove is reversed? What happens if the deleted JS component is added back but with different props? I think we should confirm the intended behavior before jumping to an implementation. As a result assigning to @laurii

larowlan → created an issue.

larowlan → created an issue. See original summary → .

Can you provide just the new changes? I can't apply the current patch - thanks

larowlan → created an issue.

Yes, agree #142 is nicer and it also gels with some thinking I had in this experiment

It also means we might not have to add new classes for everything, we could possibly use the existing render element plugins but instantiate them.

Thanks, it was unfortunate that the core issue caused this. There is discussions about relaxing the use of underscores. Glad you sorted it.

wim leers → credited larowlan → .

Picking this spike up

Reviewed this and associated issues and created the following spikes to explore some of the options and be able to size and break this up

• 📌 Version component prop definitions for SDC and Code components Active
• 📌 [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
• 📌 Spike: Explore adding configuration options to the tree item formatter to support alternate use-cases Active
• 📌 Spike: Explore storing component inputs in separate columns (aka field union) Active

larowlan → created an issue.

larowlan → created an issue.

Taking this as a concrete use case

Also thinking through the use-cases more above, I think they can be boiled down to three types. Each one assumes we are rendering a search index or newsletter or 'long teaser' view mode or any alternative rendering of the stuff we put in the full view mode.

1. Show everything in XB slots from the full view mode, except component type X

2. Show the first x deltas of specific component types from the XB slots on the full view mode. x deltas of x components. (testimonial, image gallery)

3. Show a few deltas of any component type, limited to a certain amount (long teaser)

At present there's a primitive formatter for the XB `ComponentTreeitem` that essentially calls `::toRenderable` on the tree item. All the logic to take the tree data and convert it into a render array goes via the tree item.

In order to meet this use-case it would be simple enough to expand the formatter's settings to support the configuration options listed here i.e.

• component type disallow/allow list
• component type limits
• component delta limits

I'll split off a separate child/spike issue for that

Repurposing this as a spike to explore storing the tree in a flat structure and unblocking 📌 [PP-1] Spike: Explore storing a hash lookup of component inputs Postponed which would basically be the proposal in comment #9 here, but instead of props we'd store the lookup hash

Additionally there's scope to use CTEs here like we do in Entity hierarchy v5 to retrieve an ordered tree in a single SQL query should we need it. That module has views integration to let people do things like 'is child of' and 'is parent of' in a single query - which might be attractive for issues like 🌱 [META] Support alternative renderings of prop data added for the 'full' view mode such as for search indexing or newsletters Active

larowlan → created an issue.

.

Added a link to 📌 Version component prop definitions for SDC and Code components Active to item 2.1. Wim and I basically came up with the same thing independently - nice!

larowlan → created an issue.

I'm still not convinced this module is the right place for this, there are already several solutions for this (such as element class formatter).

I _could_ see the benefit in having it in this module _if_ the formatter made use of the attribute plugins - but at present it is a hard-coded list.

So in order to be useful, I think it should be dynamic (based on the plugins) rather than a fixed list. But even then I'm still not sure this is something we should be doing - as I believe there are other modules that already do this well/better.

New title as we want to proceed with this.

Here's the explain for selecting all entities using e.g. a particular component. We use the same buildDependencyQuery code path for the uninstall validator so the query is the same with only the params changed.

+--+-----------+------------+----------+----+--------------------------------+----------+-------+-----------+----+--------+-----------+
|id|select_type|table       |partitions|type|possible_keys                   |key       |key_len|ref        |rows|filtered|Extra      |
+--+-----------+------------+----------+----+--------------------------------+----------+-------+-----------+----+--------+-----------+
|1 |SIMPLE     |dependencies|null      |ref |PRIMARY,source_entity,dependency|dependency|267    |const,const|1   |100     |Using index|
+--+-----------+------------+----------+----+--------------------------------+----------+-------+-----------+----+--------+-----------+

This is ready for review now

Thanks I've committed and pushed that change to the 2.0.x branch

XB is already 11.1+ only and will soon be 11.2+ only as there are important fixes to block config schemas in 11.2 that it has workarounds for.

Without a backport do SDCs that work in 11.2+ error in 11.1 because of schema validation issues?

Can we add/expand an e2e test for this?

smustgrave → credited larowlan → .

Can you share the attribute names
A recent security update added XSS filtering of attributes and it strips those with underscores

No worries, it's a nice to have and shouldn't hold this up

bnjmnm → credited larowlan → .

wim leers → credited larowlan → .

wim leers → credited larowlan → .

Blocked on both the related issue and a design

I think we should defer #40 through #44 to 💬 ComponentNotFoundException: Unable to find component Active
- the scope here is already large enough

This issue was specifically about dealing with config dependency removals (the original STR were creating a Code Component, then adding it to the library but _not_ being able to delete it because it was *in use*)

I've pushed @f.mazeikis's commits to a branch in 💬 ComponentNotFoundException: Unable to find component Active with all of the commits from here squashed so rebasing will be easier once this is in.

I've then rewound this branch to the commits before @f.mazeikis started addressing #40 onwards.

Added 📌 [PP-1] [Needs design] Implement design treatment for fallback component Active to implement the design changes (once ready) eluded to in #36

Putting this back to needs review in its current state.

larowlan → created an issue.

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

Committed to 11.x
Published the change record.
Whilst this is a bug it is not eligible for backport because of the constructor BC change.

Thanks all, great to see this one finally resolved!

,

Added 📌 ComponentSourceInterface::submitConfigurationForm and validateConfigurationForm are never called Active

larowlan → created an issue.

.

larowlan → created an issue.

Discussed with @catch and he was ok with adding the 'load multiple' equivalents in a follow up
Can we get that issue added and update the code in MenuActiveTrail service to add a @todo pointing to the new issue?
That plus a change record (and update the link) and this can go back to RTBC, self RTBC is fine as its only comment/deprecation changes.

Thanks!

@dww not fussy either way, if we can cleanly instantiate it - works for me. Removing the Novice tag

Left a comment on the MR regarding the deprecation link.

There's also a performance impact here, I've pinged catch about whether we need to fix that here or in a followup.

Committed to 11.x - thanks!
Published the change notice.

Committed to 11.x, thanks!

Putting back to RTBC because all the changes from my last review where I changed the status were either trivial or reverted.
I manually confirmed we still have the bubbling check from the other point.

Given the changes here would have been fine for catch to self RTBC I think it is OK for me to commit this still.

Committed to 11.x - thanks!

The blocker is in, I think a static method is the best approach if possible

Committed to 11.x and unpostponed the follow up

Thanks

There is currently no way for individual entities to provide the subtrees that should fill in those slots, to be assembled for final rendering. Let's add that in this issue.

I think we also have to allow for multiple view modes.

What happens if Full exposes 3 slots and Teaser exposes an entirely different slot.

Is delta + slot enough to distinguish - do we also need a view mode column?

What does the UI look like when editing a piece of content with multiple view mode templates? is there a view mode switcher? Do we prevent saving if a piece of content is not valid for a given view mode?

Adding 📌 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 as related

Thanks @dww, have pinged them

Committed this to 11.x so we can get more eyes on it. It's internal non production code so the risk is low in doing it this late in the alpha window.

Thanks all

Published the change record

Added a release note snippet and the corresponding tag.

Committed to 11.x - thanks!

I've also asked release managers if this is eligible for addition during beta

Looking at the process for adding a new experimental module → do we have an existing roadmap for the path to stable for the module? i.e. items #8 and #9 on that list? I think we need those before this can go in.

Who is familiar enough with how api.drupal.org works to confirm this will work? I'll ping @quietone, she might know or at least know who to ask

wim leers → credited larowlan → .

Committed to 11.x - thanks!
Published the change record

I don't see any tests for the validator class, only for the constraint

I'm confused about the scope here - the parent issues says 📌 Convert field_storage_config and field_config's form validation logic to validation constraints Needs work

I've split two issues for the missing tests.

And links to this issue.

But this issue adds the constraint and no tests?

The needs tests tag was added in #3 but then removed later without any discussion I can see (apologies if I've missed something obvious).

We're adding the constraint to the schema too, so should we be adding some tests? Should we be removing something else?

I'm curious about the use of NULL in the call to ->aggregate - it would be good to see some tests confirming that this indeed checks all langcodes for instances where the existing delta is higher.

One comment about duplication

Left one suggestion on the MR
Reviewed each new hook class against the .install file in HEAD and it all looks good to me.

Leaving it at RTBC, please let me know if you're happy with the suggestion on the MR.

Needs a reroll after PHPUnit 11 MR went in but other than that this looks great, just a couple of MR comments