Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Console] Allow to register commands privately#18101
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
unkind commentedMar 10, 2016
If you want to register your command as private service, you don't need to add "console.command" tag, because command won't work with standard interface. |
GuilhemN commentedMar 10, 2016
What do you mean@unkind? |
unkind commentedMar 10, 2016
Console application from the FrameworkBundle requires public services:https://github.com/symfony/symfony/blob/master/src/Symfony/Bundle/FrameworkBundle/Console/Application.php#L123 |
GuilhemN commentedMar 10, 2016
Did you take a look at the changes ? A public alias is always registered with a command so I thought it would be better to use it instead of its original service id. |
unkind commentedMar 10, 2016
My bad, didn't notice an alias, sorry. |
dunglas commentedMar 11, 2016
👍 |
xabbuh commentedMar 11, 2016
Looks sensible to me. 👍 |
GuilhemN commentedMar 11, 2016
BTW we can also do that for the event listeners and some other tags but we would need to create aliases (whereas here as they already exist), should I include it in this PR? |
wouterj commentedMar 12, 2016
In order to get things merged quickly, I think it's better to create a new PR for the other tags. However, I think it'll be great to allow private services for listeners and such. |
wouterj commentedMar 12, 2016
Status: Reviewed |
GuilhemN commentedMar 12, 2016
ok got it@wouterj I'll do it. |
GuilhemN commentedMar 12, 2016
see#18116 |
GuilhemN commentedMar 13, 2016
wouterj commentedMar 13, 2016
@Ener-Getick that's correct. Please be patient, the mergers often scan through the Status: Reviewed PRs to merge new PRs. However, they are still volunteers and have a huge amount of issues and PRs to manage :) |
GuilhemN commentedMar 13, 2016
@wouterj sorry as I don't know how you manage the PRs/issues (it could be the subject of an article btw) and as I know how easy it is to forget a notification, I thought it would be better to ping them directly :-) |
wouterj commentedMar 13, 2016
Don't worry, you're doing a great job! Managing PRs is not much different than how regular contributors do it: Scanning through the list on github and finding PRs ready to merge/vote/review. As a side note, I'm not a Symfony Core Team Member, just a documentor :) |
| $definition =newDefinition('Symfony\Bundle\FrameworkBundle\Tests\DependencyInjection\Compiler\MyCommand'); | ||
| $definition->addTag('console.command'); | ||
| $definition->setPublic(false); |
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.
Maybe I'm missing something but, shouldn't you keep the$definition->setPublic(false); line to test that private commands don't trigger an error?
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 removed it as the original definitions aren't used anymore (so there is not risk to have a private service) but in case this change in the future you're right that's better to keep this test 👍
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 added a@dataProvider to the first test case to avoid duplication.
nicolas-grekas commentedMar 15, 2016
👍 |
fabpot commentedMar 15, 2016
Thank you @Ener-Getick. |
…etick)This PR was merged into the 3.1-dev branch.Discussion----------[Console] Allow to register commands privately| Q | A| ------------- | ---| Bug fix? | no| New feature? | yes| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets || License | MIT| Doc PR |I'm not sure if this should be considered as a bug or a feature.It allows to declare command services as private (the command alias is used). I don't see a good reason to force the user to declare his services as public as the limitation is more internal than in his own code.Commits-------147eb79 Allow to register commands privately
dunglas commentedMay 31, 2016
@Ener-Getick it looks like this PR introduces a BC break:https://travis-ci.org/dunglas/DunglasActionBundle/builds/134206892 |
dunglas commentedMay 31, 2016
I'm working on a fix. |
GuilhemN commentedMay 31, 2016
@dunglas imo this is more an implementation detail and is easy to fix in your test: $commandId ='foo\bar'; becomes: if (method_exists(Controller::class,'json')) {$commandId ='console.command.foo_bar';}else {$commandId ='foo\bar';} |
dunglas commentedMay 31, 2016
@Ener-Getick it's easy to fix in my bundle it's true, but this is a BC break that can affect other users. IMO it should be fixed in Symfony. |
This PR was squashed before being merged into the 3.1 branch (closes#18925).Discussion----------[Console] Fix BC break introduced by#18101| Q | A| ------------- | ---| Branch? | 3.1| Bug fix? | yes| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | no| Fixed tickets |#18101 (comment)| License | MIT| Doc PR | n/a* [x] Fix testsCommits-------7a5a139 [Console] Fix BC break introduced by#18101
* 3.1: [travis] Don't use parallel on HHVM [HttpKernel] Fix RequestDataCollector starting the session [appveyor] Ignore STATUS_HEAP_CORRUPTION errors on Windows [FrameworkBundle] Skip redis cache pools test on failed connection Fixed forwarded request data in templates [Security] Fix DebugAccessDecisionManager when object is not a scalar Skip some tests on HHVM due to a PHPunit bug Use the Trusty Travis infrastructure for HHVM builds LdapUserProvider: add missing argument type doc Fixed issue with missing argument in the abstract service definition for the ldap user provider Add 3.1 to PR template branch row, remove 2.3 Improve memory efficiency [Console] Fix BC break introduced by#18101 document method name changes in Voter class add missing hint for vote() argument type [#18838] add a test to avoid regressions bumped Symfony version to 3.1.1 updated VERSION for 3.1.0 updated CHANGELOG for 3.1.0Conflicts:src/Symfony/Component/HttpKernel/Kernel.php
* 3.1: [travis] Don't use parallel on HHVM [HttpKernel] Fix RequestDataCollector starting the session [appveyor] Ignore STATUS_HEAP_CORRUPTION errors on Windows [FrameworkBundle] Skip redis cache pools test on failed connection Fixed forwarded request data in templates [Security] Fix DebugAccessDecisionManager when object is not a scalar Skip some tests on HHVM due to a PHPunit bug Use the Trusty Travis infrastructure for HHVM builds LdapUserProvider: add missing argument type doc Fixed issue with missing argument in the abstract service definition for the ldap user provider Add 3.1 to PR template branch row, remove 2.3 Improve memory efficiency [Console] Fix BC break introduced bysymfony#18101 document method name changes in Voter class add missing hint for vote() argument type [symfony#18838] add a test to avoid regressions bumped Symfony version to 3.1.1 updated VERSION for 3.1.0 updated CHANGELOG for 3.1.0Conflicts:src/Symfony/Component/HttpKernel/Kernel.php
I'm not sure if this should be considered as a bug or a feature.
It allows to declare command services as private (the command alias is used). I don't see a good reason to force the user to declare his services as public as the limitation is more internal than in his own code.