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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mbaynton created an issue. See original summary.

Les Lim’s picture

Status: Active » Needs review
FileSize
1008 bytes

JS 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() --

  public function errorElement(array $element, ConstraintViolationInterface $violation, array $form, FormStateInterface $form_state) {
    $element = parent::errorElement($element, $violation, $form, $form_state);
    return ($element === FALSE) ? FALSE : $element[$violation->arrayPropertyPath[0]];
  }

$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.

Les Lim’s picture

Issue tags: +Needs tests

Tagging.

Wim Leers’s picture

Component: field system » text.module

Wow, amazing find!

dawehner’s picture

Do 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.

xjm’s picture

Title: Required Long Text With Summary + File Upload + JS off = data loss » Required Long Text With Summary + File Upload + JS off = PHP fatal error

So 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.

mbaynton’s picture

Probably 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.

mbaynton’s picture

...as Les pointed out. Never mind

xjm’s picture

Priority: Critical » Major
Issue tags: +rc target

@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:

  1. Install Standard.
  2. Visit /admin/structure/types/manage/article/fields/node.article.body
  3. Set the field to "Required field" and save.
  4. Go to /node/add/article.
  5. Without entering text in the body field, click the "Choose file" button for the image field and select a file.
  6. If JS is enabled, the form submit button is now "broken" and does not do anything, regardless of whether you fill in the body field after that. The form does not appear to be submitted, but there is also no validation error displayed.

There are several workarounds:

  1. Enter text in the body field, then select a different image to upload, then remove it and add back the first one. The form will will then be submittable.
  2. Reload the form and re-enter your data, this time adding text to the body field first.
  3. Do not make the body field required.

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!

xjm’s picture

Issue summary: View changes
Les Lim’s picture

Credit @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.

mbaynton’s picture

Title: Required Long Text With Summary + File Upload + JS off = PHP fatal error » Required Long Text With Summary + File Upload = PHP fatal error
mbaynton’s picture

#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.

swentel’s picture

Assigned: Unassigned » swentel

Looking 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.

swentel’s picture

Test which will fail - will look later for the fix.

Status: Needs review » Needs work

The last submitted patch, 15: 2569897-15.patch, failed testing.

swentel’s picture

Status: Needs work » Needs review
FileSize
2.85 KB
3.83 KB

Better test only patch, and a fix test patch

The last submitted patch, 17: 2569897-17-test-only.patch, failed testing.

The last submitted patch, 15: 2569897-15.patch, failed testing.

The last submitted patch, 17: 2569897-17-test-only.patch, failed testing.

yched’s picture

Hm - 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 :-)

swentel’s picture

Been 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 :)

Berdir’s picture

We 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.

marcingy’s picture

Status: Needs review » Needs work

From a visual test this patch doesn't seem to fix the issue.

marcingy’s picture

Actually this may fix the description field issue, as I had another element on the form....entitiyreference autocomplete does

  public function errorElement(array $element, ConstraintViolationInterface $error, array $form, FormStateInterface $form_state) {
    return $element['target_id'];
  }

So not sure if we want to extend this issue or create a seperate issue?

marcingy’s picture

Title: Required Long Text With Summary + File Upload = PHP fatal error » Required Long Text With Summary or Entity Reference Autocomplete + File Upload = PHP fatal error
Status: Needs work » Needs review
FileSize
800 bytes
4.61 KB

Ok version with entity autocomplete

yched’s picture

Title: Required Long Text With Summary or Entity Reference Autocomplete + File Upload = PHP fatal error » Required Long Text With Summary + form rebuild = PHP fatal error
Status: Needs review » Reviewed & tested by the community

OK, 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.

yched’s picture

Status: Reviewed & tested by the community » Needs work

Sorry - the RTBC was for #17.

I dont think the hunk added by #26 should be needed ?

mbaynton’s picture

I'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?

mbaynton’s picture

swentel’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
3.83 KB

re-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.

larowlan’s picture

yched’s picture

@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.

mbaynton’s picture

@yched, thank you

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed d59b879 and pushed to 8.0.x. Thanks!

  • alexpott committed d59b879 on 8.0.x
    Issue #2569897 by swentel, Les Lim, mbaynton, yched, xjm: Required Long...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.