[HttpKernel] Remove convention based commands registration #23857
Conversation
chalasr
commented
Aug 10, 2017
| Q | A |
|---|---|
| Branch? | master |
| Bug fix? | no |
| New feature? | no |
| BC breaks? | yes |
| Deprecations? | no |
| Tests pass? | yes |
| Fixed tickets | n/a |
| License | MIT |
| Doc PR | n/a |
| return; | ||
| } | ||
|
|
||
| if (!class_exists('Symfony\Component\Finder\Finder')) { |
There was a problem hiding this comment.
the dep can be removed from composer.json
There was a problem hiding this comment.
it is still used elsewhere
There was a problem hiding this comment.
nevermind, I thought I was in FrameworkBundle, done
c33a1b1 to
2090f07
Compare
| @@ -59,20 +38,6 @@ public function testGetContainerExtensionWithInvalidClass() | |||
| $bundle->getContainerExtension(); | |||
| } | |||
|
|
|||
| public function testHttpKernelRegisterCommandsIgnoresCommandsThatAreRegisteredAsServices() | |||
There was a problem hiding this comment.
should probably have been flagged as legacy in 3.4. Do we care?
There was a problem hiding this comment.
Imo we don't care, no deprecation is triggered here anyway.
| * | ||
| * * Commands are in the 'Command' sub-directory | ||
| * * Commands extend Symfony\Component\Console\Command\Command | ||
| * Registers console commands. | ||
| * | ||
| * @param Application $application An Application instance | ||
| */ | ||
| public function registerCommands(Application $application) |
There was a problem hiding this comment.
overrides can be removed i guess
There was a problem hiding this comment.
this is not an override. It is the base definition
There was a problem hiding this comment.
TwigBundle::registerCommands etc. They are now noop.
There was a problem hiding this comment.
This is not good if you don't change the min version to forbid using TwigBundle 4.0 with HttpKernel 3.4 and the no-op was important though
There was a problem hiding this comment.
indeed, no-ops re-added in bundles
| } | ||
| $class = $ns.'\\'.$file->getBasename('.php'); | ||
| if ($this->container) { | ||
| $commandIds = $this->container->hasParameter('console.command.ids') ? $this->container->getParameter('console.command.ids') : array(); |
There was a problem hiding this comment.
should we deprecate this param as well? console.command.ids
There was a problem hiding this comment.
no, it is still used to filter out lazy commands in framework Application
There was a problem hiding this comment.
i see... but after #23796 we could right?
There was a problem hiding this comment.
not sure, implementing it would be optional so non lazy command services would still be supported. To be rediscussed at/after it :)
b1b91e3 to
6959dfa
Compare
|
Ready to me. |
| @@ -31,7 +31,6 @@ | |||
| "symfony/dependency-injection": "~3.4|~4.0", | |||
| "symfony/dom-crawler": "~3.4|~4.0", | |||
| "symfony/expression-language": "~3.4|~4.0", | |||
| "symfony/finder": "~3.4|~4.0", | |||
There was a problem hiding this comment.
there is one more in "suggest"
6959dfa to
cc1695c
Compare
| * @group legacy | ||
| * @expectedDeprecation Auto-registration of the command "Symfony\Component\HttpKernel\Tests\Fixtures\ExtensionPresentBundle\Command\FooCommand" is deprecated since Symfony 3.4 and won't be supported in 4.0. Use PSR-4 based service discovery instead. | ||
| */ | ||
| public function testRegisterCommands() |
There was a problem hiding this comment.
I would keep this test and assert no command is registered for ExtensionPresentBundle.
There was a problem hiding this comment.
not sure that testing a no-op is useful at all, we don't for others like build(). Already covered by Application tests anyway
There was a problem hiding this comment.
Here that could prevent mistakes due to merging conflicts.
| @@ -59,20 +38,6 @@ public function testGetContainerExtensionWithInvalidClass() | |||
| $bundle->getContainerExtension(); | |||
| } | |||
|
|
|||
| public function testHttpKernelRegisterCommandsIgnoresCommandsThatAreRegisteredAsServices() | |||
There was a problem hiding this comment.
Imo we don't care, no deprecation is triggered here anyway.
|
Thanks Robin. |
…tion (chalasr) This PR was merged into the 4.0-dev branch. Discussion ---------- [HttpKernel] Remove convention based commands registration | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | no | BC breaks? | yes | Deprecations? | no | Tests pass? | yes | Fixed tickets | n/a | License | MIT | Doc PR | n/a Commits ------- cc1695c [HttpKernel] Remove convention based commands registration
|
Awesome job @chalasr ! |
|
@TomasVotruba Thanks, but that's community work! This was the funniest part :) |