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
Comment | File | Size | Author |
---|---|---|---|
#38 | 1059268.patch | 15.34 KB | Berdir |
#38 | 1059268_changes_only.patch.txt | 3.37 KB | Berdir |
#30 | 1059268_field_state.patch | 15.18 KB | Berdir |
#27 | 1059268.patch | 15.02 KB | Berdir |
#13 | fixed_multiple_multivalue_file_fields2.patch | 14.36 KB | Berdir |
Comments
Comment #1
droplet CreditAttribution: droplet commentedcan you post steps to reproduce this bug ?
Comment #2
BerdirAnd, you can you reproduce it on a content type *without* field_groups? If not, then this is an issue with that module.
Comment #3
David D CreditAttribution: David D commentedI 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.
Comment #4
David D CreditAttribution: David D commentedHaving ruled out field_groups, that makes this a Drupal core issue, no?
Comment #5
tahiticlic CreditAttribution: tahiticlic commentedSame problem here.
Comment #6
gadams CreditAttribution: gadams commentedThis issue also exists with PHP 5.3.
Subscribing.
Comment #7
miro_dietikerSame 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!
Comment #8
BerdirConfirmed 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.
Comment #9
effulgentsia CreditAttribution: effulgentsia commentedsubscribe
Comment #10
sunsubscribing
Comment #11
BerdirNow, 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.
Comment #13
BerdirGetting 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.
Comment #15
David D CreditAttribution: David D commentedThank you very much for your work on this, Berdir. It looks like a nasty bug hunt.
Comment #16
skilip CreditAttribution: skilip commentedI 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.
Comment #17
olrandir CreditAttribution: olrandir commentedIs there any chance this is related to this issue I submitted for D7?
http://drupal.org/node/1147988
Any help appreciated, thanks in advance!
Comment #18
bryancasler CreditAttribution: bryancasler commentedsubscribe
Comment #19
Berdir@olrandir: Possible, not sure. Try the attached patch and report back.
Comment #20
olrandir CreditAttribution: olrandir commentedHah, 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!
Comment #21
David D CreditAttribution: David D commentedI've finally tested this patch (also manually applied), and it seems to be working for me as well. Thank you!
Comment #22
Ericmaster CreditAttribution: Ericmaster commentedsubscribe
Comment #23
olrandir CreditAttribution: olrandir commentedI'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 removingthe 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
Comment #24
olrandir CreditAttribution: olrandir commentedIs 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!
Comment #25
BerdirAh, 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.
Comment #26
rfaysubscribe.
Comment #27
BerdirOk, 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:
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.
Comment #28
olrandir CreditAttribution: olrandir commentedFirst 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.
Comment #29
BerdirAh, 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.
Comment #30
BerdirAnd now with patch.
Comment #31
olrandir CreditAttribution: olrandir commentedWorks excellently for me! Thanks for the work!
Comment #32
miro_dietikerAmazing 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!
Comment #33
marktheshark CreditAttribution: marktheshark commentedLosing images (as fields, files seem to be uploaded) here as well, on Drupal 7.2.
Will test fix and report back.
Comment #34
miro_dietikerTested 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.
Comment #35
Tobion CreditAttribution: Tobion commentedHere a duplicate bug report with some extra information: http://drupal.org/node/682038
Comment #36
WilliamB CreditAttribution: WilliamB commentedSo is there a way to get a 7.2 version fixed?
Comment #37
sunmmm, 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).
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?
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.
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?
Bogus newline.
Where is $delta initialized?
The exclusion of programmed form submissions requires a larger explanation in code.
I don't really understand why this chunk is needed - it doesn't look like it's used in the following code.
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.
Comment #38
BerdirThanks 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:
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:
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.
Comment #39
xjmTagging issues not yet using summary template.
Comment #40
bryancasler CreditAttribution: bryancasler commentedxjm, can you link to the summary template. I'm having a hard time finding it.
Comment #41
aspilicious CreditAttribution: aspilicious commentedClick on the edit button on top of the first post and than there will be some info with a link to it.
Comment #42
nemo_Anhoa CreditAttribution: nemo_Anhoa commentedI 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.
Comment #43
xjm#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!
Comment #44
xjm#38: 1059268.patch queued for re-testing.
Comment #45
Pavlos-1 CreditAttribution: Pavlos-1 commented+1
Comment #46
dddave CreditAttribution: dddave commentedMarked #1249940: Uploading files to multiple file fields in node add page will result in unexpected form submit as a dupe. Correctly so?
Comment #47
mariusz.slonina CreditAttribution: mariusz.slonina commentedPatch #38 saved my project today, tested with D7.7, many many thanks!
Comment #48
bryancasler CreditAttribution: bryancasler commentedxjm, can you re-re-submit the patch?
Comment #49
xjm@animelion: You can do this by clicking the Re-test link next to the patch.
Comment #50
xjm#38: 1059268.patch queued for re-testing.
Comment #51
bryancasler CreditAttribution: bryancasler commentedFML, thanks XJM :)
Comment #52
jgalletta CreditAttribution: jgalletta commentedSame issue here on drupal 7.7
Is an "official" backport to Drupal 7 expected?
Comment #53
droplet CreditAttribution: droplet commentedComment #54
dddave CreditAttribution: dddave commented@those who experience this problem
Please test the patch in #38 and report here if it works or not. We need feedback.
Comment #55
droplet CreditAttribution: droplet commentedTested, 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
Comment #56
nemo_Anhoa CreditAttribution: nemo_Anhoa commentedI 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 ?
Comment #57
xjm#56: This patch has not been committed yet, so I would re-apply it to 7.8,
Comment #58
catch#1276444: Filefield fails handling uploading multiple files was duplicate.
Comment #59
catch#38: 1059268.patch queued for re-testing.
Comment #60
BerdirI'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 :)
Comment #61
catchI read through the patch and it looks sane to me, and comes with plenty of tests, so I think we're in good shape.
Comment #62
catch#38: 1059268.patch queued for re-testing.
Comment #63
webchickEek. What a crappy bug. Thanks for the fix, and for the tests.
Committed and pushed to 8.x and 7.x. Thanks!
Comment #64
miro_dietikerYeppiiiee!
I'm very happy to see this fixed now. Thank you all a lot.
Comment #65
David D CreditAttribution: David D commentedWoo hoo. It's committed now? Yay and thank you!
Comment #67
Tobion CreditAttribution: Tobion commentedI 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.
Comment #68
xjm#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.
Comment #69
catchYes 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.
Comment #70
marktheshark CreditAttribution: marktheshark commentedWith 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?
Comment #71
xjm#70: You should look for an open issue describing that problem , or create a new one.
Comment #72
Tobion CreditAttribution: Tobion commentedBtw, 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.
Comment #73
jacobpov CreditAttribution: jacobpov commentedI can confirm that I have the same issue as described in #70.
Comment #74
xjm#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. :)
Comment #75
GiorgosKplease 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
Comment #75.0
GiorgosKAdded summary description according to the template