Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Command] No need to register the command manually #10154

Merged
merged 1 commit into from Oct 8, 2018

Conversation

weaverryan
Copy link
Member

Hey guys!

Basically, when instantiating the Application from FrameworkBundle, you do not need to manually add the command manually. We also had a second, duplicated section that talked about testing with the kernel/container... but we do that already in the first code block. I think just got out of sync over years of updating & refactoring. I did test that we do NOT need to manually instantiate/add the command and we do NOT need to call $kernel->boot().

@xabbuh
Copy link
Member

xabbuh commented Aug 6, 2018

But this will fail with Symfony 4 where commands are not auto-disccovered, won't it?

@weaverryan
Copy link
Member Author

@xabbuh No.... right? Because, we're booting the real kernel and passing it into Application (from FrameworkBundle). This simply mirrors what bin/console does: https://github.com/symfony/recipes/blob/29e686cb133652385d442df7c178e90907e35b9c/symfony/console/3.3/bin/console#L38

So, we should have the exact commands you'd have in a real app. As long is properly recognized as a command (e.g. autoconfigure or manual wiring), it should be here. Right?

@chalasr
Copy link
Member

chalasr commented Sep 9, 2018

Hey. Assuming the command has been registered as a service, @weaverryan is right here, no need for adding twice to the application as it has been injected during container compilation in the kernel boot process.
This chapter does not include the explicit service definition but includes a mention saying that the command must be registered as a service, so I think it's safe to assume it is.
Same for the kernel->boot() call, it's indeed not necessary.
👍 all good for me

@javiereguiluz javiereguiluz added this to the 3.4 milestone Oct 8, 2018
@javiereguiluz
Copy link
Member

Thanks Ryan.

@javiereguiluz javiereguiluz merged commit e0484d6 into symfony:3.4 Oct 8, 2018
javiereguiluz added a commit that referenced this pull request Oct 8, 2018
…erryan)

This PR was merged into the 3.4 branch.

Discussion
----------

[Command] No need to register the command manually

Hey guys!

Basically, when instantiating the `Application` from FrameworkBundle, you do not need to manually add the command manually. We also had a second, duplicated section that talked about testing with the kernel/container... but we do that already in the first code block. I think just got out of sync over years of updating & refactoring. I did test that we do NOT need to manually instantiate/add the command and we do NOT need to call `$kernel->boot()`.

Commits
-------

e0484d6 You do not need to re-add the command
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants