Problem

When adding multiple images/files to multiple image fields on a single entity, most of these images are not persisted and are list.

This is due to the way the limit validation form api feature works. It only adds the validated items to $form_state['values'], something that was changed during the development of D7 and while the default multiple values field widget implementation was updated to fix this, the file/image widget was not and still assumes that all already uploaded files are present in $form_state['values'].

What exactly happens is that when uploading a new image, even though only the relevant part of the form is returned, the full form including all widgets are rebuilt from scratch and because the rebuilding code does not find the images, they are not correctly rebuilt.

This can also be seen when disabling ajax, since in that case, the whole form is rendered again and displayed and the already uploaded images are immediatly lost while when using ajax, this bug is only visible after saving the entity.

Proposed resolution

The patch re-implements the same workaround that the default multiple values field widget uses. It stores the form values separately in form_state and fetches them from there.

A better way would be if the field API would automatically handle this and provide all existing values in $items every time the form is built.. $Items is currently only used when editing an existing entity (it then contains the current values of that entities) and even then, only those who are actually persisted and updates aren't reflected. Although not many field implementations need to do it, but currently, writing a custom multiple values field widget is extremely hard due to issues like this.

However, something like that would be impossible to backport to D7, so this needs to be fixed first before a better way to deal with this can be found.

Remaining tasks

The patch attached in #1059268-38: Files are lost when adding multiple files to multiple file fields at the same time works, has automated tests and has also been tested manually multiple times. The code needs to be reviewed so that it can be commited.

Original report by David D

I don't know if this should be listed under "file system" or "image system", but here's the problem:

The issue is that with an image field set to unlimited in a vertical tab (using field_group 7.x-1.0-rc1), multiple images may or may not get saved. By re-editing the node multiple times, all images can eventually get added, but in general only one additional image per field can be added per save. In case this is related to the contrib module, I've posted there as well. I've got a content type with a series of 6 field groups each having an unlimited image field, and the effect is happening on every image field.

Apache/2.2.16 (Unix)
PHP 5.2.14
PHP memory limit 128M
MySQL 5.0.91

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

droplet’s picture

can you post steps to reproduce this bug ?

Berdir’s picture

And, you can you reproduce it on a content type *without* field_groups? If not, then this is an issue with that module.

David D’s picture

I have just created a new content type for testing, with two added image fields. The first was a new field, and the second was one of the existing fields created for the previous content type where I'd seen the problem. I'm not using any field_groups in this content type.

I created two nodes of this type, adding 4 images to each field in the first instance, and 5 in the second. In both cases, only one of the images for the first field actually got added, while all images for the second field got added. And in both cases, the missing images were also missing in the editing view as well.

I edited the first node. I added 2 images to the first field and one to the second. All were successfully added this time. On the second node, I added 2 images to both fields. Again, all were added correctly.

Here is some additional weirdness: I didn't Preview when I initially created the nodes, but I did when I edited them. In both cases, the second (of 2) added image for the first field did not show up on the preview (but it was still there below in the editing form), even though it did get added when the node was saved.

David D’s picture

Having ruled out field_groups, that makes this a Drupal core issue, no?

tahiticlic’s picture

Same problem here.

gadams’s picture

This issue also exists with PHP 5.3.

Subscribing.

miro_dietiker’s picture

Same problem here. PHP 5.3, no field_group.
We're encountering it with file uploads (pdf, txt).
As soon as we have two multiple filefields (unlimited) and add more than one value, only one get saved per field.

Seems to be a core issue!

Berdir’s picture

Title: "unlimited" Image field failure » Files are lost when adding multiple files to multiple file fields at the same time
Version: 7.0 » 7.x-dev
Component: file system » file.module

Confirmed that this happens when you have two file or image fields and add two or more files to each field. Then when you save, only the first file of the first field is saved. All values of the second field were saved correctly. What I was able to confirm exactly:

- All files show up in {files_managed} as they should
- But they are not added to the {field_data_...} table
- They are already gone when the presave hooks are called
- In fact, not even the #value_callback (http://api.drupal.org/api/drupal/modules--file--file.field.inc/function/...) of the corresponding form field for them is called.
- They are however part of $_POST, so they are lost somewhere in between. No idea where...
- All of this is true *per save*, so when you end up with 1/2 images after the initial save, edit the node and again add 2 files each, you end up with 2/4 files.

