[Dependency Injection] [Filter] Implement filter service abstraction for creating images #922

rpkamp · 2017-05-05T12:03:49Z

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

This PR splits all the image manipulation from the ImagineController and ResolveCacheCommand classes to a new FilterService class, which was on the roadmap for 2.x:

Move Controller Business Logic into Service (Allow for use of service without Request)

The advantages of this are:

Controller is only concerned with HTTP infrastructure
Command is only concerned with CLI I/O
Controller and command are easier to read, more highlevel
Functionality is easier to use and extend in custom controllers/command from third parties
FilterManager#applyFilter is only called in one spot throughout the entire codebase, making it easier to allow for multiple filter implementations later on

This PR is missing tests for the FilterService class, but I intend to add those once everyone agrees on a general direction of this PR. I did test the functionality in the browser (by using it in a separate test project) and that works as expected.

Discussion
In the FilterService the method createFilteredImage is closely tied to getUrlOfFilteredImage, but the first is to create the image and the latter is to retrieve the URL of the created image. Same thing goes for createFilteredImageWithRuntimeFilters and getUrlOfFilteredImageWithRuntimeFilters.
I split these out because of the CQS principle, but have now introduced a temporal coupling between the two. A more pure CQS approach would be to generate a URL first and then pass it to createFilteredImage as the desired URL for the image. However, it would be a mayor overhaul at this point to accomplish that, if indeed it is at all possible.

An alternative could be to let createFilteredImage return the URL of the resulting image (whether is was obtained from cache or not) in kind of find-or-create pattern way. This breaks CQS, but might be easier to understand and is less verbose in usage because one would only need one call instead of two to create a filtered image and obtain its URL.

…eBundle into rvanlaarhoven-patch-1

… minor tweaks

Updating 2.0 with corrections from 1.0

removed html coverage generation updated readme info urls

…terService

robfrawley · 2017-05-05T12:25:57Z

Service/FilterService.php

+     * @param array|null $runtimeFilters
+     * @return \Liip\ImagineBundle\Binary\BinaryInterface
+     * @throws \Liip\ImagineBundle\Exception\Imagine\Filter\NonExistingFilterException
+     */


All doc blocks need their CS fixes such that they are ordered param, throws, return, each type is separated by a two newlines, and have their param variables aligned. We also use short class names, even if they aren't used in the current file a and require an additional use statement.

/** * @param string $path * @param string $filter * @param array|null $runtimeFilters * * @throws \Liip\ImagineBundle\Exception\Imagine\Filter\NonExistingFilterException * * @return \Liip\ImagineBundle\Binary\BinaryInterface */

I'm confused regarding using short class names, as you said the complete opposite in this comment: https://github.com/liip/LiipImagineBundle/pull/908/files/f9cb8243475a1e492cb77ac5e639d306381fe9a8#diff-3edb79e074a2b20890b43396b316b5d3L60

Or is the short class name only used for @param but not for @throws and @return?

My bad, regarding the referenced comment, I thought that was a @covers which is where we've been adding leading backslashes. That, too, should be fixed, I just skimmed over what it was too fast. :-(

We have a php-cs-fixer require-dev requirement as well as the associated style settings; I would just run that, which will take care of everything but the FQCNs.

Erm, looks like our fixer rules are out of date and most of them are skipped, so that isn't a sure-fire bet either...

Is this what you mean? 4bce009

Short of the FQCNs that still exist, yes.

