Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[HttpKernel] Add a ContainerControllerResolver (psr-11)#21768
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
[HttpKernel] Add a ContainerControllerResolver (psr-11)#21768
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| } | ||
| } | ||
| returnparent::createController($controller); |
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.
This method could be rewritten to use guard clauses, making it a lot easier to read
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 made some changes. Is it better?
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.
Much better yes!
| useSymfony\Component\HttpKernel\Tests\Controller\Psr11ControllerResolverTest; | ||
| class ControllerResolverTestextendsBaseControllerResolverTest | ||
| class ControllerResolverTestextendsPsr11ControllerResolverTest |
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.
tests in this class could stay to verify it doesn't change behavior, if this happens, it would break a lot.
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.
The tests cases were simply moved in thePsr11ControllerResolverTest class and are inherited, so I think we're safe, aren't we? :)
| * @param ControllerNameParser $parser A ControllerNameParser instance | ||
| * @param LoggerInterface $logger A LoggerInterface instance | ||
| */ | ||
| publicfunction__construct(ContainerInterface$container,ControllerNameParser$parser,LoggerInterface$logger =null) |
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.
Why not just changing the type hint and make the parser optional?
ogizanagiFeb 26, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
Because the new class is available in theHttpKernel component, whereas this one is in the framework bundle, as well as theControllerNameParser.
I'm fine with this class as is in the framework. :) I just think having a base ControllerResolver usable with psr-11 shipped with the component makes sense.
nicolas-grekas commentedFeb 28, 2017
I'm not fan of the Psr11 prefix. What about ServiceLocatorControllerResolver? ContainerControllerResolver? ServiceControllerResolver? |
ogizanagi commentedFeb 28, 2017
Why not. What is your own preference? What do others think? I like |
stof commentedFeb 28, 2017
I prefer ContainerControllerResolver actually. |
ogizanagi commentedFeb 28, 2017
Renamed to |
fabpot left a comment
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.
👍 (minor comment)
| /** | ||
| * @param ContainerInterface $container A psr-11 container instance | ||
| * @param LoggerInterface|null $logger | ||
| */ |
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.
The docblock can be removed
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.
👍 Fixed
fabpot commentedFeb 28, 2017
Thank you@ogizanagi. |
…) (ogizanagi)This PR was merged into the 3.3-dev branch.Discussion----------[HttpKernel] Add a ContainerControllerResolver (psr-11)| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | yes| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets | N/A| License | MIT| Doc PR | N/AExtracts the controller as service resolution from the framework bundle controller resolver in a `Symfony/Component/HttpKernel/Controller/Psr11ControllerResolver`, allowing you to use `HttpKernel` with your own psr-11 container.Commits-------7bbae41 [HttpKernel] Add a ContainerControllerResolver (psr-11)
Uh oh!
There was an error while loading.Please reload this page.
Extracts the controller as service resolution from the framework bundle controller resolver in a
Symfony/Component/HttpKernel/Controller/Psr11ControllerResolver, allowing you to useHttpKernelwith your own psr-11 container.