[Console] Allow to register commands privately #18101
Conversation
|
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. |
|
What do you mean @unkind? |
|
Console application from the FrameworkBundle requires public services: https://github.com/symfony/symfony/blob/master/src/Symfony/Bundle/FrameworkBundle/Console/Application.php#L123 |
|
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. |
|
My bad, didn't notice an alias, sorry. |
|
👍 |
|
Looks sensible to me. 👍 |
|
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? |
|
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. |
|
Status: Reviewed |
|
ok got it @wouterj I'll do it. |
|
see #18116 |
|
@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 :) |
|
@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 :-) |
|
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 = new Definition('Symfony\Bundle\FrameworkBundle\Tests\DependencyInjection\Compiler\MyCommand'); | ||
| $definition->addTag('console.command'); | ||
| $definition->setPublic(false); |
There was a problem hiding this comment.
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.
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.
I added a @dataProvider to the first test case to avoid duplication.
|
👍 |
|
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
|
@Ener-Getick it looks like this PR introduces a BC break: https://travis-ci.org/dunglas/DunglasActionBundle/builds/134206892 |
|
I'm working on a fix. |
|
@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';
} |
|
@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 tests Commits ------- 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.0 Conflicts: 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 by symfony#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.0 Conflicts: 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.