Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[HttpKernel] Deprecated commands auto-registration#23805
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
GuilhemN commentedAug 6, 2017
Looks like we need to update the DoctrineBundle first. |
UPGRADE-3.4.md Outdated
| HttpKernel | ||
| ---------- | ||
| * Relying on commands auto-discovery has been deprecated and won't be supported |
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 putconvention-based?
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.
is it better like that? :)
UPGRADE-4.0.md Outdated
| HttpKernel | ||
| ---------- | ||
| * Relying on commands auto-discovery has been deprecated and won't be supported |
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.
is not supported anymore
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
| # explicit commands registration | ||
| AppBundle\Command: | ||
| resource: '../../src/AppBundle/Command/*' | ||
| tags: ['console.command'] |
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 required when using auto configuration. Given thedefault config that'll be promoted, should we really add this before/after at all? Or at least mention it's not required when using the new default config?
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 for people upgrading from the standard edition < 3.3, the others won't get a deprecation so I don't think they will even notice the change.
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.
Fair point 👍
| } | ||
| $r =new \ReflectionClass($class); | ||
| if ($r->isSubclassOf('Symfony\\Component\\Console\\Command\\Command') && !$r->isAbstract() && !$r->getConstructor()->getNumberOfRequiredParameters()) { | ||
| @trigger_error(sprintf('Auto-registration of the command "%s" is deprecated since Symfony 3.4 and won\'t be supported in 4.0. Use PSR-4 based Service Discovery instead.',$class),E_USER_DEPRECATED); |
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.
is using PSR4 registration enough? that won't remove that notice, isn't it? people need also to override the registerCommands method in their bundle?
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 need as long as they register all their commands as services.
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.
enough indeed, registering the command as a service makes convention based skipped thanks to theconsole.command.ids parameter
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 don't think "Service Discovery" should be written with capital letters as it doesn't reference a fixed term or feature name.
UPGRADE-4.0.md Outdated
| services: | ||
| # ... | ||
| # implicit commands registration |
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 add "implicit registration of all commands in theCommand folder" for clarity
UPGRADE-3.4.md Outdated
| ---------- | ||
| * Relying on convention-based commands discovery has been deprecated and | ||
| won't be supported in 4.0. Use PSR-4 based service siscovery instead. |
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.
Typo :) (discovery)
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.
Oops indeed :)
| # to be removed once https://github.com/doctrine/DoctrineBundle/pull/684 is merged | ||
| services: | ||
| Doctrine\Bundle\DoctrineBundle\Command\: |
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 temporary :) This way this PR is mergeable.
UPGRADE-3.4.md Outdated
| services: | ||
| # ... | ||
| # implicit commands registration |
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 should be updated as in the upgrade 4.0 file
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
…ilhemN)This PR was squashed before being merged into the 3.4 branch (closes#23805).Discussion----------[HttpKernel] Deprecated commands auto-registration| Q | A| ------------- | ---| Branch? | 3.4| Bug fix? | no| New feature? | no <!-- don't forget updating src/**/CHANGELOG.md files -->| BC breaks? | no| Deprecations? | yes <!-- don't forget updating UPGRADE-*.md files -->| Tests pass? | yes| Fixed tickets |#23488| License | MIT| Doc PR |Deprecates commands auto-registration. See#23488 for arguments.Commits-------14215d8 [HttpKernel] Deprecated commands auto-registration
Tobion commentedAug 9, 2017
Thank you@GuilhemN |
…emN)This PR was squashed before being merged into the 3.4 branch (closes#8269).Discussion----------[Console] Commands auto-registration is deprecatedDocumentsymfony/symfony#23805.I wonder, should I updatehttps://github.com/symfony/symfony-docs/blob/17d1c39298e9415886e2c712748d3021ed7865a4/bundles/best_practices.rst L122 (`Command/ Yes`) ? As it is not really mandatory to use the `Command/` directory anymore.Commits-------493ae89 [Console] Commands auto-registration is deprecated
Deprecates commands auto-registration. See#23488 for arguments.