8000 [HttpKernel] Deprecated commands auto-registration by GuilhemN · Pull Request #23805 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[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.

Sign up for GitHub

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

Closed
wants to merge 4 commits into from
Closed

[HttpKernel] Deprecated commands auto-registration #23805

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0ee4026
[HttpKernel] Deprecated commands auto-registration
GuilhemN Aug 6, 2017
669d344
fixes
GuilhemN Aug 6, 2017
1da8b12
Service Discovery -> service discovery
GuilhemN Aug 9, 2017
01bf469
Fix tests
GuilhemN Aug 9, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
[HttpKernel] Deprecated commands auto-registration
  • Loading branch information
@GuilhemN
GuilhemN committed Aug 6, 2017
commit 0ee402698eb21a691b7984de0e53cf905f57a4ab
29 changes: 29 additions & 0 deletions UPGRADE-3.4.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,35 @@ FrameworkBundle
class has been deprecated and will be removed in 4.0. Use the
`Symfony\Component\Translation\DependencyInjection\TranslatorPass` class instead.

HttpKernel
----------

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we put convention-based?

Copy link
Contributor Author

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? :)

in 4.0. Use PSR-4 based Service Discovery instead.

Before:

```yml
# app/config/services.yml
services:
# ...

# implicit commands registration
Copy link
Contributor

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

```

After:

```yml
# app/config/services.yml
services:
# ...

# explicit commands registration
AppBundle\Command:
resource: '../../src/AppBundle/Command/*'
tags: ['console.command']
Copy link
Contributor
@ogizanagi ogizanagi Aug 6, 2017 •
edited
Loading

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 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?

Copy link
Contributor Author

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair point 👍

```

Process
-------

Expand Down
26 changes: 26 additions & 0 deletions UPGRADE-4.0.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -452,6 +452,32 @@ HttpFoundation
HttpKernel
----------

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is not supported anymore

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

indeed

in 4.0. Use PSR-4 based Service Discovery instead.

Before:

```yml
# app/config/services.yml
services:
# ...

# implicit commands registration
Copy link
Contributor

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 the Command folder" for clarity

```

After:

```yml
# app/config/services.yml
services:
# ...

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

* Removed the `kernel.root_dir` parameter. Use the `kernel.project_dir` parameter
instead.

Expand Down
2 changes: 2 additions & 0 deletions src/Symfony/Component/HttpKernel/Bundle/Bundle.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Member

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?

Copy link
Contributor Author

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.

Copy link
Member
@chalasr chalasr Aug 8, 2017 •
edited
Loading

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 the console.command.ids parameter

Copy link
Contributor

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.


$application->add($r->newInstance());
}
}
Expand Down
1 change: 1 addition & 0 deletions src/Symfony/Component/HttpKernel/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ CHANGELOG
3.4.0
-----

* deprecated commands auto registration
* added `AddCacheClearerPass`
* added `AddCacheWarmerPass`

Expand Down
4 changes: 4 additions & 0 deletions src/Symfony/Component/HttpKernel/Tests/Bundle/BundleTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,10 @@ public function testGetContainerExtension()
);
}

/**
* @group legacy
* @expectedDeprecation Auto-registration of the command "Symfony\Component\HttpKernel\Tests\Fixtures\ExtensionPresentBundle\Command\FooCommand" is deprecated since Symfony 3.4 and won't be supported in 4.0. Use PSR-4 based Service Discovery instead.
*/
public function testRegisterCommands()
{
$cmd = new FooCommand();
Expand Down
0