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.
| Comment | File | Size | Author |
|---|---|---|---|
| #27 | 2537930-24-27.interdiff.txt | 6.83 KB | mbaynton |
| #27 | 2537930-27.patch | 9.71 KB | mbaynton |
| #27 | 2537930-27-testonly.patch | 3.5 KB | mbaynton |
| #24 | 2537930-24.patch | 4.33 KB | mbaynton |
| #21 | clean_up_filewidget_s-2537930-20.patch | 5.95 KB | legolasbo |
Comments
Comment #1
mbayntonComment #2
mbayntonComment #3
legolasboAttached patch fixes the problem and I'm currently working on the tests.
Comment #4
legolasboAttached patch adds tests and fixes the failing test from the previous patch.
Comment #7
legolasboComment #8
mbaynton@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.
Comment #9
mbaynton@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_errorson 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?Comment #10
legolasbo@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.
Comment #11
legolasboPlain reroll.
Comment #12
legolasboNew situation:
Comment #13
mbayntonI 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.
Comment #14
mbayntonComment #15
legolasboGreat find @mbaynton,
I'm currently working on fixing it + adding tests.
Comment #16
mbayntonFWIW, 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.)
Comment #17
legolasbo@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.
Comment #18
legolasboAttached patch fixes the problems voiced in #13.
Comment #20
legolasboFixed the undefined index notices.
Comment #21
legolasboLol, now with actual patch.
Comment #22
legolasboComment #24
mbayntonHate 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::validateMultipleCountfrom 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.
Comment #27
mbayntonI 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.
Comment #31
xanoThis looks like there may be some overlap with #2669326: FileWidget inside subform can't find its values.
Comment #32
mbayntonAnyone at NOLA who wants to help me move this forward?
Comment #37
legolasboComment #44
smustgrave commented+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
Comment #45
kim.pepperComment #46
smustgrave commentedWasn'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!