- I also noticed that when you have multiple image/file fields, you get a repeating notice when deleting multiple files with the "Remove" button. Might be related...

- I then tried to write a test for this but found yet another issue. When you have JavaScript disabled and select a file for the first field and press submit on the second field or already have a file attached to the first field and add one to the second, the upload form element including upload button is gone. This is actually the same behavior as you have *with* JS but then, you only notice it when you save the Node.

Updating the title and component, although I'm not 100% sure that it is file.module that is responsible.

effulgentsia’s picture

subscribe

sun’s picture

Berdir’s picture

Version: 7.x-dev » 8.x-dev
Priority: Normal » Major
Status: Active » Needs review
FileSize
10 KB

Now, that was fun...

The problem is that the multivalue widget form from file.module hasn't been updated to reflect the changes in #limit_validation_errors, meaning that only validates values are in $form_state['values'].

This patch might not be perfect yet but should fix it, I reworked it to use field_form_set/get_state() just like the default multivalue form.

To do that, I had to move the logic that removes deleted values and re-orders the items into the submit callback function. Took me ages to figure what exactly was going on and how to fix it.

Note that the tests will fail, I'm working on extending them with a second file field to reproduce this behavior but that's also rather complicated and I wasn't able to finish that yet.

No idea if that patch applies against 8.x, but it needs to be commited there first anyway so we can let the testbot try it...

Edit: and after looking at the patch, now I remember that I patched DrupalWebTestCase, not sure anymore if that is still necessary now.

Status: Needs review » Needs work

The last submitted patch, fixe_multiple_multivalue_file_fields.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
14.36 KB

Getting there...

- Fixed elements with a cardinality other than one or unlimited. In contrast to the default multivalue widget form, files and images only display existing files + one upload form element.
- Simplified count logic a bit in the submit callback.
- Updated the multivalue tests, they pass now!
- Removed the need for the change in DrupalWebTestCase, had to reverse the logic in my test.

I found a new problem however. It looks like single value fields are now broken when testing. I can not reproduce outside of the tests yet, but when I test, $items still contains the deleted file when the form is rebuild results in the file being displayed again. I have no idea how to fix this at this point.

Status: Needs review » Needs work

The last submitted patch, fixed_multiple_multivalue_file_fields2.patch, failed testing.

David D’s picture

Thank you very much for your work on this, Berdir. It looks like a nasty bug hunt.

skilip’s picture

I just tested this patch with a nested managed_file field and it worked properly. A big plus for this patch! It should even get into 7 IMO.

olrandir’s picture

Is there any chance this is related to this issue I submitted for D7?
http://drupal.org/node/1147988

Any help appreciated, thanks in advance!

bryancasler’s picture

subscribe

Berdir’s picture

@olrandir: Possible, not sure. Try the attached patch and report back.

olrandir’s picture

Hah, it is. Applied the patch manually (i.e. edited the files myself) since it failed with "patch does not apply", and it solved my problem! Many thanks!

David D’s picture

I've finally tested this patch (also manually applied), and it seems to be working for me as well. Thank you!

Ericmaster’s picture

subscribe

olrandir’s picture

I've encountered a new bug that seems to have appeared after I applied the patch from #13:
In D7, when removing images from a multiple image field the image below the one I am removing the last image in the fields is removed instead of the one I selected. Additionaly, on single image fields, the image is never removed -I click the button, the throbber appears, makes it's cycle and then disappears with the image still there.

Any insight from someone more familiar with the file field module would be appreciated, while I'm also looking into it.

--edited to correct

olrandir’s picture

Is anyone able to offer any help?

I noticed it's not possible to remove the image after the node is saved, though it's possible before. I'm scratching my head on this one, so any assistance would be very welcome!

Berdir’s picture

Ah, now that was valuable information (it only fails after the node was saved).

This is exactly why the tests are failing too, I will try to figure that out.

rfay’s picture

subscribe.

Berdir’s picture

Status: Needs work » Needs review
FileSize
15.02 KB

Ok, attaching a new patch that fixes the problem with deleting files on existing nodes.

The problem is that it currently depends on $items to display them but that isn't updated when the field is deleted.

So I've added the following hack to make it work:

+  $field_state = field_form_get_state($element['#field_parents'], $field['field_name'], $langcode, $form_state);
+  if (isset($field_state['items_count']) && $field_state['items_count'] === 0) {
+    // Assume that field got deleted, force $items to an empty array.
+    $items = array();
+  }
+

This is however really ugly, if there is a better way, please tell me :) I haven't found one.

This should hopefully pass the tests.

olrandir’s picture

First of all, many thanks for the work!

Secondly: this seems to fix the issue for CT's with single value image fields, but I still have problems in CT's with multiple value image fields (i.e. one image field with multiple or unlimited images allowed). To re-iterate my issue: in a multiple value image field, when clicking remove on any image, the last image is removed instead of the one selected.

Again, thanks for your work, this was really helpful.

Berdir’s picture

Ah, missed that one.

This approach is better I think. I'm now actually storing the $items array in the $field_state instead of just the count and the simply use that if it is present.

This one seems to work, please try again.

Berdir’s picture

FileSize
15.18 KB

And now with patch.

olrandir’s picture

Works excellently for me! Thanks for the work!

miro_dietiker’s picture

Amazing to reach this state!

I've pinged our original source project of the issue to check if the patch also works for them.
Other participants please also start testing!

marktheshark’s picture

Losing images (as fields, files seem to be uploaded) here as well, on Drupal 7.2.

Will test fix and report back.

miro_dietiker’s picture

Status: Needs review » Reviewed & tested by the community

Tested this with one of our customer sites live - where the original issue was detected.

Time to pass this patch into the maintainer queue to get fixed.

Tobion’s picture

Here a duplicate bug report with some extra information: http://drupal.org/node/682038

WilliamB’s picture

So is there a way to get a 7.2 version fixed?

sun’s picture

Status: Reviewed & tested by the community » Needs work

mmm, this patch could use some more code comments about what is being performed and even more importantly, why. I could easily spend the next half hour with checking what the new code is actually doing (and only afterwards checking whether that is correct).

+++ b/modules/file/file.field.inc
@@ -447,37 +447,17 @@ function file_field_widget_form(&$form, &$form_state, $field, $instance, $langco
-  // Retrieve any values set in $form_state, as will be the case during Ajax
-  // rebuilds of this form.
...
+  $field_state = field_form_get_state($element['#field_parents'], $field['field_name'], $langcode, $form_state);
+  if (isset($field_state['items'])) {
+    // Use items array from the field state if there is any.
+    $items = $field_state['items'];
   }

1) Why are items taken form the field state? (instead of the passed in items)

2) What happens in the non-existing 'else' case? And which case is that, exactly?

3) It looks like we could retain the old comment to some extent?

+++ b/modules/file/file.field.inc
@@ -447,37 +447,17 @@ function file_field_widget_form(&$form, &$form_state, $field, $instance, $langco
-  // Re-index deltas after removing empty items.
-  $items = array_values($items);
-
-  // Update order according to weight.
-  $items = _field_sort_items($field, $items);

@@ -757,6 +737,33 @@ function file_field_widget_submit($form, &$form_state) {
+  // Re-index deltas after removing empty items.
+  $submitted_values = array_values($submitted_values);

The old code removed empty items and ordered items by their weight before building the render structure/field widgets.

The former seems to happen later now, which might mean that we see empty file widgets in between now?

The latter does not seem to be covered in the new code - all items are ordered by $delta.

+++ b/modules/file/file.field.inc
@@ -447,37 +447,17 @@ function file_field_widget_form(&$form, &$form_state, $field, $instance, $langco
   // Essentially we use the managed_file type, extended with some enhancements.
   $element_info = element_info('managed_file');
   $element += array(
     '#type' => 'managed_file',
-    '#default_value' => isset($items[$delta]) ? $items[$delta] : $defaults,
+    '#default_value' => !empty($items) ? $items[0] : $defaults,

@@ -494,16 +474,16 @@ function file_field_widget_form(&$form, &$form_state, $field, $instance, $langco
+    foreach ($items as $item) {
       $elements[$delta] = $element;
       $elements[$delta]['#default_value'] = $item;
...
     // And then add one more empty row for new uploads.
...
       $elements[$delta] = $element;
       $elements[$delta]['#default_value'] = $defaults;

This #default_value assignment looks a bit odd. I see that #default_value is overridden for each item later on, but why do we use the first item's value as default?

It looks like we're always overriding #default_value later on? Can we remove the default for #default_value?

+++ b/modules/file/file.field.inc
@@ -494,16 +474,16 @@ function file_field_widget_form(&$form, &$form_state, $field, $instance, $langco
   else {
+

Bogus newline.

+++ b/modules/file/file.field.inc
@@ -494,16 +474,16 @@ function file_field_widget_form(&$form, &$form_state, $field, $instance, $langco
     // If there are multiple values, add an element for each existing one.
-    $delta = -1;
-    foreach ($items as $delta => $item) {
+    foreach ($items as $item) {
...
       $elements[$delta]['#weight'] = $delta;
+      $delta++;
     }

Where is $delta initialized?

+++ b/modules/file/file.field.inc
@@ -494,16 +474,16 @@ function file_field_widget_form(&$form, &$form_state, $field, $instance, $langco
-    if ($field['cardinality'] == FIELD_CARDINALITY_UNLIMITED || $delta < $field['cardinality']) {
+    if (($field['cardinality'] == FIELD_CARDINALITY_UNLIMITED || $delta < $field['cardinality']) && empty($form_state['programmed'])) {

The exclusion of programmed form submissions requires a larger explanation in code.

+++ b/modules/file/file.field.inc
@@ -757,6 +737,33 @@ function file_field_widget_submit($form, &$form_state) {
+  // Go one level up in the form, to the widgets container.
+  $element = drupal_array_get_nested_value($form, array_slice($button['#array_parents'], 0, -1));
+  $field_name = $element['#field_name'];
+  $langcode = $element['#language'];
+  $parents = $element['#field_parents'];

I don't really understand why this chunk is needed - it doesn't look like it's used in the following code.

+++ b/modules/file/file.field.inc
@@ -757,6 +737,33 @@ function file_field_widget_submit($form, &$form_state) {
+  $submitted_values = drupal_array_get_nested_value($form_state['values'], array_slice($button['#array_parents'], 0, -2));
+  foreach ($submitted_values as $delta => $submitted_value) {
+    if (!$submitted_value['fid']) {
+      unset($submitted_values[$delta]);
+    }
+  }
+  $count = count($submitted_values);

Looks like this is the new code for removing empty items? Why does it have to manually grep the submitted form values instead of using $items?

Also, that $count doesn't seem to be used anywhere.

1 days to next Drupal core point release.

Berdir’s picture

Status: Needs work » Needs review
FileSize
3.37 KB
15.34 KB

Thanks for the review sun!

- Added a comment for the field state thing. As that comment tries to explain, the whole $items handling stuff is really messy right now, and every module that does not use the default multiple values handling has to do it on its own. For 8.x, we might want to fix this by always passing in the correct $items in and let field.module deal with the messy part. But I think we should first fix this rather major bug with a patch that can be backported.

- The re-indexing is now done in the form submit callback:

  // Re-index deltas after removing empty items.
  $submitted_values = array_values($submitted_values);

So when the widget_form() is called again, the $deltas have been re-numbered already.

- #default_value is not overriden when there is only a single value allowed. I moved it into that if condition now.

- $delta is passed in as an argument to this function and is always 0 when having a custom multiple values handling. The old code did some weird stuff with that -1, that isn't necessary anymore.

- I took the programmed check from http://api.drupal.org/api/drupal/modules--field--field.form.inc/function.... There is a comment, but that doesn't state *why* either. It is however rather simple, nothing will be added for a programmed form so it's pointless to add an additional new element.

- The variables from that chunk are used at the bottom of that function:

  // Update items.
  $field_state = field_form_get_state($parents, $field_name, $langcode, $form_state);
  $field_state['items'] = $submitted_values;
  field_form_set_state($parents, $field_name, $langcode, $form_state, $field_state);

It is not really necessary, but it would make those lines rather long. Which would be fine for me. I took that from this function: http://api.drupal.org/api/drupal/modules--field--field.form.inc/function.... Again, in trying to make this as similar as possible.

- As said before, $items is only set when editing entities (and then only when the from is displayed the first time). That's what this is necessary. And yeah, $count is unecessary now, that's a left over from an earlier version.

New patch with the fixes attached.

xjm’s picture

Tagging issues not yet using summary template.

bryancasler’s picture

xjm, can you link to the summary template. I'm having a hard time finding it.

aspilicious’s picture

Click on the edit button on top of the first post and than there will be some info with a link to it.

nemo_Anhoa’s picture

I have the same issues. Can I apply the latest patch (posted on June 29)with Drupal 7.7 ?
I am very new to Drupal, this is my first project in Drupal.
Thanks.

xjm’s picture

#42: I'm submitting the patch for a re-test so we can see if it still applies. If it does, then please do test it and report the results!

xjm’s picture

#38: 1059268.patch queued for re-testing.

Pavlos-1’s picture

+1

dddave’s picture

mariusz.slonina’s picture

Patch #38 saved my project today, tested with D7.7, many many thanks!

bryancasler’s picture

xjm, can you re-re-submit the patch?

xjm’s picture

@animelion: You can do this by clicking the Re-test link next to the patch.

xjm’s picture

#38: 1059268.patch queued for re-testing.

bryancasler’s picture

FML, thanks XJM :)

jgalletta’s picture

Same issue here on drupal 7.7

Is an "official" backport to Drupal 7 expected?

droplet’s picture

Issue tags: +Needs backport to D7
dddave’s picture

@those who experience this problem

Please test the patch in #38 and report here if it works or not. We need feedback.

droplet’s picture

Tested, patch is working (I haven't review the code)

found another issue during test: #1266748: Changing cardinality lower than highest existing delta causes data loss upon save

nemo_Anhoa’s picture

I apply the patch, and it pass for Drupal 7.7
My question is, does Drupal 7.8 include this patch?
Or after upgrade to 7.8, I have to reapply the patch ?

xjm’s picture

#56: This patch has not been committed yet, so I would re-apply it to 7.8,

catch’s picture

catch’s picture

#38: 1059268.patch queued for re-testing.

Berdir’s picture

I've updated the initial description according to the template, I assume I can remove the tag now.

This already was RTBC once, has been reviewed be sun and updated accordingly, looking for an RTBC :)

catch’s picture

Status: Needs review » Reviewed & tested by the community

I read through the patch and it looks sane to me, and comes with plenty of tests, so I think we're in good shape.

catch’s picture

#38: 1059268.patch queued for re-testing.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Eek. What a crappy bug. Thanks for the fix, and for the tests.

Committed and pushed to 8.x and 7.x. Thanks!

miro_dietiker’s picture

Yeppiiiee!

I'm very happy to see this fixed now. Thank you all a lot.

David D’s picture

Woo hoo. It's committed now? Yay and thank you!

Status: Fixed » Closed (fixed)

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

Tobion’s picture

Status: Closed (fixed) » Active

I updated to Drupal 7.9 which includes this "fix" but unfortunately this bug got even worse for me.
When I upload a new file some others get lost. In this cicumstance no error information is given but instead the message appears that the article has been updated although I didn't save it yet!
Also the node gets unpublished at the same time (and thus it's menu item disappearing).
I cannot even publish it again because even if I set the publish checkmark and save it, it's still unpublished.

All in all, Drupal is not usable anymore because it's ignoring my inputs and deleting and modified at will.

xjm’s picture

#67: I would suggest perhaps opening a separate issue (which you could reference here), and, in that issue, writing out some detailed steps to reproduce the behavior you are describing.

catch’s picture

Status: Active » Closed (fixed)

Yes please open a new issue and link to it from here. While the symptoms are the same it's likely to be a different cause.

marktheshark’s picture

With Drupal 7.9 I'm experiencing a different behavior. Files are not lost, but I am not able to add more than one image to a file field at a time. I need to save, edit again, save, edit again and so on. This is annoying for many images...

Is this the intended behavior?

xjm’s picture

#70: You should look for an open issue describing that problem , or create a new one.

Tobion’s picture

Btw, my issues described in #67 are resolved luckily. I updated PHP from 5.2 to 5.3 and since then the problems disappeared!
I can only advise anybody to use PHP 5.3.

jacobpov’s picture

I can confirm that I have the same issue as described in #70.

xjm’s picture

#73: Again, locate an open issue or create a new issue. This issue is focused only on the original problem, which has been fixed. So reporting it here won't get any followup. :)

GiorgosK’s picture

please advise on what should be changed on contrib code in order to accommodate this core change

after introducing this patch to druapl7 seems to have affected contrib modules
#1329056: "Add a new file" form disappear after selecting existing file

GiorgosK’s picture

Issue summary: View changes

Added summary description according to the template