Skip to content

[HttpKernel] Remove convention based commands registration #23857

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

Merged
merged 1 commit into from
Aug 12, 2017
Merged

[HttpKernel] Remove convention based commands registration #23857

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
40 changes: 1 addition & 39 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 @@ -15,7 +15,6 @@
use Symfony\Component\DependencyInjection\ContainerBuilder;
use Symfony\Component\DependencyInjection\Container;
use Symfony\Component\Console\Application;
use Symfony\Component\Finder\Finder;
use Symfony\Component\DependencyInjection\Extension\ExtensionInterface;

/**
Expand Down Expand Up @@ -153,49 +152,12 @@ final public function getName()
}

/**
* Finds and registers Commands.
*
* Override this method if your bundle commands do not follow the conventions:
*
* * Commands are in the 'Command' sub-directory
* * Commands extend Symfony\Component\Console\Command\Command
* Registers console commands.
*
* @param Application $application An Application instance
*/
public function registerCommands(Application $application)
Copy link
Contributor

Choose a reason for hiding this comment

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

overrides can be removed i guess

Copy link
Member

Choose a reason for hiding this comment

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

this is not an override. It is the base definition

Copy link
Contributor

Choose a reason for hiding this comment

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

TwigBundle::registerCommands etc. They are now noop.

Copy link
Member Author

Choose a reason for hiding this comment

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

done

Copy link
Member

Choose a reason for hiding this comment

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

This is not good if you don't change the min version to forbid using TwigBundle 4.0 with HttpKernel 3.4 and the no-op was important though

Copy link
Member Author

Choose a reason for hiding this comment

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

indeed, no-ops re-added in bundles

{
if (!is_dir($dir = $this->getPath().'/Command')) {
return;
}

if (!class_exists('Symfony\Component\Finder\Finder')) {
Copy link
Member

Choose a reason for hiding this comment

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

the dep can be removed from composer.json

Copy link
Member Author

Choose a reason for hiding this comment

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

it is still used elsewhere

Copy link
Member Author

@chalasr chalasr Aug 10, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

nevermind, I thought I was in FrameworkBundle, done

throw new \RuntimeException('You need the symfony/finder component to register bundle commands.');
}

$finder = new Finder();
$finder->files()->name('*Command.php')->in($dir);

$prefix = $this->getNamespace().'\\Command';
foreach ($finder as $file) {
$ns = $prefix;
if ($relativePath = $file->getRelativePath()) {
$ns .= '\\'.str_replace('/', '\\', $relativePath);
}
$class = $ns.'\\'.$file->getBasename('.php');
if ($this->container) {
$commandIds = $this->container->hasParameter('console.command.ids') ? $this->container->getParameter('console.command.ids') : array();
Copy link
Contributor

Choose a reason for hiding this comment

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

should we deprecate this param as well? console.command.ids

Copy link
Member Author

Choose a reason for hiding this comment

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

no, it is still used to filter out lazy commands in framework Application

Copy link
Contributor

Choose a reason for hiding this comment

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

i see... but after #23796 we could right?

Copy link
Member Author

Choose a reason for hiding this comment

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

not sure, implementing it would be optional so non lazy command services would still be supported. To be rediscussed at/after it :)

$alias = 'console.command.'.strtolower(str_replace('\\', '_', $class));
if (isset($commandIds[$alias]) || $this->container->has($alias)) {
continue;
}
}
$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);

$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 @@ -20,6 +20,7 @@ CHANGELOG
* support for the `X-Status-Code` when handling exceptions in the `HttpKernel`
has been dropped, use the `HttpKernel::allowCustomResponseCode()` method
instead
* removed convention-based commands registration

3.4.0
-----
Expand Down
35 changes: 0 additions & 35 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 @@ -12,12 +12,9 @@
namespace Symfony\Component\HttpKernel\Tests\Bundle;

use PHPUnit\Framework\TestCase;
use Symfony\Component\DependencyInjection\ContainerBuilder;
use Symfony\Component\HttpKernel\Bundle\Bundle;
use Symfony\Component\HttpKernel\Tests\Fixtures\ExtensionNotValidBundle\ExtensionNotValidBundle;
use Symfony\Component\HttpKernel\Tests\Fixtures\ExtensionPresentBundle\ExtensionPresentBundle;
use Symfony\Component\HttpKernel\Tests\Fixtures\ExtensionAbsentBundle\ExtensionAbsentBundle;
use Symfony\Component\HttpKernel\Tests\Fixtures\ExtensionPresentBundle\Command\FooCommand;

class BundleTest extends TestCase
{
Expand All @@ -31,24 +28,6 @@ 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()
Copy link
Contributor

Choose a reason for hiding this comment

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

I would keep this test and assert no command is registered for ExtensionPresentBundle.

Copy link
Member Author

Choose a reason for hiding this comment

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

not sure that testing a no-op is useful at all, we don't for others like build(). Already covered by Application tests anyway

Copy link
Contributor

Choose a reason for hiding this comment

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

Here that could prevent mistakes due to merging conflicts.

{
$cmd = new FooCommand();
$app = $this->getMockBuilder('Symfony\Component\Console\Application')->getMock();
$app->expects($this->once())->method('add')->with($this->equalTo($cmd));

$bundle = new ExtensionPresentBundle();
$bundle->registerCommands($app);

$bundle2 = new ExtensionAbsentBundle();

$this->assertNull($bundle2->registerCommands($app));
}

/**
* @expectedException \LogicException
* @expectedExceptionMessage must implement Symfony\Component\DependencyInjection\Extension\ExtensionInterface
Expand All @@ -59,20 +38,6 @@ public function testGetContainerExtensionWithInvalidClass()
$bundle->getContainerExtension();
}

public function testHttpKernelRegisterCommandsIgnoresCommandsThatAreRegisteredAsServices()
Copy link
Member Author

Choose a reason for hiding this comment

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

should probably have been flagged as legacy in 3.4. Do we care?

Copy link
Contributor

Choose a reason for hiding this comment

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

Imo we don't care, no deprecation is triggered here anyway.

{
$container = new ContainerBuilder();
$container->register('console.command.symfony_component_httpkernel_tests_fixtures_extensionpresentbundle_command_foocommand', 'Symfony\Component\HttpKernel\Tests\Fixtures\ExtensionPresentBundle\Command\FooCommand');

$application = $this->getMockBuilder('Symfony\Component\Console\Application')->getMock();
// add() is never called when the found command classes are already registered as services
$application->expects($this->never())->method('add');

$bundle = new ExtensionPresentBundle();
$bundle->setContainer($container);
$bundle->registerCommands($application);
}

public function testBundleNameIsGuessedFromClass()
{
$bundle = new GuessedNameBundle();
Expand Down
2 changes: 0 additions & 2 deletions src/Symfony/Component/HttpKernel/composer.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@
"symfony/dependency-injection": "~3.4|~4.0",
"symfony/dom-crawler": "~3.4|~4.0",
"symfony/expression-language": "~3.4|~4.0",
"symfony/finder": "~3.4|~4.0",
Copy link
Member

Choose a reason for hiding this comment

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

there is one more in "suggest"

Copy link
Member Author

Choose a reason for hiding this comment

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

done

"symfony/process": "~3.4|~4.0",
"symfony/routing": "~3.4|~4.0",
"symfony/stopwatch": "~3.4|~4.0",
Expand All @@ -51,7 +50,6 @@
"symfony/config": "",
"symfony/console": "",
"symfony/dependency-injection": "",
"symfony/finder": "",
"symfony/var-dumper": ""
},
"autoload": {
Expand Down