Back port: Incorrect handling of file upload limit exceeded - file widget disappears

Marking this postponed on the Drupal 8 issue for now.

Un-postponing, since the Drupal 8 patch was committed.

Note that my review from https://www.drupal.org/project/drupal/issues/1489692#comment-9308163 still applies to the Drupal 7 patch, in particular:

Also, if it's true that this one case should be ajax_command_prepend() but the other two calls later in the function should remain ajax_command_replace(), this is screaming for a code comment explaining why :) I would at least think that if this one needs to be ajax_command_prepend() then the one after it should be too...

Maybe not the second one, but the first one is the exact same code ($commands[] = ajax_command_replace(NULL, theme('status_messages'))) just with a different error message, so it seems very likely that it should have the same fix.

Uploading a new patch with backported test from D8 patch (failing locally when input is missing). Also changed the second ajax_command_replace() as mentioned in #4.

This bug can cause permanent loss of stored data; not just loss of form input. That qualifies it as critical.

If a multi-value file field already contains files from a previous node save and a user subsequently edits the node and triggers this bug by trying to upload a new file, then goes ahead and saves the node again while the file widget is missing, the entire contents of the file field will be deleted.

I'm quite amazed that this has gone unpatched in D7 for 10½ years (first reported in March 2012), when the fix is as simple as substituting one word on two lines of code! Not to mention the fact that the same bug affects image fields as well as file fields.

Anyway, I've just tested the patch in #5 and it prevents the aforementioned data loss for both file fields and image fields.

Unfortunately, it also introduces a slight usability issue whereby, if a user is editing other fields on a node and then triggers the file field error and immediately tries to save the node, it will actually reload the node edit form and discard all their other edits.

However, if they successfully upload a second file after triggering the error, before saving the node, then the error is cleared and the node can subsequently be saved with all other edits remaining intact.

Regardless, I would say it's important to commit this patch ASAP to eliminate the possibility of data loss. The usability issue it creates can potentially be worked on afterwards as a separate issue if necessary.

Thanks @morbiD for the testing and detailed comment!

I have tested the patch more to check for these two things:

If a multi-value file field already contains files from a previous node save and a user subsequently edits the node and triggers this bug by trying to upload a new file, then goes ahead and saves the node again while the file widget is missing, the entire contents of the file field will be deleted.

This is true, I have experienced the same behavior while testing. One mitigating circumstance is that the user should see that the field is completely missing and hopefully will consider not submitting the form. But yes, this is a good point.

Unfortunately, it also introduces a slight usability issue whereby, if a user is editing other fields on a node and then triggers the file field error and immediately tries to save the node, it will actually reload the node edit form and discard all their other edits.

I have debugged this a bit deeper and it seems like that when we click that ajax upload button with the file exceeding post_max_size (according to the issue summary), then this POST request has no $_POST and $_FILES data. If I try to upload a regular file under this limit via ajax upload button, it has these arrays set. So I suppose that this request without data must somehow broke the whole form, because if you subsequently click save button on the node form as the second action (after you try to upload the file exceeding the limit), also this regular POST request (non-ajax) submits to Drupal with missing $_POST data. This will cause the node edit form to reload, instead of submitting the changes.

I have tried the same scenario on D9 and there the node is successfully saved even if there is the size exceeded error and you click "save" directly after this error. So this must be fixed in D9 somehow.

Also this problem is present regardless of the $commands[] = ajax_command_prepend(NULL, theme('status_messages'));, because if I remove it entirely (and also remove the drupal_set_message()), the problem is still present. It is not present only if I keep the current behavior (replace the managed_file form with the error message), which is a bit weird.

This needs some additional debugging to see what's really going on.

Actually the biggest difference between D9 and D7 is that in D9 the file field ajax POSTs to the original URL (node/NID/edit), but in D7 it POSTs to the file/ajax URL. This make the whole submit process different. So possibly this could be related to: #2819375: Do not make entries in "cache_form" when viewing forms that use #ajax['callback'] (Drupal 7 port) (and the parent D9 issue, where this change was made). But the last patch from that D7 issue does not solve this problem.

This issue mentions removing the check entirely: #2392117: Forms with multiple file form elements produce upload errors with the Drupal page cache enabled , but this does not help either (it is even worse, because user gets no response if the process succeeded or failed). And the underlying issue is still there.

Okay, this is growing. Just to summarize:

Settings needed:
- Turn off display_errors in php.ini.
- Make sure the post_max_size is set to equal or lower than the upload_max_filesize in php.ini. The bug only manifests itself when the post_max_size is exceeded.
- Create a file upload field that is not limited in file size.
- Upload any file larger than the post_max_size.

1. D9 behavior
- error will appear but the file widget won't go away
- edit title and click save
- node is saved, but the file "left" in the file widget is discarded (without any error/warning) - I think this is acceptable

