This is a spin-off from #2535302: Selecting too many files with JS off causes WSOD with data loss, because there is a simple way to fix the critical component of that issue, but two additional problems including a major one remain. This issue is to address the major one.

Steps to reproduce:
1. Create a content type with a file field that allows a finite number > 1 files to be uploaded
2. Create a node of that type, and upload some valid number of files to your file field (so, no validation failures)
3. Upload additional files (all in one go; the browser will let you select multiple files because it's an <input multiple="multiple"> so that the total number of previously + newly uploaded files is > than the maximum the field allows. This should yield a validation failure, and an interface listing the uploaded files, with checkboxes and a Remove Selected button so the user can pick which files to discard.
4. Select the checkboxes for some file(s) to remove and click the Remove Selected button.

The issue is that the selected files will not be removed, because the entire field widget validation is being applied, and the submission to remove the files falls victim to the same "too many files in the field" validation. At this point, nothing the user can do will return the form to a state where submit handlers can be run. The only way to avoid loss of data entered to the form is to copy every field off to someplace outside Drupal.

Comments

mbaynton’s picture

mbaynton’s picture

Issue summary: View changes
legolasbo’s picture

Assigned: Unassigned » legolasbo
Status: Active » Needs review
StatusFileSize
new2.11 KB

Attached patch fixes the problem and I'm currently working on the tests.

legolasbo’s picture

StatusFileSize
new2.54 KB
new4.51 KB
new4.73 KB

Attached patch adds tests and fixes the failing test from the previous patch.

The last submitted patch, 3: clean_up_filewidget_s-2537930-3.patch, failed testing.

The last submitted patch, 4: 2537930-test-only.patch, failed testing.

legolasbo’s picture

Assigned: legolasbo » Unassigned
mbaynton’s picture

@legolasbo thanks for the patch. I am about to leave for a trip but will check it out in a week or so if it still needs review.

mbaynton’s picture

@legolasbo, I went back and looked at a patch I had started on this issue and reoriented myself with what's going on a bit...so conceptually I favor a different approach with this, in which you #limit_validation_errors on the remove button so that the user can choose which specific images they want to drop to get in compliance with the cardinality limit. That part of the patch is straightforward but opens a rabbit hole of other brokenness in code that doesn't quite handle that case yet. Just wondering, did you initially try this approach as well and decide it wasn't viable?

legolasbo’s picture

@mbaynton,

I went back and forth between different approaches while discussing them with @alexpott in IRC. The main problem is the one described in the comment below. Any manipulation in submit/validation handlers results in the images being removed on the new form displayed to the user, but it would still be failed on cardinality.

+++ b/core/modules/file/src/Element/ManagedFile.php
@@ -288,6 +288,26 @@ public static function processManagedFile(&$element, FormStateInterface $form_st
+      // We have to remove any excess files here because field cardinality
+      // validation runs before submit handlers and prevents handling these
+      // files in a submit handler.
legolasbo’s picture

StatusFileSize
new4.65 KB

Plain reroll.

legolasbo’s picture

StatusFileSize
new328.96 KB
new189.49 KB

New situation:

  1. Edited node with 1 image uploaded already.
  2. Uploaded 2 new images, which exceeds field cardinality of 2.
  3. Validation error triggered
  4. Selected a file
    File selected
  5. Clicked remove selected
  6. Selected file was removed, validation no longer produces errors.
    validation passes
mbaynton’s picture

I can execute the instructions depicted in #12 successfully. This is definitely better than core lacking this patch, as it gives the user a path back to submit-ability of the overall form.

Unfortunately, I'm a bit on the fence with RTBCing it, because while the "Remove Selected" button and corresponding checkboxes on the bottom row work, all the other images that were uploaded (each row besides the bottom) also have Remove buttons, but if you use these buttons they do not work for me.

mbaynton’s picture

Status: Needs review » Needs work
legolasbo’s picture

Assigned: Unassigned » legolasbo

Great find @mbaynton,

I'm currently working on fixing it + adding tests.

mbaynton’s picture

FWIW, if there's a way to just remove the broken buttons that doesn't break standalone ManagedFile form elements, I would RTBC that approach. (Though I'd probably write a new issue to see if there's a way to get them working too. I still feel like there ought to be a way to just suppress the validation from halting submission...not that I've found it.)

legolasbo’s picture

@mbaynton,

I already fixed the buttons, tests have been written and are testing ok, just cleaning up the patch now. Expect a new patch to be uploaded in the next 30 minutes.

legolasbo’s picture

Status: Needs work » Needs review
StatusFileSize
new5.92 KB
new5.81 KB

Attached patch fixes the problems voiced in #13.

Status: Needs review » Needs work

The last submitted patch, 18: clean_up_filewidget_s-2537930-18.patch, failed testing.

legolasbo’s picture

StatusFileSize
new781 bytes
new781 bytes

Fixed the undefined index notices.

legolasbo’s picture

StatusFileSize
new5.95 KB

Lol, now with actual patch.

legolasbo’s picture

Status: Needs work » Needs review

The last submitted patch, 18: clean_up_filewidget_s-2537930-18.patch, failed testing.

mbaynton’s picture

StatusFileSize
new4.33 KB

Hate to do this, especially with the very high likelihood of RC1 happening next week, but rather than RTBCing I'm throwing my hat in the ring with an alternative approach. I wrote this issue back in July due to my failed attempts to extricate FileWidget::validateMultipleCount from the code base as part of the resolution to #2535302: Selecting too many files with JS off causes WSOD with data loss, and really wanted to see that be part of the resolution to this issue. That method is redundant with EntityForm cardinality validation, and doesn't have as nice of a UX for the conditions where it's triggered.

Workable code for removing the selected image(s) is already present, but just wasn't quite working when too many files were uploaded. I tinkered for more hours than I'll admit in search of a way to make use of the existing code, and I think I found it. My hope is this yields more maintainable code down the line -- it's a net decrease in line count.

I'll do a few iterations of patches to aid my development. This patch is going to fail at least the test that checks for the specific error text output by the (redundant and therefore removed) FileWidget::validateMultipleCount method. It is mostly just for me to see what else it breaks. It needs its own tests (hopefully @legolasbo's will be pretty much drop-in for my approach too) and should probably be refactored slightly to tighten down and document what submit handler code runs when the usual validations have been defeated, before it's really ready.

Status: Needs review » Needs work

The last submitted patch, 24: 2537930-24.patch, failed testing.

The last submitted patch, 24: 2537930-24.patch, failed testing.

mbaynton’s picture

Status: Needs work » Needs review
StatusFileSize
new3.5 KB
new9.71 KB
new6.83 KB

I expect this patch to pass testing, including a test for this issue direct from #21 - thanks @legolasbo, and thanks for your fix to file.module as well.

What this patch doesn't do, that I had hinted at in #24 is make changes to submission handler logic to limit what input is used (ie, don't do anything with input that hasn't been validated) when the Remove button was clicked. Maybe someone can explain how this is a security problem -- otherwise, why would there have been validations in the first place -- but on the other hand it's actually not obvious to me that it is: although you're free to change other things about the form (like totally different fields) and have them persisted to the form state when you click remove, most of those things weren't validated anyway due to the #limit_validation_errors that already applied to the Remove button, and clicking the remove button doesn't cause any data to be permanently saved to the entity to which this field is attached - the full suite of validations must still pass when the form is submitted for saving.

The last submitted patch, 27: 2537930-27-testonly.patch, failed testing.

The last submitted patch, 27: 2537930-27-testonly.patch, failed testing.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

xano’s picture

This looks like there may be some overlap with #2669326: FileWidget inside subform can't find its values.

mbaynton’s picture

Anyone at NOLA who wants to help me move this forward?

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

legolasbo’s picture

Assigned: legolasbo » Unassigned

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

Bug reports should be targeted against the 8.6.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.8.x-dev

Drupal 8.6.x will not receive any further development aside from security fixes. Bug reports should be targeted against the 8.8.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.9.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.7 was released on June 3, 2020 and is the final full bugfix release for the Drupal 8.8.x series. Drupal 8.8.x will not receive any further development aside from security fixes. Sites should prepare to update to Drupal 8.9.0 or Drupal 9.0.0 for ongoing support.

Bug reports should be targeted against the 8.9.x-dev branch from now on, and new development or disruptive changes should be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.2.x-dev

Drupal 8 is end-of-life as of November 17, 2021. There will not be further changes made to Drupal 8. Bugfixes are now made to the 9.3.x and higher branches only. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.15 was released on June 1st, 2022 and is the final full bugfix release for the Drupal 9.3.x series. Drupal 9.3.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.4.x-dev branch from now on, and new development or disruptive changes should be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

smustgrave’s picture

+1 for Closing out as outdated
I attempted to follow the steps in the IS

But I get an error like this

Field field_file can only hold 3 values but there were 4 uploaded. The following files have been omitted as a result: test.patch.

And the files that are uploaded have a remove button next to all of them.

So not seeing the error

kim.pepper’s picture

Issue tags: +Bug Smash Initiative
smustgrave’s picture

Status: Needs review » Closed (outdated)

Wasn't able to replicate manually or programmatically.

If you still are seeing this issue please reopen with updated issue summary and steps to reproduce.

Thanks!