Thumbnail corrupted when editing an existing image node

seriously, at least look through the first page of issues before opening up a new one:
http://drupal.org/node/99395
http://drupal.org/node/79066

This is not a duplicate of my post at http://drupal.org/node/99395. This is a different phenomena with different causes. Plus, my original post was for 4.7, not 5.1. Nor does it appear to be a duplicate of http://drupal.org/node/79066 in my judgment.

I am now seeing the behavior described above on my 5.1 and it is distinct from the behavior I saw with 4.7. Setting to active.

When an image gets edited, the _image_remove() function gets called somewhere along the way. I verified this with a simple print statement that outputs to the screen. That function contains the following code which deletes all derivatives from the database:

db_query("DELETE FROM {files} WHERE nid=%d AND filename!='_original'", $node->nid);

Then, when the image_update() function gets called later on, it cannot find the thumbnail or preview images and so it chokes and throws these errors.

Can the person maintaining this module please take a look? The module isn't very well documented and it's really hard to follow the function calls for this module. It doesn't make much sense to me that a function called _image_remove would get called. Seems to me that the image.module requires a hard look.

I continue to try to make sense out of this code. From what I can tell, after the _image_remove() function is called, the _image_build_derivatives() function is supposed to take care of regenerating the resized images.

Looking at the _image_build_derivatives() function and in my file/images/temp directory, I can see the derivative images are, in fact, getting generated.

However, it appears that the _image_build_derivatives() function is failing to copy the new images into the /file/images directory. Nor does the function take care inserting the images into the files table.

I think I've found as simple fix. Not extensively tested but seems to fix the problem at hand.

#5 solved the problem for me, thank you.

Tormi

I was having the same problem, and was doing some diagnostics tonight and came to the same conclusion as Steve.

By liberally adding drupal_set_message to the module I tried to track what image was up to. The problem seems to be that _image_remove is called as part of the update, and that deletes the derivative images. If _image_build_derivatives is called with $temp = TRUE (as per the original code) then _image_insert is never trigger to rebuild the derivatives.

The patch seems to be working for me...

I just 'lost' derivatives of most of my images ... this must be fixed asap ... the patch in #5 seems to solve the problem though. +1

But is there any way to batch recreate derivatives of all image nodes? Or how do I get them back at all? Simply resaving the node doesnt work. And reupload every image one by one will take hours ... :(

by submitting your settings admin/settings/image again, all will be rebuild automatically....

patch seems ok to me, but the line _image_remove($node); could be removed ass well. _image_build_derivatives will do that also when called with false

got same problem, fixed it with the path, thx!

The patch solves the problem and editing an image does no longer remove my derivatives. Nice. But I still cant get them back. The problem is that thumbnails, etc. are created, but they are not added to the {files} table, what means they are not available actually. Seems to be a problem in the image_update() function. Is anybody facing the same problem? I simple removed the if() condition to verify and now the filepathes are added, but _original appears twice. However shouldnt be too difficult to fix that as well ...

The problem is different IMO. We cant simply set $temp to false as supposed by the patch because function image_form_image_alter() is also called e.g. on previews. What means if an image node is previewed, but never submitted the image is still added to the image folder and not to the temp folder only as expected. Its OK to generate derivatives in temp since they are moved (or actually copied) to the image folder later.

The function image_form_image_alter() is registered as #validate handler. To obtain a node object the line $node = (object)$form_values; casts the $form_values and passes them to _image_build_derivatives(). In _image_build_derivatives() the derivatives are created correctly (in the temp subfolder) and also added to the $node->images array. The expected behaviour is that the images array is saved in function image_update(), but it seems that after the (object) cast the values are not passed by reference correctly, what leads to an image array in function image_update() that does not contain the pathes to the images created in _image_build_derivatives() but the default values (= _original) only. Thats the reason why derivatives are lost on updates as far as I can see. The derivaties are deleted immediatly after they are created ... (The easiest way to verify my description is to print out the $node->images array in image_update() and then exit().)

I am not sure the #validate handler is the right place to derive thumbnails etc. from the original image ... also image previews dont work correctly (they show the original image not the derived one for new image nodes). All the behaviour was observed using PHP4, not sure how PHP5 handles the references in that special cases. This must be verified.

The easiest way to solve the problem is to add a call to _image_build_derivatives() in image_update(), but I guess there should/must be a cleaner way to implement this (esp. to avoid derivatives being created multiple times when an image node is updated/changed).

I am not going to work on this. The code is pure spaghetti as far as I'm concerned. The maintainer should straighten it out. I don't have the time to devote to unraveling it.

Here is a bad patch to fix this. The actual copy of derivatives (in temp) is created in _image_insert(), so it should be sufficient to call with $temp = FALSE again and reuse the created derivatives here. The patch is untested atm ;) Sorry! And I really hope Steve will create a better one ...

