Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 2 commits into from Jun 12, 2016
Merged

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

merged 2 commits into from Jun 12, 2016

Conversation

flaviocopes
Copy link
Contributor

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

Other image formats do not support exif and running fixOrientation() on a PNG image generates an error to the image.
@@ -95,6 +95,10 @@ public function cropResize($width = null, $height = null, $background = 'transpa
*/
public function fixOrientation()
{
if (!in_array(exif_imagetype($this->source->getInfos()), array(IMAGETYPE_JPEG, IMAGETYPE_TIFF_II, IMAGETYPE_TIFF_MM))) {
return $this;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be better to throw an unsupported image type exception IMO.

@Gregwar what do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@soullivaneuh
Copy link
Collaborator

on a PNG image generates an error to the image.

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

@flaviocopes
Copy link
Contributor Author

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
Copy link

hirbod commented Jan 5, 2016

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

@soullivaneuh
Copy link
Collaborator

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
Copy link
Owner

Gregwar commented Jan 6, 2016

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
Copy link

hirbod commented Jan 6, 2016

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

@rhukster
Copy link

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

@MaZderMind
Copy link

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 Gregwar merged commit a89da22 into Gregwar:master Jun 12, 2016
@Gregwar
Copy link
Owner

Gregwar commented Jun 12, 2016

Thanks for contributing

@Gregwar
Copy link
Owner

Gregwar commented Jun 12, 2016

(And sorry for the delays)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants