Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
This is a major because it causes loss of user input.
Steps to reproduce:
1. Configure a content type with a required Long Text With Summary field and a file upload
2. Turn JavaScript off
3. Select file(s) and click upload
This will cause the entire form to POST to a PHP fatal error. Other data entered into the form is lost as a result.
Comment | File | Size | Author |
---|---|---|---|
#31 | 2569897-31.patch | 3.83 KB | swentel |
#2 | core-2569897-1-longtextwithsummary_errorelement.patch | 1008 bytes | Les Lim |
#15 | 2569897-15.patch | 2.85 KB | swentel |
#17 | 2569897-17.patch | 3.83 KB | swentel |
#17 | 2569897-17-test-only.patch | 2.85 KB | swentel |
Comments
Comment #2
Les LimJS doesn't need to be off to observe this issue. If there is a required Long Text with Summary widget, and a file upload AJAX operation is triggered, a PHP fatal happens and the AJAX operation fails.
The error occurs in TextareaWithSummaryWidget::errorElement() --
$violation->arrayPropertyPath
is an empty array at this point, so attempting to access the [0] index causes the method to return$element[NULL]
.Here's a patch that refactors the conditions in the errorElement() method. I'm not sure if this would be better fixed upstream.
Comment #3
Les LimTagging.
Comment #4
Wim LeersWow, amazing find!
Comment #5
dawehnerDo we actually understand why we need $element[...] in the first place?
There are two cases:
a) Delta element level violations (like the required one)
b) Property level violations (the one with a property path)
Ideally I think we should explicit convert the code to check the possible property level violations, so checking for summary and value, to make the code more readable.
Comment #6
xjmSo note that a fatal on POST is not data loss, because the data is never actually saved, and the user does have an indication that something broke because they get an error.
Not downgrading just yet because from a product standpoint this could be severe enough to warrant a critical, depending on whether (e.g.) Standard is affected. Looking into that now.
Comment #7
mbayntonProbably should mention that another manifestation of this, probably much more frequently encountered, is that with JS on, uploads silentl fail when the long text with summary is empty.
Comment #8
mbaynton...as Les pointed out. Never mind
Comment #9
xjm@catch and I tried to clarify the critical priority documentation WRT data loss: https://www.drupal.org/node/45111/revisions/view/8746958/8892751
We also discussed the issue with @webchick to evaluate whether this was critical from a product perspective, since the Article node type includes both an image field and a long text field with summary. To reproduce this bug with Standard:
/admin/structure/types/manage/article/fields/node.article.body
/node/add/article
.There are several workarounds:
Since it does not affect Standard out of the box, since there is no loss of already saved data, and since there are workaround, we agreed that the issue is not critical. However, due to the severity of this bug and the extremely bad UX of the form mysteriously becoming "broken" for no obvious reason, the release managers agreed to make this an rc target (we would consider committing a fix for it during the release candidate phase while most other commits are frozen). Of course it can also get fixed now too. :)
Thanks @Les Lim for finding this!
Comment #10
xjmComment #11
Les LimCredit @mbaynton for the find - he brought it up at a meetup last night, then spent a couple of hours testing various scenarios to identify the scope of the problem.
Comment #12
mbayntonComment #13
mbaynton#2571755: Required text field with a Text Editor + AJAX form rebuild = unsubmittable form is another permutation of filling out the fields in the wrong order leading to a broken form (in a different way this time) when a required long text with summary is present. Arguably even worse UX since the entire form becomes irreversibly hosed; mentioning here as precedent in case making it an RC target as well becomes a necessity.
Comment #14
swentel CreditAttribution: swentel commentedLooking for tests
Also getting 'An invalid form control with name='body[0][value]' is not focusable.' console error, but maybe that goes away if the PHP error is fixed.
Comment #15
swentel CreditAttribution: swentel commentedTest which will fail - will look later for the fix.
Comment #17
swentel CreditAttribution: swentel commentedBetter test only patch, and a fix test patch
Comment #21
yched CreditAttribution: yched commentedHm - I'm not sure I get what TextareaWithSummaryWidget::errorElement() is currently doing wrong, and what it means more generally on how WidgetInterface::errorElement() is supposed to be implemented correcty.
Seems weird that an ajax file upload operation would trigger validation and a call to some other field/widget errorElement() ? Wondering what is actually happening there :-)
Comment #22
swentel CreditAttribution: swentel commentedBeen stepping through with xdebug validateForm() is indeed called which yeah is annoying in this case. I also thought that upload had their dedicated (menu/router) callbacks, but the path seems to be /node/add/page?element_parents=field_image/widget/0&ajax_form=1
digging further, weird one indeed :)
Comment #23
BerdirWe removed the separate routes/callbacks when we stopped caching (ajax) forms already on the initial request. We have to rebuild the form to be able to work with it. But that actually doesn't make a difference, it would then do the same.
Comment #24
marcingy CreditAttribution: marcingy at Examiner.com commentedFrom a visual test this patch doesn't seem to fix the issue.
Comment #25
marcingy CreditAttribution: marcingy at Examiner.com commentedActually this may fix the description field issue, as I had another element on the form....entitiyreference autocomplete does
So not sure if we want to extend this issue or create a seperate issue?
Comment #26
marcingy CreditAttribution: marcingy at Examiner.com commentedOk version with entity autocomplete
Comment #27
yched CreditAttribution: yched commentedOK, I get what happens. Tricky business.
TL;DR : the patch is the right fix, it's tested, thus, RTBC :-)
This is not about file uploads really, but happens with any ajax button / form rebuilding button in the form - for example, the "add more" button for a multi-value field on the page. Like :
- Go to /admin/structure/types/manage/article/fields
- Set the "body" field to required
- Add a new field, of type 'Integer', set the field to cardinality "unlimited"
- Go to /node/add/article
- Press the "add another item" button of the integer field
--> nothing happens, internally the ajax call fataled out. With JS disabled, it's a regular submit/rebuild and you see the fatal error page.
What happens is :
- On entity form validation, widgets are invoked to map entity constraint violations to errors on an actual form element (WidgetBase::flagErrors($violations), which defers to each widget's ::errorElement($violation) to find the correct sub-element to flag as error.
- This step omits violations on fields for which a FAPI error has already been reported, because otherwise "required" errors would be reported twice (once by the low-level FAPI #required, and once more by the entity constraint violation)
- But ajax/rebuild buttons like "file upload", "add another value" typically use #limit_validation_errors to mask errors on other parts of the form (e.g to avoid reporting "Node title cannot be empty" when pressing the "upload" button). This causes FormState::setError() to simply ignore errors on some parts of the form. And this interferes with the "omit violations on fields for which a FAPI error has already been reported" logic mentioned in the previous point.
End result :
- On regular entity form submits (like the "Save" button) "NotNull" violations for required fields usually don't reach the widget, because FAPI has already flagged a #required error on the field, and widgets don't report violations on fields already having FAPI errors.
- On submits triggered by buttons with #limit_validation_errors, the FAPI #required errors are reported but immediately ignored, and the "associated" NotNull violation thus reaches the widget, which might not expect it.
Meaning, the widgets have to be prepared to receive violations like NotNull, whose property path point to the field as a whole, instead of only violations on a specific property on a specific delta. So the widget cannot assume that all violations it receives will have a property path like [field name, delta, property]
The irony is that the fatal happens during the work we do to assign form errors that will ultimately get ditched because of #limit_validation_errors...
So, very long story very short : the fix in the patch is correct :-)
I quickly checked the other errorElement() implementations in core, and they are correct.
Comment #28
yched CreditAttribution: yched commentedSorry - the RTBC was for #17.
I dont think the hunk added by #26 should be needed ?
Comment #29
mbayntonI'd unraveled most of the analysis @yched provided, but didn't have much confidence in what a "correct" fix would be when I ran into terms like "delta" and "property path." How much of that is Drupalisms vs Symfony? Could someone help me find more info on Drupal's use of Symfony\Component\Validator\ConstraintViolationInterface?
Comment #30
mbaynton#2571755: Required text field with a Text Editor + AJAX form rebuild = unsubmittable form is indeed still a Thing with #17 applied, by the way.
Comment #31
swentel CreditAttribution: swentel commentedre-uploading 17 and marking RTBC based on #27 - we should create a follow up if there are others because we want tests for those at well then.
Comment #32
larowlanAh so exact same issue as #2557299: Any AJAX call disregards machine name verification when AJAX is used and leads to a fatal error
Comment #33
yched CreditAttribution: yched commented@mbaynton : violations and property paths are vanilla Symfonisms, and we use them as is. Around that, we have some Drupal code & logic to handle what to do with violations once we found them in our data structures (entities with fields, each field having multiple values indexed by a numeric "delta"), and visually flag them on the corresponding forms.
Comment #34
mbaynton@yched, thank you
Comment #35
alexpottCommitted d59b879 and pushed to 8.0.x. Thanks!