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] Expose the AbstractController's container to its subclasses#23180
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
nicolas-grekas 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.
Even if we usually don't encourage protected properties, I'm 👍 here:
extendingAbstractController::getSubscribedServices() is a legitimate use case that's really missing in 3.3.
Extending setContainer or using ControllerTrait as an alternative is unsupported because both are internal.
Using service argument injection doesn't fit the really-lazy use case (when eg. you first have some authorization check in an action before using a service.)
We could also add get/has methods. Not sure if that'd be better thought. Hence my vote.
| use ControllerTrait; | ||
| private$container; | ||
| protected$container; |
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.
Pleas add a docblock comment to help IDE with the type of this.
Useful if an application provides their own base Controller thatreferences items in the container. It also makes it simpler for thatbase controller to add additional optional dependencies by only overridinggetSubscribedServices instead of having to reimplement setContainer anduse ControllerTrait.
BPScott commentedJun 15, 2017
Added the docblock. Adding the get/has methods would also solve my use-case. Am happy to add those too or in place of changing the visibility if desired.Controller does both (protected $container comes via ContainerAwareTrait). Laziness is exactly the use case I'm aiming for.This is what I'm trying to do - my base controller will contain various utility methods, and I don't want the overhead of creating the services used by those utilities until I know I need them. |
fabpot commentedJun 15, 2017
Thank you@BPScott. |
…r to its subclasses (BPScott)This PR was merged into the 3.3 branch.Discussion----------[FrameworkBundle] Expose the AbstractController's container to its subclasses| Q | A| ------------- | ---| Branch? | 3.3| Bug fix? | no| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets | n/a| License | MIT| Doc PR | n/aThis is useful if an application provides their own base Controller thatreferences items in the container. It also makes it simpler for thatbase controller to add additional optional dependencies by only overridinggetSubscribedServices instead of having to reimplement setContainer anduse ControllerTrait.Commits-------ee17131 Expose the AbstractController's container to its subclasses
This is useful if an application provides their own base Controller that
references items in the container. It also makes it simpler for that
base controller to add additional optional dependencies by only overriding
getSubscribedServices instead of having to reimplement setContainer and
use ControllerTrait.