From 8895bc1b5bab9af56fbc4458337f83046347bab5 Mon Sep 17 00:00:00 2001 From: Nicolas Grekas Date: Tue, 3 Jul 2018 13:59:14 +0200 Subject: [PATCH] [Process][Console] deprecated defining commands as strings --- UPGRADE-4.2.md | 38 ++++++++++++++ UPGRADE-5.0.md | 34 +++++++++++++ src/Symfony/Component/Console/CHANGELOG.md | 9 ++++ .../Console/Helper/ProcessHelper.php | 33 ++++++++---- .../Tests/Helper/ProcessHelperTest.php | 19 ++++++- src/Symfony/Component/Dotenv/Dotenv.php | 2 +- src/Symfony/Component/Process/CHANGELOG.md | 7 +++ src/Symfony/Component/Process/PhpProcess.php | 9 +++- src/Symfony/Component/Process/Process.php | 51 ++++++++++++++++--- .../Tests/ProcessFailedExceptionTest.php | 10 ++-- .../Component/Process/Tests/ProcessTest.php | 31 ++++------- 11 files changed, 197 insertions(+), 46 deletions(-) diff --git a/UPGRADE-4.2.md b/UPGRADE-4.2.md index b50dde0b79..73d0a9dc7c 100644 --- a/UPGRADE-4.2.md +++ b/UPGRADE-4.2.md @@ -11,6 +11,25 @@ Config * Deprecated constructing a `TreeBuilder` without passing root node information. +Console +------- + + * Deprecated passing a command as a string to `ProcessHelper::run()`, + pass the command as an array of arguments instead. + + Before: + ```php + $processHelper->run($output, 'ls -l'); + ``` + + After: + ```php + $processHelper->run($output, array('ls', '-l')); + + // alternatively, when a shell wrapper is required + $processHelper->run($output, Process::fromShellCommandline('ls -l')); + ``` + DoctrineBridge -------------- @@ -37,6 +56,25 @@ Form {% endfor %} ``` +Process +------- + + * Deprecated the `Process::setCommandline()` and the `PhpProcess::setPhpBinary()` methods. + * Deprecated passing commands as strings when creating a `Process` instance. + + Before: + ```php + $process = new Process('ls -l'); + ``` + + After: + ```php + $process = new Process(array('ls', '-l')); + + // alternatively, when a shell wrapper is required + $process = Process::fromShellCommandline('ls -l'); + ``` + Security -------- diff --git a/UPGRADE-5.0.md b/UPGRADE-5.0.md index 1b1312d632..b0f6e99c63 100644 --- a/UPGRADE-5.0.md +++ b/UPGRADE-5.0.md @@ -21,6 +21,21 @@ Console * Removed the `getHorizontalBorderChar()` method in favor of the `getBorderChars()` method in `TableStyle`. * Removed the `setVerticalBorderChar()` method in favor of the `setVerticalBorderChars()` method in `TableStyle`. * Removed the `getVerticalBorderChar()` method in favor of the `getBorderChars()` method in `TableStyle`. + * The `ProcessHelper::run()` method takes the command as an array of arguments. + + Before: + ```php + $processHelper->run($output, 'ls -l'); + ``` + + After: + ```php + $processHelper->run($output, array('ls', '-l')); + + // alternatively, when a shell wrapper is required + $processHelper->run($output, Process::fromShellCommandline('ls -l')); + ``` + DependencyInjection ------------------- @@ -78,6 +93,25 @@ HttpFoundation * The `getClientSize()` method of the `UploadedFile` class has been removed. * The `getSession()` method of the `Request` class throws an exception when session is null. +Process +------- + + * Removed the `Process::setCommandline()` and the `PhpProcess::setPhpBinary()` methods. + * Commands must be defined as arrays when creating a `Process` instance. + + Before: + ```php + $process = new Process('ls -l'); + ``` + + After: + ```php + $process = new Process(array('ls', '-l')); + + // alternatively, when a shell wrapper is required + $process = Process::fromShellCommandline('ls -l'); + ``` + Security -------- diff --git a/src/Symfony/Component/Console/CHANGELOG.md b/src/Symfony/Component/Console/CHANGELOG.md index 7682feb593..06e4355ad9 100644 --- a/src/Symfony/Component/Console/CHANGELOG.md +++ b/src/Symfony/Component/Console/CHANGELOG.md @@ -1,6 +1,15 @@ CHANGELOG ========= +4.2.0 +----- + + * allowed passing commands as `array($process, 'ENV_VAR' => 'value')` to + `ProcessHelper::run()` to pass environment variables + * deprecated passing a command as a string to `ProcessHelper::run()`, + pass it the command as an array of its arguments instead + * made the `ProcessHelper` class final + 4.1.0 ----- diff --git a/src/Symfony/Component/Console/Helper/ProcessHelper.php b/src/Symfony/Component/Console/Helper/ProcessHelper.php index 82935baeaa..4d88d7041c 100644 --- a/src/Symfony/Component/Console/Helper/ProcessHelper.php +++ b/src/Symfony/Component/Console/Helper/ProcessHelper.php @@ -20,18 +20,20 @@ use Symfony\Component\Process\Process; * The ProcessHelper class provides helpers to run external processes. * * @author Fabien Potencier + * + * @final since Symfony 4.2 */ class ProcessHelper extends Helper { /** * Runs an external process. * - * @param OutputInterface $output An OutputInterface instance - * @param string|array|Process $cmd An instance of Process or an array of arguments to escape and run or a command to run - * @param string|null $error An error message that must be displayed if something went wrong - * @param callable|null $callback A PHP callback to run whenever there is some - * output available on STDOUT or STDERR - * @param int $verbosity The threshold for verbosity + * @param OutputInterface $output An OutputInterface instance + * @param array|Process $cmd An instance of Process or an array of the command and arguments + * @param string|null $error An error message that must be displayed if something went wrong + * @param callable|null $callback A PHP callback to run whenever there is some + * output available on STDOUT or STDERR + * @param int $verbosity The threshold for verbosity * * @return Process The process that ran */ @@ -44,9 +46,22 @@ class ProcessHelper extends Helper $formatter = $this->getHelperSet()->get('debug_formatter'); if ($cmd instanceof Process) { - $process = $cmd; - } else { + $cmd = array($cmd); + } + + if (!\is_array($cmd)) { + @trigger_error(sprintf('Passing a command as a string to "%s()" is deprecated since Symfony 4.2, pass it the command as an array of arguments instead.', __METHOD__), E_USER_DEPRECATED); + $cmd = array(\method_exists(Process::class, 'fromShellCommandline') ? Process::fromShellCommandline($cmd) : new Process($cmd)); + } + + if (\is_string($cmd[0] ?? null)) { $process = new Process($cmd); + $cmd = array(); + } elseif (($cmd[0] ?? null) instanceof Process) { + $process = $cmd[0]; + unset($cmd[0]); + } else { + throw new \InvalidArgumentException(sprintf('Invalid command provided to "%s()": the command should an array whose first is element is either the path to the binary to run of a "Process" object.', __METHOD__)); } if ($verbosity <= $output->getVerbosity()) { @@ -57,7 +72,7 @@ class ProcessHelper extends Helper $callback = $this->wrapCallback($output, $process, $callback); } - $process->run($callback); + $process->run($callback, $cmd); if ($verbosity <= $output->getVerbosity()) { $message = $process->isSuccessful() ? 'Command ran successfully' : sprintf('%s Command did not run successfully', $process->getExitCode()); diff --git a/src/Symfony/Component/Console/Tests/Helper/ProcessHelperTest.php b/src/Symfony/Component/Console/Tests/Helper/ProcessHelperTest.php index eb619539ee..92f2f44715 100644 --- a/src/Symfony/Component/Console/Tests/Helper/ProcessHelperTest.php +++ b/src/Symfony/Component/Console/Tests/Helper/ProcessHelperTest.php @@ -25,6 +25,10 @@ class ProcessHelperTest extends TestCase */ public function testVariousProcessRuns($expected, $cmd, $verbosity, $error) { + if (\is_string($cmd)) { + $cmd = \method_exists(Process::class, 'fromShellCommandline') ? Process::fromShellCommandline($cmd) : new Process($cmd); + } + $helper = new ProcessHelper(); $helper->setHelperSet(new HelperSet(array(new DebugFormatterHelper()))); $output = $this->getOutputStream($verbosity); @@ -41,7 +45,7 @@ class ProcessHelperTest extends TestCase $executed = false; $callback = function () use (&$executed) { $executed = true; }; - $helper->run($output, 'php -r "echo 42;"', null, $callback); + $helper->run($output, array('php', '-r', 'echo 42;'), null, $callback); $this->assertTrue($executed); } @@ -81,12 +85,21 @@ EOT; OUT out message RES 252 Command did not run successfully +EOT; + + $PHP = '\\' === \DIRECTORY_SEPARATOR ? '"!PHP!"' : '"$PHP"'; + $successOutputPhp = <<getCommandLine(); $successOutputProcessDebug = str_replace("'php' '-r' 'echo 42;'", $args, $successOutputProcessDebug); + $fromShellCommandline = \method_exists(Process::class, 'fromShellCommandline') ? array(Process::class, 'fromShellCommandline') : function ($cmd) { return new Process($cmd); }; return array( array('', 'php -r "echo 42;"', StreamOutput::VERBOSITY_VERBOSE, null), @@ -100,7 +113,9 @@ EOT; array($syntaxErrorOutputVerbose.$errorMessage.PHP_EOL, 'php -r "fwrite(STDERR, \'error message\');usleep(50000);fwrite(STDOUT, \'out message\');exit(252);"', StreamOutput::VERBOSITY_VERY_VERBOSE, $errorMessage), array($syntaxErrorOutputDebug.$errorMessage.PHP_EOL, 'php -r "fwrite(STDERR, \'error message\');usleep(500000);fwrite(STDOUT, \'out message\');exit(252);"', StreamOutput::VERBOSITY_DEBUG, $errorMessage), array($successOutputProcessDebug, array('php', '-r', 'echo 42;'), StreamOutput::VERBOSITY_DEBUG, null), - array($successOutputDebug, new Process('php -r "echo 42;"'), StreamOutput::VERBOSITY_DEBUG, null), + array($successOutputDebug, $fromShellCommandline('php -r "echo 42;"'), StreamOutput::VERBOSITY_DEBUG, null), + array($successOutputProcessDebug, array(new Process(array('php', '-r', 'echo 42;'))), StreamOutput::VERBOSITY_DEBUG, null), + array($successOutputPhp, array($fromShellCommandline('php -r '.$PHP), 'PHP' => 'echo 42;'), StreamOutput::VERBOSITY_DEBUG, null), ); } diff --git a/src/Symfony/Component/Dotenv/Dotenv.php b/src/Symfony/Component/Dotenv/Dotenv.php index 29e3daab04..c97f166bef 100644 --- a/src/Symfony/Component/Dotenv/Dotenv.php +++ b/src/Symfony/Component/Dotenv/Dotenv.php @@ -330,7 +330,7 @@ final class Dotenv throw new \LogicException('Resolving commands requires the Symfony Process component.'); } - $process = new Process('echo '.$matches[0]); + $process = \method_exists(Process::class, 'fromShellCommandline') ? Process::fromShellCommandline('echo '.$matches[0]) : new Process('echo '.$matches[0]); $process->inheritEnvironmentVariables(true); $process->setEnv($this->values); try { diff --git a/src/Symfony/Component/Process/CHANGELOG.md b/src/Symfony/Component/Process/CHANGELOG.md index 354db592a1..028a416171 100644 --- a/src/Symfony/Component/Process/CHANGELOG.md +++ b/src/Symfony/Component/Process/CHANGELOG.md @@ -1,6 +1,13 @@ CHANGELOG ========= +4.2.0 +----- + + * added the `Process::fromShellCommandline()` to run commands in a shell wrapper + * deprecated passing a command as string when creating a `Process` instance + * deprecated the `Process::setCommandline()` and the `PhpProcess::setPhpBinary()` methods + 4.1.0 ----- diff --git a/src/Symfony/Component/Process/PhpProcess.php b/src/Symfony/Component/Process/PhpProcess.php index 4c560ef9b2..c74c14007d 100644 --- a/src/Symfony/Component/Process/PhpProcess.php +++ b/src/Symfony/Component/Process/PhpProcess.php @@ -29,11 +29,12 @@ class PhpProcess extends Process * @param string|null $cwd The working directory or null to use the working dir of the current PHP process * @param array|null $env The environment variables or null to use the same environment as the current PHP process * @param int $timeout The timeout in seconds + * @param array|null $php Path to the PHP binary to use with any additional arguments */ - public function __construct(string $script, string $cwd = null, array $env = null, int $timeout = 60) + public function __construct(string $script, string $cwd = null, array $env = null, int $timeout = 60, array $php = null) { $executableFinder = new PhpExecutableFinder(); - if (false === $php = $executableFinder->find(false)) { + if (false === $php = $php ?? $executableFinder->find(false)) { $php = null; } else { $php = array_merge(array($php), $executableFinder->findArguments()); @@ -51,9 +52,13 @@ class PhpProcess extends Process /** * Sets the path to the PHP binary to use. + * + * @deprecated since Symfony 4.2, use the $php argument of the constructor instead. */ public function setPhpBinary($php) { + @trigger_error(sprintf('The "%s()" method is deprecated since Symfony 4.2, use the $php argument of the constructor instead.', __METHOD__), E_USER_DEPRECATED); + $this->setCommandLine($php); } diff --git a/src/Symfony/Component/Process/Process.php b/src/Symfony/Component/Process/Process.php index ed48b5f234..d65c6ac92c 100644 --- a/src/Symfony/Component/Process/Process.php +++ b/src/Symfony/Component/Process/Process.php @@ -129,21 +129,25 @@ class Process implements \IteratorAggregate ); /** - * @param string|array $commandline The command line to run - * @param string|null $cwd The working directory or null to use the working dir of the current PHP process - * @param array|null $env The environment variables or null to use the same environment as the current PHP process - * @param mixed|null $input The input as stream resource, scalar or \Traversable, or null for no input - * @param int|float|null $timeout The timeout in seconds or null to disable + * @param array $command The command to run and its arguments listed as separate entries + * @param string|null $cwd The working directory or null to use the working dir of the current PHP process + * @param array|null $env The environment variables or null to use the same environment as the current PHP process + * @param mixed|null $input The input as stream resource, scalar or \Traversable, or null for no input + * @param int|float|null $timeout The timeout in seconds or null to disable * * @throws RuntimeException When proc_open is not installed */ - public function __construct($commandline, string $cwd = null, array $env = null, $input = null, ?float $timeout = 60) + public function __construct($command, string $cwd = null, array $env = null, $input = null, ?float $timeout = 60) { if (!function_exists('proc_open')) { throw new RuntimeException('The Process class relies on proc_open, which is not available on your PHP installation.'); } - $this->commandline = $commandline; + if (!\is_array($command)) { + @trigger_error(sprintf('Passing a command as string when creating a "%s" instance is deprecated since Symfony 4.2, pass it as an array of its arguments instead, or use the "Process::fromShellCommandline()" constructor if you need features provided by the shell.', __CLASS__), E_USER_DEPRECATED); + } + + $this->commandline = $command; $this->cwd = $cwd; // on Windows, if the cwd changed via chdir(), proc_open defaults to the dir where PHP was started @@ -163,6 +167,35 @@ class Process implements \IteratorAggregate $this->pty = false; } + /** + * Creates a Process instance as a command-line to be run in a shell wrapper. + * + * Command-lines are parsed by the shell of your OS (/bin/sh on Unix-like, cmd.exe on Windows.) + * This allows using e.g. pipes or conditional execution. In this mode, signals are sent to the + * shell wrapper and not to your commands. + * + * In order to inject dynamic values into command-lines, we strongly recommend using placeholders. + * This will save escaping values, which is not portable nor secure anyway: + * + * $process = Process::fromShellCommandline('my_command "$MY_VAR"'); + * $process->run(null, ['MY_VAR' => $theValue]); + * + * @param string $command The command line to pass to the shell of the OS + * @param string|null $cwd The working directory or null to use the working dir of the current PHP process + * @param array|null $env The environment variables or null to use the same environment as the current PHP process + * @param mixed|null $input The input as stream resource, scalar or \Traversable, or null for no input + * @param int|float|null $timeout The timeout in seconds or null to disable + * + * @throws RuntimeException When proc_open is not installed + */ + public static function fromShellCommandline(string $command, string $cwd = null, array $env = null, $input = null, ?float $timeout = 60) + { + $process = new static(array(), $cwd, $env, $input, $timeout); + $process->commandline = $command; + + return $process; + } + public function __destruct() { $this->stop(0); @@ -892,9 +925,13 @@ class Process implements \IteratorAggregate * @param string|array $commandline The command to execute * * @return self The current Process instance + * + * @deprecated since Symfony 4.2. */ public function setCommandLine($commandline) { + @trigger_error(sprintf('The "%s()" method is deprecated since Symfony 4.2.', __METHOD__), E_USER_DEPRECATED); + $this->commandline = $commandline; return $this; diff --git a/src/Symfony/Component/Process/Tests/ProcessFailedExceptionTest.php b/src/Symfony/Component/Process/Tests/ProcessFailedExceptionTest.php index 25712af7de..d8fdb5c1f0 100644 --- a/src/Symfony/Component/Process/Tests/ProcessFailedExceptionTest.php +++ b/src/Symfony/Component/Process/Tests/ProcessFailedExceptionTest.php @@ -24,7 +24,7 @@ class ProcessFailedExceptionTest extends TestCase */ public function testProcessFailedExceptionThrowsException() { - $process = $this->getMockBuilder('Symfony\Component\Process\Process')->setMethods(array('isSuccessful'))->setConstructorArgs(array('php'))->getMock(); + $process = $this->getMockBuilder('Symfony\Component\Process\Process')->setMethods(array('isSuccessful'))->setConstructorArgs(array(array('php')))->getMock(); $process->expects($this->once()) ->method('isSuccessful') ->will($this->returnValue(true)); @@ -52,7 +52,7 @@ class ProcessFailedExceptionTest extends TestCase $errorOutput = 'FATAL: Unexpected error'; $workingDirectory = getcwd(); - $process = $this->getMockBuilder('Symfony\Component\Process\Process')->setMethods(array('isSuccessful', 'getOutput', 'getErrorOutput', 'getExitCode', 'getExitCodeText', 'isOutputDisabled', 'getWorkingDirectory'))->setConstructorArgs(array($cmd))->getMock(); + $process = $this->getMockBuilder('Symfony\Component\Process\Process')->setMethods(array('isSuccessful', 'getOutput', 'getErrorOutput', 'getExitCode', 'getExitCodeText', 'isOutputDisabled', 'getWorkingDirectory'))->setConstructorArgs(array(array($cmd)))->getMock(); $process->expects($this->once()) ->method('isSuccessful') ->will($this->returnValue(false)); @@ -85,7 +85,7 @@ class ProcessFailedExceptionTest extends TestCase $this->assertEquals( "The command \"$cmd\" failed.\n\nExit Code: $exitCode($exitText)\n\nWorking directory: {$workingDirectory}\n\nOutput:\n================\n{$output}\n\nError Output:\n================\n{$errorOutput}", - $exception->getMessage() + str_replace("'php'", 'php', $exception->getMessage()) ); } @@ -100,7 +100,7 @@ class ProcessFailedExceptionTest extends TestCase $exitText = 'General error'; $workingDirectory = getcwd(); - $process = $this->getMockBuilder('Symfony\Component\Process\Process')->setMethods(array('isSuccessful', 'isOutputDisabled', 'getExitCode', 'getExitCodeText', 'getOutput', 'getErrorOutput', 'getWorkingDirectory'))->setConstructorArgs(array($cmd))->getMock(); + $process = $this->getMockBuilder('Symfony\Component\Process\Process')->setMethods(array('isSuccessful', 'isOutputDisabled', 'getExitCode', 'getExitCodeText', 'getOutput', 'getErrorOutput', 'getWorkingDirectory'))->setConstructorArgs(array(array($cmd)))->getMock(); $process->expects($this->once()) ->method('isSuccessful') ->will($this->returnValue(false)); @@ -131,7 +131,7 @@ class ProcessFailedExceptionTest extends TestCase $this->assertEquals( "The command \"$cmd\" failed.\n\nExit Code: $exitCode($exitText)\n\nWorking directory: {$workingDirectory}", - $exception->getMessage() + str_replace("'php'", 'php', $exception->getMessage()) ); } } diff --git a/src/Symfony/Component/Process/Tests/ProcessTest.php b/src/Symfony/Component/Process/Tests/ProcessTest.php index 9d36d247c5..2492ceb88d 100644 --- a/src/Symfony/Component/Process/Tests/ProcessTest.php +++ b/src/Symfony/Component/Process/Tests/ProcessTest.php @@ -55,13 +55,13 @@ class ProcessTest extends TestCase { try { // Check that it works fine if the CWD exists - $cmd = new Process('echo test', __DIR__); + $cmd = new Process(array('echo', 'test'), __DIR__); $cmd->run(); } catch (\Exception $e) { $this->fail($e); } - $cmd = new Process('echo test', __DIR__.'/notfound/'); + $cmd = new Process(array('echo', 'test'), __DIR__.'/notfound/'); $cmd->run(); } @@ -1447,7 +1447,7 @@ class ProcessTest extends TestCase public function testRawCommandLine() { - $p = new Process(sprintf('"%s" -r %s "a" "" "b"', self::$phpBin, escapeshellarg('print_r($argv);'))); + $p = Process::fromShellCommandline(sprintf('"%s" -r %s "a" "" "b"', self::$phpBin, escapeshellarg('print_r($argv);'))); $p->run(); $expected = << 'Foo', 'BAR' => 'Bar'); $cmd = '\\' === DIRECTORY_SEPARATOR ? 'echo !FOO! !BAR! !BAZ!' : 'echo $FOO $BAR $BAZ'; - $p = new Process($cmd, null, $env); + $p = Process::fromShellCommandline($cmd, null, $env); $p->run(null, array('BAR' => 'baR', 'BAZ' => 'baZ')); $this->assertSame('Foo baR baZ', rtrim($p->getOutput())); $this->assertSame($env, $p->getEnv()); } - /** - * @param string $commandline - * @param null|string $cwd - * @param null|array $env - * @param null|string $input - * @param int $timeout - * @param array $options - * - * @return Process - */ - private function getProcess($commandline, $cwd = null, array $env = null, $input = null, $timeout = 60) + private function getProcess($commandline, string $cwd = null, array $env = null, $input = null, ?int $timeout = 60): Process { - $process = new Process($commandline, $cwd, $env, $input, $timeout); + if (\is_string($commandline)) { + $process = Process::fromShellCommandline($commandline, $cwd, $env, $input, $timeout); + } else { + $process = new Process($commandline, $cwd, $env, $input, $timeout); + } $process->inheritEnvironmentVariables(); if (self::$process) { @@ -1507,10 +1501,7 @@ EOTXT; return self::$process = $process; } - /** - * @return Process - */ - private function getProcessForCode($code, $cwd = null, array $env = null, $input = null, $timeout = 60) + private function getProcessForCode(string $code, string $cwd = null, array $env = null, $input = null, ?int $timeout = 60): Process { return $this->getProcess(array(self::$phpBin, '-r', $code), $cwd, $env, $input, $timeout); }