[2.8] Move argument resolving from ControllerResolver #14971

wouterj · 2015-06-13T15:25:47Z

Description

This closes #11457. Changes since that PR:

The ControllerResolver has a new method setArgumentResolver, too keep BC with people overriding ControllerResolver#doGetArguments
Deprecations instead of removal, so it can be merged into 2.8
Added 2 new argument resolvers: A Security user resolver (which was the original problem to create this PR) and a PSR Request resolver

PR Info Table

Q	A
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	yes
Tests pass?	yes
Fixed tickets	#1547, #10710
License	MIT
Doc PR	tbd

Todo

Add priority (is this actually needed?)
Make tests pass
Update docs

Koc · 2015-06-13T19:45:05Z

src/Symfony/Component/HttpKernel/Controller/ControllerResolver.php

@@ -102,34 +118,20 @@ public function getArguments(Request $request, $controller)
            $r = new \ReflectionFunction($controller);
        }

+        $reflector = new \ReflectionMethod($this, 'doGetArguments');
+        if ($reflector->getDeclaringClass()->getName() !== __CLASS__) {
+            trigger_error('The ControllerResolverInterface::doGetArguments() method is deprecated since version 2.7 and will be removed in 3.0. Use the ArgumentResolverManager and custom ArgumentResolverInterface implementations instead.', E_USER_DEPRECATED);


maybe 2.8?

BTW deprecation errors now comes with @ operator

Thanks, both mistakes are fixed now

aitboudad · 2015-06-14T10:12:51Z

src/Symfony/Bundle/SecurityBundle/ArgumentResolver/UserArgumentResolver.php

+/**
+ * @author Wouter J <wouter@wouterj.nl>
+ */
+class UserArgumentResolver implements ArgumentResolverInterface


I think this can be confused if we want to resolve User by ParamConverter ?

Sorry, I'm not sure I understand what you try to say.

Hmm, do you mean using the Doctrine Param Converter? That's why I made this resolver only work with UserInterface. When using the Doctrine Param Converter, one would actually need to typehint to a specific implementation.

Btw, this may be a good reason to implement priority for argument resolvers.

yes I mean Doctrine Param Converter and implement priority sound good to me.

fabpot · 2015-06-15T01:55:16Z

I would be interested in having measures in terms of performance impact of these changes.

wouterj · 2015-06-30T09:35:40Z

Unfortunately, I can't make benchmarks using Blackfire on my Windows PC. The timeline in the profiler doesn't show much difference between applying this PR and using the 2.8 branch.

I would appreciate if someone with a Linux machine could run some benchmarks using blackfire on this :)

stof · 2015-06-30T10:12:17Z

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php

@@ -153,6 +153,10 @@ public function load(array $configs, ContainerBuilder $container)
            $definition->replaceArgument(2, E_COMPILE_ERROR | E_PARSE | E_ERROR | E_CORE_ERROR);
        }

