Image/file field breaks after uploading two files

I can confirm this, drove me nuts on a test site back in CPH.
Good reminder to try and debug it tomorrow.

This needs tests.

Curious, does this happen with any multivalue field, or is it specific to images?

a) I have confirmed this bug
b) This seems to only effect images & files. I also tried a text field, decimal field, long text and a long text w/ summary fields and they each worked fine

So I've started narrowing this down a bit...

in file_ajax_upload() (file.module:245) There is the following:

  ...
  $output = theme('status_messages') . drupal_render($form);
  $js = drupal_add_js();
dsm($js['settings']['data']);
  $settings = call_user_func_array('array_merge_recursive', $js['settings']['data']);
dsm($settings);
  $commands = array();
  $commands[] = ajax_command_replace(NULL, $output, $settings);
  
  return array('#type' => 'ajax', '#commands' => $commands, '#header' => FALSE);

The dsm($settings) outputs this after the first upload:

basePath (String, 1 characters ) /
ajax (Array, 2 elements)
  edit-field-image-und-0-remove-button (Array, 10 elements)
  edit-field-image-und-1-upload-button (Array, 10 elements)
tableDrag (Array, 1 element)

but after the second upload:

basePath (String, 1 characters ) /
ajax (Array, 2 elements)
  edit-field-image-und-0-remove-button (Array, 10 elements)
  edit-field-image-und-1-remove-button (Array, 10 elements)
tableDrag (Array, 1 element)

Notice the two remove buttons, but missing "upload" button... getting close

Moving to file module.

bleen == stuck

subscribe

Hello,

I have just made out a will with the version drupal-alpha7, and the bug seems to have been corrected.

Best regards,

Sebastien

@sebyoga, you don't have to make out your will just because of one bug! And I definitely don't recommend using drupal as a pattern :-) There's no need to take this quite so seriously.

Confirming this bug, subscribing.

Confirming this bug, and adding some more symptoms:
* Once you upload one image, if you remove it (while having the upload button visible), the upload button stops working (in the sense that though data is sent to the server, along with image data, the display is not refreshed to reflect this. Saving the node at this point shows that the image was uploaded normally).

* Once you upload both images, and try to remove one:
* if the image is the first in the list, both images disappear
* if the image is the second in the list, it gets removed, but breaks the remove button for the other image.

subscribe

yet another symptom ....

if you accidentally hit the "upload" button before you actually choose a file, then the browse button stops working untill/unless you reload the page... so to sum up:

1. After adding the second file/image the "add another file" disappears.
2. removing the first image (from a list of two) hides BOTH images (doesnt actually remove the other image though).
3. removing the second image (from a list of two) breaks the remove button from image one.
4. Hitting the "upload" button before browsing to a file/image makes the browse button stop working

Ok, i reproduce this Bug, subscribe !

I have found a another bug, When you preview a add / edit node, and after you click on button "upload" on the input file upload, the page is reloaded, and the node is not save.

OK, so nearly everything about #ajax is broken after the commit of #756762: AJAX should follow same rules for whether to call drupal_rebuild_form() as non-AJAX submissions. That may very well be this as well.

Could somebody do a git bisect and find out what commit broke this?

And just for grins... Please try the patch in #756762-93: AJAX should follow same rules for whether to call drupal_rebuild_form() as non-AJAX submissions and see if it fixes this.

Re #19: No, it doesn't fix the bug :-(

git bisect to the rescue.

This regression happened in #763376-111: Not validated form values appear in $form_state

I thought the problem will be that field_ui uses #limit_validation_errors = array('xxx'), which changed significantly in that issue. But a quick fiddle didn't do it for me.

I couldn't pin anything down, but I think part of this stems from the code in file.module at line 597-598:

form_set_value($values_element, NULL, $form_state);
drupal_array_set_nested_value($form_state['input'], $values_element['#parents'], NULL);

That dovetails with the discovery of #21.

For example, commenting out the form_set_value 'fixes' problem #2 (removing first image removes both). But the UI gets messed up. I'm guessing it's related to form_set_value wrongly overwriting $element['#parents'].

@webchick: Re #3: I just saw this issue, so not sure if it's only reproducible under AJAX, but if so, then #789186: Improve drupalPostAJAX() to be used more easily, leading to better AJAX test coverage will facilitate writing a test for this.

Something in the attached patch fixes the issue. I don't know what, but will continue "bisecting" the patch.

The attached patch is the reverse diff of changes to form.inc and file.module in commit #412920. That is the commit relating to #763376-111: Not validated form values appear in $form_state, which is where rfay found the bug was introduced.

I whittled it down to this new section that introduces the bug, thus the attached patch fixes this specific bug. However simply removing this code will of course probably introduce new bugs. I'm continuing to bisect this line-by-line and get into the nitty gritty.

