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

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

Merged
merged 31 commits into from Jun 26, 2018
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
6b41c3b
added FilterSetCollection public service
maximgubar May 23, 2018
11c0c12
fixed static tests
maximgubar May 23, 2018
6a8cff3
removed setters from Filters and FilterSets
maximgubar May 24, 2018
add533f
fixed static tests
maximgubar May 24, 2018
95db82b
added filter-specific objects
maximgubar May 29, 2018
d907670
fixed static tests
maximgubar May 29, 2018
e213124
applied requested review changes
maximgubar Jun 4, 2018
851a38b
fixed static tests
maximgubar Jun 4, 2018
27c8794
removed FilterConfiguration object dependency
maximgubar Jun 4, 2018
5c4d68f
renamed FilterSet to Stack
maximgubar Jun 4, 2018
e306612
added filters config objects
maximgubar Jun 4, 2018
db0a518
added filters config objects
maximgubar Jun 6, 2018
d6c599f
fixed static tests
maximgubar Jun 6, 2018
6906000
added optional return types, removed unnecessary phpdocs
maximgubar Jun 6, 2018
4ea4acc
added Point and Size objects
maximgubar Jun 6, 2018
ebc5744
replaced trivial ternary operations with null coalesce operator
maximgubar Jun 6, 2018
02e4022
applied PR fixes
maximgubar Jun 6, 2018
c4923d0
added @internal to all config-related factories
maximgubar Jun 11, 2018
c564a62
introduced StckFactoryInterface
maximgubar Jun 11, 2018
9971029
moved filter NAME const from factory to instance
maximgubar Jun 11, 2018
36d7843
fixed static tests
maximgubar Jun 11, 2018
91bfe94
applied PR fixes
maximgubar Jun 11, 2018
9df4d4b
added @codeCoverageIgnore
maximgubar Jun 11, 2018
be890e3
applied PR fixes
maximgubar Jun 11, 2018
8458b37
renamed filterSet to stack
maximgubar Jun 12, 2018
6613b61
removed self-referencing constant
maximgubar Jun 12, 2018
cdab3b8
fixed static tests
maximgubar Jun 13, 2018
f8cdfb7
fixed small issues:
maximgubar Jun 13, 2018
2a397ae
fixed Watermark factory arguments order and type sanitize
maximgubar Jun 18, 2018
f1ed641
Merge branch '2.0' into config-reader
maximgubar Jun 25, 2018
0624a2a
removed backslashes from native constants
maximgubar Jun 25, 2018
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
51 changes: 51 additions & 0 deletions Config/Filter.php
@@ -0,0 +1,51 @@
<?php

/*
* This file is part of the `liip/LiipImagineBundle` project.
*
* (c) https://github.com/liip/LiipImagineBundle/graphs/contributors
*
* For the full copyright and license information, please view the LICENSE.md
* file that was distributed with this source code.
*/

namespace Liip\ImagineBundle\Config;

final class Filter implements FilterInterface
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

{
/**
* @var string
*/
private $name;

/**
* @var array
*/
private $options = [];

/**
* @param string $name
* @param array $options
*/
public function __construct(string $name, array $options)
{
$this->name = $name;
$this->options = $options;
}

/**
* @return string
*/
public function getName(): string
{
return $this->name;
}

/**
* @return array
*/
public function getOptions(): array
{
return $this->options;
}
}
68 changes: 68 additions & 0 deletions Config/Filter/Type/Crop.php
@@ -0,0 +1,68 @@
<?php

/*
* This file is part of the `liip/LiipImagineBundle` project.
*
* (c) https://github.com/liip/LiipImagineBundle/graphs/contributors
*
* For the full copyright and license information, please view the LICENSE.md
* file that was distributed with this source code.
*/

namespace Liip\ImagineBundle\Config\Filter\Type;

use Liip\ImagineBundle\Config\FilterInterface;

final class Crop implements FilterInterface
{
/**
* @var string
*/
private $name;

/**
* @var array
*/
private $start;

/**
* @var array
*/
private $size;

/**
* @param string $name
* @param array $start
* @param array $size
*/
public function __construct(string $name, array $start, array $size)
{
$this->name = $name;
$this->start = $start;
$this->size = $size;
}

/**
* @return string
*/
public function getName(): string
{
return $this->name;
}

/**
* @return array
*/
public function getStart(): array
{
return $this->start;
}

/**
* @return array
*/
public function getSize(): array
{
return $this->size;
}
}
38 changes: 38 additions & 0 deletions Config/Filter/Type/Strip.php
@@ -0,0 +1,38 @@
<?php

/*
* This file is part of the `liip/LiipImagineBundle` project.
*
* (c) https://github.com/liip/LiipImagineBundle/graphs/contributors
*
* For the full copyright and license information, please view the LICENSE.md
* file that was distributed with this source code.
*/

namespace Liip\ImagineBundle\Config\Filter\Type;

use Liip\ImagineBundle\Config\FilterInterface;

