Support server-side massaging and validating of ComponentInputsForm values

Per #3500994: Remove references to 'props' outside of SDC - use 'inputs' instead 😊

As described at #3500385-6: Display validation errors in "page data" (content entity form), AFAICT this blocks being able to fix the entire envisioned/expected scope of that issue?

Reviewed #3499554: Move client-side assumptions about prop form data shape into a series of prop-specific transforms in-depth. Reviewed this MR at a high level. +1'd your pivotal suggestion at https://git.drupalcode.org/project/experience_builder/-/merge_requests/5...

This blocks at a minimum:

• #3487284 — see #3487284-6: [META] Add explicit end-to-end test coverage for using all core field widgets including multiple cardinalities in the "Page Data" (content entity) form, first bullet
• #3492061 — see #3492061-2: Include the preview HTML in the layout controller payload

Bumping priority and tagging to reflect that.

Would it be possible to describe what the plan is on a high level? Does the change being proposed here impact the diagrams in https://git.drupalcode.org/project/experience_builder/-/blob/0.x/docs/di...?

#11:

The absence of such a write-up is precisely why I found it hard to jump in and help make progress here and all other child issues of #3467954: META: Evolve XB UI's data model to allow non-SDC components' inputs, DynamicPropSource support, etc.. Which is why I spent the entire day so far reviewing all of 'em and attempting to connect the dots that @larowlan sees in his mind but hasn't yet written down in .

Note that #3499554: Move client-side assumptions about prop form data shape into a series of prop-specific transforms already made significant modifications, and it did update docs/redux-integrated-field-widgets.md but not docs/diagrams/react-editor-high-level.md. OTOH, that diagram is so high-level, that it actually still is accurate. 😅 All of this relates to the Renders Component form and Generates through user interaction labels in that diagram.

Started reviewing a while ago, and the introduction of ApiPreviewController::updateComponent() already surprised me given the issue title! 😅

1. The issue title and summary appear out-of-date: they refer to @larowlan's squashed changes where he was indeed introducing all that, but that's no longer happening.
   Except … that all that is still happening, but in an indirect way! This MR leverages almost all of XB's abstractions that currently exist. It relies on ClientServerConversionTrait::clientModelToInput() calling ComponentSourceInterface::validateComponentInput(), which for SDC and "code" components calls:

         $resolvedInputValues = array_map(
         // @phpstan-ignore-next-line
           fn(array $prop_source): mixed => PropSource::parse($prop_source)
             ->evaluate($entity),
           $inputValues,
         );
   

   … which is how @larowlan is able to make it so that "target_id": "434" on the client (coming from a field widget) is transformed to src + alt + width + height for example.
   (Screenshot attached. Contrast $inputValues with $resolvedInputValues.)

   Issue summary updated.

2. I think it'd be a good idea to get https://git.drupalcode.org/project/experience_builder/-/merge_requests/5... omitted from this MR and left for #3492061: Include the preview HTML in the layout controller payload to implement, to allow greater focus and simpler reviews of this MR, just like you suggested, @larowlan 🙏
3. Only after browsing pretty much the entire MR did I discover the #1 comment @larowlan left for reviewers: https://git.drupalcode.org/project/experience_builder/-/merge_requests/5... 😅
   Responded there in detail. I'd like @jessebaker and @longwave at minimum (and preferably also @bnjmnm) to review what @larowlan is proposing/describing in that MR comment, because this a drastic change on at least 3 fronts.

FYI, back in September I created https://git.drupalcode.org/issue/experience_builder-3463842/-/compare/0.... as a way to get server-side massaging+validating of values passed back to the client — but it only works for AJAXy field widgets.

With #3493941: Maintain a per-component set of prop expressions/sources now in, this is now the next one in the plan at #3467954: META: Evolve XB UI's data model to allow non-SDC components' inputs, DynamicPropSource support, etc..

Let's try to push this further now that @jessebaker and @bnjmnm have given their blessing over at https://git.drupalcode.org/project/experience_builder/-/merge_requests/5... 😊 (Plus, @larowlan explained how this brings us one small step closer to #3492059: [META] Conflict-free concurrent editing, too.)

Whew, that conflicted massively with #3493941: Maintain a per-component set of prop expressions/sources. I'm not familiar enough with XB's front-end codebase to be productive in there 😅 I did my best, but probably screwed some things up!

I resolved every conflict to the best of my ability, except for that in ui/tests/fixtures/layout-default.json — we can do that more easily in #3504494: OpenAPI spec insufficiently precise for `LayoutComponent`.

There is at least one clear next step: \Drupal\experience_builder\Controller\ApiPreviewController::updateComponent() currently requires 'propSources', but that makes no sense for Block components:

    if (!\array_key_exists('propSources', $body)) {
      throw new BadRequestHttpException('Missing prop sources');
    }

Attempted to fix that :)

I can't have done the worst job, because now 15 e2e tests are passing compared to >11 before.

This already looks fantastic now!

Down to one failing e2e test.

Thanks to your overnight rebase+simplification (thanks! 🤩👍) of the MR, I think one more thing revealed itself as being unnecessarily complicated — because thanks to this MR, things can be simplified a lot. I'd swear you wrote something similar somewhere else, but can't find it.

Either way: I'm excited about what I'm seeing! 🥳

I'd love for @longwave to review this from the BE perspective and @bnjmnm or @balintbrews from the FE perspective, to enable @larowlan to drive this home on his last workday prior to PTO! 😄

Hm the last failing e2e test also failed during nightly testing, like this:

      cy:command ✔  findByLabelText	Remove Default placeholder image
      cy:command ✔  click	
        cy:fetch ➟  GET http://localhost/web/session/token
                    Status: 200
        cy:fetch ➟  PATCH http://localhost/web/xb-field-form/node/1
                    Status: 200
          cy:xhr ➟  POST http://localhost/web/xb-field-form/node/1?_wrapper_format=xb_template&ajax_form=1&_wrapper_format=drupal_ajax
                    Status: 200
        cy:fetch ➟  POST http://localhost/web/api/preview/node/1
                    Status: 200
      cy:command ✔  get	[class*="contextualPanel"] .js-media-library-open-button[data-once="drupal-ajax"]
      cy:command ✔  first	
      cy:command ✘  click	

And over here, that same test passes that bit, but fails shortly after that.

This looks amazing! 😄

Reviewed: https://git.drupalcode.org/project/experience_builder/-/merge_requests/5...

Pushed 2 tiny commits (one to simplify reviews by moving code back to the original location, one with language nits).

I've asked @longwave (BE) and @balintbrews (FE) to review this, and to fix the one (BE) regression I spotted.

I tested this locally and during the update validation the form disappears and reappears after the validation is done. I hope this is no fundamental because this would be a major regression to the user experience and would simply not be acceptable.

Discussed with @wim leers that #24 is a regression that was introduced here during the last 24h and should be addressed in this issue. 👍

Yes, of course, but the overall MR definitely needs review. To potentially surface more regressions, but hopefully not :D

@longwave is currently working on a review 👍

Also crediting @jessebaker and @bnjmnm for their pre-emptive review/guidance at https://git.drupalcode.org/project/experience_builder/-/merge_requests/5....

So the issue that Wim raised where "Alternative text is required" appears is because in the XB preview #value for the image field is

[
  'display' => false,
  'description' => '',
  'upload' => '',
  'alt' => '',
  'title' => '',
  'fids' => [],
]

but if I do a normal form submission without uploading an image, many of those items - including alt, which is the problem here - are not present:

[
  'fids' => [],
  'display' => false,
  'description' => '',
]

In managed file fields, the alt text input doesn't appear until an image has been selected for upload, at which point the value array is:

[
  'alt' => '',
  'fids' => [
    0 => '2',
  ],
  'display' => 1,
  'description' => '',
]

I think just skipping validation errors will skip the case where an image is selected and alt text is required?

I think this is a front end problem. The layout endpoint returns all the entity field data, but for the preview we should only be sending the input values that are actually present in the form instead of keeping all the original data and overwriting values that have changed.

edit: digging into the form build process a bit more, alt, title and display are added by ImageWidget::process() but #access is set to FALSE if there is no valid file. This means that the empty value is present in the form state - but no element is rendered on the front end and so the value is not expected to come back when the form is submitted. If this is not considered a front end issue we could pre-filter entity_form_fields so only values with #access => TRUE are sent to the front end in the first place?

That #access ⇒ TRUE|FALSE sure sounds like a likely culprit. And even quite possibly a looming information disclosure vulnerability? The client should never even be able to know/see that information? The server should indeed prevent it from being sent to the client!

When looking at a node whose field_image is populated:

When looking at another where it's still empty (should be fine because it's optional):

So I think @longwave is right: we need to find the root cause for field_image[0][alt]: "" being included among the server-provided entity_form_fields.

I bet the complexity lies in how XB interprets ImageWidget's

    // Add the additional alt and title fields.
    $element['alt'] = [
      '#title' => new TranslatableMarkup('Alternative text'),
      '#type' => 'textfield',
…
      '#access' => (bool) $item['fids'] && $element['#alt_field'],
      '#required' => $element['#alt_field_required'],
      '#element_validate' => $element['#alt_field_required'] == 1 ? [[static::class, 'validateRequiredFields']] : [],
    ];

Those last 3 are all relating to the conditionality.

I've added filtering to ApiLayoutController::getEntityData() to remove values from entity_form_fields that have #access => FALSE and should not be accessible by the client - this might leak information to the client even if it's not displayed in the form, so it needs to be done server side.

However this doesn't fix the error; even though field_image[0][alt] is not present in the payload any longer, the validation is still triggered - run out of time today to figure out why this is happening.

dfaf6b18 takes care of the alt validation message. The form generated by setEntityFields was not taking access into account so it was validating fields that should be ignored as their access is false.

Added a test for ::filterFormValues(), added some comments and cleaned up some minor bits, this is ready for review again.

#34 🤯 Wow 😄

@bnjmnm comes in to save the day with a one-line change 👏

I have no more remarks about the BE pieces. As the person who built the ComponentInputsForm and surrounding infrastructure from the ground up, I think it should be @bnjmnm reviewing the FE pieces and doing the final sign-off 😊

Fixed the merge conflict.

Next up:

• #3504494: OpenAPI spec insufficiently precise for `LayoutComponent`, which will unblock
• #3492061: Include the preview HTML in the layout controller payload, which will unblock #3487284: [META] Add explicit end-to-end test coverage for using all core field widgets including multiple cardinalities in the "Page Data" (content entity) form if we also do #3493943: SDC+JS Component Sources: split default values into `resolved` and `source`, which likely blocks #3501902: Adding the Image component results in a state considered invalid too.