I have tested this patch, creating and editing existing images, and it seems to work. What type of tests should be performed to be sure this patch can be committed?

Damn, I may be getting this because I was on the wrong branch. On my DRUPAL-5 setup, Using todays TRUNK/HEAD - which for CVS reasons I still don't grok is not DRUPAL-5 branch - I can create, preview and save. Editing and saving still resizes it back to fullsize (discarding the derivatives).

So I re-checkout the correct BRANCH (doh!)

 cvs -d :pserver:anonymous@cvs.drupal.org:/cvs/drupal-contrib co -d image -r DRUPAL-5 contributions/modules/image

I Create, Preview, Save, OK.
Edit, Preview, Submit [The selected file "" could not be copied] and it's buggered again.

Right, so do the patch.
/sites/default/modules/image$ patch -p0 < image_deriv.patch
Create, Preview, Submit. OK.
Edit, Submit. OK.
Edit, change the image file, submit, OK. (I thought I had trouble with that, my preview version went missing when changing sizes, but no)

So yes, the patch 'patches' the problem. For me, for now.
As profix898 says, it's a dirty way of working around a bug. I think it doesn't conceptually 'fix' whatever is actually wrong in the internals of the code, it just re-does it.

I do now realize that the original code may have been clearing the other derivatives for a reason - changing the size of the image from a greater-than-screen-size to a less-than-preview-size renders the 'preview' redundant so it's better to just delete em all and start again. Still, they were NOT getting rebuilt again - probably due to how the validate (or preview or something) phase no longer works on a handle of the node object, but a copy of it. This was a recent-ish FormAPI or nodeapi() core change.

This patch, as I'm sure profix898 will agree, is an interim branch patch, but although it 'works' probably should not be committed. The module needs work, and I'm not sure where to begin without rewriting the bugger.

For convenience however, for the CVS-and-PHP-impaired, I'll attach the patched replacement for now. Please find enclosed the complete image.module file [1.209.2.11] with patch #14 applied. :-)
(take the .txt off the end)

dman, HEAD and DRUPAL-5 are separate branches but because the same patches have been applied to both they're identical. There should be no difference in behavior between the two.

I've been exploring how the module does its stuff, and I think the problem does lie in image_form_image_alter().

It re-generates the thumbnails, but puts them in /images/temp. However, the amended paths aren't sent back to $node->images which remembers the original paths only. This seems to be the cause of the subsequent copy failures.

Rather than using $temp = false I've patched the code by running form_set_value in image_form_image_alter() for each of the sizes after the derivatives are generated, and updating the paths. This then reflects back in to the node, and everything seems ok.

This seems to get round the issue that using $temp = false puts thumbnails in the /images directory where they never get cleaned up.

I've attached a patch file (but this is the first time I ever created a patch file so apologies if I did it wrong!)

drewish: Thanks. one day I'll truly understand why they can't be one and the same thing. I'm trying to manage branches on my own stuff, and I just can't get what good it is having a TRUNK at all is once I've done a 5.x branch.