Now that I have looked through this a little closer, it appears that the #limit_validation_errors value for file field elements is causing this error. If it is simply not set in file_managed_file_process() (as per attached patch), the problem goes away, however other bugs arise. E.g. An error message is not displayed if the file is not of the correct type.

I'm not familiar enough with Drupal 7 forms API in order to understand how to fix this properly, but will work on a fix with some direction and guidance.

Set to needs review for the testbots to run on the patches.

There are known bugs with that patch, and there are no tests for this bug, so test results are meaningless.

confirmed bug

if you comment out in file.module :

#limit_validation_errors' => array($element['#parents']),

the issue goes away. But no idea what what new bugs it will bring.

See also #675414: Use #limit_validation_errors on File upload and removal

#limit_validation_errors also removes the unvalidated stuff from $form_state['values'].

By setting this key to $element['#parents'], all the other file widgets get removed from form_state. However, file_field_widget_form() needs this data to rebuild the form.

This patch uses array_slice to widen the scope a bit.
Note that the value of #limit_validation_errors is sort of like the coordinates within the $form array - so array('field_image', 'und', 0) translates to $form['field_image']['und'][0].

We could use a test for this. Also the contents of #31 should be massaged into a code comment for the patch.

I agree about the tests - I don't think this specific issue is likely to regress, but it's mildly concerning that we have no coverage for multi-file uploads.

I'm looking into tests now - but I'm not yet sure if drupalPostAJAX() will suffice because there are javascript behaviors bound to the upload button.

This issue is what #789186: Improve drupalPostAJAX() to be used more easily, leading to better AJAX test coverage was born for. (And what it was finally committed for). I'm sure hoping we can get a test that's (almost) real.

Here's the patch with the #limit_validation_errors tags grouped together, with a comment.

While working on this, I've also found more bugs in the remove button. Try this with a file field:

• upload -> remove -> upload (last upload fails)
• upload -> upload -> remove[1] - remove[0] (second remove fails)
• upload -> upload -> remove[0] (removes both files)

My intuition though is the remove bugs are probably a separate issue.

Regarding the tests I'd be happy for some pointers - drupalPostAJAX is gobbledygook to me.

Okay, applied and tested #38.

Using #15 as a guide, here are my results:

1. After adding the second file/image the "add another file" disappears.

• FIXED!

• Not fixed. In fact, now it never hides the second image on the edit screen. Also, it fails differently depending on the order that you remove the images. If you click remove on the first image, and then the second, the second image does not vanish from the edit screen, but both images are removed from the node once you hit save. If you click remove on the second image in the list, and then the first, the second image vanishes from the edit screen, but not the first, and, upon hitting save, the first image remains on the node.

• (see above)

• Not fixed enough. The browse button does work, but when hitting upload, the image doesn't appear in the edit screen. However, the upload button does in fact work, and the image is attached to the node when I hit save.

Here's some more info on the "more bugs" described in #38:

• upload -> remove -> upload (last upload fails)
• The last upload doesn't exactly fail, it just doesn't display on the edit screen. If I hit save, it is attached to the node.
• upload -> upload -> remove[1] - remove[0] (second remove fails)
• (see item 2 in my list above)
• upload -> upload -> remove[0] (removes both files)
• I can't replicate this issue.

For the record, I'm testing this using Firefox 3.6.10 on Mac OS X 10.6

So, I'd say it "needs work".

Loosely related; #925854: Image fields are not aligned in forms

I confirmed Shawn's report from comment #39. Summarizing:

This patch fixes this bug, but closer inspection and testing reveals more bugs that could not be seen or tested before. These bugs are all related to the "Remove" button. These additional bugs may or may not be related to this issue and patch.

In addition to Shawn's findings, I also noticed that the "weight" field becomes visible after removing an image or file, as per the attached screenshot.

I also confirmed that the remaining bugs continue to apply to file field widgets, as well as image field widgets.

It may be worth committing the current patch and working out the remaining issues separately?

Putting this back to CNR. I think the best thing here would be to add a boilerplate test that verifies this particular fix (if drupalPostAjax() is up to it now, not clear if it is). Commit this patch along with that test. Then open a new issue with tests for the other bugs found and fix those separately.

While this is waiting for more review, can someone offer some starting points for writing the test for this? Would it be implemented in JS or PHP? What type of test would it be?

While working on a test, I found 2 more bugs. First, the $delta calculation is wrong in FileFieldTestCase->uploadNodeFile(). It's just coincidence that it happens to work for a delta of zero or one, but breaks if you try to add 2 or more files.
Second, we don't need to set the encoding, this is done automatically in form.inc.

The test doesn't work, but I'll attach it as a separate patch so folks can see what we're up against.

I've confirmed the remove bugs are unrelated, and caused by #740834: Form elements cannot be rendered without form_builder()

