Media Library dialogs triggered from page data do not have buttons yet

As I start to look into this, I'm running into an issue where if the

• Page Data form has been visible at any point
• That form has a media library widget (does not need to be interacted with - it just needs to have been rendered)

Then if we switch to the props form, the media library "Add media" button will not work

I logged the differences in the options used by ajax.$form.ajaxSubmit(ajax.options); between a working "Add media" and a broken one (some hashes decoded for your convenience) https://www.diffchecker.com/45TipW82/

They are largely the same, but ajax_page_state[libraries] is quite different
WORKS

ckeditor5/internal.drupal.ckeditor5
ckeditor5/internal.drupal.ckeditor5.codeBlock
ckeditor5/internal.drupal.ckeditor5.emphasis
ckeditor5/internal.drupal.ckeditor5.htmlEngine
ckeditor5/internal.drupal.ckeditor5.image
ckeditor5/internal.drupal.ckeditor5.table
comment/drupal.comment
core/ckeditor5.autoformat
core/ckeditor5.blockquote
core/ckeditor5.horizontalLine
core/ckeditor5.link
core/ckeditor5.list
core/ckeditor5.pasteFromOffice
core/ckeditor5.removeFormat
core/ckeditor5.sourceEditing
core/drupal.autocomplete
core/drupal.collapse
core/drupal.vertical-tabs
experience_builder/extensions
experience_builder/xb.transform.cast
experience_builder/xb.transform.dateTime
experience_builder/xb.transform.firstRecord
experience_builder/xb.transform.link
experience_builder/xb.transform.mainProperty
experience_builder/xb.transform.mediaSelection
file/drupal.file
filter/drupal.filter
media_library/widget
menu_ui/drupal.menu_ui
node/drupal.node
node/form
path/drupal.path
text/drupal.text

DOES NOT WORK

core/internal.jquery.form,
experience_builder/xb.drupal.ajax,
media_library/widget,

Given that the non-working one is much smaller, I suspect the AJAX request is reloading a JS asset that is actually already present. This has been the cause of the exact same symptoms in the past, so that's where I'll poke around FN

The buttons weren't appearing because we were setting #access to FALSE on any actions arrays in 'experience_builder.api.entity_form' adding a check so it only does that if the form is rendered in xb_stark allows the buttons to show up

NEW CHALLENGE
I'm running into problems when the following is true

• If the entity form has an image field AND a media library field
• The media library field has a value, so remove must be run before I can Add media

When 👆is true and I click Add media I get an exception from \Drupal\file\Element\ManagedFile that originates from a call in FormAjaxSubscriber because it thinks the triggering element is the image field's AJAX upload element instead of the Add media button. The subscriber runs a callback intended for a ManagedFile button, but the request contents are for opening the Media Library dialog and it errors out. It's pretty specific so I'm sure I can find a specific cause, but if this rings a bell for anyone LMK.

Assigning to @wim-leers without the expectation of pushing it the whole way through, but if he can dislodge the button triggering that's great (but if other priorities emerge I don't feel stuck on this so also happy to resume this after the US holiday .

After removing a media item, an AJAX refresh occurs, \Drupal\media_library\Plugin\Field\FieldWidget\MediaLibraryWidget::formElement checks $referenced_entities = $items->referencedEntities(); and correctly identifies the number of referenced entities as 0. This is evident in the UI because the "Add media" button is visible and accompanied by the following message which is only

Opening media library. One media item remaining.

HOWEVER

When "Add media" is clicked \Drupal\media_library\Plugin\Field\FieldWidget\MediaLibraryWidget::formElement runs again, including the call to $referenced_entities = $items->referencedEntities(); , but $referenced_entities is returning a value as if the Remove never took place, so the widget concludes there are no more $remaining, and sets the opener button to #disabled which makes it ineligible for being seen as the triggering element.

Where this is currently at:
IN THE PAGE DATA FORM
The media library widget, if it's empty, will open fine, with buttons and it is possible to add media.
HOWEVER, if I am using the XB UI on a node where the ML field already has a value, I'm not able to open the Media Library dialog via "add media" if it is preceded by removing the media. I've narrowed this down to the AJAX rebuild not being aware of the media removal - it thinks the available slots are still full so it sets "Add media" to #disabled which prevents the dialog from opening due to #disabled not being a valid triggering element.

