Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

[FrameworkBundle] Commands as a service #23624

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 9 commits into from
Closed

[FrameworkBundle] Commands as a service #23624

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
da49f81
[FrameworkBundle] Commands as a service
ro0NL Jul 22, 2017
48228aa
favor getKernel()->getContainer()->getParameter()
ro0NL Aug 1, 2017
76097b3
move cache:pool:prune to console.xml
ro0NL Aug 1, 2017
bc90a30
bypass commnand auto discovery
ro0NL Aug 1, 2017
9b0a5a4
class named services
ro0NL Aug 5, 2017
d99e0b9
marked commands final
ro0NL Aug 6, 2017
a404323
filesystem required dep in assets:install
ro0NL Aug 6, 2017
d0b35d0
removed bc layers in favor of final
ro0NL Aug 6, 2017
2067d8a
patched security bundle
ro0NL Aug 6, 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
2 changes: 1 addition & 1 deletion 2 src/Symfony/Bridge/Twig/Command/DebugCommand.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@ protected function execute(InputInterface $input, OutputInterface $output)
if (__CLASS__ !== get_class($this)) {
$r = new \ReflectionMethod($this, 'getTwigEnvironment');
if (__CLASS__ !== $r->getDeclaringClass()->getName()) {
@trigger_error(sprintf('Usage of method "%s" is deprecated since version 3.4 and will no longer be supported in 4.0.', get_class($this).'::getTwigEnvironment'), E_USER_DEPRECATED);
@trigger_error(sprintf('Usage of method "%s" is deprecated since version 3.4 and will no longer be supported in 4.0. Construct the command with its required arguments instead.', get_class($this).'::getTwigEnvironment'), E_USER_DEPRECATED);

$this->twig = $this->getTwigEnvironment();
}
Expand Down
2 changes: 1 addition & 1 deletion 2 src/Symfony/Bridge/Twig/Command/LintCommand.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ protected function execute(InputInterface $input, OutputInterface $output)
if (__CLASS__ !== get_class($this)) {
$r = new \ReflectionMethod($this, 'getTwigEnvironment');
if (__CLASS__ !== $r->getDeclaringClass()->getName()) {
@trigger_error(sprintf('Usage of method "%s" is deprecated since version 3.4 and will no longer be supported in 4.0.', get_class($this).'::getTwigEnvironment'), E_USER_DEPRECATED);
@trigger_error(sprintf('Usage of method "%s" is deprecated since version 3.4 and will no longer be supported in 4.0. Construct the command with its required arguments instead.', get_class($this).'::getTwigEnvironment'), E_USER_DEPRECATED);

$this->twig = $this->getTwigEnvironment();
}
Expand Down
4 changes: 3 additions & 1 deletion 4 src/Symfony/Bundle/FrameworkBundle/Command/AboutCommand.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@
* A console command to display information about the current installation.
*
* @author Roland Franssen <franssen.roland@gmail.com>
*
* @final since version 3.4
*/
class AboutCommand extends ContainerAwareCommand
{
Expand All @@ -45,7 +47,7 @@ protected function execute(InputInterface $input, OutputInterface $output)
$io = new SymfonyStyle($input, $output);

/** @var $kernel KernelInterface */
$kernel = $this->getContainer()->get('kernel');
$kernel = $this->getApplication()->getKernel();

$io->table(array(), array(
array('<info>Symfony</>'),
Expand Down
4 changes: 2 additions & 2 deletions 4 src/Symfony/Bundle/FrameworkBundle/Command/AbstractConfigCommand.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ protected function listBundles($output)
$headers = array('Bundle name', 'Extension alias');
$rows = array();

$bundles = $this->getContainer()->get('kernel')->getBundles();
$bundles = $this->getApplication()->getKernel()->getBundles();
usort($bundles, function ($bundleA, $bundleB) {
return strcmp($bundleA->getName(), $bundleB->getName());
});
Expand Down Expand Up @@ -117,7 +117,7 @@ private function initializeBundles()
// Re-build bundle manually to initialize DI extensions that can be extended by other bundles in their build() method
// as this method is not called when the container is loaded from the cache.
$container = $this->getContainerBuilder();
$bundles = $this->getContainer()->get('kernel')->getBundles();
$bundles = $this->getApplication()->getKernel()->getBundles();
foreach ($bundles as $bundle) {
if ($extension = $bundle->getContainerExtension()) {
$container->registerExtension($extension);
Expand Down
34 changes: 28 additions & 6 deletions 34 src/Symfony/Bundle/FrameworkBundle/Command/AssetsInstallCommand.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,17 +26,34 @@
*
* @author Fabien Potencier <fabien@symfony.com>
* @author Gábor Egyed <gabor.egyed@gmail.com>
*
* @final since version 3.4
*/
class AssetsInstallCommand extends ContainerAwareCommand
{
const METHOD_COPY = 'copy';
const METHOD_ABSOLUTE_SYMLINK = 'absolute symlink';
const METHOD_RELATIVE_SYMLINK = 'relative symlink';

private $filesystem;

/**
* @var Filesystem
* @param Filesystem $filesystem
*/
private $filesystem;
public function __construct($filesystem = null)
{
parent::__construct();

if (!$filesystem instanceof Filesystem) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Argh... null implies new api?

Copy link
Member

Choose a reason for hiding this comment

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

null has never been an allowed name so LGTM as is

Copy link
Contributor Author

@ro0NL ro0NL 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.

actually it is.. https://github.com/symfony/symfony/blob/v3.3.6/src/Symfony/Component/Console/Command/Command.php#L51

leaning to make filesystem a required dep..

Copy link
Member

Choose a reason for hiding this comment

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

fine for me

@trigger_error(sprintf('Passing a command name as the first argument of "%s" is deprecated since version 3.4 and will be removed in 4.0. If the command was registered by convention, make it a service instead.', __METHOD__), 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.

%s()?

Copy link
Member

Choose a reason for hiding this comment

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

message is confusing if I was not passing anything as argument


$this->setName(null === $filesystem ? 'assets:install' : $filesystem);
Copy link
Member

Choose a reason for hiding this comment

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

parent::__construct($filesystem);? calling setName() now is a behavior change, it's called in configure anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm you're right.. configure() always wins.. ill have a look at it.

Copy link
Member

Choose a reason for hiding this comment

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

thanks


return;
}

$this->filesystem = $filesystem;
}

/**
* {@inheritdoc}
Expand Down Expand Up @@ -79,10 +96,17 @@ protected function configure()
*/
protected function execute(InputInterface $input, OutputInterface $output)
{
// BC to be removed in 4.0
if (null === $this->filesystem) {
$this->filesystem = $this->getContainer()->get('filesystem');
$baseDir = $this->getContainer()->getParameter('kernel.project_dir');
}

$kernel = $this->getApplication()->getKernel();
$targetArg = rtrim($input->getArgument('target'), '/');

if (!is_dir($targetArg)) {
$targetArg = $this->getContainer()->getParameter('kernel.project_dir').'/'.$targetArg;
$targetArg = (isset($baseDir) ? $baseDir : $kernel->getContainer()->getParameter('kernel.project_dir')).'/'.$targetArg;

if (!is_dir($targetArg)) {
// deprecated, logic to be removed in 4.0
Expand All @@ -95,8 +119,6 @@ protected function execute(InputInterface $input, OutputInterface $output)
}
}

$this->filesystem = $this->getContainer()->get('filesystem');

// Create the bundles directory otherwise symlink will fail.
$bundlesDir = $targetArg.'/bundles/';
$this->filesystem->mkdir($bundlesDir, 0777);
Expand All @@ -122,7 +144,7 @@ protected function execute(InputInterface $input, OutputInterface $output)
$exitCode = 0;
$validAssetDirs = array();
/** @var BundleInterface $bundle */
foreach ($this->getContainer()->get('kernel')->getBundles() as $bundle) {
foreach ($kernel->getBundles() as $bundle) {
if (!is_dir($originDir = $bundle->getPath().'/Resources/public')) {
continue;
}
Expand Down
62 changes: 47 additions & 15 deletions 62 src/Symfony/Bundle/FrameworkBundle/Command/CacheClearCommand.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@
use Symfony\Component\Console\Input\InputOption;
use Symfony\Component\Console\Output\OutputInterface;
use Symfony\Component\Console\Style\SymfonyStyle;
use Symfony\Component\Filesystem\Filesystem;
use Symfony\Component\HttpKernel\CacheClearer\CacheClearerInterface;
use Symfony\Component\HttpKernel\KernelInterface;
use Symfony\Component\Finder\Finder;

Expand All @@ -23,9 +25,34 @@
*
* @author Francis Besset <francis.besset@gmail.com>
* @author Fabien Potencier <fabien@symfony.com>
*
* @final since version 3.4
*/
class CacheClearCommand extends ContainerAwareCommand
{
private $cacheClearer;
private $filesystem;

/**
* @param CacheClearerInterface $cacheClearer
* @param Filesystem|null $filesystem
*/
public function __construct($cacheClearer = null, Filesystem $filesystem = null)
{
parent::__construct();

if (!$cacheClearer instanceof CacheClearerInterface) {
@trigger_error(sprintf('Passing a command name as the first argument of "%s" is deprecated since version 3.4 and will be removed in 4.0. If the command was registered by convention, make it a service instead.', __METHOD__), E_USER_DEPRECATED);

$this->setName(null === $cacheClearer ? 'cache:clear' : $cacheClearer);

return;
}

$this->cacheClearer = $cacheClearer;
$this->filesystem = $filesystem ?: new Filesystem();
}

/**
* {@inheritdoc}
*/
Expand Down Expand Up @@ -54,28 +81,34 @@ protected function configure()
*/
protected function execute(InputInterface $input, OutputInterface $output)
{
// BC to be removed in 4.0
if (null === $this->cacheClearer) {
$this->cacheClearer = $this->getContainer()->get('cache_clearer');
$this->filesystem = $this->getContainer()->get('filesystem');
$realCacheDir = $this->getContainer()->getParameter('kernel.cache_dir');
}

$io = new SymfonyStyle($input, $output);

$realCacheDir = $this->getContainer()->getParameter('kernel.cache_dir');
$kernel = $this->getApplication()->getKernel();
$realCacheDir = isset($realCacheDir) ? $realCacheDir : $kernel->getContainer()->getParameter('kernel.cache_dir');
// the old cache dir name must not be longer than the real one to avoid exceeding
// the maximum length of a directory or file path within it (esp. Windows MAX_PATH)
$oldCacheDir = substr($realCacheDir, 0, -1).('~' === substr($realCacheDir, -1) ? '+' : '~');
$filesystem = $this->getContainer()->get('filesystem');

if (!is_writable($realCacheDir)) {
throw new \RuntimeException(sprintf('Unable to write in the "%s" directory', $realCacheDir));
}

if ($filesystem->exists($oldCacheDir)) {
$filesystem->remove($oldCacheDir);
if ($this->filesystem->exists($oldCacheDir)) {
$this->filesystem->remove($oldCacheDir);
}

$kernel = $this->getContainer()->get('kernel');
$io->comment(sprintf('Clearing the cache for the <info>%s</info> environment with debug <info>%s</info>', $kernel->getEnvironment(), var_export($kernel->isDebug(), true)));
$this->getContainer()->get('cache_clearer')->clear($realCacheDir);
$this->cacheClearer->clear($realCacheDir);

if ($input->getOption('no-warmup')) {
$filesystem->rename($realCacheDir, $oldCacheDir);
$this->filesystem->rename($realCacheDir, $oldCacheDir);
} else {
$warning = 'Calling cache:clear without the --no-warmup option is deprecated since version 3.3. Cache warmup should be done with the cache:warmup command instead.';

Expand All @@ -90,7 +123,7 @@ protected function execute(InputInterface $input, OutputInterface $output)
$io->comment('Removing old cache directory...');
}

$filesystem->remove($oldCacheDir);
$this->filesystem->remove($oldCacheDir);

if ($output->isVerbose()) {
$io->comment('Finished');
Expand All @@ -101,31 +134,30 @@ protected function execute(InputInterface $input, OutputInterface $output)

private function warmupCache(InputInterface $input, OutputInterface $output, $realCacheDir, $oldCacheDir)
{
$filesystem = $this->getContainer()->get('filesystem');
$io = new SymfonyStyle($input, $output);

// the warmup cache dir name must have the same length than the real one
// to avoid the many problems in serialized resources files
$realCacheDir = realpath($realCacheDir);
$warmupDir = substr($realCacheDir, 0, -1).('_' === substr($realCacheDir, -1) ? '-' : '_');

if ($filesystem->exists($warmupDir)) {
if ($this->filesystem->exists($warmupDir)) {
if ($output->isVerbose()) {
$io->comment('Clearing outdated warmup directory...');
}
$filesystem->remove($warmupDir);
$this->filesystem->remove($warmupDir);
}

if ($output->isVerbose()) {
$io->comment('Warming up cache...');
}
$this->warmup($warmupDir, $realCacheDir, !$input->getOption('no-optional-warmers'));

$filesystem->rename($realCacheDir, $oldCacheDir);
$this->filesystem->rename($realCacheDir, $oldCacheDir);
if ('\\' === DIRECTORY_SEPARATOR) {
sleep(1); // workaround for Windows PHP rename bug
}
$filesystem->rename($warmupDir, $realCacheDir);
$this->filesystem->rename($warmupDir, $realCacheDir);
}

/**
Expand All @@ -138,7 +170,7 @@ private function warmupCache(InputInterface $input, OutputInterface $output, $re
protected function warmup($warmupDir, $realCacheDir, $enableOptionalWarmers = true)
{
// create a temporary kernel
$realKernel = $this->getContainer()->get('kernel');
$realKernel = $this->getApplication()->getKernel();
$realKernelClass = get_class($realKernel);
$namespace = '';
if (false !== $pos = strrpos($realKernelClass, '\\')) {
Expand Down Expand Up @@ -274,7 +306,7 @@ protected function buildContainer()
}
}
EOF;
$this->getContainer()->get('filesystem')->mkdir($warmupDir);
$this->filesystem->mkdir($warmupDir);
file_put_contents($file = $warmupDir.'/kernel.tmp', $code);
require_once $file;
$class = "$namespace\\$class";
Expand Down
38 changes: 31 additions & 7 deletions 38 src/Symfony/Bundle/FrameworkBundle/Command/CachePoolClearCommand.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,26 @@
*/
final class CachePoolClearCommand extends ContainerAwareCommand
Copy link
Contributor

Choose a reason for hiding this comment

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

The getContainer() is missing here, right?

Copy link
Member

Choose a reason for hiding this comment

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

getContainer() is protected so the deprecation would be triggered only if called from a subclass, but this is final :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Indeed... I should have reviewed more carefully 😊

{
private $poolClearer;

/**
* @param Psr6CacheClearer $poolClearer
*/
public function __construct($poolClearer = null)
{
parent::__construct();

if (!$poolClearer instanceof Psr6CacheClearer) {
@trigger_error(sprintf('Passing a command name as the first argument of "%s" is deprecated since version 3.4 and will be removed in 4.0. If the command was registered by convention, make it a service instead.', __METHOD__), E_USER_DEPRECATED);

$this->setName(null === $poolClearer ? 'cache:pool:clear' : $poolClearer);

return;
}

$this->poolClearer = $poolClearer;
}

/**
* {@inheritdoc}
*/
Expand All @@ -50,18 +70,22 @@ protected function configure()
*/
protected function execute(InputInterface $input, OutputInterface $output)
{
// BC to be removed in 4.0
if (null === $this->poolClearer) {
$this->poolClearer = $this->getContainer()->get('cache.global_clearer');
$cacheDir = $this->getContainer()->getParameter('kernel.cache_dir');
}

$io = new SymfonyStyle($input, $output);
$kernel = $this->getApplication()->getKernel();
$pools = array();
$clearers = array();
$container = $this->getContainer();
$cacheDir = $container->getParameter('kernel.cache_dir');
$globalClearer = $container->get('cache.global_clearer');

foreach ($input->getArgument('pools') as $id) {
if ($globalClearer->hasPool($id)) {
if ($this->poolClearer->hasPool($id)) {
$pools[$id] = $id;
} else {
$pool = $container->get($id);
$pool = $kernel->getContainer()->get($id);
Copy link
Contributor

Choose a reason for hiding this comment

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

parent::getContainer()?

Copy link
Member

Choose a reason for hiding this comment

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

it won't be containeraware in 4.0, but this will still be supported

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, it was just to be consistent with the code above but that's fine for me.

Copy link
Contributor Author

@ro0NL ro0NL Jul 29, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

We basically favor kernel aware over container aware. See also #23632

We might favor kernel injection over kernel aware. That would be the most flexible, but not sure if needed.


if ($pool instanceof CacheItemPoolInterface) {
$pools[$id] = $pool;
Expand All @@ -75,7 +99,7 @@ protected function execute(InputInterface $input, OutputInterface $output)

foreach ($clearers as $id => $clearer) {
$io->comment(sprintf('Calling cache clearer: <info>%s</info>', $id));
$clearer->clear($cacheDir);
$clearer->clear(isset($cacheDir) ? $cacheDir : $kernel->getContainer()->getParameter('kernel.cache_dir'));
}

foreach ($pools as $id => $pool) {
Expand All @@ -84,7 +108,7 @@ protected function execute(InputInterface $input, OutputInterface $output)
if ($pool instanceof CacheItemPoolInterface) {
$pool->clear();
} else {
$globalClearer->clearPool($id);
$this->poolClearer->clearPool($id);
}
}

Expand Down
Loading
Morty Proxy This is a proxified and sanitized view of the page, visit original site.