Gregwar / Image

A PHP library to handle images
MIT License
1k stars 190 forks source link

Only process fixOrientation if the image is jpg/tiff #109

Closed flaviocopes closed 8 years ago

flaviocopes commented 8 years ago

Other image formats do not support exif and running fixOrientation() on a PNG image generates an error to the image.

soullivaneuh commented 8 years ago

on a PNG image generates an error to the image.

I'm surprised PNG image does not support this. What is the error?

flaviocopes commented 8 years ago

Sample image:

thumb-zip

Sample code:

Image::open('/PATH/6f2a945e-a96f-11e5-977e-facec0c54287.png')
     ->fixOrientation()
     ->save('/PATH/out.jpg');

try with the current Image code, and with my PR. You'll see that without the PR, the image is never saved as exif_imagetype() fails without generating any error, and $this is never returned, so save is not executed on the image and fails too.

I'm against raising an exception because in my use case I get a list of image manipulation actions in a row, and then execute them all regardless of the file type (I don't know it yet when adding the actions), so returning $this and continuing execution for me is essential.

hirbod commented 8 years ago

I'm also against raising an exception. The PR is legit and should be merged!

soullivaneuh commented 8 years ago

I'm against raising an exception because in my use case I get a list of image manipulation actions in a row, and then execute them all regardless of the file type (I don't know it yet when adding the actions), so returning $this and continuing execution for me is essential.

This is a receivable argument. But I'm not sure of the best method between silent the method on the library or catch the concerned exception on the project code.

@Gregwar what do you think?

Gregwar commented 8 years ago

Actually, I don't think this should be an exception since this is not really an error, I mean the fact that calling fixOrientation on images that are not concerned just doesn't do anything (moreover IMO we can't assume the image will be of a certain type in the processing chain, it should be agnostic)

hirbod commented 8 years ago

What about a paramater? $abort_on_error with defaul value set to false?

rhukster commented 8 years ago

Any movement on this PR? Would be great to see this merged. Thanks!

MaZderMind commented 8 years ago

This Issue just hit me down the road while building a webpage in @getgrav. In my oppininion calling a fix* method on something that doesn't need fixing (because its a png without orientation information) shouldn't fail.

Gregwar commented 8 years ago

Thanks for contributing

Gregwar commented 8 years ago

(And sorry for the delays)