When rotating images using the ImageMagick toolkit, the changes aren't being saved to the original file. Processing does occur (I can see CPU usage and it takes a couple seconds). I believe the problem comes down to a misunderstanding of the way file_move() and file_copy() work. According to the docs, both functions take a source path as the first parameter and a destination directory as the second parameter. The source parameter is then updated to point to the new path. imageapi seemed to be written as if the second parameter was a destination filename. This caused the files not to be copied around correctly and the manipulated file didn't replace the source file. The temporary files were also left hanging around.

I noticed that the GD toolkit does not use the file_move and file_copy operations. It would help to know why ImageMagick needs to use temporary files when GD doesn't.

Note: This issue was originally filed as part of issue #263059: Fix rotation functions.

CommentFileSizeAuthor
#10 imageapi_im_batch.patch1.17 KBdrewish
#3 imageapi_im_264006.patch1.21 KBdrewish

Comments

drewish’s picture

drewish commented

gd uses doesn't use temp files because it does it in memory... was there a patch for this?

drewish’s picture

drewish commented

oh, i guess i posted this to the wrong issue...

drewish’s picture

drewish commented
Version: 5.x-1.1 » 6.x-1.x-dev
Component: Code » ImageAPI Imagick
StatusFileSize
new imageapi_im_264006.patch1.21 KB

okay here's the relevant part of the patch from the other issue.

junyor’s picture

junyor commented
Title: Changings aren't saved when rotating images using ImageMagick » Changes aren't saved when rotating images using ImageMagick
Version: 6.x-1.x-dev » 5.x-1.1
Status: Active » Needs work

This is an issue in the 5.x release and I'd like to see it fixed there. About this patch, dopry said:

Sorry, but you should keep using tempnam() otherwise two manipulations for the same file will end up with the same name in file_directory_temp(). This is likely to happen if there are two imagecache presets for the same image on one page... I often have a thumb and full size image on the same page.

instead of explode/implode you can use dirname/basename() on the file path.
Is manipulating the path really necessary or will just adding the FILE_EXISTS_REPLACE work for the file_move?

So, I need to do some more testing to make sure duplicate files aren't used/overwritten when multiple manipulations are done.

junyor’s picture

junyor commented
Status: Needs work » Needs review

Ahh, looked at your patch again and I see that you aren't doing the temp file stuff here anymore. Cool. I'll test your patch when I get a chance.

drewish’s picture

drewish commented
Version: 5.x-1.1 » 6.x-1.x-dev

cool, love to hear your feedback.

typically commits are done to HEAD and then backported to ensure that when you upgrade you don't run into old bugs.

junyor’s picture

junyor commented

Gotcha.

dopry’s picture

dopry commented
Version: 6.x-1.x-dev » 5.x-1.1
Status: Needs review » Active

whee, committed to DRUPAL-5 and HEAD.

dopry’s picture

dopry commented
Status: Active » Fixed

oh yeah fixed.

drewish’s picture

drewish commented
Status: Fixed » Reviewed & tested by the community
StatusFileSize
new imageapi_im_batch.patch1.17 KB

actually it didn't get committed to DRUPAL-5, here's a re-roll.

drewish’s picture

drewish commented
Status: Reviewed & tested by the community » Fixed

went ahead and committed this to DRUPAL-5 since you thought you already had.

Anonymous’s picture

Anonymous (not verified) commented
Status: Fixed » Closed (fixed)

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