The spin-off issues is #926016: Several bugs when trying to remove files from a multivalued file/image field

Is the test incomplete? Or just doesn't pass yet?

Bevan: drupalPostAJAX() is an enigma - it's possible I'm using it wrong, or perhaps drupalPostAJAX() has limitations that prevent it from working on file fields. This has never been done before so there are no docs or examples. This issue may have to go in without a test.

Though I believe the patch to the file module is correct.

I'm on the fence about this bug being critical as it only affects a small subset of users. It is certainly a major bug but it could be fixed in, say, Drupal 7.1. I'm not changing the priority yet -- gotta think about it some more.

Would this be considered a regression? It wasn't broken in FileField/ImageField, now it's broken in it's replacement?

Also, I would consider "I want more than 2 files/images" to be an important use case.

@tim.plunkett: This is a regression that happened just since #763376-111: Not validated form values appear in $form_state, if I'm not mistaken. I think we just need to get the #limit_validation_errors correct, and it will be fine. Either that, or there's a problem with the implementation of #763376. But I think it's just that that issue didn't get the #lmit_validation_errors fixed properly in this case.

@Dries: I think you might be right that this is not critical. Unless of course there's a problem remaining in the actual FAPI implementation that's causing this. So I would be completely OK with downgrading this as long as @effulgentsia said "Oh, it's just #limit_validation_errors is wrong", instead of saying "Ooops."

Correct, this is a regression - it works in 7.0-alpha6.

Dries: I can't agree this affects a small subset - especially considering file is the replacement for upload.module. I would think it affects virtually everyone (certainly this breaks my own site, which is currently on alpha6). Also why would we wait until 7.1 and not fix it during beta?

I agree with handling each issue separately (sorry Dries, I know you prefer reviewing fewer larger patches than many small ones, but sometimes it's easier to keep community effort focused on one issue at a time).

From a usability perspective, it is possible to work around this problem. You can upload 2 files, click "Save", re-edit your node, and upload 2 more, and keep doing that until you've uploaded all the files you want. So maybe that makes this not critical. But, I'm still leaning towards this being critical. It's such an obvious bug (in the sense that lots of users will encounter it) related to a pretty fundamental operation of a CMS (attaching files to content), that Drupal will seem pretty shoddy if released with this broken.

Furthermore, #38 is a simple and correct fix for the bug. Yes, file.module is currently setting #limit_validation_errors incorrectly, and is therefore, unable to have access to values it needs, because those values haven't been validated, and #38 fixes that bug by setting #limit_validation_errors to encompass all the data that the form processing for this form relies on. Given a choice between downgrading priority and committing a fix without tests, I would vote for committing #38 as-is.

But we should be able to add tests to this. Apologies if the lack of drupalPostAJAX() documentation has slowed that down. I'll work on getting that documented. However, this particular issue doesn't require it. The bug happens if you disable JS too, and #38 fixes it. So it should be possible to test this purely using drupalPost(), emulating the steps you take in a browser that has JS disabled. Any takers on that? Sorry, I'd offer to do it, but I have some other issues I'm trying to help out on before being unavailable for a couple weeks again.

I expect releasing D7 with this bug would attract a lot of criticism. I also think this is a critical bug (not "major"), and that it probably affects a large set of D7 users, likely more than half. With patch from #38 or #45 committed, think this can be lowered to major status while a non-ajax test is written and contributed. However in order to keep the bug from recurring again, I don't think the bug-fix should be accepted without the corresponding test. The issue we split off into #926016: Several bugs when trying to remove files from a multivalued file/image field is perhaps not "critical", but "major".

From a usability perspective, it is possible to work around this problem. You can upload 2 files, click "Save", re-edit your node, and upload 2 more, and keep doing that until you've uploaded all the files you want.

This does not work for me neither with nor without javascript, on latest head with the default install profile and one configuration change to make the Article->Image field take "Unlimited" values, instead of just "1" value.

Thanks effulgentsia for highlighting that the test can simply use drupalPost() and not drupalPostAJAX to test this.

I think this would be RTBC if it had a working test that fails without the #limit_validation_errors patch, and passes with it.

Good point about the no-js testing route, thanks effulgentsia!

The first patch is the test only - it's expected to fail.
The second file is the complete patch.

Looks good!

FYI, another issue that is not related, has caught me out a few times while testing this. It is related to using the "Enter" key to save a node form with files on it: #927176: "Enter" key removes file instead of saving node

Is there any reason we can't set #limit_validation_errors inline like it was before? Patch attached.
Leaving at RTBC, because if there is reasoning against my patch, #55 is RTBC.

Sorry, I don't like #57 because the comment has to be repeated. But I suppose a maintainer can decide which format is preferred.

Great work, folks!

I went with #55 due to the double comment issue.

Committed to HEAD.