Use Autorotate Filter from Imagine library

[franmomu]

Q	A
Branch?	2.0
Bug fix?	no
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets
License	MIT
Doc PR

When I was taking a look at AutoRotateFilterLoader, I've realised that this was also implemented in the Imagine library. So this commit removes the implementation of this bundle to rely on the library one. I ended up removing the test because it's just a call, like StripFilterLoader.

[dbu]

good catch, i am all in favor of this.

i could see keeping a single test case as a kind of "integration test" to notice if imagine does a BC break or something like that.

[franmomu]

Also there are some issues related with automatic rotation that are not really solved, but apparently could be about having exif_read_data enabled on the server.

#737 #532 #741 #954

and I guess that after this, if there should throw an exception or not, should be part of Imagine instead of the bundle.

[franmomu]

good catch, i am all in favor of this.

i could see keeping a single test case as a kind of "integration test" to notice if imagine does a BC break or something like that.

I was thinking what to do... what about just this:

$this->assertTrue(class_exists(Autorotate::class));

Because at first I thought about checking a call to metadata, but I don't like checking the implementation of the library.

[dbu]

$this->assertTrue(class_exists(Autorotate::class));

i would like us to instantiate the autorotate and call filter on it, to see things are wired together properly. while i agree we get into the grey area of testing the upstream library, i think testing the integration is useful, and if the upstream library is misbehaving, we do care as our users will possibly complain here rather than figure out that its a library problem.

[michellesanver]

Yay! Thanks for this MR! So much less code, it's the best kind.

[franmomu]

Yay! Thanks for this MR! So much less code, it's the best kind.

It feels always good to remove some code 😬

[robfrawley]

The upstream implementation only checks for the ifd0.Orientation EXIF tag, whereas our prior implementation also checked for the exif.Orientation tag. The inclusion of the latter check was originally merged via #476 without any specific comments about its use, so I have absolutely no idea as to whether the check for the exif.Orientation tag resolved some previously reported issue(s) that are still relevant, is completely unnecessary and never served any useful purpose, or only applies to an insignificant number of EOF/legacy/non-compliant EXIF metadata writers and can safely be ignored moving forward.

To be completely clear, I am not recommending reverting this PR; we should absolutely be using the upstream implementation whenever possible (only creating our own duplicated logic in the face of severe upstream deficiencies with critical user-facing consequences). I only point this out to document the change (such that it can be properly searched for in case it does cause any issues) and to bring it to the maintainers' attention so that if the exif.Orientation tag (or related autorotation issues) show up in new issues we can head on over to github.com/avalanche123/Imagine and submit an upstream PR to bring back the secondary check there.

Otherwise, as to the entirety of this PR, removed code is always great; noone ever complained about less code to maintain! :-)

Great PR @franmomu!

[franmomu]

Thank you @robfrawley, thanks for pointing out about the exif.Orientation, good catch.

[michellesanver]

Thanks @robfrawley!