[Filters] [Config] [DI] Add Filter configuration class as public service #1098

maximgubar · 2018-05-23T09:08:53Z

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

In this PR I propose to introduce a service, which retrieves filter_sets, filters configuration and exposes it via interfaces.

robfrawley · 2018-05-23T17:12:01Z

Overalls, this looks great.

I do have some ideas I've been wanting to implement regarding filter-sets/filter that would be good to bring up now. The number of times people confuse the difference between filter-sets and filters is countless, I submit we should change the terminology of filter-set to "instructions" or something different. Thoughts? Other naming ideas?

The "quality" setting is also going to be deprecated because we now have "png_compression_level" and "jpeg_quality" now instead, so this interface should likely accommodate those new options (this has already been done in the underlying imagine library).

Also, do we need interfaces for classes we're marking as final? It seems like possibly extra, unnecessary code to maintain if we aren't intending it to be an extension point.

robfrawley · 2018-05-23T17:31:50Z

Also, with one of the PRs I'm converting from 1.x to 2.0 I actually created a "configuration" class as well (instead of adding tons of constructor arguments, as done in 1.x it seemed better to pass a config class), so perhaps we should move everything in this PR from Config/ to Config/Filter namespace so we can define other configs for other components under the same Config/ namespace (like the Config/Controller/ControllerConfig.php class from my work) without having tons of unrelated config classes together in the same namespace level.

dbu

good idea with expressing the configuration as objects. i think the basic structure is good.

as discussed before, i am not sure if the interfaces add all that much. though the implementation being final is not an argument against interfaces, actually the opposite. if somebody wants to change something, they have to implement the interface themselves. with the config classes being final, you can't adjust them. so overall i am ok to keep the interfaces.

could it make sense to make the filter classes specific to the type of filter they are? then instead of generic "options" we would have specific getters for width, height, blur factor, whatever the filter is doing... that would make the configuration much more explicit.

i would drop the setters from the interfaces, and also from the implementations and use constructor arguments instead, to make the configuration immutable. if we do that, it also makes sense to merge the factory and builder classes, as setting the information is no longer defined in the interface. imho that simplifies the codebase and makes it more clear.

people confuse the difference between filter-sets and filters is countless, I submit we should change the terminology

in rokka.io we call the filter-set "stack". what would you think of that name? oderwise, maybe "ImageFormat", as in the end it defines the format for one image. "instructions" is a bit weird as its just a different interpretation of what a filter is.

and should we call the thing now called filter a FilterDefinition? its more verbose, but in the context of the classes using these definitions, there are also the "actual" imagine filters which transform the image. this configuration container class only represents the definition for the filter, not the actual filter ;-)

The "quality" setting is also going to be deprecated because we now have "png_compression_level" and "jpeg_quality"

makes sense. is the configuration in the bundle already transformed in that way? do we currently use quality 1-10 for png and 100-1 for jpeg compression?

namespace

that makes sense to me, lets move the filters and stacks there.

dbu · 2018-05-24T06:24:16Z

Config/FilterInterface.php

+
+namespace Liip\ImagineBundle\Config;
+
+interface FilterInterface


i would like to do a phpdoc that explains what a filter is (the definition for one image transformation operation)

@dbu could you provide an example ? cause I'm not sure that I understand this comment.

/** * A filter contains the configuration for an image transformation operation. * * The type of operation is defined by the filter class. The operation parameters are specific to * the filter. */

dbu · 2018-05-24T06:24:45Z

Config/FilterInterface.php

+    /**
+     * @param string $name
+     */
+    public function setName(string $name): void;


imo the interface should not define setters. the whole definition is a read-only thing.

completely agree with you :)

dbu · 2018-05-24T06:26:19Z

Config/FilterSet.php

+    /**
+     * @return int
+     */
+    public function getQuality(): int


this should have a phpdoc explaining what it is. (resp, as rob says the jpg quality and the png compression level values should have that doc, to explain the value range)

dbu · 2018-05-24T06:27:14Z

Config/Filter.php

+    /**
+     * @param string $name
+     */
+    public function setName(string $name): void


i'd rather use constructor arguments than setters

got it, will refactor it.

robfrawley · 2018-05-24T16:23:17Z

"Stacks" isn't a bad name at all. I'm fine with stack, transformation_stack, instructions, instruction_stack, named_transformations, stack_definition, transformation_definition, etc. Not too picky, as long as it isn't filter_sets anymore (and ImageFormat seems like it too would cause confusion: they don't just define the image format, but the operations to perform on the image data, the loading/caching mechanism, etc). Once we settle on a good name here we can deprecate the current bundle configuration entry, provide a new one, and handle normalization for BC until 3.0.
I think FilterDefinition/StackDefinition might be a good way to differentiate between config classes and actual filter implementations (or even FilterConfig/StackConfig or another variant). There are already a bunch of classes and/or interfaces similarly or exactly named the same as others, in different namespaces, of course, but it always causes issues and confusion for newcomers. We should attempt to avoid creating new overlap as much as possible.
I don't think I was clear about my position on "final"/"interfaces" prior: let's try it again. :-) What I was trying to point out is that unless we intend for this to be an extension point, which I don't currently see any real use-cases for (but do correct me if I'm wrong) we should: 1. mark them as final, and 2. not provide interfaces. In other words, we should publish a working version of this change with no extension points, recieved feedback and bug reports, stabalized the API, and wait for feedback that extending these classes is even desirable to our users. If our users don't even want or need it, the only thing providing interfaces at this time accomplishes is locking us to a contract that we'll have to maintain and we'll have to stick to indefinetely. Until 3.0 at the earliest, anyway.

I've already made that mistake with earlier contributions and PRs I've merged from others; since, I've tried to limit interfaces at all costs and expand final usage. It makes things easier, especially early in a feature's lifetime. Anyway, just my two cents. :-)
If you are really stuck on the inclusion of interfaces, perhaps we should introduce an @experimental tag, like Symfony uses, to mark sections of code as pre-release and not production-API stable?
This bundle is currently supporting all quality factors: the deprecated quality option (1-100), the new png_compression_level option (0-9), and the new jpeg_quality option (1-100). At some point we need to throw a deprecation on usage of quality and normalize/map it to the two new options automatically.
After you've gotten a final PR together, do take a look at the coveralls code coverage data to ensure you're properly all testing the new code; it looks like the coverage percentage droped a bit with this PR.

Anyway, thanks for all the hard work @maximgubar. The PR looks solid and I'm excited to get it included.

dbu

nice, i like it! i would love to bring down the number of interfaces and meta infrastructure classes a bit, see some of my comments.

dbu · 2018-05-29T15:18:23Z

Config/Filter/Type/Thumbnail.php

