- Issue created by @larowlan
- Merge request !536Issue #3499550: Support server side validation and massage of prop values ā (Merged) created by larowlan
- š¦šŗAustralia larowlan š¦šŗš.au GMT+10
Opened a draft MR based on work that was in the parent meta. Pausing for now as there are higher priorities in the parent.
- š§šŖBelgium wim leers Ghent š§šŖšŖšŗ
Per š Remove references to 'props' outside of SDC - use 'inputs' instead Active š
- š§šŖBelgium wim leers Ghent š§šŖšŖšŗ
As described at #3500385-6: Display validation errors in page data form ā , AFAICT this blocks being able to fix the entire envisioned/expected scope of that issue?
- š¦šŗAustralia larowlan š¦šŗš.au GMT+10
Pausing this to work on š Move clientside assumptions about prop form data shape into a series of prop specific transforms Active
Remaining tasks:
- PHPUnit coverage for
\Drupal\experience_builder\Controller\ApiPreviewController::updateComponent
- Modify
\Drupal\experience_builder\PropSource\StaticPropSource::massageFormValuesTemporaryRemoveThisExclamationExclamationExclamation
to mimic form submission aspects of\Drupal\experience_builder\ClientDataToEntityConverter::setEntityFields
and\Drupal\Core\Entity\Entity\EntityFormDisplay::extractFormValues
- the e2e fails are because form values for things like datetime need to make use of valueCallbacks on render elements as well asmassageFormValues
. We're already doingmassageFormValues
and are hacking our way around#element_validate
callbacks (which can also set the widget value) - but we need to bite the bullet and make use of the form submitter because of things like value callbacks on element plugins. Hopefully there's some logic in\Drupal\experience_builder\ClientDataToEntityConverter::setEntityFields
that we can break out into a helper service or similar. Then the pieces of\Drupal\Core\Entity\Entity\EntityFormDisplay::extractFormValues
could probably go into component source specific plugins that make use of widgets
We're fighting a lot of flexibility and inconsistency between widgets and entity data already, but element validate and value callbacks make things harder still.
- PHPUnit coverage for
- š§šŖBelgium wim leers Ghent š§šŖšŖšŗ
Reviewed š Move clientside assumptions about prop form data shape into a series of prop specific transforms Active 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...
- š§šŖBelgium wim leers Ghent š§šŖšŖšŗ
This blocks at a minimum:
- #3487284 ā see #3487284-6: [PP-3] Discover cases where is no 1:1 map between field, prop and widget values ā , first bullet
- #3492061 ā see #3492061-2: [PP-1] Include the preview HTML in the layout controller payload ā
Bumping priority and tagging to reflect that.
- š«š®Finland lauriii Finland
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...?
- š§šŖBelgium wim leers Ghent š§šŖšŖšŗ
#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 š Some components cannot be used in XB because UI prevents SDC props being named `name` Active . 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 š Move clientside assumptions about prop form data shape into a series of prop specific transforms Active already made significant modifications, and it did update
docs/redux-integrated-field-widgets.md
but notdocs/diagrams/react-editor-high-level.md
. OTOH, that diagram is so high-level, that it actually still is accurate. š relates to theRenders Component form
andGenerates 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! š - š§šŖBelgium wim leers Ghent š§šŖšŖšŗ
- 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 onClientServerConversionTrait::clientModelToInput()
callingComponentSourceInterface::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 tosrc
+alt
+width
+height
for example.
(Screenshot attached. Contrast$inputValues
with$resolvedInputValues
.)Issue summary updated.
- 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 š Include the preview HTML in the layout controller payload Active to implement, to allow greater focus and simpler reviews of this MR, just like you suggested, @larowlan š
- 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.
- 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.
- š§šŖBelgium wim leers Ghent š§šŖšŖšŗ
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.
- šŗšøUnited States bnjmnm Ann Arbor, MI
wim leers ā credited bnjmnm ā .
- š§šŖBelgium wim leers Ghent š§šŖšŖšŗ
With š Maintain a per-component set of prop expressions/sources Active now in, 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 š META: Conflict free concurrent editing Active , too.)
- š§šŖBelgium wim leers Ghent š§šŖšŖšŗ
Whew, that conflicted massively with š Maintain a per-component set of prop expressions/sources Active . 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 š OpenAPI spec insufficiently precise for `LayoutComponent` Active .There is at least one clear next step:
\Drupal\experience_builder\Controller\ApiPreviewController::updateComponent()
currently requires'propSources'
, but that makes no sense forBlock
components:if (!\array_key_exists('propSources', $body)) { throw new BadRequestHttpException('Missing prop sources'); }
Attempted to fix that :)
- š§šŖBelgium wim leers Ghent š§šŖšŖšŗ
I can't have done the worst job, because now 15 e2e tests are passing compared to >11 before.
- š¦šŗAustralia larowlan š¦šŗš.au GMT+10
Addressed #18 - it is much simpler than that now that š Maintain a per-component set of prop expressions/sources Active is in, we just send the model. That's it. Rebased, then squashed to do that and then removed š Include the preview HTML in the layout controller payload Active
Will start on the review feedback and the remaining tasks (including fixing tests).
- š§šŖBelgium wim leers Ghent š§šŖšŖšŗ
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! š
- š§šŖBelgium wim leers Ghent š§šŖšŖšŗ
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.
- š§šŖBelgium wim leers Ghent š§šŖšŖšŗ
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.
- š«š®Finland lauriii Finland
I tested this locally and during the update validation the form disappears and reappears after the validation is done. I hope this is not intentional or fundamental because this would be a major regression to the user experience and would simply not be acceptable. The experience of the preview updating as changes are being made must be seamless, as it has been prior to now.
- š§šŖBelgium wim leers Ghent š§šŖšŖšŗ
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....
- š¦šŗAustralia larowlan š¦šŗš.au GMT+10
That's the style tag added in DummyPropsEditForm, it can go, it was something I tried to get the test passing
- š¬š§United Kingdom longwave UK
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?
- š¬š§United Kingdom longwave UK
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.
- š§šŖBelgium wim leers Ghent š§šŖšŖšŗ
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-providedentity_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.
- š¬š§United Kingdom longwave UK
I've added filtering to
ApiLayoutController::getEntityData()
to remove values fromentity_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. - š¦šŗAustralia larowlan š¦šŗš.au GMT+10
This doesn't happen in head because we don't use the converter service, we use a custom convert method in preview controller
I vote we revert back to that and sort it separately
- š¦šŗAustralia larowlan š¦šŗš.au GMT+10
Thanks @bnjmnm, we should revert https://git.drupalcode.org/project/experience_builder/-/merge_requests/5... then as I did that as a temporary workaround
- š¦šŗAustralia larowlan š¦šŗš.au GMT+10
I confirmed #34 solved the issue so I reverted f3fd9d2f
#34 changed the behaviour in HEAD - previously if you sent a field you didn't have access it would generate an error.
Now the field is ignored.
I updated the test to reflect this and it now passes - that is 9592dd2a
Would be good to get a +1 from Ted/Wim/Lauri/Alex on the new behaviour
This should be green again and the 2 bugs are now fixed.
- š¬š§United Kingdom longwave UK
Added a test for
::filterFormValues()
, added some comments and cleaned up some minor bits, this is ready for review again. - š§šŖBelgium wim leers Ghent š§šŖšŖšŗ
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 š - šŗšøUnited States bnjmnm Ann Arbor, MI
bnjmnm ā changed the visibility of the branch 3471978-try-using-admin-theme-without-fence to hidden.
-
wim leers ā
committed 9ea94d82 on 0.x authored by
larowlan ā
Issue #3499550 by larowlan, wim leers, longwave, bnjmnm, jessebaker:...
-
wim leers ā
committed 9ea94d82 on 0.x authored by
larowlan ā
- š§šŖBelgium wim leers Ghent š§šŖšŖšŗ
Next up:
- š OpenAPI spec insufficiently precise for `LayoutComponent` Active , which will unblock
- š Include the preview HTML in the layout controller payload Active , which will unblock š Discover cases where is no 1:1 map between field, prop and widget values Active if we also do š Split model values into resolved and raw Active , which likely blocks š Adding the Image component results in a state considered invalid Active too.