[HttpKernel] Deprecated commands auto-registration #23805

GuilhemN · 2017-08-06T12:18:41Z

Q	A
Branch?	3.4
Bug fix?	no
New feature?	no
BC breaks?	no
Deprecations?	yes
Tests pass?	yes
Fixed tickets	#23488
License	MIT
Doc PR

Deprecates commands auto-registration. See #23488 for arguments.

GuilhemN · 2017-08-06T12:24:46Z

Looks like we need to update the DoctrineBundle first.

chalasr · 2017-08-06T12:35:22Z

UPGRADE-3.4.md

+HttpKernel
+----------
+
+ * Relying on commands auto-discovery has been deprecated and won't be supported


should we put convention-based?

is it better like that? :)

chalasr · 2017-08-06T12:35:48Z

UPGRADE-4.0.md

@@ -452,6 +452,32 @@ HttpFoundation
 HttpKernel
 ----------

+ * Relying on commands auto-discovery has been deprecated and won't be supported


is not supported anymore

ogizanagi · 2017-08-06T12:46:48Z

UPGRADE-3.4.md

+       # explicit commands registration
+       AppBundle\Command:
+           resource: '../../src/AppBundle/Command/*'
+           tags: ['console.command']


Not required when using auto configuration. Given the default 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?

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.

Fair point 👍

nicolas-grekas · 2017-08-08T06:29:06Z

src/Symfony/Component/HttpKernel/Bundle/Bundle.php

@@ -191,6 +191,8 @@ public function registerCommands(Application $application)
            }
            $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);


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?

No need as long as they register all their commands as services.

enough indeed, registering the command as a service makes convention based skipped thanks to the console.command.ids parameter

I don't think "Service Discovery" should be written with capital letters as it doesn't reference a fixed term or feature name.

Tobion · 2017-08-08T23:44:03Z

UPGRADE-4.0.md

+   services:
+       # ...
+
+       # implicit commands registration


maybe add "implicit registration of all commands in the Command folder" for clarity

ogizanagi · 2017-08-09T10:58:32Z

UPGRADE-3.4.md

+----------
+
+ * Relying on convention-based commands discovery has been deprecated and
+   won't be supported in 4.0. Use PSR-4 based service siscovery instead.


Typo :) (discovery)

Oops indeed :)

GuilhemN · 2017-08-09T11:02:41Z

src/Symfony/Bundle/SecurityBundle/Tests/Functional/app/Acl/config.yml

+
+# to be removed once https://github.com/doctrine/DoctrineBundle/pull/684 is merged
+services:
+    Doctrine\Bundle\DoctrineBundle\Command\:


This is temporary :) This way this PR is mergeable.

Tobion · 2017-08-09T11:34:30Z

UPGRADE-3.4.md

+   services:
+       # ...
+
+       # implicit commands registration


this should be updated as in the upgrade 4.0 file

…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  | BC breaks? | no | Deprecations? | yes  | 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 · 2017-08-09T16:55:43Z

Thank you @GuilhemN

…emN) This PR was squashed before being merged into the 3.4 branch (closes #8269). Discussion ---------- [Console] Commands auto-registration is deprecated Document symfony/symfony#23805. I wonder, should I update https://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

[HttpKernel] Deprecated commands auto-registration

0ee4026

carsonbot added Status: Needs Review HttpKernel Deprecation labels Aug 6, 2017

GuilhemN changed the base branch from master to 3.4 August 6, 2017 12:19

chalasr reviewed Aug 6, 2017

View reviewed changes

ogizanagi reviewed Aug 6, 2017

View reviewed changes

nicolas-grekas added this to the 3.4 milestone Aug 6, 2017

GuilhemN mentioned this pull request Aug 6, 2017

Register commands using PSR-4 based Service Discovery doctrine/DoctrineBundle#684

Closed

fixes

669d344

ogizanagi approved these changes Aug 6, 2017

View reviewed changes

chalasr approved these changes Aug 7, 2017

View reviewed changes

nicolas-grekas reviewed Aug 8, 2017

View reviewed changes

nicolas-grekas approved these changes Aug 8, 2017

View reviewed changes

Tobion reviewed Aug 8, 2017

View reviewed changes

GuilhemN force-pushed the implicitcommands branch from a8930bd to bd315bc Compare August 9, 2017 10:58

ogizanagi reviewed Aug 9, 2017

View reviewed changes

Service Discovery -> service discovery

1da8b12

GuilhemN force-pushed the implicitcommands branch from bd315bc to 3164ab8 Compare August 9, 2017 10:59

GuilhemN commented Aug 9, 2017

View reviewed changes

Tobion reviewed Aug 9, 2017

View reviewed changes

GuilhemN force-pushed the implicitcommands branch from 3164ab8 to 882a360 Compare August 9, 2017 11:57

Fix tests

01bf469

GuilhemN force-pushed the implicitcommands branch from 882a360 to 01bf469 Compare August 9, 2017 12:03

Tobion approved these changes Aug 9, 2017

View reviewed changes

Tobion closed this Aug 9, 2017

chalasr mentioned this pull request Aug 9, 2017

Deprecate auto-discovery of commands in their directories #23488

Closed

GuilhemN deleted the implicitcommands branch August 9, 2017 17:04

GuilhemN mentioned this pull request Aug 9, 2017

[Console] Commands auto-registration is deprecated symfony/symfony-docs#8269

Closed

chalasr mentioned this pull request Aug 10, 2017

[Console] Add documentation about the console.command "command" attribute (lazy commands) symfony/symfony-docs#8147

Closed

This was referenced Oct 18, 2017

Release v3.4.0-BETA1 #24609

Merged

Release v4.0.0-BETA1 #24612

Merged

kevinpapst mentioned this pull request Jan 12, 2018

Load commands explicit as auto-dicovery way deprecated in Symfony 3.4 avanzu/AdminThemeBundle#215

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[HttpKernel] Deprecated commands auto-registration #23805

[HttpKernel] Deprecated commands auto-registration #23805

GuilhemN commented Aug 6, 2017

GuilhemN commented Aug 6, 2017

chalasr Aug 6, 2017

GuilhemN Aug 6, 2017

chalasr Aug 6, 2017

GuilhemN Aug 6, 2017

ogizanagi Aug 6, 2017 •

edited

Loading

GuilhemN Aug 6, 2017

ogizanagi Aug 6, 2017

nicolas-grekas Aug 8, 2017

GuilhemN Aug 8, 2017

chalasr Aug 8, 2017 •

edited

Loading

Tobion Aug 8, 2017

Tobion Aug 8, 2017

ogizanagi Aug 9, 2017

GuilhemN Aug 9, 2017

GuilhemN Aug 9, 2017 •

edited

Loading

Tobion Aug 9, 2017

GuilhemN Aug 9, 2017

Tobion commented Aug 9, 2017

[HttpKernel] Deprecated commands auto-registration #23805

[HttpKernel] Deprecated commands auto-registration #23805

Conversation

GuilhemN commented Aug 6, 2017

GuilhemN commented Aug 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ogizanagi Aug 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chalasr Aug 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuilhemN Aug 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tobion commented Aug 9, 2017

ogizanagi Aug 6, 2017 •

edited

Loading

chalasr Aug 8, 2017 •

edited

Loading

GuilhemN Aug 9, 2017 •

edited

Loading