+
+    /**
+     * @param string      $name
+     * @param array       $size


can you specify what kind of array this is? is it a hashmap with width and height?

dbu · 2018-05-29T15:19:34Z

Factory/Config/FilterFactoryInterface.php

+interface FilterFactoryInterface
+{
+    /**
+     * @return string


can we say that the name must match the Filter::getName that this factory handles?

dbu · 2018-05-29T15:20:08Z

Config/FilterFactoryCollection.php

+    private $filterFactories;
+
+    /**
+     * FilterCollection constructor.


i'd remove docblocks that don't add any information over whats in the php code

dbu · 2018-05-29T15:20:49Z

Config/FilterFactoryCollection.php

+     */
+    public function __construct(FilterFactoryInterface ...$filterFactories)
+    {
+        $this->filterFactories = $filterFactories;


imho we should convert this to a hashmap with name => factory so that we don't have to loop over the array in getFilterFactoryByName

dbu · 2018-05-29T15:21:34Z

Config/FilterFactoryCollectionInterface.php

+
+use Liip\ImagineBundle\Factory\Config\FilterFactoryInterface;
+
+interface FilterFactoryCollectionInterface


imho there is only one way how this can be implemented and hence no point in having an interface

dbu · 2018-05-29T15:24:56Z

Resources/config/imagine.xml

+        <service id="Liip\ImagineBundle\Factory\Config\Filter\StripFactory" alias="liip_imagine.factory.config.filter.strip"/>
+
+        <service id="liip_imagine.factory.config.filter.thumbnail" class="Liip\ImagineBundle\Factory\Config\Filter\ThumbnailFactory" />
+        <service id="Liip\ImagineBundle\Factory\Config\Filter\ThumbnailFactory" alias="liip_imagine.factory.config.filter.thumbnail"/>


i don't think that we need an autowiring capable alias for these. they should be private services not useable outside of DI configuration

dbu · 2018-05-29T15:26:42Z

Resources/config/imagine.xml

+            <argument type="service" id="liip_imagine.factory.config.filter.strip"/>
+            <argument type="service" id="liip_imagine.factory.config.filter.thumbnail"/>
+        </service>
+        <service id="Liip\ImagineBundle\Config\FilterFactoryCollection" alias="liip_imagine.config.filter_factory_collection"/>


neither do we need autowiring for this. the only thing that should be available for autowiring should be the actual configuration object.

@dbu, btw, do we need to have SF4 compatibility ?

it would be nice, but i would not mix it into this pull request.

if we have new services that should be autowire-enabled, we can already define the right alias here, but i would not do anything outside the scope of modeling the configuration.

dbu · 2018-05-29T15:30:43Z

Config/FilterSetCollection.php

+     * @param FilterConfiguration       $filterConfiguration
+     * @param FilterSetBuilderInterface $filterSetBuilder
+     */
+    public function __construct(FilterConfiguration $filterConfiguration, FilterSetBuilderInterface $filterSetBuilder)


does it make sense to use the FilterConfiguration object? looks to me like that is a previous attempt at this problem that did not go far enough? it smells like we should deprecate FilterConfiguration and pass the plain array here, or not?

dbu · 2018-05-29T15:35:17Z

Config/FilterSetCollection.php

+
+    /**
+     * @param FilterConfiguration       $filterConfiguration
+     * @param FilterSetBuilderInterface $filterSetBuilder


having factories for so many things, why do we not have a factory for the StackCollection? we could then define the public service for this as a thing that uses the StackCollectionFactory, and not have the value object that is used outside have the factory in it. the FilterSetBuilder could take that role, to not explode the number of infrastructure classes that we use.

for the symfony config, it would look like this: https://symfony.com/doc/current/service_container/factories.html

dbu · 2018-05-29T15:39:03Z

Config/Filter.php

+
+namespace Liip\ImagineBundle\Config;
+
+final class Filter implements FilterInterface


once you implemented all filters, this will become obsolete, right?

Koc · 2018-06-06T10:27:46Z

Config/Filter/Type/Background.php

+    /**
+     * @return string
+     */
+    public function getName(): string


Max, this is wrong. If color is optional - use ?string as return type. Also phpdocs which duplicates actual typehints/return types are useless.

thanks for the hint, will refactor.

Koc · 2018-06-06T10:28:29Z

Config/Filter/Type/Crop.php

+
+    /**
+     * @param string $name
+     * @param array  $start start coordinates {x,y}


Why not provide value object for Point?

good idea, will add it

Koc · 2018-06-06T10:34:10Z

Config/FilterFactoryCollection.php

+    /**
+     * @param FilterFactoryInterface ...$filterFactories
+     */
+    public function __construct(FilterFactoryInterface ...$filterFactories)


Variadic here looks so strange. IMHO it is better use array of FilterFactoryInterface and instanceof/private method to ensure type of each item in array.

got your point, but how usage of array and additional checks of type is better ?
since this object actually plays a role of collection and it is constructed with FilterFactoryInterface(s) why just not declare it directly ?

as you loop over the parameter anyways, you could also do an instanceof check, and have the @param FilterFactoryInterface[] $filterFactories to allow code sniffers to detect errors.

if we ever need to add an (optional) constructor argument, the current form will be a problem as we can't add anything else without a BC break.

why would we ever need to to add something to constructor of this class ?
the purpose of this object is to store FilterFactoryInterfaces only and thats it. it shouldn't contain any logic. tbh, in this case specifically, I don't see advantages of array implementation.

i don't mind much either way.

thinking of it, we should mark all the factory part as @internal to tell people to not use that in their project. the exposed thing is the configuration object model, not the factories to build it. if we do that we can also leave the variadic imho.

yes, because final means you can't extend the class, but @internal means you should not interact with it at all, e.g. instantiate it in your code and so something with it.

I guess within Symfony the pattern used most often is an add() method to build the collection instead of a constructor argument. obviously this has more performance overhead however. I think this is an interesting approach but since variadic in user land is still a new concept there is less experience with it.

but I wonder if it wouldn't be better to use a factory method instead of the constructor. ie. something like:

class FilterFactoryCollection { static public function createFromList(FilterFactoryInterface ...$filterFactories) { $collection = new self; $collection->addList(FilterFactoryInterface ...$filterFactories); } }

this keeps the options open for if we ever do need to add anything to the constructor. while keeping the convenience and more or less the overhead reduction benefits.

this object has only one purpose, to store the FilterFactoryInterfaces (which are clearly its dependencies and should be specified), I dont see any valid case which would lead to altering constructor. Also, I am not sure is it a good practice to do indirect object injections (via setters).

ok lets keep it as proposed. like I said, a good chance of this feeling "weird" is simply the lack of experience with variadic in user land .. and honestly this is not the most "risky" place .. if we do in the end realize we need support additional constructor parameters .. we can then still use a static factory method for that case.

dbu

added a bunch of small review comments. but i like where this is going, i think its coming to a good form.

dbu · 2018-06-06T11:47:03Z

Config/Filter/Type/Background.php

+
+    /**
+     * @param string      $name
+     * @param string|null $color


i am ok with using a string for the $color, but we should explain here what it is. RRGGBB? with optional transparency?

dbu · 2018-06-06T11:47:34Z

Config/Filter/Type/Background.php

+    /**
+     * @param string      $name
+     * @param string|null $color
+     * @param string|null $transparency


we should state the range of this. 0 to 100? or 0 to 1? or something else?

dbu · 2018-06-06T11:48:06Z

Config/Filter/Type/Background.php

+     * @param string      $name
+     * @param string|null $color
+     * @param string|null $transparency
+     * @param string|null $position


is that a list of keywords like TOP|LEFT|BOTTOM|RIGHT, or something else?

dbu · 2018-06-06T11:48:46Z

Config/Filter/Type/Background.php

+     * @param string|null $color
+     * @param string|null $transparency
+     * @param string|null $position
+     * @param array       $size


keys x and y? or do we use a value object for this?

dbu · 2018-06-06T11:49:43Z

Config/Filter/Type/Downscale.php

+    /**
+     * @return float|null
+     */
+    public function getBy()


: ?float instead of the docblock

dbu · 2018-06-06T12:08:10Z

Factory/Config/Filter/BackgroundFactory.php

+     */
+    public function create(array $options): FilterInterface
+    {
+        $color = isset($options['color']) ? $options['color'] : null;


the most condensed for these checks would be $color = $options['color'] ?? null;

is it a typo ?
should it be like this ?
$color = $options['color'] ?: null;

no. that will generate a warning when the index 'color' is not set. ?? is the null coalesce operator introduced in PHP 7 to shorten exactly the construct you used above.

dbu · 2018-06-06T12:09:43Z

Factory/Config/Filter/InterlaceFactory.php

+    public function create(array $options): FilterInterface
+    {
+        $mode = ImageInterface::INTERLACE_LINE;
+        if (!empty($options['mode'])) {


why empty and not isset? i'd go with the pattern $mode = $options['mode'] ?? ImageInterface::INTERLACE_LINE;

dbu · 2018-06-06T12:11:08Z

Factory/Config/Filter/ResizeFactory.php

+    public function create(array $options): FilterInterface
+    {
+        $size = [];
+        if (isset($options['size']) && is_array($options['size'])) {


this is silently ignoring non-array size options, which is a problem imho. we should do the $size = $options['size'] ?? []; . the constructor type declaration on $size will detect if the size option has been set to an invalid value.

dbu · 2018-06-06T12:12:11Z

Factory/Config/StackFactory.php

+     * @param string      $name
+     * @param string|null $dataLoader
+     * @param int|null    $quality
+     * @param array       $filters


FilterInterface[]

dbu · 2018-06-06T12:12:53Z

Factory/Config/StackFactory.php

+     */
+    public function create(string $name, string $dataLoader = null, int $quality = null, array $filters)
+    {
+        return new Stack($name, $dataLoader, $quality, $filters);


also, is there value to use a factory for this over just doing new Stack() in the place where we need the stack? feels to me like the factory is too much abstraction.

using new is breaking DI principle. it makes it hardcoded and non-extendable and not testable.

i don't see the problem. we don't want a mock stack anywhere, its a value object. to me it looks overengineered with this. we do not need to DI what kind of stack is created as its just a value object. but its not a huge deal either.

we already mock it in our unit tests

i don't see the need for mocking the Stack object in a unit test, its straightforward enough imho. yes, its not a perfectly atomic unit test, but good enough as all of the build process is just infrastructure code to build the configuration object.

Koc · 2018-06-06T19:16:42Z

Config/Filter/Type/Background.php

-     * @param string|null $position
+     * @param string|null $color        background color HEX value
+     * @param string|null $transparency possible values 0..100
+     * @param string|null $position     position of the input image on the newly created background image. Valid values: topleft, top, topright, left, center, right, bottomleft, bottom, and bottomright


Maybe we can use value objects/enum here also.

@Koc for all parameters or for specific one ?

i would not use value objects here. if the imagine library would use them, we should use them here too, but as they don't i feel like its a bit too much.

Koc · 2018-06-06T19:25:39Z

Config/Filter/Argument/Size.php

+     */
+    private $height;
+
+    public function __construct(int $width = null, int $height = null)


Does size without width and height has any business value? Same for point. It looks like incomplete/invalid object. If Size is optional for some operations - maybe se can use typehing ?Size in this operations

@Koc there are cases, when one of the dimensions could be null, how would you suggest to implement it if this approach causes doubts ?

Lets add a phpdoc to this method? "To allow keeping aspect ratio, it is allowed to only specify one of width or height. It is however not allowed to specify neither dimension."

fixed static tests

dbu

there are some comments that seem to have slipped through. and some more phpdoc that we can remove because it does not provide extra information.

the only larger bit i don't like is that the filter names are arguments and the constant is in the factories. i would instead put the constant into the filter class and reference it from the factory, and not have a constructor argument $name in the filters, but use the constant in getName

dbu · 2018-06-11T12:01:04Z

Config/Filter/Type/AutoRotate.php

+    private $name;
+
+    /**
+     * @param string $name


lets remove this, its redundant

dbu · 2018-06-11T12:02:06Z

Config/Filter/Type/Interlace.php

+
+    /**
+     * @param string $name
+     * @param string $mode


dbu · 2018-06-11T12:02:38Z

Config/Filter/Type/RelativeResize.php

+     * @param float|null $heighten
+     * @param float|null $widen
+     * @param float|null $increase
+     * @param float|null $scale


please remove this phpdoc as it does not add anything

dbu · 2018-06-11T12:03:02Z

Config/Filter/Type/Strip.php

+    private $name;
+
+    /**
+     * @param string $name


please remove

dbu · 2018-06-11T12:03:22Z

Config/Filter/Type/Thumbnail.php

+     * @param Size        $size
+     * @param string|null $mode
+     * @param bool|null   $allowUpscale
+     * @param string|null $filter


please remove the docblock as it contains no information

dbu · 2018-06-11T12:05:22Z

Config/StackBuilder.php

+     * @param string $filterSetName
+     * @param array  $filterSetData
+     *
+     * @return StackInterface


please remove, this is all visible in the method signature

dbu · 2018-06-11T12:05:33Z

Config/StackCollection.php

+final class StackCollection
+{
+    /**
+     * @var array


dbu · 2018-06-11T12:05:36Z

Config/StackCollection.php

+    /**
+     * @var array
+     */
+    private $filterSets = [];


dbu · 2018-06-11T12:05:54Z

Config/StackCollection.php

+
+    /**
+     * @param StackBuilderInterface $stackBuilder
+     * @param array                 $filtersConfiguration


please remove, its all in the method signature

dbu · 2018-06-11T12:06:54Z

Config/StackInterface.php

+    /**
+     * @return string|null
+     */
+    public function getDataLoader();


as we are on php 7.1 or better, please use the return type declaration getDataLoader(): ?string and remove the phpdoc. same for getQuality

dbu · 2018-06-11T14:28:31Z

Config/Filter/Type/Background.php

+/**
+ * @codeCoverageIgnore
+ */
+final class Background extends FilterAbstract implements FilterInterface


you can remove the implements FilterInterface because thats already given by extending FilterAbstract

dbu · 2018-06-11T14:36:27Z

Config/StackBuilderInterface.php

+     * @param string $filterSetName
+     * @param array  $filterSetData
+     *
+     * @return StackInterface


this does not add anything over the phpdoc, lets remove it

dbu · 2018-06-11T14:37:39Z

Factory/Config/FilterFactoryInterface.php

+interface FilterFactoryInterface
+{
+    /**
+     * Filter name


dbu · 2018-06-11T14:38:01Z

Factory/Config/StackFactoryInterface.php

+     * @param int|null          $quality
+     * @param FilterInterface[] $filters
+     *
+     * @return Stack


could we instead do the return type declaration on the method itself?

dbu

i don't see any further things that i would want to change. i vote for merge (squashing commits). @robfrawley are you okay with that?

- optional return types - typo in Paste filter name

dbu · 2018-06-18T07:38:46Z

@robfrawley i would prefer to have some final feedback from you before we merge this. do you have time to look at this soonish?

lsmith77 · 2018-06-22T07:48:12Z

Binary/Locator/FileSystemInsecureLocator.php

-        if (false === mb_strpos($path, '..'.DIRECTORY_SEPARATOR) &&
-            false === mb_strpos($path, DIRECTORY_SEPARATOR.'..') &&
-            false !== file_exists($absolute = $root.DIRECTORY_SEPARATOR.$path)
+        if (false === mb_strpos($path, '..'.\DIRECTORY_SEPARATOR) &&


why the backslashes here?

lsmith77 · 2018-06-22T07:48:24Z

Binary/Locator/FileSystemLocator.php

@@ -60,7 +60,7 @@ public function locate(string $path): string
     */
    protected function generateAbsolutePath(string $root, string $path): ?string
    {
-        if (false !== $absolute = realpath($root.DIRECTORY_SEPARATOR.$path)) {
+        if (false !== $absolute = realpath($root.\DIRECTORY_SEPARATOR.$path)) {


why the backslashes here?

lsmith77 · 2018-06-22T07:50:26Z

DependencyInjection/Factory/Loader/FileSystemLoaderFactory.php

@@ -127,7 +127,7 @@ private function getBundleResourcePaths(ContainerBuilder $container)
        }

        return array_map(function ($path) {
-            return $path.DIRECTORY_SEPARATOR.'Resources'.DIRECTORY_SEPARATOR.'public';
+            return $path.\DIRECTORY_SEPARATOR.'Resources'.\DIRECTORY_SEPARATOR.'public';


why the backslashes here?

lsmith77 · 2018-06-22T07:51:31Z

Tests/AbstractTest.php

@@ -50,8 +50,8 @@

    protected function setUp()
    {
-        $this->fixturesPath = realpath(__DIR__.DIRECTORY_SEPARATOR.'Fixtures');
-        $this->temporaryPath = sys_get_temp_dir().DIRECTORY_SEPARATOR.'liip_imagine_test';
+        $this->fixturesPath = realpath(__DIR__.\DIRECTORY_SEPARATOR.'Fixtures');


why the backslashes here?

lsmith77 · 2018-06-22T07:57:14Z

Config/FilterFactoryCollection.php

+    /**
+     * @param FilterFactoryInterface ...$filterFactories
+     */
+    public function __construct(FilterFactoryInterface ...$filterFactories)


I guess within Symfony the pattern used most often is an add() method to build the collection instead of a constructor argument. obviously this has more performance overhead however. I think this is an interesting approach but since variadic in user land is still a new concept there is less experience with it.

but I wonder if it wouldn't be better to use a factory method instead of the constructor. ie. something like:

class FilterFactoryCollection { static public function createFromList(FilterFactoryInterface ...$filterFactories) { $collection = new self; $collection->addList(FilterFactoryInterface ...$filterFactories); } }

this keeps the options open for if we ever do need to add anything to the constructor. while keeping the convenience and more or less the overhead reduction benefits.

lsmith77 · 2018-06-22T08:00:46Z

maybe we should have a 2.1 branch for this? or should we actually start using master again for future minor/major new versions?

maximgubar · 2018-06-26T08:30:48Z

@lsmith77 we started to use master branch again, the base branch of this PR was changed.

added FilterSetCollection public service

6b41c3b

maximgubar requested a review from dbu May 23, 2018 09:08

fixed static tests

11c0c12

dbu reviewed May 24, 2018

View reviewed changes

maximgubar added 2 commits May 24, 2018 11:07

removed setters from Filters and FilterSets

6a8cff3

fixed static tests

add533f

robfrawley changed the title ~~added FilterSetCollection public service~~ [Filters] [Config] [DI] Add Filter configuration class as public service May 27, 2018

maximgubar added 2 commits May 29, 2018 17:04

added filter-specific objects

95db82b

fixed static tests

d907670

dbu reviewed May 29, 2018

View reviewed changes

maximgubar added 7 commits June 4, 2018 14:13

applied requested review changes

e213124

fixed static tests

851a38b

removed FilterConfiguration object dependency

27c8794

renamed FilterSet to Stack

5c4d68f

added filters config objects

e306612

added filters config objects

db0a518

fixed static tests

d6c599f

Koc suggested changes Jun 6, 2018

View reviewed changes

dbu reviewed Jun 6, 2018

View reviewed changes

maximgubar added 4 commits June 6, 2018 15:13

added optional return types, removed unnecessary phpdocs

6906000

added Point and Size objects

4ea4acc

replaced trivial ternary operations with null coalesce operator

ebc5744

applied PR fixes

02e4022

Koc reviewed Jun 6, 2018

View reviewed changes

maximgubar added 2 commits June 11, 2018 11:51

added @internal to all config-related factories

c4923d0

introduced StckFactoryInterface

c564a62

fixed static tests

dbu reviewed Jun 11, 2018

View reviewed changes

maximgubar added 4 commits June 11, 2018 15:17

moved filter NAME const from factory to instance

9971029

fixed static tests

36d7843

applied PR fixes

91bfe94

added @codeCoverageIgnore

9df4d4b

dbu reviewed Jun 11, 2018

View reviewed changes

applied PR fixes

be890e3

dbu approved these changes Jun 11, 2018

View reviewed changes

maximgubar added 4 commits June 12, 2018 11:34

renamed filterSet to stack

8458b37

removed self-referencing constant

6613b61

fixed static tests

cdab3b8

fixed small issues:

f8cdfb7

- optional return types - typo in Paste filter name

fixed Watermark factory arguments order and type sanitize

2a397ae

lsmith77 reviewed Jun 22, 2018

View reviewed changes

maximgubar added 2 commits June 25, 2018 16:48

Merge branch '2.0' into config-reader

f1ed641

removed backslashes from native constants

0624a2a

maximgubar changed the base branch from 2.0 to master June 26, 2018 08:25

lsmith77 closed this Jun 26, 2018

lsmith77 reopened this Jun 26, 2018

lsmith77 merged commit 82aeb77 into master Jun 26, 2018

lsmith77 deleted the config-reader branch June 26, 2018 11:51


		namespace Liip\ImagineBundle\Config;

		interface FilterInterface


		use Liip\ImagineBundle\Factory\Config\FilterFactoryInterface;

		interface FilterFactoryCollectionInterface


		namespace Liip\ImagineBundle\Config;

		final class Filter implements FilterInterface

[Filters] [Config] [DI] Add Filter configuration class as public service #1098

[Filters] [Config] [DI] Add Filter configuration class as public service #1098

Conversation

maximgubar commented May 23, 2018 • edited by lsmith77

robfrawley commented May 23, 2018 • edited

robfrawley commented May 23, 2018 • edited

dbu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robfrawley commented May 24, 2018 • edited

dbu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Koc Jun 6, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maximgubar commented May 23, 2018 •

edited by lsmith77

robfrawley commented May 23, 2018 •

edited

robfrawley commented May 23, 2018 •

edited

robfrawley commented May 24, 2018 •

edited

Koc Jun 6, 2018 •

edited