[Filter] Add resolution filter loader

[robfrawley]

Q	A
Branch?	1.0
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#123, #938
License	MIT
Doc PR

Many questions have been asked about "retina support" including #123 and #938. While I do not believe this bundle needs to enable any retina-specific functionality, as it already allows users to define any number of "filter sets" to generate any number of variations required, we should expose a filter to properly handle changing the image resolution, or "resampling".

This PR adds a ResampleFilterLoader that allows for creating different "filter sets" with different resulting image resolution (not to be confused with dimensions). Included is the ability to set x and y resolution integers, a corresponding unit (one of "inch" or "centimeter"), and an optional resampling filter (one of \Imagine\Image\ImageInterface::FILTER_%s).

This is a first-pass at adding resampling support, and as such I'm looking for comments about the manner in which this is implemented, specifically ResampleFilterLoader.php:45. The imagine-library package this bundle leverages for image transformations doesn't offer any means for resampling outside of saving out the file and reading it back in, which isn't the most straightforward manner to handle this operation.

Any thought on this implementation or ideas for an alternate?

This PR also introduces the following items intrinsically need for the filter loader implemented:

• \Liip\ImagineBundle\Exception\Imagine\Filter\LoadFilterException
  Added for exceptions thrown from within filters loader implementations
• Utility/OptionsResolver/OptionsResolver:setDefined()
  Added to allow for setting optional options that should not have a corresponding default value

Tests for all added functionality have now been implemented, as well. Additions to the documentation are now yet implemented.

[robfrawley]

Looks like Packagist endpoints are having some trouble right now (or another similar issue), resulting in Composer failing to build anything in our Travis tests. I've gone ahead and made @Seldaek aware.

The Travis issue mentioned above has been closed and fixed in composer/composer#6342 by @Seldaek.

[Koc]

do we need leading backslashes here?

[robfrawley]

I added them as a very "micro-optimization" as it does remove some opcodes (otherwise it first searches in the defined namespace, followed by the global namespace (right?), the latter of which we want to force for the first two formats, but now that you bring this up, we could likely change:

array('\Imagine\Image\ImageInterface::FILTER_%s', '\Imagine\Image\ImageInterface::%s', '\%s', '%s')

to

array('\Imagine\Image\ImageInterface::FILTER_%s', '\Imagine\Image\ImageInterface::%s', '%s')

Of course, we don't need them anywhere, but I think it adds clarity and acts as a small speedup.

[robfrawley]

@antoligy @cedricziel Pending any objections, I'm going to merge this for inclusion in the 0.9.0 release.

[robfrawley]

Last call for comments @antoligy @cedricziel

[alexwilson]

Seems like the obvious points have already been made, I'm very happy with this to be merged.

Is it possible to process the file with a buffered stream versus buffering it into a file? (I'm actually unfamiliar with the underlying IMagick operation so it may be a limitation there).

Also, as a more general point, do we have a given convention for when operations are IMagick only, because if so I think we can possibly start adding filters a little more quickly (I've got a few between projects relying on IMagick only features, including another attempt at DPR adjustment).

[robfrawley]

@antoligy Thanks for the quick review. Here is a verbose response. ;-)

Is it possible to process the file with a buffered stream versus buffering it into a file? (I'm actually unfamiliar with the underlying IMagick operation so it may be a limitation there).

The resampling functionality exposed by the imagine-library package is not well documented, but from what I've been able to gather by looking at the brief explanation in their docs, as well as the source code itself, we must explicitly save the file for the resolution to be applied. I can't find any way around this.

Also, as a more general point, do we have a given convention for when operations are IMagick only, because if so I think we can possibly start adding filters a little more quickly (I've got a few between projects relying on IMagick only features, including another attempt at DPR adjustment).

The way I implemented the functional resample test case was confusing. It's not the filter implementation that is imagick-only, but only the test itself that required imagick, as the Imagine\Imagick\Imagine class was used to read the resulting image's resolution for the test's assertion.

I've gone ahead and abstracted the test case a bit to support both imagick and gmagick. It isn't currently possible for the test to support gd as its imageresolution() function wasn't added until PHP 7.2.

As to your question about driver-specific filters, I do not have any particular issue with allowing such implementation, so long as the following conditions are met:

• [Requirement] The filter documentation is obvious and explicit about driver support.
• [Requirement] An instructive and detailed exception is thrown when an unsupported filter is used.
• [Recommendation] Pre-processing of the configuration to throw an immediate exception if unsupported filters are configured. This exception would be thrown regardless of whether the filter is ever actually called during the request. This would avoid surprises for the user by ensuring their configured filters are valid for their environment (for example, we don't want the exception to only crop up three months later when the user removed their cached images; we want it to be thrown the moment it enters their config).
• [Recommendation] Coinciding with the above recommendation, we could add a new interface for filters that are driver-aware (DriverDependantFilterInterface or something) that adds a new method to return an array of supported drivers, like supportedDrivers(): string[] or something similar.

---

The latest commit (22d874c) enables multiple driver support for the resample filter test (as explained above) and adds some additional file types and resolution sizes to the resample test via a new data provider.

[gnutix]

@robfrawley I can't seem to find this code in the current version of this bundle, and yet I can't find any commit / remarks about it's deprecation or removal. Any idea what happened ?