+        if (interface_exists('Symfony\Bridge\PsrHttpMessage\HttpMessageFactoryInterface')) {


there should also be a config setting to force disabling it in case you don't want to use the parameter converter, allowing to get rid of its overhead

In that case, we should have a configure option for all resolvers imo (so one config option in SecurityBundle too). I'm not sure I like that.

stof · 2015-06-30T10:26:38Z

and all the argument resolvers are missing their test coverage

xabbuh · 2015-07-02T20:21:06Z

src/Symfony/Component/HttpKernel/DependencyInjection/RegisterArgumentResolversPass.php

+     */
+    public function process(ContainerBuilder $container)
+    {
+        if (!$container->hasDefinition($this->managerService) && !$container->hasAlias($this->managerService)) {


if (!$container->has($this->managerService)

henrikbjorn · 2015-08-23T13:55:26Z

@wouterj how far along is this? Also could the Argument resolvement be put into a seperate repository so we can play around with it (want to use some in Silex context)

wouterj · 2015-08-23T13:59:41Z

@henrikbjorn imo, it's just waiting for someone with an UNIX based PC to benchmark it properly (I'm on Windows).

If you want to play around with it, you can do something like this in a Silex test app:

$ composer require "symfony/symfony"
$ cd vendor/symfony
$ rm -rf symfony
$ git clone http://github.com/WouterJ/symfony
$ git checkout -b pr_11457

henrikbjorn · 2015-08-23T15:02:46Z

Well im am on a Mac, so i could benchmark it if you have instructions on how to do so.

henrikbjorn · 2015-08-23T15:52:47Z

src/Symfony/Component/HttpKernel/HttpKernel.php

+            ));
+        }
+
+        if (method_exists($resolver, 'setArgumentResolverManager')) {


If i create a ControllerResolver and use the method setArgumentResolverManager on it to set the manager directly on the resolver, and i then do not give HttpKernel an instance of that manager, the HttpKernel class will set it and overwrite the correct bootstrapping that was done.

Is there a purpose for having this code here?

The ControllerResolver#setArgumentResolverManager() method is tagged as @internal, which means you shouldn't use it in your own code. It only exists, so we can call it here in the HttpKernel.

This is to be BC. In 3.0, ControllerResolver#setArgumentResolverManager() will be removed and the complete argument resolving part will be called by HTTP kernel directly (ControllerResolver will only resolve the controller then). So if you want to add custom argument resolvers, do it by customizing HttpKernel instead of ControllerResolver.

wouterj · 2015-08-23T16:11:03Z

I think a good benchmark would be to fork the standard edition and create 2 controllers (doing nothing more than returning a response): One controller with no arguments and one controller with some arguments (e.g. (Request $request, $someRoutePlaceholder)).

These 2 controllers then should be benchmarked with Blackfire (for instance) against the current 2.8 codebase and the code base of this PR.

henrikbjorn · 2015-08-23T16:34:48Z

I dont want to do stuff with Blackfire!
If there is an alternative solution (terminal based) i am all for helping.

henrikbjorn · 2015-08-23T17:00:44Z

src/Symfony/Bundle/SecurityBundle/ArgumentResolver/UserArgumentResolver.php

+ * file that was distributed with this source code.
+ */
+
+namespace Symfony\Bundle\SecurityBundle\ArgumentResolver;


Maybe this would be better suited in the Component?

Koc · 2015-10-01T07:40:09Z

So does only missing performance test blocks merging this PR?

//cc @fabpot

fabpot · 2015-10-01T08:17:17Z

@Koc Performance testing is indeed blocking this PR.

Koc · 2015-12-11T10:59:23Z

Sad that this PR wouldn't merged. @wouterj has maked awesome work.

wouterj · 2015-12-11T11:01:46Z

Well, I left my branch in my fork, so if someone has the time and energy to benchmark + improve this PR, feel free to base your contributions on my branch. For now, I don't have time and resources to work on this PR.

wouterj added 3 commits June 10, 2015 09:27

Extracted controller argument resolving from the ControllerResolver

54a0653

Added PsrServerRequest argument resolver

3dfdb61

Added User Argument Resolver

434f92f

wouterj mentioned this pull request Jun 13, 2015

[WIP] Refactored argument resolving out of the ControllerResolver #11457

Closed

4 tasks

Koc reviewed Jun 13, 2015
View reviewed changes

Review

a1d66c6

wouterj force-pushed the pr_11457 branch from ba12237 to a1d66c6 Compare June 13, 2015 22:22

wouterj added 2 commits June 14, 2015 00:24

Fabbot

cf69cc7

Fix tests

3a1d99c

aitboudad reviewed Jun 14, 2015
View reviewed changes

wouterj added 2 commits June 14, 2015 13:58

Fix deprecation trigger with TraceableControllerResolver

1741b2a

Add priority

4924e86

stof reviewed Jun 30, 2015
View reviewed changes

Lots of fixes thanks to review

1300ed2

xabbuh reviewed Jul 2, 2015
View reviewed changes

henrikbjorn mentioned this pull request Aug 23, 2015

Use ArgumentResolver instead of ControllerResolver silexphp/Silex#1223

Closed

2 tasks

henrikbjorn reviewed Aug 23, 2015
View reviewed changes

fabpot added the HttpKernel label Oct 5, 2015

wouterj closed this Dec 11, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[2.8] Move argument resolving from ControllerResolver #14971

[2.8] Move argument resolving from ControllerResolver #14971

wouterj commented Jun 13, 2015

Koc Jun 13, 2015

Koc Jun 13, 2015

wouterj Jun 13, 2015

aitboudad Jun 14, 2015

wouterj Jun 14, 2015

wouterj Jun 14, 2015

aitboudad Jun 14, 2015

fabpot commented Jun 15, 2015

wouterj commented Jun 30, 2015

stof Jun 30, 2015

wouterj Jun 30, 2015

stof commented Jun 30, 2015

xabbuh Jul 2, 2015

henrikbjorn commented Aug 23, 2015

wouterj commented Aug 23, 2015

henrikbjorn commented Aug 23, 2015

henrikbjorn Aug 23, 2015

wouterj Aug 23, 2015

wouterj commented Aug 23, 2015

henrikbjorn commented Aug 23, 2015

henrikbjorn Aug 23, 2015

Koc commented Oct 1, 2015

fabpot commented Oct 1, 2015

Koc commented Dec 11, 2015

wouterj commented Dec 11, 2015

[2.8] Move argument resolving from ControllerResolver #14971

[2.8] Move argument resolving from ControllerResolver #14971

Conversation

wouterj commented Jun 13, 2015

Description

PR Info Table

Todo

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

fabpot commented Jun 15, 2015

wouterj commented Jun 30, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stof commented Jun 30, 2015

Choose a reason for hiding this comment

henrikbjorn commented Aug 23, 2015

wouterj commented Aug 23, 2015

henrikbjorn commented Aug 23, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wouterj commented Aug 23, 2015

henrikbjorn commented Aug 23, 2015

Choose a reason for hiding this comment

Koc commented Oct 1, 2015

fabpot commented Oct 1, 2015

Koc commented Dec 11, 2015

wouterj commented Dec 11, 2015