Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[FrameworkBundle] Return the invokable service if its name is the class name#18289
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
| $controller =$this->container->get($class); | ||
| }else { | ||
| $controller =parent::instantiateController($class); | ||
| } |
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 we should make that check increateController() where we also retrieve the controller from the container when theservice_name:method_name notation is used:https://github.com/dunglas/symfony/blob/invokable_service/src/Symfony/Bundle/FrameworkBundle/Controller/ControllerResolver.php#L56-L71
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 is not possible without a larger refactoring becausecreateController is never called in this case:https://github.com/symfony/symfony/blob/master/src/Symfony/Component/HttpKernel/Controller/ControllerResolver.php#L70
An alternative is to refactor the wholegetController method but it's risky and intrusive. It's why I've chosen this simple fix.
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.
Hm, yeah, it's probably not worth to make such big changes.
xabbuh commentedMar 25, 2016
👍 (with@stof's comment applied) |
| { | ||
| $controller =parent::instantiateController($class); | ||
| if ($this->container->has($class)) { | ||
| $controller =$this->container->get($class); |
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 simple way to fix my comment about ContainerAwareInterface is to return directly here
stof commentedMar 25, 2016
Status: needs work |
dunglas commentedMar 25, 2016
@stof fixed Status: needs review |
fabpot commentedMar 25, 2016
Thank you@dunglas. |
dunglas commentedMar 25, 2016
I've unintentionally opened this PR against master, but it should be merged in 2.7 (as described in the PR header).@fabpot is it possible to merge it back in other branches? |
… name is the class name (dunglas)This PR was merged into the 2.7 branch.Discussion----------[FrameworkBundle][2.7] Return the invokable service if its name is the class name| Q | A| ------------- | ---| Branch? | 2.7| Bug fix? | yes| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets | n/A| License | MIT| Doc PR | n/aBackport#18289 to 2.7 as this is a bug fix.Commits-------5c87d76 [FrameworkBundle] Return the invokable service if its name is the class name
if a service is invokable and has the same name than its class name, the controller resolver of FrameworkBundle doesn't retrieve the service and tries to construct a new instance of the class instead.
This is a very rare edge case, but this fix is useful fordunglas/DunglasActionBundle#36: referencing auto-registered controllers following the ADR style in YAML and XML routing files will be more intuitive.
Currently:
defaults: { _controller: 'Your\Action\FQN:__invoke' }, after this fix:defaults: { _controller: 'Your\Action\FQN' }.This PR also fix a currently useless test.