2. D7 behavior without patch
- error will appear and the file witget disappears
- edit title and click save
- node is saved, but the uploaded file and any other existing files in that field are discarded (without any warning) - this will cause the mentioned dataloss

3. D7 behavior with patch
- error will appear and the file witget won't go away
- edit title and click save
- node is not saved, but instead the page is reloaded (because of missing POST data)

The problem with the page reload in the last scenario (3.) seems to be caused by the fact that the file remains in the file upload widget. If I remove it before the save button is clicked, this scenario will work like the D9 and node will be saved.

When trying to accomplish that I have run into another issue: #1060296: ajax_command_invoke - does it work with $selector = NULL?, because ajax_command_invoke() does not work with the NULL selector, as the documentation states.

 * @param $selector
 *   A jQuery selector string. If the command is a response to a request from
 *   an #ajax form element then this value can be NULL.
 * @param $method
 *   The jQuery method to invoke.
 * @param $arguments
 *   (optional) A list of arguments to the jQuery $method, if any.
 *
 * @return
 *   An array suitable for use with the ajax_render() function.
 */
function ajax_command_invoke($selector, $method, array $arguments = array())

So if we somehow manage to unset the file input value before or after running the ajax_command_prepend(), then it would work. Something like this should work (but we need to figure out the element dynamically here or in JS):

$commands[] = ajax_command_invoke('#edit-field-image-und-0-upload', 'val', array(''));
$commands[] = ajax_command_prepend(NULL, theme('status_messages'));

OK, adding a patch that should work to workaround the problem with the form reload after the ajax upload error (see scenario no. 3). See interdiff.

It could be easier, but this is only working for fields with cardinality == 1:

$element_id = drupal_html_id('edit-' . implode('-', $form_parents) . '-upload');
$commands[] = ajax_command_invoke('#' . $element_id, 'val', array(''));
$commands[] = ajax_command_prepend(NULL, theme('status_messages'));

But fields with cardinality != 1 have stripped numeric elements from the parents and thus they must be targetted differently. See the $new_path here:

function file_field_widget_process($element, &$form_state, $form) {
...
  // Adjust the Ajax settings so that on upload and remove of any individual
  // file, the entire group of file fields is updated together.
  if ($field['cardinality'] != 1) {
    $parents = array_slice($element['#array_parents'], 0, -1);
    $new_path = 'file/ajax/' . implode('/', $parents) . '/' . $form['form_build_id']['#value'];
    ...
  }
...
}

I have also updated the test to add an additional form save action to check if the subsequent form save is successfull (thanks again @morbiD).

We are aware that there are problems with D7 tests now, so we would need to wait until they are working again. But if possible, please review this approach. I have tested it on node edit form with multiple file and image fields with different cardinalities set and all use-cases I have tested worked.

Running tests on a local drupalci, the test only patch results in all tests passing except for this one failure:

Managed file element test 327 passes, 1 fail, and 0 exceptions

The specific failure message is:

<testcase classname="File.FileManagedFileElementTestCase" name="testManagedFileExceedMaximumFileSize" status="failed" assertions="11"><failure message="fail: [Other] Line 503 of modules/file/tests/file.test:&#10;After uploading a file that exceeds the maximum file size, the &quot;Upload&quot; button is displayed.&#10;&#10;" type="fail"/><system-out><![CDATA[]]></system-out></testcase>

Now running tests for 2749245-11.patch

Nice work @poker10!

One mitigating circumstance is that the user should see that the field is completely missing and hopefully will consider not submitting the form.

Oh, if only... The whole reason I discovered this data loss scenario is because a user ignored the warning and dropped 78 files from a single file field. (Of course it was one with no previous revisions to revert to either, so I had to retrieve the files from backups). The best part was, they hadn't even made any other edits to the node and clicking "Save" was just their way of getting back to the node view.

I will never cease to be amazed by users' capacities for blindly ignoring warnings and carrying on completely oblivious!

Anyway, back on topic, I've just tried the new patch and it solves the page reload issue. I haven't found any other ways to break the form yet, so it seems good to go.

Not sure if my review on its own is enough for RTBC though!

Tests passed, but I'm only testing with PHP 7.4 + MySQL 5.7

Ideally I'd prefer to see the full suite of tests passing before we commit these last few patches.

Looks good, and thanks for the review/feedback @morbiD - always good to get a +1 for a fix from someone that's encountered the bug in the wild!

Thanks everyone!

I just upgraded to Drupal 7.92 and we have the W3C HTML Validator complaining:
> Error: The value of the for attribute of the label element must be the ID of a non-hidden form control.

Basically now the label don't get the right "for" attribute. The "for" attribute given does not have any corresponding ID, it misses the "-upload" that the input has.

Forget about it, I had a hook overriding theme_form_element_label and was missing to reflect the attribute "label_for".

Okay, glad you figured it out and that it wasn't a problem in D7 core. Thanks!