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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Deprecate auto-discovery of commands in their directories #23488

Closed
nicolas-grekas opened this Issue Jul 12, 2017 · 13 comments

Comments

Projects
None yet
6 participants
@nicolas-grekas
Member

nicolas-grekas commented Jul 12, 2017

Q A
Bug report? no
Feature request? yes
BC Break report? no
RFC? yes
Symfony version 3.4

Now that commands can be lazy services very easily, we should deprecate their convention-based auto-discovery (the special "Command" folder in each bundles).

@javiereguiluz

This comment has been minimized.

Show comment
Hide comment
@javiereguiluz

javiereguiluz Jul 12, 2017

Member

What would be the implications of this change? Before, if I define commands in the Command/ folder of my bundle, everything works. What should I do now if I still have the same commands in the same folder? Thanks!

Member

javiereguiluz commented Jul 12, 2017

What would be the implications of this change? Before, if I define commands in the Command/ folder of my bundle, everything works. What should I do now if I still have the same commands in the same folder? Thanks!

@chalasr

This comment has been minimized.

Show comment
Hide comment
@chalasr

chalasr Jul 13, 2017

Member

@javiereguiluz Actually if you are using the 3.3 default container configuration, your commands would still be registered naturally and behave the same as before (thanks to the PSR-4 based discovery).

👍 This implies to register all core commands as services, btw make them container unaware as possible

Member

chalasr commented Jul 13, 2017

@javiereguiluz Actually if you are using the 3.3 default container configuration, your commands would still be registered naturally and behave the same as before (thanks to the PSR-4 based discovery).

👍 This implies to register all core commands as services, btw make them container unaware as possible

@lyrixx

This comment has been minimized.

Show comment
Hide comment
@lyrixx

lyrixx Jul 13, 2017

Member

I'm 👎 for this. It's really convenient for people that don't like PSR-4 base discovery.
More over it will break so many vendored bundles that relies on this feature.

Member

lyrixx commented Jul 13, 2017

I'm 👎 for this. It's really convenient for people that don't like PSR-4 base discovery.
More over it will break so many vendored bundles that relies on this feature.

@chalasr

This comment has been minimized.

Show comment
Hide comment
@chalasr

chalasr Jul 13, 2017

Member

@lyrixx that would not break until 4.0, just trigger deprecations as usual so that distributed/final bundles can register their commands as services beforehand (using PSR-4 discovery or full definitions).

Member

chalasr commented Jul 13, 2017

@lyrixx that would not break until 4.0, just trigger deprecations as usual so that distributed/final bundles can register their commands as services beforehand (using PSR-4 discovery or full definitions).

@lyrixx

This comment has been minimized.

Show comment
Hide comment
@lyrixx

lyrixx Jul 13, 2017

Member

Yes, I know that ;)

Member

lyrixx commented Jul 13, 2017

Yes, I know that ;)

@javiereguiluz

This comment has been minimized.

Show comment
Hide comment
@javiereguiluz

javiereguiluz Jul 16, 2017

Member

@chalasr thanks for the info. So, if you use the default 3.3 config, everything keeps working. What about applications that upgrade to 3.3, 3.4, etc. but don't want to use autowiring and the default Symfony config? Will commands break for those apps?

Member

javiereguiluz commented Jul 16, 2017

@chalasr thanks for the info. So, if you use the default 3.3 config, everything keeps working. What about applications that upgrade to 3.3, 3.4, etc. but don't want to use autowiring and the default Symfony config? Will commands break for those apps?

@chalasr

This comment has been minimized.

Show comment
Hide comment
@chalasr

chalasr Jul 16, 2017

Member

@javiereguiluz I think the plan should be to trigger a deprecation notice when a command is registered via this convention, we are able to detect that so no, it won't break in 3.x, only in 4.0, the upgrade path being to register the commands as a service (container aware commands are still ok). The PSR-4 loader should be the easiest way to upgrade for non-distributed bundles as it replicates the convention-based behaviour.

Member

chalasr commented Jul 16, 2017

@javiereguiluz I think the plan should be to trigger a deprecation notice when a command is registered via this convention, we are able to detect that so no, it won't break in 3.x, only in 4.0, the upgrade path being to register the commands as a service (container aware commands are still ok). The PSR-4 loader should be the easiest way to upgrade for non-distributed bundles as it replicates the convention-based behaviour.

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Jul 16, 2017

Member

There is no need for autowiring nor the default config to reason about this.
You can think of this as a proposal to turn a hard coded convention into an equivalent explicit config rule.
Let's say one wants to opt out completely from our default config, then there would be just one rule needed:

App\Command\:
  resource: ../src/Command
  tags: [console.command]

Which would be the explicit equivalent of the current behavior.
Symfony always prefers explicit and configuration over implicit and conventions. This is an opportunity to remove one of the last places where we are not aligned to our philosophy, and enhance flexibility and discoverability, since that's the benefit.

Member

nicolas-grekas commented Jul 16, 2017

There is no need for autowiring nor the default config to reason about this.
You can think of this as a proposal to turn a hard coded convention into an equivalent explicit config rule.
Let's say one wants to opt out completely from our default config, then there would be just one rule needed:

