Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.6k
[HttpKernel] Remove convention based commands registration#23857
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
chalasr commentedAug 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
the dep can be removed from composer.json
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.
it is still used elsewhere
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.
nevermind, I thought I was in FrameworkBundle, done
c33a1b1 to2090f07Compare| @@ -59,20 +38,6 @@ public function testGetContainerExtensionWithInvalidClass() | |||
| $bundle->getContainerExtension(); | |||
| } | |||
| public function testHttpKernelRegisterCommandsIgnoresCommandsThatAreRegisteredAsServices() | |||
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.
should probably have been flagged as legacy in 3.4. Do we care?
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
overrides can be removed i guess
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.
this is not an override. It is the base definition
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.
TwigBundle::registerCommands etc. They are now noop.
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.
done
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
should we deprecate this param as well?console.command.ids
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.
no, it is still used to filter out lazy commands in framework Application
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 see... but after#23796 we could right?
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.
not sure, implementing it would be optional so non lazy command services would still be supported. To be rediscussed at/after it :)
b1b91e3 to6959dfaCompareReady 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
there is one more in "suggest"
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.
done
6959dfa tocc1695cCompare| * @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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I would keep this test and assert no command is registered forExtensionPresentBundle.
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.
not sure that testing a no-op is useful at all, we don't for others likebuild(). Already covered by Application tests anyway
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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/aCommits-------cc1695c [HttpKernel] Remove convention based commands registration
Awesome job@chalasr ! |
@TomasVotruba Thanks, but that's community work! This was the funniest part :) |