Stuart: Your fix looks a lot better. It fixes the problem a lot closer to the source. form_set_value() is the right way to do this, if this has to be done.
I'm not convinced that xxx_form_alter()[#validate] is structurally the right place to be doing real edit actions like image_build_derivatives() does. Among other things, this is why the code was so tricky to understand.

There may be reasons why this is not in image_form_submit() but I'm not game to mess with the code.

possibly

function image_form_alter($form_id, &$form) {
  if ($form_id == 'image_node_form') {
    $form['#submit']['image_form_image_alter'] = array();
  }
}

would be more correct.

... on investigation I see this would miss the ability to preview thumbnails before saving.
OK, lets go with this then.

+1 workable solution.
Plus the patch works.
Plus it's faster than patch #14

Anyone want to test as well? Try modifying the image after saving with new source images that are much bigger and smaller than the first, seeing that the derivatives come and go as appropriate. Looks like the images/temp/{files} are still being left behind. This is probably a seperate issue.

mmm. following up to myself.
I propose the file_copy() in _image_insert() - which shifts the derivatives from images/temp to images/ should become file_move(). On fail (like for bad permissions) the temp file is left where it was, so functionality should be safe.

Attached patch (against current DRUPAL-5) incorporates Stuarts changes above, and adds this tempfile cleanup.

@dman: I agree completely with all your comments and I also agree that form_set_value is much closer (although validate is still not the right place to derive images IMO). Not sure about the temp folder patch. The temp files (older than x hours) are cleaned up on the next cron run. Not sure why it is implemented this way! It makes sense to simply move from temp to images, but I may miss something here, as well.

dman, tried out the patch and it does prevent errors but it doesn't seem to solve it completely. editing an image still looses the thumbnail (though it seems like it's displaying the thumbnail with the wrong dimensions).

i'm going to advocate that we come up with a bit more radical patch that un-spaghetti-izes the code. personally i'm of the opinion that _image_build_derivatives() shouldn't be calling _image_insert() or _image_remove(). the caller of _image_build_derivatives() should take care of that. i think it'll help clean up the code.

having the same problem and subscribing to the thread to await for the best solution.

subscribing

As I am not relying on images yet I'll turn the module off until there's an agreed-good fix.

subscribing

#21 seems to solve the problem for me.

i confirm 21 worked for me also

Can you please commit the patch to head. I can then be a tester . . : - )

subscribing

Okay here's a patch that starts to un-spaghetti-ize the image module. It:

• gets rid of image_form_image_alter() and moves that to an hook_submit() implementation
• renames _image_remove() to _image_remove_derivatives() which is what it actually does
• cleans up the flow of _image_build_derivatives(), which no longer inserts or removes rows from the database

Please give this a try and let me know if you run into any errors. Also, update to the latest version before trying to apply this. I just committed some other fixes.

some more small changes, and i converted it to unix line endings

Patch #34 works nicely for me, and sounds like it makes sense, although I haven't deeply reviewed the code.

Thanks dman. If everyone else who's subscribed to this thread could apply the patch and test it I'd be very grateful. I'd like to see this get fixed.

#34 patch worked for me too.

#34 seems to be working for me as well. Resubmitting the admin/settings/image page does NOT seem to rebuild the derivatives, though I suppose that is a different issue.

#34 seems OK for me too so far.

I still seem to get the error messages, even with images added after the patch, however the thumbnails seem to remain intact.

Hi all. Sorry in advance cause this is my first post here. Great news that something is going on with this useful module. I have run into some trouble with it lately so I tried to find some patches and surprisingly suceeded. But. I have some problems with patching the 5.x-1.x-dev from 2007-Mar-15.

This is my first time patching anything so a little question. Which patches am I supposed to apply? Applying directly patch form #34 doesnt work. I also tried to patch all patches that i found along this thread but faild as well. Can anyone give me a hand pls?
Thx and keep up.

