Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[2.8] Move argument resolving from ControllerResolver#14971
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
maybe2.8?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
BTW deprecation errors now comes with@ operator
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Thanks, both mistakes are fixed now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think this can be confused if we want to resolve User by ParamConverter ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Sorry, I'm not sure I understand what you try to say.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Hmm, do you mean using the Doctrine Param Converter? That's why I made this resolver only work withUserInterface. 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
yes I mean Doctrine Param Converter and implement priority sound good to me.
fabpot commentedJun 15, 2015
I would be interested in having measures in terms of performance impact of these changes. |
wouterj commentedJun 30, 2015
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 :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 commentedJun 30, 2015
and all the argument resolvers are missing their test coverage |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
if (!$container->has($this->managerService)
henrikbjorn commentedAug 23, 2015
@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 commentedAug 23, 2015
@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: |
henrikbjorn commentedAug 23, 2015
Well im am on a Mac, so i could benchmark it if you have instructions on how to do so. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
If i create a ControllerResolver and use the methodsetArgumentResolverManager 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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
TheControllerResolver#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 commentedAug 23, 2015
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. 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 commentedAug 23, 2015
I dont want to do stuff with Blackfire! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Maybe this would be better suited in the Component?
Koc commentedOct 1, 2015
So does only missing performance test blocks merging this PR? //cc@fabpot |
fabpot commentedOct 1, 2015
@Koc Performance testing is indeed blocking this PR. |
Koc commentedDec 11, 2015
Sad that this PR wouldn't merged.@wouterj has maked awesome work. |
wouterj commentedDec 11, 2015
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. |
Description
Thiscloses#11457. Changes since that PR:
setArgumentResolver, too keep BC with people overridingControllerResolver#doGetArgumentsPR Info Table
Todo