Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Also see this thread: http://drupal.org/node/632828
Apparently, this has something to do with the image_attach functions not working properly.
Comment | File | Size | Author |
---|---|---|---|
#7 | 659182.image_attach.submit-handler-clean-form-zero-key.patch | 1.85 KB | joachim |
#3 | _659182.image_attach.unset_iid0.patch.txt | 998 bytes | jonathan1055 |
Comments
Comment #1
jonathan1055 CreditAttribution: jonathan1055 commentedYes I have a patch for this fix to image_attach, first noticed in #632828: Disconnects attached images when node is published. Will post it tomorrow, not got time tonight.
Jonathan
Comment #2
qwerdox CreditAttribution: qwerdox commentedThanks!!!
Comment #3
jonathan1055 CreditAttribution: jonathan1055 commentedThe problem occurred when Image changed to multiple images. Scheduler calls node_save() to save the node with a new status value, which invokes image_attach_nodeapi() with op='update'. The existing code calls unset($node->iids[0]) to remove the '-none-' selection item from the form. However, during other operations, eg in node_save() the image attach iid array is zero-keyed, so doing unset($node->iids[0]) detaches the first image.
The fix is to test the value of $node->iids[0] and only do the unset if the value is 0, which it will be only when saving the node via the edit form.
The patch was created against a dev version from a couple of weeks ago, so you may get an offset warning when applying it. If you would like me to re-roll the patch let me know and I'll download the latest dev.
Jonathan
Comment #4
jonathan1055 CreditAttribution: jonathan1055 commentedComment #5
joachim CreditAttribution: joachim commentedGood catch! Thanks for reporting this and doing the detective work and the patch!
Just to check I understand this right: the only time we get $node->iids keyed by image node id is when we're coming from a node save form, and the rest of the time that array is keyed in numerical sequence?
If that is the case, then I think it would be cleaner to deal with this little bit of form value cleanup in a submit handler we add to the form, and that way come into hook_nodeapi with clean values in all cases.
Comment #6
qwerdox CreditAttribution: qwerdox commentedRocking! thanks!
Comment #7
joachim CreditAttribution: joachim commentedI can reproduce the bug with the following code in the Execute PHP block:
Then go see the node (you have to load the node after this code has run; you can't run it on the node page itself).
The first image has gone.
Here's a patch implementing the approach outline in #4.
Could either of you test this please, both with the scheduler setup and regular image attach node tasks?
Comment #8
joachim CreditAttribution: joachim commentedBetter title to prevent duplicates; marking as critical.
I'd like to get this committed and a new beta released with the fix. Please test the patch!
Comment #9
jonathan1055 CreditAttribution: jonathan1055 commentedI will test this at the weekend (hopefully Saturday) sorry can't do it before then.
Jonathan
Comment #10
jonathan1055 CreditAttribution: jonathan1055 commentedHi Joachim,
I cannot say for definite that everywhere except node-edit the array is 0-keyed, but each time I have investigated, it has been that way. So I think this is a better way to fix it, and image_attach_nodeapi does not have to do any clean-up.
I have tested this patch with scheduler and with ordinary adding images, selecting -none-, selecting -none- and an image, etc. It works fine, so I would say apply it.
Jonathan
Comment #11
joachim CreditAttribution: joachim commentedCommitted to CVS.
Given this is a data loss bug, I'm going to roll another beta release with this in, probably tomorrow.
Thanks qwerdox for reporting, and jonathan1055 for figuring it out and testing -- this makes my job as maintainer a whole lot easier and I really appreciate it :D