Skip to content

[Console] Fix BC break in console.command.ids parameter #24073

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

Merged
merged 1 commit into from
Sep 8, 2017

Conversation

chalasr
Copy link
Member

@chalasr chalasr commented Sep 3, 2017

Q A
Branch? 3.4
Bug fix? no
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets symfony/flex#142 (comment)
License MIT
Doc PR n/a

To make command services lazy loaded when their name is known, we need to exclude them from runtime registration of other (non lazy-loadable) command services.
We also need to have them in the console.command.ids parameter in order to not register them by convention.
It is managed using false as values instead of ids for the parameter, excluding false values from runtime registration. Problem is that it makes FrameworkBundle's 3.3 Application incompatible with Console 3.4+ given the check for false is missing.

This PR fixes it using another parameter referencing ids of command services which can be lazy loaded.
I would be happy to not add the parameter if someone has a suggestion that allows to do so.

@chalasr chalasr added this to the 3.4 milestone Sep 3, 2017
@chalasr chalasr force-pushed the command-ids-bc branch 3 times, most recently from e1894a3 to 706b556 Compare September 3, 2017 08:05
@ogizanagi
Copy link
Contributor

Would it be acceptable to backport

as forward compatibility layer?

@chalasr
Copy link
Member Author

chalasr commented Sep 3, 2017

@ogizanagi That was my first intention, but in symfony/flex#142 (comment) @stof pointed that adding invalid service ids in the console.command.ids parameter is a BC break. I'm still not totally sure if we should care in fact.

@nicolas-grekas nicolas-grekas requested a review from stof September 3, 2017 14:18
@fabpot
Copy link
Member

fabpot commented Sep 8, 2017

Thank you @chalasr.

@fabpot fabpot merged commit 99e95c3 into symfony:3.4 Sep 8, 2017
fabpot added a commit that referenced this pull request Sep 8, 2017
…(chalasr)

This PR was merged into the 3.4 branch.

Discussion
----------

[Console] Fix BC break in console.command.ids parameter

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | symfony/flex#142 (comment)
| License       | MIT
| Doc PR        | n/a

To make command services lazy loaded when their name is known, we need to exclude them from [runtime registration of other (non lazy-loadable) command services](https://github.com/symfony/symfony/blob/master/src/Symfony/Bundle/FrameworkBundle/Console/Application.php#L176).
We also need to have them in the `console.command.ids` parameter in order to [not register them by convention](https://github.com/symfony/symfony/blob/3.4/src/Symfony/Component/HttpKernel/Bundle/Bundle.php#L188).
It is managed using `false` as values instead of ids for the parameter, [excluding false values from runtime registration](https://github.com/symfony/symfony/blob/master/src/Symfony/Bundle/FrameworkBundle/Console/Application.php#L177). Problem is that it makes FrameworkBundle's 3.3 Application incompatible with Console 3.4+ given the check for `false` is missing.

This PR fixes it using another parameter referencing ids of command services which can be lazy loaded.
I would be happy to not add the parameter if someone has a suggestion that allows to do so.

Commits
-------

99e95c3 Fix BC break in console.command.ids parameter
@chalasr chalasr deleted the command-ids-bc branch September 8, 2017 17:07
@chalasr chalasr removed the request for review from stof September 8, 2017 17:36
chalasr pushed a commit that referenced this pull request Dec 25, 2017
This PR was merged into the 4.0 branch.

Discussion
----------

[Console] Simplify parameters in DI

| Q             | A
| ------------- | ---
| Branch?       | 4.0
| Bug fix?      | no
| New feature?  |no <!-- don't forget to update src/**/CHANGELOG.md files -->
| BC breaks?    | no
| Deprecations? | no <!-- don't forget to update UPGRADE-*.md files -->
| Tests pass?   | yes
| Fixed tickets |
| License       | MIT
| Doc PR        |

Currently the container gets filled with alot of ugly params like

```
'console.command.ids' => array(
                'console.command.symfony_bundle_frameworkbundle_command_aboutcommand' => 'console.command.about',
                'console.command.symfony_bundle_frameworkbundle_command_assetsinstallcommand' => 'console.command.assets_install',
                'console.command.symfony_bundle_frameworkbundle_command_cacheclearcommand' => 'console.command.cache_clear',
...
            ),
'console.lazy_command.ids' => array(
                'console.command.about' => true,
                'console.command.assets_install' => true,
                'console.command.cache_clear' => true,
...
```

We can get rid of these in 4.0 with a little refactoring.
- SF 4.0 does not include the auto-registration of commands anymore which was the reason why the `console.command.ids` used the class name as index to prevent commands already defined as service to not triggger auto-registration. -> The param does not need the index lookup anymore in 4.0
- What I now also changed is that this param only contains the command IDs of services that are NOT lazy loaded. This way, we don't need `console.lazy_command.ids` at all. This is a simplification of #24073 and still ensures framework bundle console application is compatible with console component 3.x and 4.x

Commits
-------

ae47805 [Console] Simplify parameters in DI
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants