Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

[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

Closed
realityking wants to merge1 commit intosymfony:masterfromrealityking:console_dic

Conversation

@realityking
Copy link
Contributor

QA
Bug fix?no
New feature?yes
BC breaks?no
Deprecations?no
Tests pass?yes
Fixed tickets-
LicenseMIT
Doc PR-

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:

  • Get feedback whether the code remaining in FrameworkBundle is actually necessary.

Copy link
Contributor

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

Copy link
ContributorAuthor

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
Copy link
Member

Do you have an example of application using both theConsole andDependencyInjection components without the fullstack framework ?

I would give advantage to a proper dependency injection instead of promotingContainerAware things. Like what I've done for the commandtwig:lint (#9855).

@realityking
Copy link
ContributorAuthor

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
Copy link
Member

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
Copy link
ContributorAuthor

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
Copy link
ContributorAuthor

I did my homework, there are a few open source projects that have a copy of this code in their repository:

@fabpot
Copy link
Member

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.

Copy link
Contributor

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? 👶

Copy link
ContributorAuthor

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
Copy link
ContributorAuthor

@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
Copy link
Member

@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
Copy link
Contributor

@GromNaN I do have some CLI projects which use the DIC, however I've never injected the container in my commands...

@GromNaN
Copy link
Member

@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.
Not injecting the container is great ! So you don't need theContainerAwareCommand proposed in this PR.

@gnugat
Copy link
Contributor

@GromNaN nobody needs theContainerAwareCommand when using the Console and the DIC without FrameworkBundle. As@fabpot said, implementing this interface only have sense when usinghttps://github.com/symfony/symfony/blob/master/src/Symfony/Bundle/FrameworkBundle/Console/Application.php#L80-L84.

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 needContainerAwareCommand.

@fabpot
Copy link
Member

Closing now.

@fabpotfabpot closed thisJul 25, 2014
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@realityking@GromNaN@stof@fabpot@gnugat@staabm@cordoval

[8]ページ先頭

©2009-2025 Movatter.jp