When these steps are performed outside of experience builder, the removed media is known because form/form_state are properly restored from key_value_expire on the AJAX rebuild. When this happens in XB, there is an attempt to do this, but the request has a build_id different than the one needed to retrieve the cache from key_value_expire.

There's some additional info in the MR explaining various changes.

It looks like the hidden build_id input in the form is initially getting the new build ID, but it changes back to the original. Gonna try adding a mutation observer + ref to ensure those changes stick.

Where I'm leaving off today:

• 😎 Media library widget/modal now works right in the page data tab. You can remove media and add new media etc.
• 😎 Media library value changes in the Page Data form DO get sent to the Autosave Controller
• 🤦 The values don't survive ClientDataToEntityConverter yet. I left a comment on the MR - might be an easy thing once @wim.leers has a look?
• 🤦 something in the changes done so far have broken the ability for props-form-ML-widgets-FOR_IMAGES to update the layout when they change. It's a media library widget being used unconventionally so I'm not shocked it might be impacted by getting things status-quoey. Not too worried about it but it's currently an issue.

The ClientDataToEntityConverter hint reminds me of 🐛 Unexpected empty subtree error Active which I just triaged.

Investigated.

I'm happy to report I have explanations. 👍

I'm sad to report that I don't have a solution. 😭

I'm hopeful that my findings help you, @bnjmnm 😊

Part one: 99% similar, but that's a dead end!

Upon selecting a media library item and clicking the button in the Media Library modal dialog, two requests are fired:

1. one Drupal AJAX system-triggered one (and is sent to /xb/api/form/content-entity/…), to update the form. This works correctly, which is why the selected media item appears in the widget.

   The request looks like this:
   

   Note the presence of the triggering element!

   This is what

2. the other is triggered by XB itself (and is sent to /xb/api/layout/…), because it updated the Redux state and uses this to talk to the server, to essentially re-perform most of the same logic: once again we need to map HTML form inputs to corresponding content entity field values. That necessarily involves WidgetInterface::extractFormValues().

   The request looks like this:
   

   Note how the image[media_library_selection] form input indeed has the same value. Which is why @bnjmnm is rightfully puzzled why it doesn't work: it seems right.

   However, also note the absence of the triggering element!

The remarkable thing: it is NOT due to the absence of the triggering element that things fall apart for /xb/api/layout/…!

Part two: ::addItems() execution is needed

The symptom that led me to discovering what the actual root cause was: the request to /xb/api/form/content-entity/… ends up executing \Drupal\media_library\Plugin\Field\FieldWidget\MediaLibraryWidget::addItems().

(::addItems() is what actually inspects the triggering element, reads media_selection, loads the Media entities associated with the listed selected IDs, and sets it on $field_state['items'][]. That is what \Drupal\media_library\Plugin\Field\FieldWidget\MediaLibraryWidget::extractFormValues() extracts its value from!)

The actual root cause: \Drupal\experience_builder\ClientDataToEntityConverter::setEntityFields() submits the entire content entity form! (This also explains why it was not a problem for the per-component instance ComponentInputsForm at /xb/api/form/component-instance/…: there, the form consists of only a few widgets at a time, and only the experience_builder:image SDC used the Media Library, so it's even only a single widget.)

Although even that shouldn't be a problem, because that should still eventually result in all submit handlers being executed, including those for the widget. However: there's form validation errors, due to the the xb_page title being empty by default/initially, and that is considered a validation error.

      // If there are no errors and the form is not rebuilding, submit the form.
      if (!$form_state->isRebuilding() && !FormState::hasAnyErrors()) {
        $submit_response = $this->formSubmitter->doSubmitForm($form, $form_state);

— lines 587–589 in FormBuilder::processForm(), line 589 is executed for /xb/api/form/content-entity/…, but not for /xb/api/layout/…. Because there are errors.

Now, if I force the code to think there's never any errors, it still fails, because

  public function doSubmitForm(&$form, FormStateInterface &$form_state) {
    if (!$form_state->isSubmitted()) {
      return;
    }

    // Execute form submit handlers.
    $this->executeSubmitHandlers($form, $form_state);

— lines 37–43 in FormSubmitter never gets to executing the submit handlers, because ::isSubmitted() is FALSE.

If I bypass that too, then I see that what I said a bit earlier is wrong: the MediaLibraryWidget::addItems() submit handler is absent, because it doesn't live at the form level — it's an AJAX-only kinda thing! 😬

(In fact: I specifically tried adding a new Media field + media library widget on a node, using the traditional node form. Populated the media field with one media item, saved it. Then go back to the edit form but turn JS off. Because that's essentially what we must achieve: reconstructing the desired entity values using only form inputs. Turns out that that doesn't work at all: removing works, but the "add item" results in the original selection being restored. Unless you delete the form state that appeared in the key_value_expire DB table while you loaded the original form: then clicking does nothing.)

How to proceed from here

Now, unfortunately, the whole point of /xb/api/layout/… is that it is able to send only form inputs, and get the corresponding content entity (field values). If we'd have to make it perform a sequence to trigger specific AJAX elements in order, that'd defeat the purpose.

⚠️ AFAICT this does mean that some Field Widgets will not work in XB without modification: any that rely on "AJAX-exclusive" patterns such as this:

    // When a selection is made this hidden button is pressed to add new media
    // items based on the "media_library_selection" value.
    $element['media_library_update_widget'] = [
      '#type' => 'submit',
…
      '#ajax' => [
        'callback' => [static::class, 'updateWidget'],
…
      ],
…
      '#validate' => [[static::class, 'validateItems']],
      '#submit' => [[static::class, 'addItems']],
…
      '#limit_validation_errors' => !empty($referenced_entities) ? $limit_validation_errors : [],
    ];

— MediaLibraryWidget::formElement()

I see only two solutions, but @bnjmnm might see more:

1. Update the client-side entity_form_fields (aka selectPageData()) Redux value tracking to be updated based on AJAX results (but do so only for AJAX-enabled field widgets). This is AFAICT the simplest.

   It would let AJAX field widgets work without change. But it would also mean slower updates of the preview for AJAX-heavy field widgets, because first the AJAX form update must be completed, which allows updating entity_form_fields, which in turn allows updating the preview (i.e. /xb/api/layout/…).

   IOW: two sequential requests instead of parallel.

2. Subclass the MediaLibraryWidget to simply accept media_library_selection, without requiring a triggering element to call getNewMediaItems() and assign its return value to $field_state['items'].

   I suspect this is simpler, but the goal for XB is to make all field widgets work in it, so this would net us closer to that.

There is a third option: if you manage to fix the last bullet in #12, perhaps you can generalize it:

something in the changes done so far have broken the ability for props-form-ML-widgets-FOR_IMAGES to update the layout when they change. It's a media library widget being used unconventionally so I'm not shocked it might be impacted by getting things status-quoey. Not too worried about it but it's currently an issue.

I can see slightly deeper than the symptoms, but lack the insight into the deeper client-side changes you made here to determine the root cause efficiently. So what follows is a partial analysis.

The reason: in HEAD, the model entry for an experience_builder:image component instance looks like this after selecting an image from the media library:

Array
(
    [11db286d-6019-47ae-82d0-4641e1e7fd6a] => Array
        (
            [source] => Array
                (
                    [image] => Array
                        (
                            [expression] => ℹ︎entity_reference␟{src↝entity␜␜entity:media:image␝field_media_image␞␟entity␜␜entity:file␝uri␞␟url,alt↝entity␜␜entity:media:image␝field_media_image␞␟alt,width↝entity␜␜entity:media:image␝field_media_image␞␟width,height↝entity␜␜entity:media:image␝field_media_image␞␟height}
                            [sourceType] => static:field_item:entity_reference
                            [sourceTypeSettings] => Array
                                (
                                    [storage] => Array
                                        (
                                            [target_type] => media
                                        )

                                    [instance] => Array
                                        (
                                            [handler] => default:media
                                            [handler_settings] => Array
                                                (
                                                    [target_bundles] => Array
                                                        (
                                                            [image] => image
                                                        )

                                                )

                                        )

                                )

                        )

                )

            [resolved] => Array
                (
                    [image] => 1
                )

        )

)

But due to the changes this MR makes, it now instead looks like this:

                            [551bacd0-3bdf-4e47-8ecb-9efbe9eccfc8] => Array
                                (
                                    [source] => Array
                                        (
                                        )

                                    [resolved] => Array
                                        (
                                        )

(Copy/paste a serialized dump in the auto-save entries in the DB into this script to view 'em like this:

$autosave = '<serialize() result here>';
print_r(unserialize($autosave)->data['data']['model']);

Oh, wait, that third option I mention at the end … might actually be more viable than I thought! Just stumbled upon this @larowlan MR comment and he made me realize that the client-side transforms ( 📌 Move clientside assumptions about prop form data shape into a series of prop specific transforms Active ) functionality is not yet applied there!

See his comments

• #3500385-8: Display validation errors in page data form →
• #3500385-10: Display validation errors in page data form →

And of course: see the mediaSelection client-side transform (in transforms.ts) that 📌 Move clientside assumptions about prop form data shape into a series of prop specific transforms Active added! 😄

I am out of my depth in that area, but I bet that:

• between the two of you, you can figure it out 😄
• 📌 [Needs design] Display validation errors in page data form Active landing first would make this MR much simpler? Or, instead, doing the subset of that MR that makes the content entity form use the already existing client-side transforms

I feel stupid now for not having thought of that during my hours of debugging 😬🙈

Also related.

All 3 issues I added in the previous comment and this one are children of 🐛 Some components cannot be used in XB because UI prevents SDC props being named `name` Active .

I have this working when we hit ApiLayoutController::post after selecting an item from the media library.

But its not working when I hit 'publish'

At the same breakpoint in the converter in publish all, the image is empty

As far as I can see something in the subsequent conversion causes the media values to be clobbered by the media widget buttons - and I only have those values in $form_state->getValues()

I think its because the form cache I'm getting no longer has the correct media widget in form_state->storage

I've pushed the current state up.

Note this isn't using any of the transforms because we have to keep things in Form API state here.
With components we keep things in the shape we need
I did start another branch trying to use the serializer and keeping things in the format we want them to be, but that hit a dead end because media widgets work in media IDs and the serializer works in UUIDs

larowlan → closed merge request !674

This is now passing tests.

I think the next steps are:

* Normalizer for datetime
* Normalizer for boolean
* Normalizer for options_select

And if you add that to the items already in the MR

* Normalizer for media
* Normalizer for autocomplete
* Normalizer for image

You start to see that this list looks just like the list of things we alter in widget_info_alter to add transforms for the frontend.

Except these work in reverse.

For example boolean - in the transform we have `boolean_field[value]` and we transform it to just value.

Here we need to take `boolean_field[value]` and make it `boolean_field[0][value]`.

Similarly for media, we have `media_field[selection][0][target_id]` - in the transform we turn it into target_id. To make a valid entity we want to extract `media_field[0][target_id]`.

So this leads us to start looking at the 'widget' as the source of truth for 'what normalizer should I use'

Which when we look at how the page data form works - it starts to make even more sense because that is built from entity form displays, which contain widgets.

So I have two paths here:

* Add a datetime, list_string/list_integer and boolean normalizer OR
* Add a custom normalizer for Content Entities that consults the entity form display and normalizes/denormalizes based on the widget configured

The first option would be the easy option _but_ it would hard-code assumptions that 'date time fields use the date time widget' and 'boolean fields use the checkbox widget'.

The second option would allow us to make it more fluid and is in the same spirit as the way we have per widget transforms for the component props form (albeit these are in PHP instead)

Thoughts @wim leers @bnjmnm on which approach to take? I'm leaning towards option two as like with the FE transforms it gives contrib widgets a way to be part of the conversion process.

Did a 1hr spike on option 2 and it looks like it is going to work _and_ will require a normalizer per widget which is consistent with what we did for transforms. Pushed the wip to a separate branch but will continue tomorrow.

I hope the following comment makes sense. I'm basing it on what I've been able to glean from issue comments, but I haven't yet inspected the code that's in 0.x or in this MR, so it's possible that some of what I write here is incorrect, in which case, apologies for that.

Backing up a bit, here's what we have in Drupal core, outside of XB:

• Widget::formElement() is a server-side field-to-widget (F->W) transform.
• Widget::massageFormValues() is a server-side widget-to-field (W->F) transform.
• Formatter::viewElements() is a server-side field-to-html (F->H) transform.

XB aims to enable component-based design, and component-based design is about ditching the F->H concept (i.e., formatters), and replacing it with components, which are props-to-html (P->H) transforms, which then means we need field-to-props (F->P) transforms, which XB implements server-side as "prop expressions". So, with XB, the above becomes:

• Widget::formElement() is a server-side field-to-widget (F->W) transform.
• Widget::massageFormValues() is a server-side widget-to-field (W->F) transform.
• Prop expressions are server-side field-to-prop (F->P) transforms.
• Components are server-side (if SDCs) or client-side (if JS components) props-to-html (P->H) transforms.

What I think (please correct me if I'm wrong) 📌 Move clientside assumptions about prop form data shape into a series of prop specific transforms Active added is client-side widget-to-prop (W->P) transforms. Note that this doesn't have any server-side analog in the above lists.

What I think #23.2 is suggesting is to add client-side widget-to-field (W->F) transforms, which if feasible, would be nice in terms of parity with what happens server-side.

If we do #23.3, then do we still need the W->P transforms, or could we replace those with client-side prop expression evaluators, i.e., F->P transforms, so that W->P could be refactored as W->F, then F->P? Because if we can do that, then that will result in full parity between client-side and server-side for everything except F->W (Widget::formElement()), which we don't yet have a use case for needing a client-side version of (or do we?).

Just to clarify the above. This is only about the page data form IE not about components

What I'm proposing is server side W to F (field items) so we can then just use core's serializer to take F valies to entity

And the converse F to W so we can send an entity in Json that matches the page data form (widget driven) structure we have to work with client side

In theory a lot of the W to F could happen client side but the transforms we need would be different to the ones we have for W to P because props are simpler than field items. But there will be some W to F transforms that have to happen server side (media, image, entity autocomplete) so I think it makes sense to make it all server side. Unless I'm mistaken we don't have the same need for client side rerendering with the page data form that we want for components. So I think that's acceptable.

Oh ok, yeah the scope of #26 makes sense, and given that scope, server-side makes sense too. But in that case, are you proposing:
- client has W
- client makes call to server to convert W to F
- client gets back F and makes another call to server to save F to entity

If so, what's the benefit of that vs.
- client has W
- client submits W to server
- server saves W to entity using Form/Widget API

I'm assuming the latter of the above is what was tried earlier in this issue and it ran into problems? If so, is there a way to quickly summarize what those problems were? I think the former is a fine way to go if there are compelling reasons why that's superior to the latter.

What I'm proposing is

* server converts entity to field values (F) and then into widget format (W)
* client receives data in W format
* client keeps and updates data in W format
* clients sends W format to server
* server takes W to F and then to entity

There is no extra call to the backend, its just that instead of messing with the form system to convert W to F to E, we use the serializer.
This sidesteps a number of issues we've experienced in previous attempts here AND a big one that we haven't even started on yet which is to do with multi-users publishing in bulk. The form system relies on tokens. Tokens are session specific. To get form API working, we were having to store the form build ID (And keep it up to date) in auto save so we could retrieve the media library items from form state. You probably already know this as form API maintainer - but for those who don't they're stored in field state which is kept in form state storage between ajax calls using form cache. Form cache is keyed by form build ID which updates on every rebuild. So we had to keep this up to date both client and server side. And that means if one user makes an edit but another users tries to publish it we have to recreate the token based on the publishing user's session - even though we're already using a CSRF token in the header.

The approach to do this all server side with the serializer instead of form API - but with custom normalizers for widgets that are edge cases - lets us side step all of that. It does introduce a requirement where complex widgets need a corresponding normalizer - but that is no different to what we're doing with props forms where complex widgets need a transform.

I have a passing branch with hard-coded normalizers, but my second branch introduces per-widget normalizers (only for those that are complex) and I think is more elegant and gives an extension point for contrib.

Ah yes, #28 sounds right. However, does it let you side step the Form API problems entirely, or do you still have many of those same problems for AJAX interactions? For example, doesn't interacting with the Media Library still require the form build ID and all that? Or did you come up with a way to side step it there as well?

No, because all of that interacts with EntityFormController which is just a regular ajax form.

All of this is just to work with turning entity_form_fields from auto save into an entity and back.

I've updated the MR to use the widget based normalizer approach

#23: The reliance on normalizers (and having to define custom ones for some widgets) is kinda breaking the promise that @lauriii and @effulgentsia wanted XB to uphold: that your existing widgets would continue to work. Of course, the client-side transforms we introduced ( 📌 Move clientside assumptions about prop form data shape into a series of prop specific transforms Active ) already is sort of breaking that, but this feels … worse/a bigger burden. Still, if that's what we have to do, to make everything work reliably, then so be it.
simple widgets (aka most of them, specifically the majority of contrib/custom ones) should not require a custom normalizer. But let's see how this plays out first 🤞

Nicely phrased: 👏 — and really, all of #23 is really helpful to understand this unexpected pivot. Thanks for doing that write-up! 👍

Definitely prefer the second path you described → , for the reasons you cited.

#24: First: YAY for validating the second path! Second: I hope is an oversimplified/short statement for "every widget must define metadata for which normalizer it should use", and not "every widget must write a PHP normalizer class". 🤞

#25: Superb write-up of how XB works at a high level — I have exactly this in my head, but haven't found the time yet to create a diagram for it. I'm really glad you wrote it down textually 😄

#3499554 … W->P

Yes! Except that that doesn't work for everything, which is why there's also #3499550. That calls Widget::massageFormValues(), for those Ps where doing W→P directly is impossible, and we need W→F→P.

What I think #23.2 is suggesting is to add […] W->F

No, because it's proposing sever-side normalizers, so it can't be client-side. Because of what @larowlan wrote in #26: .

#28: → this is basically the first possible solution I saw in #14, which indeed has major downsides (sequential requests, "imperative form updates" as @effulgentsia described somewhere, which the Media Library Widget definitely uses).

It does introduce a requirement where complex widgets need a corresponding normalizer

Music to my ears — this addresses my main concern 👍

Looking forward to review in detail! 😄🤓

2 current concerns about the proposed MR

Discussed this MR at a high level with @effulgentsia and @bnjmnm made me realize I had 2 concerns while writing #34 already that I didn't express on the issue:

1. Keeping these new (de)normalizers in sync with the field widget implementations. 👉 Pretty obvious, I think. But I also think it's not any worse than the client-side transforms we already have: those also need to be kept in sync.
2. Having both
   • For /xb/api/form/component-instance/…: #3499554+#3499550 (client-side transforms + optional server-side massaging)
   • For /xb/api/form/content-entity/…: this MR aka (de)normalizers

   … why not one of them?

   Why is it A) necessary, and B) worth maintaining two pieces of complex infrastructure?

Re-checking @larowlan's comments to attempt to answer that without having the time to review the MR today, I see these in #26:

• Just to clarify the above. This is only about the page data form IE not about components

  Doesn't answer why, but does confirm the observation.

• In theory a lot of the W to F could happen client side but the transforms we need would be different to the ones we have for W to P because props are simpler than field items. But there will be some W to F transforms that have to happen server side (media, image, entity autocomplete) so I think it makes sense to make it all server side. Unless I'm mistaken we don't have the same need for client side rerendering with the page data form that we want for components. So I think that's acceptable.

  Aha!

@larowlan thinks we need real-time (aka without talking to the server first) updates of field widgets that are used as inputs for component instances, but not for content entities' structured data aka "the content entity form" aka "the page data tab".

That real-time bit is not implemented today, but we have plans for that in points 3 and 4 of 📌 Implement endpoint for realtime preview Active .

So that is why it'd be worth maintaining 2 parallel pieces of infrastructure:

1. #3499554+#3499550 so that if you type into component instance forms, we'll be able to update the preview without talking to the server for many (most?) inputs, at least once #3453690 points 3+4 are done
2. this MR for content entity forms, for which we accept we won't have real-time previews

Is that a correct interpretation of what you wrote, @larowlan? If so, that's a decision with significant "Product" impact that'll need @lauriii's sign-off.

What I don't understand yet

What I don't understand yet, and reviewing the MR likely will answer it, but @larowlan could probably bring clarity faster.

Why can we get the MediaLibraryWidget to work using the #3499554+#3499550 tandem in one form (component instance), but not the other (content entity)? Is it because client-side transforms currently do W→P and not W→F → ? If so, that seems trivial to add support for, but I'm probably missing something.

Confirmed that landing 🐛 "Add media" button doesn't always open the media library Active will get the new e2e test to pass

Assigning to @wim leers to help determine if these tests that check the criteria is met pass... should this go in as is and address other serializing in additional issues?

Did a first deep review pass of the MR. I understand how it works now. See my write-up on the MR, and my ℹ️-prefixed MR comments, specifically this one.

The "what I don't understand yet" from #35 still stands. Because while I can see how this largely works (I do wonder why it fails to work for image widgets, especially with \Drupal\experience_builder\Normalizer\ImageWidgetNormalizer present, see the screenshot in my write-up on the MR), I still don't think it'd be wise to have 2 parallel approaches, as described in #35.

Assigning to @effulgentsia, because he indicated he had a long conversation with @larowlan over the weekend about this, so I'm hoping he can confirm/deny/explain 😄

I really wanted to like this MR, especially after #28 and the related conversation that @larowlan and I had in Slack about it. However:

The reliance on normalizers (and having to define custom ones for some widgets) is kinda breaking the promise that @lauriii and @effulgentsia wanted XB to uphold: that your existing widgets would continue to work.

Ouch. Yeah, that's a deal breaker for me, unless we've exhausted every other option.

Of course, the client-side transforms we introduced [in #3499554] already is sort of breaking that, but this feels … worse/a bigger burden.

I think what's worse about this MR is that at least in-theory the client-side transforms should be optional. If a contrib/custom widget doesn't include one (or opt into using the no-op one), the system should fall back to a server request that uses Form API + Widget API to go from widget input to field value and then the prop expression to go from field value to prop value. In this way, Drupal's existing ecosystem of widgets continue to work, and ones that include client-side transforms (or can use the no-op one) just work faster (without a network request once we implement 📌 Implement endpoint for realtime preview Active ). Note that I don't know if the current state of 0.x implements this automatic fallback to server, but even if it doesn't currently, in principle we should be able to do this.

Whereas this MR makes server-side (de)normalizers required for every widget that requires any transformation from widget input to field value, and it's not clear from this MR how we'd be able to add a fallback to Form API + Widget API for widgets for which we lack that (de)normalizer.

This sidesteps a number of issues we've experienced in previous attempts here AND a big one that we haven't even started on yet which is to do with multi-users publishing in bulk. The form system relies on tokens. Tokens are session specific. To get form API working, we were having to store the form build ID (And keep it up to date)

I want to look into this. I don't see what's so bad about storing the form build ID and keeping it up to date. And I don't see why CSRF tokens are required when doing a programmatic form submission. Access checks, however, might be an issue, if the person publishing has different permissions than the person who last triggered an autosave.

If we can make the Form API approach work, I'd prefer we start with that. But that wouldn't make the normalizer work in this MR wasted. I think we can still add those in later, as optional improvements for widgets for which we write them. For example, widgets that come with normalizers could perhaps support concurrent editing (or rather, the anti-concurrent lock could be just for the widget, not the entire entity form) whereas widgets without normalizers would have to anti-concurrent lock the entire form. That would be a nice way to gently incentivize widget maintainers to add normalizers rather than flat out making them not work in XB.

I want to correct what I wrote in #25:

Widget::massageFormValues() is a server-side widget-to-field (W->F) transform.

Reviewing this MR reminded me how wrong this is. If the above were true, then the denormalizers in this MR could just call the widget's massageFormValues() method. But the reason they can't is that W->F is actually client-side widget values -> server-side widget values -> field values, or WC->WS->F. WS->F is massageFormValues(), but WC->WS is spread throughout Form API callbacks (#process, #value_callback, #element_validate, and possibly others). That's why the denormalizers in this MR duplicate logic that's in the widget and the form elements created by the widget, but can't easily de-duplicate it. And that's why if we want the denormalizers to be optional, we need to figure out how to make the programmatic form submission approach work.

My guess is that this new problem I found is related 🐛 Changing a value in Page form then publishing, Auto-save shows a change when there is not one Active
because small difference in entity_form_fields result in a change being shown in "Review X changes" when there isn't one.

larowlan → changed the visibility of the branch 3506467-media-library-dialogs to active.

larowlan → changed the visibility of the branch 3506467-entity-transforms-widget-normalizers to hidden.

larowlan → reopened merge request !674

larowlan → closed merge request !738

Ok, I _think_ have this passing.
I'm seeing consecutive green runs of media-library.cy.js locally. Took ages to track down reliability issues in the tests.

Ok its green finally 😩

greeeeeen at last

@effulgentsia's #38:

Note that I don't know if the current state of 0.x implements this automatic fallback to server, but even if it doesn't currently, in principle we should be able to do this.

It doesn't, but agreed that this would be a way to make every widget work, even if suboptimally (i.e. with a network request, as you wrote).

@effulgentsia's #39++ — crucial insight indeed 👏 I wish I spotted that! 🤓

@larowlan's:

• asset weight bug root cause analysis at https://git.drupalcode.org/project/experience_builder/-/merge_requests/6... 👏 — I bet this explains a bunch of the challenges @bnjmnm was running into.
• core bug discovery (see 🐛 ImageItem::defaultStorageSettings should override display_default Active ), which not only affects this MR but also 🐛 ImageItem::defaultStorageSettings should override display_default Active ), fortunately he was able to work around it
• many self-review remarks on the MR help understand some of the moves/subtle changes that are otherwise hard to grok 👍

Finally: AFAICT the changes in ClientDataToEntityConverter to ensure form ID/build ID/token are retained and respected are a crucial stepping stone towards what @effulgentsia described in #38 wrt automatic fallback for field widgets.

The MR looks great to me in the ~45 mins I time-boxed the review to. Nothing concerning appeared to me*, only reassurances! I do think the person to do the final review pass here should be @bnjmnm since he's been over this area of the codebase the most — if he also doesn't spot any concerns and only reassurances, then let's get this in!

I posted about half a dozen questions, but I expect all of them will be trivial for @larowlan to answer.

*The only concern I have: AFAICT this makes XB equally reliant on the form cache as all of Drupal's existing forms. That actually means it's in no way worse than all of existing Drupal, which is fine! But it's still worth calling out. IOW: after 6 hours (by default, see \Drupal\Core\Form\FormCache::setCache()), this may stop working. Ideally, we'd have a or follow-up to harden that: a test that explicitly wipes the form cache to simulate >6 hours having passed, and then verifies that any forms/field widgets in XB continue to work just fine (or ask the page to be refreshed — not unreasonable).

Did a review, which was largely manually testing different things and confirming they are in fact fine. Added a docs request to an existing thread on the MR and updated a function name in the Media Library Cypress test.

So, to clarify @bnjmnm's #50: once docs are added and the questions I posted on the MR are answered by @larowlan, @bnjmnm will approve the MR and we can get this merged! 😊

Addressed the reviews

larowlan → credited longwave → .

Got to the bottom of the issue ✨ Add an "Include image in display" checkbox to the image field Needs review

Basically FileItem sets a 'display_default' setting to FALSE which ends up in form state because \Drupal\file\Plugin\Field\FieldWidget\FileWidget::formElement sets a #default_value drawn from that.
But because this field is not visible, in \Drupal\file\Plugin\Field\FieldWidget\FileWidget::process it is set to TRUE.
And the code that processes the hidden field in \Drupal\Core\Form\FormBuilder::handleInputElement refuses to set a value in form state because a value already exists and was set from the #default_value.

So that is a bug in ImageItem - we already have an override so I'll add the fix there but opened a concrete core issue - 🐛 ImageItem::defaultStorageSettings should override display_default Active

Per #51, going ahead and merging 👍

Quoting @effulgentsia from #38:

Note that I don't know if the current state of 0.x implements this automatic fallback to server, but even if it doesn't currently, in principle we should be able to do this.

It doesn't exist yet, nor is there an issue for it. Created it, and made it a stable blocker: 📌 Implement automatic fallback to server-side widget value transforms in "page data" (content entity form) Active .

Argh, GitLab refused to merge the green MR that awaited me: I had to merge in upstream. The only commit that was added is the one for 📌 Replace warning-causing EOL dependency eslint-config-react-app Active , but now the Media Library end-to-end test fails on

      cy:command ✘  get	[class*="contextualPanel"] .js-media-library-open-button[data-once="drupal-ajax"]

Then again, there were 3 failures, so here's hoping they're all random due to unreliable CI infra… Retrying! 🤞

Now only 1 failed, but a different one. 👍

Merging!

Argh, another refusal to merge because 📌 Try to get component-operations.cy.js passing without an install in beforeEach Active just landed. Incredible.

Nice to see this land! I tested this and it looks like the widget isn't showing when there's an image in the auto-saved state:

Ah yeah we don't consider auto save state in the form controller

I'll open an issue on Monday

Thanks, @larowlan!

Opened 📌 EntityFormController should load auto save state if it exists Active but we might be talking about different things
From your GIF I wonder if that's re-rendering the initial state rather than Drupal not taking into account the saved state in the form.
IE a frontend issue rather than a backend one.

Will tackle both over there if they're related.

I am not sure why but manually testing at previous commits I figured out this issue introduced 🐛 "There are no users matching "/" whe publishing a node Active . It might also be resetting sticky

The problem reported in #67 may already be represented by 🐛 "There are no users matching '' error message appears after reloading the editor page Active where some work has taken place already