App\Command\:
  resource: ../src/Command
  tags: [console.command]

Which would be the explicit equivalent of the current behavior.
Symfony always prefers explicit and configuration over implicit and conventions. This is an opportunity to remove one of the last places where we are not aligned to our philosophy, and enhance flexibility and discoverability, since that's the benefit.

@javiereguiluz

This comment has been minimized.

Show comment
Hide comment
@javiereguiluz

javiereguiluz Jul 16, 2017

Member

@nicolas-grekas I agree with you about removing hardcoded things 👍


Unrelated to this, but related to hardcoding, maybe you can propose a solution on this issue: symfony/recipes#104 Apparently, the Kernel used in Flex apps has the config/ string hardcoded as the config dir location, so there's no simple way to override that. Thanks!

Member

javiereguiluz commented Jul 16, 2017

@nicolas-grekas I agree with you about removing hardcoded things 👍


Unrelated to this, but related to hardcoding, maybe you can propose a solution on this issue: symfony/recipes#104 Apparently, the Kernel used in Flex apps has the config/ string hardcoded as the config dir location, so there's no simple way to override that. Thanks!

fabpot added a commit that referenced this issue Jul 22, 2017

feature #23519 [TwigBundle] Commands as a service (ro0NL)
This PR was squashed before being merged into the 3.4 branch (closes #23519).

Discussion
----------

[TwigBundle] Commands as a service

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | yes
| Tests pass?   | yes
| Fixed tickets | #... <!-- #-prefixed issue number(s), if any -->
| License       | MIT
| Doc PR        | symfony/symfony-docs#... <!--highly recommended for new features-->

tiny step towards #23488

Commits
-------

9839140 [TwigBundle] Commands as a service
@iltar

This comment has been minimized.

Show comment
Hide comment
@iltar

iltar Jul 22, 2017

Contributor

Configuration over convention! 🎉

Contributor

iltar commented Jul 22, 2017

Configuration over convention! 🎉

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Aug 4, 2017

Member

Doing this would also fix a long standing issue with commands: right now, if a single command fails to instantiate, all commands are unusable. By making commands lazy, we would fix that, see eg. #22243

Member

nicolas-grekas commented Aug 4, 2017

Doing this would also fix a long standing issue with commands: right now, if a single command fails to instantiate, all commands are unusable. By making commands lazy, we would fix that, see eg. #22243

nicolas-grekas added a commit that referenced this issue Aug 6, 2017

feature #23624 [FrameworkBundle] Commands as a service (ro0NL)
This PR was squashed before being merged into the 3.4 branch (closes #23624).

Discussion
----------

[FrameworkBundle] Commands as a service

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | yes
| Tests pass?   | yes/no
| Fixed tickets | #... <!-- #-prefixed issue number(s), if any -->
| License       | MIT
| Doc PR        | symfony/symfony-docs#... <!--highly recommended for new features-->

Next step towards #23488

It's a work in progress if we want to do all commands at once (im fine :)). But i think we should review `assets:install` first.

Also im assuming framework commands can rely on `getApplication()->getKernel()` from the framework application (we already do that in some commands). That saves a dep on `@kernel`.

And filesystem as a service; perhaps drop that as well :)

Commits
-------

de1dc0b [FrameworkBundle] Commands as a service
@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Aug 6, 2017

Member

Unlocked now that #23624 is merged

Member

nicolas-grekas commented Aug 6, 2017

Unlocked now that #23624 is merged

Tobion added a commit that referenced this issue Aug 9, 2017

feature #23805 [HttpKernel] Deprecated commands auto-registration (Gu…
…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

symfony-splitter pushed a commit to symfony/http-kernel that referenced this issue Aug 9, 2017

feature #23805 [HttpKernel] Deprecated commands auto-registration (Gu…
…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 | symfony/symfony#23488
| License       | MIT
| Doc PR        |

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

Commits
-------

14215d8185 [HttpKernel] Deprecated commands auto-registration

symfony-splitter pushed a commit to symfony/security-bundle that referenced this issue Aug 9, 2017

feature #23805 [HttpKernel] Deprecated commands auto-registration (Gu…
…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 | symfony/symfony#23488
| License       | MIT
| Doc PR        |

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

Commits
-------

14215d8185 [HttpKernel] Deprecated commands auto-registration
@chalasr

This comment has been minimized.

Show comment
Hide comment
@chalasr

chalasr Aug 9, 2017

Member

Deprecated in #23805 🎉

Member

chalasr commented Aug 9, 2017

Deprecated in #23805 🎉

@chalasr chalasr closed this Aug 9, 2017

fabpot added a commit to symfony/swiftmailer-bundle that referenced this issue Sep 27, 2017

feature #187 Commands as a service (ro0NL)
This PR was squashed before being merged into the 3.0-dev branch (closes #187).

Discussion
----------

Commands as a service

See symfony/symfony#23488
Closes #189

Tend to leave these container aware for now.

Commits
-------

5934f84 Commands as a service
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment