Changed taskExec to use symfony process placeholders

src/Common/CommandArguments.php

@@ -15,7 +15,48 @@ trait CommandArguments
     protected $arguments = '';
 
     /**
-     * Pass argument to executable. Its value will be automatically escaped.
+     * @var array
+     */
+    protected $argumentsEnv = [];
+
+
+    /**
+     * Adds an argument and its placeholder value, to be executed
+     *
+     * @param string $argument
+     * @param string|null $key
+     * @param string|null $prefix
+     * @param string|null $separator
+     * @return void
+     */
+    protected function addArgument($argument, $key = null, $prefix = null, $separator = ' ')
+    {
+        if (is_null($key)) {
+            $key = "arg" . md5($argument);
+        }
+
+        $this->arguments .= null == $prefix ? '' : $separator . $prefix;
+        if (!is_null($argument)) {
+            $this->arguments .= ' "${:' . $key . '}"';
+            $this->argumentsEnv[$key] = $argument;
+        }
+    }
+
+    /**
+     * Escape a command line argument.
+     *
+     * @param string $argument
+     * @return string
+     *
+     * @deprecated Use \Robo\Common\ProcessUtils::escapeArgument() instead.
+     */
+    public static function escape($argument)
+    {
+        return ProcessUtils::escapeArgument($argument);
+    }
+
+    /**
+     * Pass argument to executable. It will be changed to a placeholder.
      *
      * @param string $arg
      *
@@ -27,8 +68,8 @@ public function arg($arg)
     }
 
     /**
-     * Pass methods parameters as arguments to executable. Argument values
-     * are automatically escaped.
+     * Pass methods parameters as arguments to executable. Argument are
+     * automatically passed as placeholders
      *
      * @param string|string[] $args
      *
@@ -40,7 +81,10 @@ public function args($args)
         if (!is_array($args)) {
             $args = $func_args;
         }
-        $this->arguments .= ' ' . implode(' ', array_map('static::escape', $args));
+
+        foreach ($args as $arg) {
+            $this->addArgument($arg);
+        }
         return $this;
     }
 
@@ -58,25 +102,9 @@ public function rawArg($arg)
         return $this;
     }
 
-    /**
-     * Escape the provided value, unless it contains only alphanumeric
-     * plus a few other basic characters.
-     *
-     * @param string $value
-     *
-     * @return string
-     */
-    public static function escape($value)
-    {
-        if (preg_match('/^[a-zA-Z0-9\/\.@~_-]+$/', $value)) {
-            return $value;
-        }
-        return ProcessUtils::escapeArgument($value);
-    }
-
     /**
      * Pass option to executable. Options are prefixed with `--` , value can be provided in second parameter.
-     * Option values are automatically escaped.
+     * Option values are automatically passed as placeholders.
      *
      * @param string $option
      * @param string $value
@@ -89,15 +117,15 @@ public function option($option, $value = null, $separator = ' ')
         if ($option !== null and strpos($option, '-') !== 0) {
             $option = "--$option";
         }
-        $this->arguments .= null == $option ? '' : " " . $option;
-        $this->arguments .= null == $value ? '' : $separator . static::escape($value);
+
+        $this->addArgument($value, null, $option, $separator);
         return $this;
     }
 
     /**
      * Pass multiple options to executable. The associative array contains
      * the key:value pairs that become `--key value`, for each item in the array.
-     * Values are automatically escaped.
+     * Values are passed as placeholders.
      *
      * @param array $options
      * @param string $separator

src/Common/ProcessUtils.php

@@ -10,9 +10,9 @@
 use Symfony\Component\Process\Exception\InvalidArgumentException;
 
 /**
- * ProcessUtils is a bunch of utility methods. We want to allow Robo 1.x
- * to work with Symfony 4.x while remaining backwards compatibility. This
- * requires us to replace some deprecated functionality removed in Symfony.
+ * ProcessUtils is a bunch of utility methods.
+ * These methods are usually private, and are needed to execute and escape
+ * some functions for display purposes
  */
 class ProcessUtils
 {
@@ -24,58 +24,47 @@ private function __construct()
     }
 
     /**
-     * Escapes a string to be used as a shell argument.
-     *
-     * This method is a copy of a method that was deprecated by Symfony 3.3 and
-     * removed in Symfony 4; it will be removed once there is an actual
-     * replacement for escapeArgument.
-     *
-     * @param string $argument
-     *   The argument that will be escaped.
+     * Symfony Process has a private method to replace Placeholders in command lines,
+     * which we use to rebuild a Command Description.
      *
+     * @param string $commandline
+     * @param array $env
      * @return string
-     *   The escaped argument.
      */
-    public static function escapeArgument($argument)
+    public static function replacePlaceholders(string $commandline, array $env)
     {
-        //Fix for PHP bug #43784 escapeshellarg removes % from given string
-        //Fix for PHP bug #49446 escapeshellarg doesn't work on Windows
-        //@see https://bugs.php.net/bug.php?id=43784
-        //@see https://bugs.php.net/bug.php?id=49446
-        if ('\\' === DIRECTORY_SEPARATOR) {
-            if ('' === $argument) {
-                return escapeshellarg($argument);
-            }
-
-            $escapedArgument = '';
-            $quote = false;
-            foreach (preg_split('/(")/', $argument, -1, PREG_SPLIT_NO_EMPTY | PREG_SPLIT_DELIM_CAPTURE) as $part) {
-                if ('"' === $part) {
-                    $escapedArgument .= '\\"';
-                } elseif (self::isSurroundedBy($part, '%')) {
-                    // Avoid environment variable expansion
-                    $escapedArgument .= '^%"' . substr($part, 1, -1) . '"^%';
-                } else {
-                    // escape trailing backslash
-                    if ('\\' === substr($part, -1)) {
-                        $part .= '\\';
-                    }
-                    $quote = true;
-                    $escapedArgument .= $part;
-                }
+        return preg_replace_callback('/"\$\{:([_a-zA-Z]++[_a-zA-Z0-9]*+)\}"/', function ($matches) use ($commandline, $env) {
+            if (!isset($env[$matches[1]]) || false === $env[$matches[1]]) {
+                throw new InvalidArgumentException(sprintf('Command line is missing a value for parameter "%s": ', $matches[1]).$commandline);
             }
-            if ($quote) {
-                $escapedArgument = '"' . $escapedArgument . '"';
-            }
-
-            return $escapedArgument;
-        }
 
-        return "'" . str_replace("'", "'\\''", $argument) . "'";
+            return self::escapeArgument($env[$matches[1]]);
+        }, $commandline);
     }
 
-    private static function isSurroundedBy($arg, $char)
+    /**
+     * Used by Symfony Process to form the final command line, with escaped parameters.
+     * Robo uses placeholders to handle the process arguments without escaping the whole string.
+     *
+     * @param string|null $argument
+     * @return string
+     */
+    public static function escapeArgument(?string $argument): string
     {
-        return 2 < strlen($arg) && $char === $arg[0] && $char === $arg[strlen($arg) - 1];
+        if ('' === $argument || null === $argument) {
+            return '""';
+        }
+        if ('\\' !== \DIRECTORY_SEPARATOR) {
+            return "'".str_replace("'", "'\\''", $argument)."'";
+        }
+        if (false !== strpos($argument, "\0")) {
+            $argument = str_replace("\0", '?', $argument);
+        }
+        if (!preg_match('/[\/()%!^"<>&|\s]/', $argument)) {
+            return $argument;
+        }
+        $argument = preg_replace('/(\\\\+)$/', '$1$1', $argument);
+
+        return '"'.str_replace(['"', '^', '%', '!', "\n"], ['""', '"^^"', '"^%"', '"^!"', '!LF!'], $argument).'"';
     }
 }

src/Task/Base/Exec.php

@@ -11,6 +11,7 @@
 use Robo\Result;
 use Robo\Common\CommandReceiver;
 use Robo\Common\ExecOneCommand;
+use Robo\Common\ProcessUtils;
 
 /**
  * Executes shell script. Closes it when running in background mode.
@@ -98,8 +99,9 @@ public function background($arg = true)
      */
     protected function getCommandDescription()
     {
-        return $this->getCommand();
+        return ProcessUtils::replacePlaceholders($this->getCommand(), $this->argumentsEnv);
     }
+
     /**
      * {@inheritdoc}
      */
@@ -132,7 +134,7 @@ public function run()
     {
         $this->hideProgressIndicator();
         // TODO: Symfony 4 requires that we supply the working directory.
-        $result_data = $this->execute(Process::fromShellCommandline($this->getCommand(), getcwd()));
+        $result_data = $this->execute(Process::fromShellCommandline($this->getCommand(), getcwd(), $this->argumentsEnv));
         $result = new Result(
             $this,
             $result_data->getExitCode(),

src/Task/Remote/Rsync.php

@@ -6,6 +6,7 @@
 use Robo\Task\BaseTask;
 use Robo\Exception\TaskException;
 use Robo\Common\ExecOneCommand;
+use Robo\Common\ProcessUtils;
 
 /**
  * Executes rsync in a flexible manner.
@@ -432,6 +433,11 @@ public function run()
         return $this->executeCommand($command);
     }
 
+    public function getCommandDescription()
+    {
+        return ProcessUtils::replacePlaceholders($this->getCommand(), $this->argumentsEnv);
+    }
+
     /**
      * Returns command that can be executed.
      * This method is used to pass generated command from one task to another.

tests/integration/ExecTest.php

@@ -29,6 +29,10 @@ public function testExecLsCommand()
 
     public function testMultipleEnvVars()
     {
+        if (strncasecmp(PHP_OS, 'WIN', 3) == 0) {
+          $this->markTestSkipped('Environment variables is not supported on Windows.');
+        }
+
         $task = $this->taskExec('env')->interactive(false);
         $task->env('FOO', 'BAR');
         $task->env('BAR', 'BAZ');

tests/phpunit/Common/CommandArgumentsTest.php

@@ -13,58 +13,33 @@ class CommandArgumentsTest extends TestCase
     public function casesArgs() {
         return [
             'no arguments' => [
-                ' ',
-                ' ',
+                '',
+                '',
                 [],
             ],
             'empty string' => [
-                " ''",
                 ' ""',
-                [''],
+                ' ""',
+                [""],
             ],
             'space' => [
                 " ' '",
                 ' " "',
                 [' '],
             ],
-            'no escape - a' => [
-                " a",
-                " a",
-                ['a'],
-            ],
-            'no escape - A' => [
-                " A",
-                " A",
-                ['A'],
-            ],
-            'no escape - 0' => [
-                " 0",
-                " 0",
-                ['0'],
-            ],
-            'no escape - --' => [
-                " --",
-                " --",
-                ['--'],
-            ],
-            'no escape - @_~.' => [
-                " @_~.",
-                " @_~.",
-                ['@_~.'],
-            ],
             '$' => [
                 " 'a\$b'",
-                ' "a$b"',
+                ' a$b',
                 ['a$b'],
             ],
             '*' => [
                 " 'a*b'",
-                ' "a*b"',
+                ' a*b',
                 ['a*b'],
             ],
             'multi' => [
-                " '' a '\$PATH'",
-                ' "" a "$PATH"',
+                ' "" \'a\' \'$PATH\'',
+                ' "" a $PATH',
                 ['', 'a', '$PATH'],
             ],
         ];