fluke777, make sure you've got the latest version of the module before you try to apply the patch. you'll only want to test out one patch at a time. please try #34. see http://drupal.org/node/60108 for more info on applying patches.

Thx for response drewish, I finally succeeded :-). Omitting -p0 did the trick. Seems that this patch is working fine. At least editing image no more breaks thumbnail. Thanks guys for your hard work.

A couple of nits

• there is no need for the image_image_alter function at the end of the patch. I assume this was there for testing. It should be removed before this patch gets committed.
• 'Unable to create scalled %label image' -- should be scaled

Otherwise, I think the patch looks pretty good. It works for me.

I made these two minor changes and reposted the resulting patch.

thanks for your patience everyone. i've gone ahead and committed vhmauery's patch from #44. there's more cleanup that can be done on this but for now i'd like people to be able to download a working module.

I'll apply the latest version tonight. Thanks to drewish and all the others that have worked on this.

banglogic, i'm assuming you mean to say download the latest version... since the patch has been committed ;)

Yes, sorry, "download the latest version" and install it on my site is what I mean. In fact, I just installed this updated version on two sites. I can confirm the issue is resolved. Ahhh... Thanks to vhmauery for getting a working patch written.

Image is, by far, the most important module for family site (http://diniandken.com). I know you've put a lot of work into getting it straightened out. Many thanks drewish.
-k²

Hi, I just downloaded 5.x of the module again (assuming that would include the fix) and replaced the existing module with the new code and then performed the database update. Unfortunately it doesn't fix the problem for me, it seems, the problem is still there.
When creating a new image-node, I get
The selected file could not be copied, because no file by that name exists. Please check that you supplied the correct filename.
immediately after the upload. The image displays ok in sizes thumbnail and preview, but orig is not accessible afterwards.
When attempting to edit the node then, I see only the thumbnail and if I just press "submit" I get

* The selected file could not be copied, because no file by that name exists. Please check that you supplied the correct filename.
* The selected file could not be copied, because no file by that name exists. Please check that you supplied the correct filename.

So to me it seems I still have the same problem? Or do I need to update the whole Drupal-Installation? No, the fix is within the module-code, isn't it? Hope I am not acting stupid here, but I put this to "active", is that correct? Hope I don't mess-up your way of working

Tim_O, i think you've run into a bug with the image file paths. i don't think it's related to this so i'd appreciate it if you could open a new issue and include details of your system's files settings (both core and for the image module).

Hello,

I'm trying to apply the last image.module_124474_0.patch patch, but it seems not to apply fine on last image 5.x-1.x-dev module version from March 24, 2007 - 01:10.

I try to apply the patch copying it on the sites/all/modules/image directory and typing:

%patch < image.module_124474_0.patch

but the command line returns:

patching file image.module
Reversed (or previously applied) patch detected!   Assume -R? [n]

and then:

Apply anyway? [n]

if you apply anyway it returns errors.
I verified and I'm trying to apply it on the fresh new downloaded version of image.module, which still does the file error when editing an image (so the patch is not still applied from drupal manteiners).

I'm doing something wrong? Is this patch for another subversion of the image.module?

I really need to solve it asap!!

thx!

forget my last entry.... the latest version of image.module has solved this error.

That's because the version you downloaded had already been patched and committed.

This error seemed to have reappeared as soon as I downloaded the new module once again. I tried both versions, dev and official release. I installed and then took off the image scale module, could that have affected something?

I tried deleting and overwriting the image module but nothing seems to work, everything worked fine before I tried to install the image scaling module and downloaded the latest image module...

Can somebody help?

Thanks!!

Charles

jeforma, make sure it's not an issue with the image scaling module.

Problem fixed. Sorry for the trouble.

I had not set back to the GD2 Library before uninstalling the module.

Thanks! :)

Charles