The image rotation functionality in imageapi did not work correctly for either toolkit. This patch fixes all the problems I found, including:

1) ImageMagick complained about the background color passed in. The default value was converted to 0 because it was treated as an integer instead of as a string.
2) ImageMagick did not rotate the image. It seems like this came 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.
3) GD2 does rotations counter-clockwise by default, but imageapi treated rotation as clockwise.

Note: The code to get the destination directory in imageapi_imagemagick_image_close() could probably be better, but I'm not intimately familiar with the PHP string manipulation functions and this approach worked.

CommentFileSizeAuthor
#7 imageapi_263059.patch1.53 KBdrewish
#6 imageapi_263059.patch2.31 KBdrewish
#5 imageapi_im_rotate_263059.patch847 bytesdrewish
#3 contrib-5.imageapi.rotate2.junyor.patch1.34 KBjunyor
contrib-5.imageapi.rotate.junyor.patch2.98 KBjunyor

Comments

dopry’s picture

dopry commented
Status: Needs review » Needs work

@junyor... can we split the rotate specific stuff from the more general imagemagick stuff?

bg color for rotate looks good, rotation reversal for GD looks good.

please avoid spurious changes like changing case in patches... the long GD command names I like CamelCasedBecauseTheyAreSoLong.

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?

junyor’s picture

junyor commented

Why is it that ImageMagick needs a copy of the file while GD2 doesn't? file_copy() and file_move() handle duplicate filenames, so that shouldn't be a problem. Manipulating the path was necessary in order to get the destination directory (rather than the file name).

junyor’s picture

junyor commented
Status: Needs work » Needs review
StatusFileSize
new contrib-5.imageapi.rotate2.junyor.patch1.34 KB

Attached is a patch that just covers the first and third reported issues. I'll file a separate report about the ImageMagick problems.

junyor’s picture

junyor commented

See #264006: Changes aren't saved when rotating images using ImageMagick.

drewish’s picture

drewish commented
StatusFileSize
new imageapi_im_rotate_263059.patch847 bytes

i think this is a better way to handle the imagemagick part

edit: meant to say rotate part.

drewish’s picture

drewish commented
Version: 5.x-1.1 » 6.x-1.x-dev
StatusFileSize
new imageapi_263059.patch2.31 KB

i did some more testing with imagemagick and ran into the problem you were describing.

the real problems seems to be that file_move/file_copy() expects the path to be either absolute ( /var/www/foo.png ) or relative to the files directory ( imagecache/foo.png ). from an api perspective this is really frustrating because saving via GD doesn't have the same behavior. the temp file was setup before we started batching operations and i think we can drop it now.

this patch is for head and needs to be backported once it's committed.

drewish’s picture

drewish commented
StatusFileSize
new imageapi_263059.patch1.53 KB

split out the imagemagick temp parts and dumped them over on #264006: Changes aren't saved when rotating images using ImageMagick leaving the less controversial parts here.

junyor’s picture

junyor commented

@drewish: Why change the bgcolor stuff only for ImageMagick? It's wrong from when the default is set in imageapi.

dopry’s picture

dopry commented

It's the difference in how GD2 and ImageMagick work. GD2 creates an 'image object' that lives in memory. The Imagick extension does this as well. ImageMagick is a command line utility and needs to use a file to operate on.

drewish’s picture

drewish commented

junyor, I only corrected it for Imagemagick because GD worked fine with the default on my installation. and 0x000000 is valid PHP for specifying an integer.

junyor’s picture

junyor commented

@drewish: GD doesn't use the background color, IIRC. I don't think it's supposed to be an integer, but a string value.

dopry’s picture

dopry commented
Status: Needs review » Patch (to be ported)

This has been applied to 6.x/HEAD.

drewish’s picture

drewish commented
Status: Patch (to be ported) » Needs review

Junyor, are you sure you're not using dopry's fallback rotate code? what do you see that's GD related on admin/logs/status ?

dopry’s picture

dopry commented
Status: Needs review » Fixed

back port committed to DRUPAL-5.

drewish’s picture

drewish commented
Status: Fixed » Patch (to be ported)

whoops, cross posted.

drewish’s picture

drewish commented
Status: Patch (to be ported) » Fixed

lol, cross posted again... well if the bug is with the fallback functions open a new issue.

junyor’s picture

junyor commented

There's nothing GD-related in watchdog.

dopry’s picture

dopry commented

@junyor: not in watchdog on admin/logs/status

junyor’s picture

junyor commented

Whoops! Nothing GD related on the Status report, either.

Anonymous’s picture

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

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