Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[WIP][Console] Add ContainerAwareCommand#10087
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.
You have to either usesetContainer or make$container protected
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, overlooked that one. I'll wait for the feedback if we still need this piece of code before fixing it.
GromNaN commentedJan 20, 2014
Do you have an example of application using both the I would give advantage to a proper dependency injection instead of promoting |
realityking commentedJan 20, 2014
Motivation is our in house application that is not open source. Until there's a way to lazy load services for commands we're not ready to go DIC for them as we have some frequently running cronjobs using the console. Another application would be phpBB, but as far as I can tell they're using dependency injection for their commands. I'd agree that this is probably a rare set up, so this is probably rather unattractive if ContainerAwareCommand can't be removed completely from FrameworkBundle. |
stof commentedJan 20, 2014
I don't see the benefit of adding this class in the Console component: the container won't be injected in the command unless you use an Application implementation supporting it, which is not in the component either. and the class in FrameworkBundle cannot be removed for BC reasons |
realityking commentedJan 20, 2014
I should have worded that last part more carefully. We obviously need to retain the class in the FrameworkBundle for 2.x. I meant if we can remove all the code in it so we can eventually remove it. |
realityking commentedJan 26, 2014
I did my homework, there are a few open source projects that have a copy of this code in their repository: |
fabpot commentedMar 3, 2014
I also fail to see the benefit of such a class in the Console component as the most important code to make it work transparently is still in the framework bundle:https://github.com/symfony/symfony/blob/master/src/Symfony/Bundle/FrameworkBundle/Console/Application.php#L80-L84. |
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.
he is not the author, or add yourself too right? 👶
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.
Well since it's essentially a copy ofSymfony\Bundle\FrameworkBundle\Command\ContainerAwareCommand plus a small modification togetContainer so that only seemed fair.
realityking commentedMar 3, 2014
@fabpot I understand where you're coming from, as this is "some assembly required". As mentioned above, this only makes sense if we can remove this condition:https://github.com/realityking/symfony/blob/master/src/Symfony/Bundle/FrameworkBundle/Command/ContainerAwareCommand.php#L35 I think it's safe, but I'd like someone with more knowledge of the ins and outs to confirm that. |
fabpot commentedMar 26, 2014
@realityking The code you mention can be safely removed, but again, you would still need the code from the FrameworkBundle Application class to make this work "out of the box". |
gnugat commentedApr 13, 2014
@GromNaN I do have some CLI projects which use the DIC, however I've never injected the container in my commands... |
GromNaN commentedApr 13, 2014
@gnugat I'm using a lot the Console standalone or with Pimple and I regret that so many open-source Commands are tied to the Symfony/DI component. |
gnugat commentedApr 13, 2014
@GromNaN nobody needs the The Console's default Application doesn't inject the container to commands which implement this interface.@realityking If you want to inject the container into your commands, you don't need |
fabpot commentedJul 25, 2014
Closing now. |
In the spirit of#7435 I propose adding a ContainerAwareCommand to the Console Component. This is helpful when implementing both the DIC and the Console.
I'm fairly certain the code remaining in ContainerAwareCommand in the FrameworkBundle could also be dropped as the Container is injected into every command by the Application. This would also make it a little easier to move code between Projects using the Framework and those using only the Components.
Todo: