[1.0][CacheResolver] Resolver get rid of get browser path #284

makasim · 2013-12-12T21:08:56Z

This PR is attemp to address @LouTerrailloune comment.

It is based on #282 PR so please consider reviewing and merging that one first.

To say briefly about changes:

[ResolverInterface] The resolve method can return NULL which means we dont have a cache yet and a string. String is an url where the image located.
[ResolverInterface] The method getBrowserPath was removed.
[CacheManager] The method getBrowserPath now use resolve method to find a cached image url. If it fails it generates an url of filter action.
Removed dependency on cache manager almost in all resolver. WebPathResolver still needed it

havvg · 2013-12-13T08:12:00Z

Imagine/Cache/CacheManager.php

@@ -120,7 +119,13 @@ protected function getResolver($filter)
     */
    public function getBrowserPath($path, $filter, $absolute = false)
    {
-        return $this->getResolver($filter)->getBrowserPath($path, $filter, $absolute);
+        //call it to make sure the resolver for the give filter exists.
+        $this->getResolver($filter);


There is no use to this behavior. The resolve method will try to retrieve the resolver anyway and throws an exception if none is applicable.

That's the only thing left, as far as I can see. What's the benefit of this line?

The only case I can see, would be calling an invalid filter from a template. The issue I see: the whole template would not be rendered with this line in place. Without this line, the template would be fine as it and only contain an invalid URL to an image (via generateUrl).

LouTerrailloune · 2013-12-13T08:24:06Z

I don't think setting the ResolverInterface:store return value to string is a good idea. For exemple, the resolver might load the image data from a database or local cache.
The redirection returned by ResolverInterface:resolve is not really needed, because it's only used when the webserver is missconfigured or when the file is propagating to a CDN.

So, in my opinion, ResolverInterface:resolve should return only a string or null (equivalent to your getBrowserPath), the filterAction do no call resolve and just load the data and call ResolverInterface:store. CacheManager:resolve return the filterAction url if the resolver return null. CacheManager:getBrowserPath and ResolverInterface:getBrowserPath are removed.

havvg · 2013-12-13T09:22:16Z

@LouTerrailloune The Resolver is not responsible for actually loading the image data. That's the purpose of the DataLoader. Returning the URL to retrieve the given image path on this resolver is fine for ResolverInterface::resolve.

I don't think setting the ResolverInterface:store return value to string is a good idea. For exemple, the resolver might load the image data from a database or local cache.

Did you actually refer to the store method? It's returning the modified Response, not a string.

The redirection returned by ResolverInterface:resolve is not really needed, because it's only used when the webserver is missconfigured or when the file is propagating to a CDN.

The URL returned by the resolve method defines the URL to load the image from utilizing the resolver again e.g. a CDN like S3, or locally saved in the web path etc.

havvg · 2013-12-13T09:27:43Z

Controller/ImagineController.php

-        if ($filteredImagePath instanceof Response) {
-            return $filteredImagePath;
+        if ($url = $this->cacheManager->resolve($originalImagePath, $filter)) {
+            return new RedirectResponse($url, 301);


I don't like this. It's cleaner than before, but now the resolve method is responsible of detecting whether the given path has been stored within this resolver and resolve the given path to a URL utilising the resolver.

I would prefer a new method isStored or something to decouple the two facts.

The controller then can distinguish between an already stored image and a to-be-stored one.
This will also remove the ambiguous return value of the Resolver::resolve method. It's always string: the URL. While the new method will take care of the previous null return value, question would result in a false of isStored.

I think it is a good idea in general. What should happen if one call resolve when image is not stored yet? I would say an exception.

LouTerrailloune · 2013-12-13T09:40:45Z

Did you actually refer to the store method? It's returning the modified Response, not a string.

Yes, typo.

The URL returned by the resolve method defines the URL to load the image from utilizing the resolver again e.g. a CDN like S3, or locally saved in the web path etc.

Most of the time, the url of the cached picture is returned by getBrowserPath (not resolve), via the templating extension/helper.

The Resolver is not responsible for actually loading the image data. That's the purpose of the DataLoader.

May be I have a resolver that store filtered picture (cropped, watermarked, etcc) into a database. In that case, the filterAction is alway hit and resolve load data from the databse and return it in a Response.

LouTerrailloune · 2013-12-13T09:50:06Z

Regarding my use case about database cache, we need to keep resolve and getBrowserPath (which can be renamed getUrl).

resolve return a Response with data (from cache) or null. (or may be a redirect)
getBrowserPath return a string or null.

In CacheManager, resolve is just a proxy that call the resolver resolve method, getBrowserPath return the result of the resolver getBrowserPath or the url of the filterAction if the resolver returns null.

havvg · 2013-12-13T11:24:54Z

Most of the time, the url of the cached picture is returned by getBrowserPath (not resolve), via the templating extension/helper.

That's fine because the bundles Resolver actually resulted in resolve within getBrowserPath with slight changes. That was only necessary because of the different targetPath, which will be removed. Thus the getBrowserPath will not be required anymore.

May be I have a resolver that store filtered picture (cropped, watermarked, etcc) into a database. In that case, the filterAction is alway hit and resolve load data from the databse and return it in a Response.

If you got a Resolver which stores the images in a database, and you want this resolver to serve the files, you would always hit the database. If that's ok for you, it's fine and still possible. If not, I misunderstand this use-case and would like to know what exactly you are trying to achieve.

LouTerrailloune · 2013-12-13T12:39:28Z

If you got a Resolver which stores the images in a database, and you want this resolver to serve the files, you would always hit the database. If that's ok for you, it's fine and still possible. If not, I misunderstand this use-case and would like to know what exactly you are trying to achieve.

This is what I want to do. However, in this PR, resolve only return an url and then a Resolver cannot create a Response (from database in my exemple) and return it to the controller. This is why I think we need to keep the Response return value.

havvg · 2013-12-13T13:06:58Z

Yes that's fine, you will have to move some logic around, because you mixed up the resolver with the controller. That's an issue of the old version.

makasim · 2013-12-13T20:45:32Z

So to sum up all the discussion:

resolve has to return an url (string) always.
if you call resolve before actually storing image you endup with an exception.
new method isStored has to be added.

Do I miss something?

makasim · 2013-12-13T20:46:57Z

@LouTerrailloune we should definitly think of the cases when the image stored in database or something like that. But my main goal right now to decouple things and clean the bundle a bit. Later we can come back and rethink our use case.

havvg · 2013-12-14T09:50:59Z

Do I miss something?

Not as far as I can see.

makasim · 2013-12-14T13:09:25Z

rebased against lates resolver-do-not-expose-target-path branch

havvg · 2013-12-15T17:51:39Z

For some reason I currently can't create a PR with Github, it's not recognising the branch. I added the ResolverInterface::isStored at https://github.com/havvg/LiipImagineBundle/tree/resolver-get-rid-of-get-browser-path

havvg · 2013-12-15T18:49:49Z

PR is now available, created another branch which is recognised by Github.

add ResolverInterface::isStored

makasim · 2013-12-16T10:47:46Z

I think it is almost fine but I would like to review it a bit more today in the evening to make sure I dont miss\broke anything

havvg · 2013-12-16T10:48:39Z

Fine by me, let me know :-)

makasim · 2013-12-18T07:46:52Z

@havvg I fixed WebPathResolver. It was not return string on resolve always as interface demand. Also updated UPGRADE.md

havvg · 2013-12-18T07:52:10Z

Imagine/Cache/Resolver/AbstractFilesystemResolver.php

     * @param string $path The resource path to convert.
     * @param string $filter The name of the imagine filter.
     *
     * @return string
     */
-    abstract protected function getFilePath($path, $filter);
+    protected function getFilePath($path, $filter)


Please move this logic back into WebPathResolver. It's bound to the web root.

makasim · 2013-12-18T08:04:28Z

@havvg done

makasim · 2013-12-18T13:51:10Z

@havvg ping

…rowser-path [1.0][CacheResolver] Resolver get rid of get browser path

havvg · 2013-12-18T13:55:10Z

Thank you!

havvg reviewed Dec 13, 2013
View reviewed changes

havvg mentioned this pull request Dec 13, 2013

[1.0][resolver] do not expose targetPath #282

Merged

makasim and others added 3 commits December 14, 2013 14:48

[resolver] do not expose targetPath.

8b8a1e7

remove "targetPath" from bundle dictionary

42e7395

[resolver] get rid of CacheResolver::getBrowserPath method.

2c791ec

add ResolverInterface::isStored

608c9fb

havvg mentioned this pull request Dec 16, 2013

add ResolverInterface::isStored formapro-forks/LiipImagineBundle#3

Merged

havvg and others added 2 commits December 16, 2013 11:32

add NotResolvableException

4a09f7c

Merge pull request #3 from havvg/resolver-get-rid-of-get-browser-path

b5e14a5

add ResolverInterface::isStored

makasim added 2 commits December 18, 2013 09:45

[resolver][web-path] always return string on resolve.

24c3b9a

update UPGRaDE.md

1222d66

havvg reviewed Dec 18, 2013
View reviewed changes

fix comments.

ff6b7a7

makasim mentioned this pull request Dec 18, 2013

[WIP][1.0][FilterManager] Filter manager drop get method. Add ability to configure at runtime. #288

Closed

havvg added a commit that referenced this pull request Dec 18, 2013

Merge pull request #284 from formapro-forks/resolver-get-rid-of-get-b…

11e889a

…rowser-path [1.0][CacheResolver] Resolver get rid of get browser path

havvg merged commit 11e889a into liip:develop Dec 18, 2013

makasim deleted the resolver-get-rid-of-get-browser-path branch December 18, 2013 14:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[1.0][CacheResolver] Resolver get rid of get browser path #284

[1.0][CacheResolver] Resolver get rid of get browser path #284

makasim commented Dec 12, 2013

havvg Dec 13, 2013

havvg Dec 18, 2013

LouTerrailloune commented Dec 13, 2013

havvg commented Dec 13, 2013

havvg Dec 13, 2013

makasim Dec 13, 2013

LouTerrailloune commented Dec 13, 2013

LouTerrailloune commented Dec 13, 2013

havvg commented Dec 13, 2013

LouTerrailloune commented Dec 13, 2013

havvg commented Dec 13, 2013

makasim commented Dec 13, 2013

makasim commented Dec 13, 2013

havvg commented Dec 14, 2013

makasim commented Dec 14, 2013

havvg commented Dec 15, 2013

havvg commented Dec 15, 2013

makasim commented Dec 16, 2013

havvg commented Dec 16, 2013

makasim commented Dec 18, 2013

havvg Dec 18, 2013

makasim commented Dec 18, 2013

makasim commented Dec 18, 2013

havvg commented Dec 18, 2013

[1.0][CacheResolver] Resolver get rid of get browser path #284

[1.0][CacheResolver] Resolver get rid of get browser path #284

Conversation

makasim commented Dec 12, 2013

havvg Dec 13, 2013

Choose a reason for hiding this comment

havvg Dec 18, 2013

Choose a reason for hiding this comment

LouTerrailloune commented Dec 13, 2013

havvg commented Dec 13, 2013

havvg Dec 13, 2013

Choose a reason for hiding this comment

makasim Dec 13, 2013

Choose a reason for hiding this comment

LouTerrailloune commented Dec 13, 2013

LouTerrailloune commented Dec 13, 2013

havvg commented Dec 13, 2013

LouTerrailloune commented Dec 13, 2013

havvg commented Dec 13, 2013

makasim commented Dec 13, 2013

makasim commented Dec 13, 2013

havvg commented Dec 14, 2013

makasim commented Dec 14, 2013

havvg commented Dec 15, 2013

havvg commented Dec 15, 2013

makasim commented Dec 16, 2013

havvg commented Dec 16, 2013

makasim commented Dec 18, 2013

havvg Dec 18, 2013

Choose a reason for hiding this comment

makasim commented Dec 18, 2013

makasim commented Dec 18, 2013

havvg commented Dec 18, 2013