8000 [Console] Simplify parameters in DI by Tobion · Pull Request #25593 · symfony/symfony · GitHub
[go: up one dir, main page]

Skip to content

[Console] Simplify parameters in DI #25593

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
Dec 25, 2017

Conversation

Tobion
Copy link
Contributor
@Tobion Tobion commented Dec 24, 2017
Q A
Branch? 4.0
Bug fix? no
New feature? no
BC breaks? no
Deprecations? no
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 [Console] Fix BC break in console.command.ids parameter #24073 and still ensures framework bundle console application is compatible with console component 3.x and 4.x

@Tobion Tobion force-pushed the refactor-command-params branch from 8152efa to ae47805 Compare December 24, 2017 06:07
@chalasr
Copy link
Member
chalasr commented Dec 24, 2017

Can we remove this check https://github.com/symfony/symfony/blob/master/src/Symfony/Bundle/FrameworkBundle/Console/Application.php#L166 or is it needed for 3-4 compatibility?

@Tobion
Copy link
Contributor Author
Tobion commented Dec 25, 2017

It's needed as long as framework bundle supports 3.4 console.

@chalasr
Copy link
Member
chalasr commented Dec 25, 2017

Thank you @Tobion.

@chalasr chalasr merged commit ae47805 into symfony:4.0 Dec 25, 2017
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
@Tobion Tobion deleted the refactor-command-params branch December 25, 2017 19:25
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.

4 participants
0