The @cover instances should be the only examples of FQCNs (and I don't know that we have a real policy for @see yet), but we do always use short names for method doc blocks

https://github.com/liip/LiipImagineBundle/blob/1.0/Tests/Binary/Loader/FileSystemLoaderTest.php#L22

Ah yes, that makes sense. Thanks.

robfrawley · 2017-05-06T21:19:38Z

The RST documentation (which is what is pushed to Symfony's online documentation website) requires changes and additional pertinent to this change, as well.

The general intention (and the vast majority of implementation) looks really great. I'll try to provide a full review by the end of this weekend with any comments I have. Thanks for this; it ticks off one of the larger TODO items from our 2.x roadmap.

…rvice

rpkamp · 2017-05-08T19:53:12Z

I've merged this PR with master so I could use short array syntax again 🎉

Koc · 2017-05-08T20:07:36Z

Lets wait until #926 will be merged into 2.0

Koc · 2017-05-08T20:14:21Z

Also closes #806

rpkamp · 2017-05-09T08:02:22Z

Not sure what's going on with #926 since it's closed but not merged?

robfrawley · 2017-05-09T14:16:11Z

The 1.x branch will be merged with 2.0 once we release 1.8.0 (see #928).

Koc · 2017-05-10T04:06:37Z

You need apply your service also here https://github.com/liip/LiipImagineBundle/blob/2.0/Async/ResolveCacheProcessor.php
//cc @makasim

robfrawley · 2017-05-10T04:16:21Z

All branches up to date.

…rvice

rpkamp · 2017-05-10T20:24:10Z

I've updated the ResolveCacheProcessor to make use of the new FilterService.
See 05afc99

rpkamp · 2017-05-14T20:08:32Z

Does anyone have any thoughts on the CQS vs Temporal Coupling discussion in my initial PR message?

The more I think about it, the more I think I'd rather have a CQS violation than temporal coupling, so instead of one method to query what the URL would be and a command to create the image, create one find-or-create method that creates the image if it doesn't exist and then return the URL, or if the image already existed just return the URL.
The reason I like it better is because it easier to understand and harder to get wrong for people starting out with this bundle.

robfrawley · 2017-05-14T22:30:26Z

@rpkamp

Let createFilteredImage return the URL of the resulting image (whether is was obtained from cache or not) in kind of find-or-create pattern way. This breaks CQS, but might be easier to understand and is less verbose.

Yes, I absolutely prefer this approach.

…service

By coupling the creation of the filtered image to resolvig the URL of the resolving image the use case of requesting the URL of a filtered image that has not been created yet (temporal coupling issue) is no longer possible. The current implementation breaks CQS, but this is not as bad as the previous temporal coupling.

rpkamp · 2017-05-20T15:46:01Z

@robfrawley

Yes, I absolutely prefer this approach.

Done. This approach makes more sense for this project than the previous approach 😃

rpkamp · 2017-05-25T13:31:35Z

Could I get a review on this one please?

Koc · 2017-08-04T21:20:05Z

Nice PR, can anybody review?

makasim · 2017-08-07T08:40:03Z

LGTM

robfrawley · 2017-08-10T05:11:31Z

This looks fine to me as well. I've been focused on getting 1.9 released before I worried about any 2.x PRs.

rpkamp · 2017-11-27T20:20:26Z

This PR has been open for a long time (since May of this year).
Anything I can do to advance the merging of this PR?

makasim · 2017-11-27T20:23:38Z

@rpkamp Thank you for the contribution.

rvanlaarhoven and others added 13 commits August 5, 2016 17:16

Removed deprecated factory_class and factory_method

a1abe98

Merge branch 'patch-1' of https://github.com/rvanlaarhoven/LiipImagin…

94facff

…eBundle into rvanlaarhoven-patch-1

Merge branch 'rvanlaarhoven-patch-1' into 2.0

6d399f6

correct example filter yml definitions in docs (fixes #818) and other…

210922c

… minor tweaks

correct accidental regression introduced in prior commit

c8648ca

Merge pull request #820 from liip/image-cache-manager-typo

f8aa614

Updating 2.0 with corrections from 1.0

Merge branch 'master' into 2.0

6f00396

added coveralls reporting to travis

dba4474

added coveralls reporting to travis

d8435eb

removed html coverage generation updated readme info urls

updated readme for branch

c708b79

Split image filtering functionality from ImagineController to new Fil…

32c0a1f

…terService

README displays usage of new FilterService instead of ImagineController

58603f3

Use new FilterService in ResolveCacheCommand

6681c81

rpkamp changed the title ~~Image service~~ Introduce FilterService as abstraction for creating images May 5, 2017

Use long array notation since PHP 5.3 is still supported in Travis

f774704

robfrawley reviewed May 5, 2017

View reviewed changes

rpkamp added 3 commits May 5, 2017 14:35

CS fixes of docblocks

4bce009

No longer use unused classes in ResolveCacheCommand

9bf0ce8

Use short class names instead of FQCN in docblocks

5122ca8

robfrawley added this to the 2.0.0 milestone May 6, 2017

rpkamp added 2 commits May 8, 2017 21:43

Merge branch '2.0' of github.com:liip/LiipImagineBundle into image-se…

3c8bf52

…rvice

Use short array syntax now that PHP5.3 support was dropped

b60debc

rpkamp added 2 commits May 10, 2017 20:05

Merge branch '2.0' of github.com:liip/LiipImagineBundle into image-se…

d5abad7

…rvice

Use FilterService in ResolveCacheProcessor

05afc99

rpkamp added 2 commits May 20, 2017 17:17

Merge branch '2.0' of github.com:rpkamp/LiipImagineBundle into image-…

d897e54

…service

rpkamp added 2 commits May 20, 2017 17:49

Update README to reflect removal of FilterService::create... methods

fdd9302

Use short ternary syntax for logger in FilterService

edd673e

rpkamp added 3 commits October 12, 2017 21:22

Merge filter service with new Async processing

605190d

No longer require @dev for enqueue-bundle

fe47041

Require enqueue-bundle ^0.7, not ^0.7.0

270c250

makasim merged commit b272ae3 into liip:2.0 Nov 27, 2017

lsmith77 removed the State: Reviewing This item is being reviewed to determine if it should be accepted. label Nov 27, 2017

rpkamp deleted the image-service branch November 28, 2017 08:53

robfrawley changed the title ~~Introduce FilterService as abstraction for creating images~~ [Dependency Injection] [Filter] Implement filter service abstraction for creating images Mar 12, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Dependency Injection] [Filter] Implement filter service abstraction for creating images #922

[Dependency Injection] [Filter] Implement filter service abstraction for creating images #922

rpkamp commented May 5, 2017

robfrawley May 5, 2017

rpkamp May 5, 2017

rpkamp May 5, 2017

robfrawley May 5, 2017 •

edited

Loading

robfrawley May 5, 2017

robfrawley May 5, 2017

rpkamp May 5, 2017

robfrawley May 5, 2017

robfrawley May 5, 2017 •

edited

Loading

rpkamp May 5, 2017

robfrawley commented May 6, 2017 •

edited

Loading

rpkamp commented May 8, 2017

Koc commented May 8, 2017

Koc commented May 8, 2017

rpkamp commented May 9, 2017

robfrawley commented May 9, 2017

Koc commented May 10, 2017 •

edited

Loading

robfrawley commented May 10, 2017

rpkamp commented May 10, 2017 •

edited

Loading

rpkamp commented May 14, 2017

robfrawley commented May 14, 2017

rpkamp commented May 20, 2017

rpkamp commented May 25, 2017

Koc commented Aug 4, 2017

makasim commented Aug 7, 2017

robfrawley commented Aug 10, 2017

rpkamp commented Nov 27, 2017

makasim commented Nov 27, 2017

[Dependency Injection] [Filter] Implement filter service abstraction for creating images #922

[Dependency Injection] [Filter] Implement filter service abstraction for creating images #922

Conversation

rpkamp commented May 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robfrawley May 5, 2017 • edited Loading

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 May 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robfrawley commented May 6, 2017 • edited Loading

rpkamp commented May 8, 2017

Koc commented May 8, 2017

Koc commented May 8, 2017

rpkamp commented May 9, 2017

robfrawley commented May 9, 2017

Koc commented May 10, 2017 • edited Loading

robfrawley commented May 10, 2017

rpkamp commented May 10, 2017 • edited Loading

rpkamp commented May 14, 2017

robfrawley commented May 14, 2017

rpkamp commented May 20, 2017

rpkamp commented May 25, 2017

Koc commented Aug 4, 2017

makasim commented Aug 7, 2017

robfrawley commented Aug 10, 2017

rpkamp commented Nov 27, 2017

makasim commented Nov 27, 2017

robfrawley May 5, 2017 •

edited

Loading

robfrawley May 5, 2017 •

edited

Loading

robfrawley commented May 6, 2017 •

edited

Loading

Koc commented May 10, 2017 •

edited

Loading

rpkamp commented May 10, 2017 •

edited

Loading