[1.0][resolver] remove request parameter #281

makasim · 2013-12-09T09:49:57Z

@havvg squashed and rebased over develop branch version. Hope I dont lost anything while rebase.

This is the work I did (that come with next PRs after this one is merged):

There other changes I am currently working on (would not be included in this PR):

ResolverInterface::resolve method return null or Response
ResolverInterface::store accept $path. right now it require $targetPath
ResolverInterface::store would return anything.
FilterManager::applyFilter takes third parameter runtimeConfig. It would allows configure filters in runtime.
FilterManager::get was removed.
...
...

these changes could be found in the branch https://github.com/formapro-forks/LiipImagineBundle/tree/resolver-do-not-expose-target-path

havvg · 2013-12-09T09:57:18Z

Imagine/Cache/Resolver/NoCacheResolver.php

    {
-        $this->setBasePath($request->getBaseUrl());
+        $this->setBasePath($this->container->get('request')->getBaseUrl());


https://github.com/liip/LiipImagineBundle/pull/278/files#r8191764

@makasim There is no interface yet, you should introduce it with this patch: Liip\ImagineBundle\Imagine\Cache\Resolver\RequestAwareInterface with a setRequest method only, just like https://github.com/symfony/symfony/blob/master/src/Symfony/Component/DependencyInjection/ContainerAwareInterface.php.

@havvg how should this method be used?

I cannot inject request as a service because in this case I have to put resolver to request scope and all services that use the resolver too. I dont think this would work.

Or are you about injectig request with strict="false" to prevent scoping errors

if we can update minimum supported version of symfony up to 2.3 we can use synchronized services feature.

I'm fine with changing the required version to 2.3 as it's an LTS, we should encourage upgrading to this version. The 0.x version line will still be available for older versions.

so I am gonna update required symfony version up to 2.3 and use synced services.

makasim · 2013-12-09T11:37:47Z

@havvg done, I've also added some functional tests, more would come with other PRs.

havvg · 2013-12-09T12:43:35Z

Could you squash the commit and add empty newlines where missing?

makasim · 2013-12-09T12:59:32Z

Could you squash the commit and add empty newlines where missing?

@havvg done

havvg · 2013-12-09T16:02:39Z

Imagine/Cache/Resolver/NoCacheResolver.php

    {
-        $this->setBasePath($request->getBaseUrl());
+        if (false == $this->request) {
+            throw new \LogicException('The request was not injected, inject it before using resolver.');


How about changing the injection to the constructor? There is no use to those resolvers (NoCacheResolver & WebPathResolver) without the request. This would also remove duplicate code of this check.

I dont think it is possible, container should have ability to change request service on sub request and set it to null when we out of request scope.

That's simply not possible with constructor injection.

I would suggest

to move the logic to AbstractFilesystemResolver.

or we could also try to use router context to get base url. I believe it is not in the request scope so we can inject it using constructor

honestly saying I would drop NoCacheResolver (or rename it) because it could work only with WebPathResolver.

any chance to merge it without change request related stuff? WebPathResolver requires separate PR with cleanups and refactoring. So maybe we can address this request realted duplications to that time?

makasim · 2013-12-10T08:52:04Z

@havvg moved request injection to base class.

makasim · 2013-12-10T08:53:17Z

@havvg bay the way there is kind of mess with that AbstractFilesystemResolver::basePath property. It is set by setter and used everywhere except resolve method where the value from request is used. This must be fixed somehow.

havvg · 2013-12-10T10:49:46Z

Resources/config/imagine.xml

@@ -176,8 +176,7 @@

        <!-- Cache resolver -->

-        <service id="liip_imagine.cache.resolver.web_path" class="%liip_imagine.cache.resolver.web_path.class%">
-            <tag name="liip_imagine.cache.resolver" resolver="web_path" />
+        <service id="liip_imagine.cache.resolver.abstract_filesystem" class="Liip\ImagineBundle\Imagine\Cache\Resolver\AbstractFilesystemResolver" abstract="true" public="false">


Please use a class name parameter for this.

it is not really useful

do you see any real use case where one would want to overwrite it?

As you can rename the classes of the resolvers, you should also be able to rename its parent class, as it might have changed, too, altering the complete class tree.

Actually, I would remove the name completely in favor of the abstract attribute on the service.

The abstract=true ensures you cannot create the service, thus the class attribute can be removed.

@havvg done: remove class name. Also squashed and rebased.

makasim · 2013-12-10T12:48:48Z

bump @havvg

…arameter [1.0][resolver] remove request parameter

havvg · 2013-12-10T12:51:25Z

Thank you!

havvg · 2013-12-10T12:52:13Z

I will take an insight look into your fork this evening, you mentioned some changes I would like to see in the 1.0.0 release version.

makasim · 2013-12-10T12:55:39Z

@havvg I would split it into pieces and do small PRs in the main direction of that branch. They would come soon

havvg · 2013-12-10T12:56:27Z

That would be great, small pieces to select and discuss accordingly, looking forward to it!

makasim mentioned this pull request Dec 9, 2013

[1.0][resolver] remove request parameter #278

Closed

havvg reviewed Dec 9, 2013
View reviewed changes

havvg reviewed Dec 10, 2013
View reviewed changes

[resolver] remove request from resolve method.

3369089

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

Merge pull request #281 from formapro-forks/resolver-remove-request-p…

6796e2f

…arameter [1.0][resolver] remove request parameter

havvg merged commit 6796e2f into liip:develop Dec 10, 2013

makasim deleted the resolver-remove-request-parameter branch December 10, 2013 12:55

makasim mentioned this pull request Dec 10, 2013

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

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[1.0][resolver] remove request parameter #281

[1.0][resolver] remove request parameter #281

makasim commented Dec 9, 2013

havvg Dec 9, 2013

makasim Dec 9, 2013

makasim Dec 9, 2013

havvg Dec 9, 2013

makasim Dec 9, 2013

makasim commented Dec 9, 2013

havvg commented Dec 9, 2013

makasim commented Dec 9, 2013

havvg Dec 9, 2013

makasim Dec 9, 2013

makasim Dec 9, 2013

makasim Dec 10, 2013

makasim commented Dec 10, 2013

makasim commented Dec 10, 2013

havvg Dec 10, 2013

makasim Dec 10, 2013

makasim Dec 10, 2013

havvg Dec 10, 2013

havvg Dec 10, 2013

havvg Dec 10, 2013

makasim Dec 10, 2013

makasim commented Dec 10, 2013

havvg commented Dec 10, 2013

havvg commented Dec 10, 2013

makasim commented Dec 10, 2013

havvg commented Dec 10, 2013

[1.0][resolver] remove request parameter #281

[1.0][resolver] remove request parameter #281

Conversation

makasim commented Dec 9, 2013

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

makasim commented Dec 9, 2013

havvg commented Dec 9, 2013

makasim commented Dec 9, 2013

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

makasim commented Dec 10, 2013

makasim commented Dec 10, 2013

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

makasim commented Dec 10, 2013

havvg commented Dec 10, 2013

havvg commented Dec 10, 2013

makasim commented Dec 10, 2013

havvg commented Dec 10, 2013