final class Strip implements FilterInterface
{
/**
* @var string
*/
private $name;

/**
* @param string $name
Copy link
Member

Choose a reason for hiding this comment

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

please remove

*/
public function __construct(string $name)
{
$this->name = $name;
}

/**
* @return string
*/
public function getName(): string
{
return $this->name;
}
}
103 changes: 103 additions & 0 deletions Config/Filter/Type/Thumbnail.php
@@ -0,0 +1,103 @@
<?php

/*
* This file is part of the `liip/LiipImagineBundle` project.
*
* (c) https://github.com/liip/LiipImagineBundle/graphs/contributors
*
* For the full copyright and license information, please view the LICENSE.md
* file that was distributed with this source code.
*/

namespace Liip\ImagineBundle\Config\Filter\Type;

use Liip\ImagineBundle\Config\FilterInterface;

final class Thumbnail implements FilterInterface
{
/**
* @var string
*/
private $name;

/**
* @var array
*/
private $size;

/**
* @var string
*/
private $mode;

/**
* @var bool
*/
private $allowUpscale;

/**
* @var string
*/
private $filter;

/**
* @param string $name
* @param array $size
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

* @param string|null $mode
* @param bool|null $allowUpscale
* @param string|null $filter
Copy link
Member

Choose a reason for hiding this comment

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

please remove the docblock as it contains no information

*/
public function __construct(
string $name,
array $size,
string $mode = null,
bool $allowUpscale = null,
string $filter = null
) {
$this->name = $name;
$this->size = $size;
$this->mode = $mode;
$this->allowUpscale = $allowUpscale;
$this->filter = $filter;
}

/**
* @return string
*/
public function getName(): string
{
return $this->name;
}

/**
* @return array
*/
public function getSize()
{
return $this->size;
}

/**
* @return string|null
*/
public function getMode()
{
return $this->mode;
}

/**
* @return bool|null
*/
public function isAllowUpscale()
{
return $this->allowUpscale;
}

/**
* @return string|null
*/
public function getFilter()
{
return $this->filter;
}
}
59 changes: 59 additions & 0 deletions Config/FilterFactoryCollection.php
@@ -0,0 +1,59 @@
<?php

/*
* This file is part of the `liip/LiipImagineBundle` project.
*
* (c) https://github.com/liip/LiipImagineBundle/graphs/contributors
*
* For the full copyright and license information, please view the LICENSE.md
* file that was distributed with this source code.
*/

namespace Liip\ImagineBundle\Config;

use Liip\ImagineBundle\Exception\Config\Filter\NotFoundException;
use Liip\ImagineBundle\Factory\Config\FilterFactoryInterface;

final class FilterFactoryCollection implements FilterFactoryCollectionInterface
{
/**
* @var FilterFactoryInterface[]
*/
private $filterFactories;

/**
* FilterCollection constructor.
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

*
* @param FilterFactoryInterface ...$filterFactories
*/
public function __construct(FilterFactoryInterface ...$filterFactories)
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 ?

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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).

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

{
$this->filterFactories = $filterFactories;
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

}

/**
* @param string $name
*
* @throws NotFoundException
*
* @return FilterFactoryInterface
*/
public function getFilterFactoryByName(string $name): FilterFactoryInterface
{
foreach ($this->filterFactories as $filterFactory) {
if ($filterFactory->getName() === $name) {
return $filterFactory;
}
}

throw new NotFoundException(sprintf("Filter factory with name '%s' was not found.", $name));
}

/**
* @return FilterFactoryInterface[]
*/
public function getAll()
{
return $this->filterFactories;
}
}
29 changes: 29 additions & 0 deletions Config/FilterFactoryCollectionInterface.php
@@ -0,0 +1,29 @@
<?php

/*
* This file is part of the `liip/LiipImagineBundle` project.
*
* (c) https://github.com/liip/LiipImagineBundle/graphs/contributors
*
* For the full copyright and license information, please view the LICENSE.md
* file that was distributed with this source code.
*/

namespace Liip\ImagineBundle\Config;

use Liip\ImagineBundle\Factory\Config\FilterFactoryInterface;

interface FilterFactoryCollectionInterface
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

{
/**
* @param string $name
*
* @return FilterFactoryInterface
*/
public function getFilterFactoryByName(string $name): FilterFactoryInterface;

/**
* @return FilterFactoryInterface[]
*/
public function getAll();
}
20 changes: 20 additions & 0 deletions Config/FilterInterface.php
@@ -0,0 +1,20 @@
<?php

/*
* This file is part of the `liip/LiipImagineBundle` project.
*
* (c) https://github.com/liip/LiipImagineBundle/graphs/contributors
*
* For the full copyright and license information, please view the LICENSE.md
* file that was distributed with this source code.
*/

namespace Liip\ImagineBundle\Config;

interface FilterInterface
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

/**
 * 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.
 */

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

{
/**
* @return string
Copy link
Member

Choose a reason for hiding this comment

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

we should say what the name is for. is it only for debugging purposes? do we match actual filters by filter class or by filter name?

i think filters should be matched by filter name, to allow having the same class represent different filters.

Copy link
Member

Choose a reason for hiding this comment

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

^

*/
public function getName(): string;
}