feature symfony#21470 [Process] Deprecate not inheriting env vars + compat related settings (nicolas-grekas)

This PR was merged into the 3.3-dev branch.

Discussion
----------

[Process] Deprecate not inheriting env vars + compat related settings

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | no
| Deprecations? | yes
| Tests pass?   | no
| Fixed tickets | -
| License       | MIT
| Doc PR        | -

Turning compat on/off is not a feature in itself.

About env vars: if one has unwanted env vars, one will still be able to remove them explicitly for the command. From my experience, not having eg PATH or HTTP_PROXY, etc. is more problematic. I'd prefer people to care about setting/unsetting the environment vars **they know about**, rather than allowing them to start with no ENV and discover later that they missed setting some var.

Commits
-------

df14451 [Process] Deprecate not inheriting env vars + compat related settings

Author: fabpot

.travis.yml

@@ -96,7 +96,7 @@ script:
     - if [[ ! $deps && ! $PHP = hhvm* ]]; then echo "$COMPONENTS" | parallel --gnu '$PHPUNIT --exclude-group tty,benchmark,intl-data {}'"$REPORT"; fi
     - if [[ ! $deps && ! $PHP = hhvm* ]]; then echo -e "\\nRunning tests requiring tty"; $PHPUNIT --group tty; fi
     - if [[ ! $deps && $PHP = hhvm* ]]; then $PHPUNIT --exclude-group benchmark,intl-data; fi
-    - if [[ ! $deps && $PHP = ${MIN_PHP%.*} ]]; then echo -e "1\\n0" | xargs -I{} sh -c 'echo "\\nPHP --enable-sigchild enhanced={}" && ENHANCE_SIGCHLD={} php-$MIN_PHP/sapi/cli/php .phpunit/phpunit-4.8/phpunit --colors=always src/Symfony/Component/Process/'; fi
+    - if [[ ! $deps && $PHP = ${MIN_PHP%.*} ]]; then echo -e "1\\n0" | xargs -I{} sh -c 'echo "\\nPHP --enable-sigchild enhanced={}" && SYMFONY_DEPRECATIONS_HELPER=weak ENHANCE_SIGCHLD={} php-$MIN_PHP/sapi/cli/php .phpunit/phpunit-4.8/phpunit --colors=always src/Symfony/Component/Process/'; fi
     - if [[ $deps = high ]]; then echo "$COMPONENTS" | parallel --gnu -j10% 'cd {}; composer update --no-progress --ansi; $PHPUNIT --exclude-group tty,benchmark,intl-data'$LEGACY"$REPORT"; fi
     - if [[ $deps = low ]]; then echo "$COMPONENTS" | parallel --gnu -j10% 'cd {}; composer update --no-progress --ansi --prefer-lowest --prefer-stable; $PHPUNIT --exclude-group tty,benchmark,intl-data'"$REPORT"; fi
     # Test the PhpUnit bridge using the original phpunit script

UPGRADE-3.3.md

@@ -63,6 +63,15 @@ HttpKernel
  * The `Psr6CacheClearer::addPool()` method has been deprecated. Pass an array of pools indexed
    by name to the constructor instead.
 
+Process
+-------
+
+ * Not inheriting environment variables is deprecated.
+
+ * Configuring `proc_open()` options is deprecated.
+
+ * Configuring Windows and sigchild compatibility is deprecated - they will be always enabled in 4.0.
+
 Security
 --------
 

UPGRADE-4.0.md

@@ -225,6 +225,15 @@ HttpKernel
  * The `Psr6CacheClearer::addPool()` method has been removed. Pass an array of pools indexed
    by name to the constructor instead.
 
+Process
+-------
+
+ * Environment variables are always inherited in sub-processes.
+
+ * Configuring `proc_open()` options has been removed.
+
+ * Configuring Windows and sigchild compatibility is not possible anymore - they are always enabled.
+
 Security
 --------
 

src/Symfony/Component/Process/CHANGELOG.md

@@ -1,6 +1,14 @@
 CHANGELOG
 =========
 
+3.3.0
+-----
+
+ * deprecated not inheriting environment variables
+ * deprecated configuring `proc_open()` options
+ * deprecated configuring enhanced Windows compatibility
+ * deprecated configuring enhanced sigchild compatibility
+
 2.5.0
 -----
 

src/Symfony/Component/Process/PhpProcess.php

@@ -33,7 +33,7 @@ class PhpProcess extends Process
      * @param int         $timeout The timeout in seconds
      * @param array       $options An array of options for proc_open
      */
-    public function __construct($script, $cwd = null, array $env = null, $timeout = 60, array $options = array())
+    public function __construct($script, $cwd = null, array $env = null, $timeout = 60, array $options = null)
     {
         $executableFinder = new PhpExecutableFinder();
         if (false === $php = $executableFinder->find()) {
@@ -52,6 +52,9 @@ public function __construct($script, $cwd = null, array $env = null, $timeout =
             // command with exec
             $php = 'exec '.$php;
         }
+        if (null !== $options) {
+            @trigger_error(sprintf('The $options parameter of the %s constructor is deprecated since version 3.3 and will be removed in 4.0.', __CLASS__), E_USER_DEPRECATED);
+        }
 
         parent::__construct($php, $cwd, $env, $script, $timeout, $options);
     }

src/Symfony/Component/Process/Process.php

@@ -58,7 +58,7 @@ class Process implements \IteratorAggregate
     private $lastOutputTime;
     private $timeout;
     private $idleTimeout;
-    private $options;
+    private $options = array('suppress_errors' => true);
     private $exitcode;
     private $fallbackStatus = array();
     private $processInformation;
@@ -145,7 +145,7 @@ class Process implements \IteratorAggregate
      *
      * @throws RuntimeException When proc_open is not installed
      */
-    public function __construct($commandline, $cwd = null, array $env = null, $input = null, $timeout = 60, array $options = array())
+    public function __construct($commandline, $cwd = null, array $env = null, $input = null, $timeout = 60, array $options = null)
     {
         if (!function_exists('proc_open')) {
             throw new RuntimeException('The Process class relies on proc_open, which is not available on your PHP installation.');
@@ -171,7 +171,10 @@ public function __construct($commandline, $cwd = null, array $env = null, $input
         $this->pty = false;
         $this->enhanceWindowsCompatibility = true;
         $this->enhanceSigchildCompatibility = '\\' !== DIRECTORY_SEPARATOR && $this->isSigchildEnabled();
-        $this->options = array_replace(array('suppress_errors' => true, 'binary_pipes' => true), $options);
+        if (null !== $options) {
+            @trigger_error(sprintf('The $options parameter of the %s constructor is deprecated since version 3.3 and will be removed in 4.0.', __CLASS__), E_USER_DEPRECATED);
+            $this->options = array_replace($this->options, $options);
+        }
     }
 
     public function __destruct()
@@ -268,26 +271,23 @@ public function start(callable $callback = null)
         $descriptors = $this->getDescriptors();
 
         $commandline = $this->commandline;
-        $envline = '';
 
-        if (null !== $this->env && $this->inheritEnv) {
-            if ('\\' === DIRECTORY_SEPARATOR && !empty($this->options['bypass_shell']) && !$this->enhanceWindowsCompatibility) {
-                throw new LogicException('The "bypass_shell" option must be false to inherit environment variables while enhanced Windows compatibility is off');
-            }
-            $env = '\\' === DIRECTORY_SEPARATOR ? '(SET %s)&&' : 'export %s;';
-            foreach ($this->env as $k => $v) {
-                $envline .= sprintf($env, ProcessUtils::escapeArgument("$k=$v"));
+        $env = $this->env;
+        $envBackup = array();
+        if (null !== $env && $this->inheritEnv) {
+            foreach ($env as $k => $v) {
+                $envBackup[$k] = getenv($v);
+                putenv(false === $v || null === $v ? $k : "$k=$v");
             }
             $env = null;
-        } else {
-            $env = $this->env;
+        } elseif (null !== $env) {
+            @trigger_error(sprintf('Not inheriting environment variables is deprecated since Symfony 3.3 and will always happen in 4.0. Set "Process::inheritEnvironmentVariables()" to true instead.', __METHOD__), E_USER_DEPRECATED);
         }
         if ('\\' === DIRECTORY_SEPARATOR && $this->enhanceWindowsCompatibility) {
-            $commandline = 'cmd /V:ON /E:ON /D /C "('.$envline.$commandline.')';
+            $commandline = 'cmd /V:ON /E:ON /D /C ('.str_replace("\n", ' ', $commandline).')';
             foreach ($this->processPipes->getFiles() as $offset => $filename) {
-                $commandline .= ' '.$offset.'>'.ProcessUtils::escapeArgument($filename);
+                $commandline .= ' '.$offset.'>"'.$filename.'"';
             }
-            $commandline .= '"';
 
             if (!isset($this->options['bypass_shell'])) {
                 $this->options['bypass_shell'] = true;
@@ -297,18 +297,20 @@ public function start(callable $callback = null)
             $descriptors[3] = array('pipe', 'w');
 
             // See https://unix.stackexchange.com/questions/71205/background-process-pipe-input
-            $commandline = $envline.'{ ('.$this->commandline.') <&3 3<&- 3>/dev/null & } 3<&0;';
+            $commandline = '{ ('.$this->commandline.') <&3 3<&- 3>/dev/null & } 3<&0;';
             $commandline .= 'pid=$!; echo $pid >&3; wait $pid; code=$?; echo $code >&3; exit $code';
 
             // Workaround for the bug, when PTS functionality is enabled.
             // @see : https://bugs.php.net/69442
             $ptsWorkaround = fopen(__FILE__, 'r');
-        } elseif ('' !== $envline) {
-            $commandline = $envline.$commandline;
         }
 
         $this->process = proc_open($commandline, $descriptors, $this->processPipes->pipes, $this->cwd, $env, $this->options);
 
+        foreach ($envBackup as $k => $v) {
+            putenv(false === $v ? $k : "$k=$v");
+        }
+
         if (!is_resource($this->process)) {
             throw new RuntimeException('Unable to launch a new process.');
         }
@@ -1089,6 +1091,8 @@ public function getEnv()
      *
      * An environment variable value should be a string.
      * If it is an array, the variable is ignored.
+     * If it is false or null, it will be removed when
+     * env vars are otherwise inherited.
      *
      * That happens in PHP when 'argv' is registered into
      * the $_ENV array for instance.
@@ -1099,15 +1103,7 @@ public function getEnv()
      */
     public function setEnv(array $env)
     {
-        // Process can not handle env values that are arrays
-        $env = array_filter($env, function ($value) {
-            return !is_array($value);
-        });
-
-        $this->env = array();
-        foreach ($env as $key => $value) {
-            $this->env[$key] = (string) $value;
-        }
+        $this->env = $env;
 
         return $this;
     }
@@ -1148,9 +1144,13 @@ public function setInput($input)
      * Gets the options for proc_open.
      *
      * @return array The current options
+     *
+     * @deprecated since version 3.3, to be removed in 4.0.
      */
     public function getOptions()
     {
+        @trigger_error(sprintf('The %s() method is deprecated since version 3.3 and will be removed in 4.0.', __METHOD__), E_USER_DEPRECATED);
+
         return $this->options;
     }
 
@@ -1160,9 +1160,13 @@ public function getOptions()
      * @param array $options The new options
      *
      * @return self The current Process instance
+     *
+     * @deprecated since version 3.3, to be removed in 4.0.
      */
     public function setOptions(array $options)
     {
+        @trigger_error(sprintf('The %s() method is deprecated since version 3.3 and will be removed in 4.0.', __METHOD__), E_USER_DEPRECATED);
+
         $this->options = $options;
 
         return $this;
@@ -1174,9 +1178,13 @@ public function setOptions(array $options)
      * This is true by default.
      *
      * @return bool
+     *
+     * @deprecated since version 3.3, to be removed in 4.0. Enhanced Windows compatibility will always be enabled.
      */
     public function getEnhanceWindowsCompatibility()
     {
+        @trigger_error(sprintf('The %s() method is deprecated since version 3.3 and will be removed in 4.0. Enhanced Windows compatibility will always be enabled.', __METHOD__), E_USER_DEPRECATED);
+
         return $this->enhanceWindowsCompatibility;
     }
 
@@ -1186,9 +1194,13 @@ public function getEnhanceWindowsCompatibility()
      * @param bool $enhance
      *
      * @return self The current Process instance
+     *
+     * @deprecated since version 3.3, to be removed in 4.0. Enhanced Windows compatibility will always be enabled.
      */
     public function setEnhanceWindowsCompatibility($enhance)
     {
+        @trigger_error(sprintf('The %s() method is deprecated since version 3.3 and will be removed in 4.0. Enhanced Windows compatibility will always be enabled.', __METHOD__), E_USER_DEPRECATED);
+
         $this->enhanceWindowsCompatibility = (bool) $enhance;
 
         return $this;
@@ -1198,9 +1210,13 @@ public function setEnhanceWindowsCompatibility($enhance)
      * Returns whether sigchild compatibility mode is activated or not.
      *
      * @return bool
+     *
+     * @deprecated since version 3.3, to be removed in 4.0. Sigchild compatibility will always be enabled.
      */
     public function getEnhanceSigchildCompatibility()
     {
+        @trigger_error(sprintf('The %s() method is deprecated since version 3.3 and will be removed in 4.0. Sigchild compatibility will always be enabled.', __METHOD__), E_USER_DEPRECATED);
+
         return $this->enhanceSigchildCompatibility;
     }
 
@@ -1214,9 +1230,13 @@ public function getEnhanceSigchildCompatibility()
      * @param bool $enhance
      *
      * @return self The current Process instance
+     *
+     * @deprecated since version 3.3, to be removed in 4.0.
      */
     public function setEnhanceSigchildCompatibility($enhance)
     {
+        @trigger_error(sprintf('The %s() method is deprecated since version 3.3 and will be removed in 4.0. Sigchild compatibility will always be enabled.', __METHOD__), E_USER_DEPRECATED);
+
         $this->enhanceSigchildCompatibility = (bool) $enhance;
 
         return $this;
@@ -1231,6 +1251,10 @@ public function setEnhanceSigchildCompatibility($enhance)
      */
     public function inheritEnvironmentVariables($inheritEnv = true)
     {
+        if (!$inheritEnv) {
+            @trigger_error(sprintf('Not inheriting environment variables is deprecated since Symfony 3.3 and will always happen in 4.0. Set "Process::inheritEnvironmentVariables()" to true instead.', __METHOD__), E_USER_DEPRECATED);
+        }
+
         $this->inheritEnv = (bool) $inheritEnv;
 
         return $this;
@@ -1240,9 +1264,13 @@ public function inheritEnvironmentVariables($inheritEnv = true)
      * Returns whether environment variables will be inherited or not.
      *
      * @return bool
+     *
+     * @deprecated since version 3.3, to be removed in 4.0. Environment variables will always be inherited.
      */
     public function areEnvironmentVariablesInherited()
     {
+        @trigger_error(sprintf('The %s() method is deprecated since version 3.3 and will be removed in 4.0. Environment variables will always be inherited.', __METHOD__), E_USER_DEPRECATED);
+
         return $this->inheritEnv;
     }
 

src/Symfony/Component/Process/ProcessBuilder.php

@@ -26,7 +26,7 @@ class ProcessBuilder
     private $env = array();
     private $input;
     private $timeout = 60;
-    private $options = array();
+    private $options;
     private $inheritEnv = true;
     private $prefix = array();
     private $outputDisabled = false;
@@ -120,9 +120,13 @@ public function setWorkingDirectory($cwd)
      * @param bool $inheritEnv
      *
      * @return $this
+     *
+     * @deprecated since version 3.3, to be removed in 4.0.
      */
     public function inheritEnvironmentVariables($inheritEnv = true)
     {
+        @trigger_error(sprintf('The %s() method is deprecated since version 3.3 and will be removed in 4.0.', __METHOD__), E_USER_DEPRECATED);
+
         $this->inheritEnv = $inheritEnv;
 
         return $this;
@@ -217,9 +221,13 @@ public function setTimeout($timeout)
      * @param string $value The option value
      *
      * @return $this
+     *
+     * @deprecated since version 3.3, to be removed in 4.0.
      */
     public function setOption($name, $value)
     {
+        @trigger_error(sprintf('The %s() method is deprecated since version 3.3 and will be removed in 4.0.', __METHOD__), E_USER_DEPRECATED);
+
         $this->options[$name] = $value;
 
         return $this;
@@ -262,12 +270,10 @@ public function getProcess()
             throw new LogicException('You must add() command arguments before calling getProcess().');
         }
 
-        $options = $this->options;
-
         $arguments = array_merge($this->prefix, $this->arguments);
         $script = implode(' ', array_map(array(__NAMESPACE__.'\\ProcessUtils', 'escapeArgument'), $arguments));
 
-        $process = new Process($script, $this->cwd, $this->env, $this->input, $this->timeout, $options);
+        $process = new Process($script, $this->cwd, $this->env, $this->input, $this->timeout, $this->options);
 
         if ($this->inheritEnv) {
             $process->inheritEnvironmentVariables();

src/Symfony/Component/Process/Tests/ProcessBuilderTest.php

@@ -15,13 +15,23 @@
 
 class ProcessBuilderTest extends \PHPUnit_Framework_TestCase
 {
+    /**
+     * @group legacy
+     */
     public function testInheritEnvironmentVars()
     {
         $proc = ProcessBuilder::create()
             ->add('foo')
             ->getProcess();
 
         $this->assertTrue($proc->areEnvironmentVariablesInherited());
+
+        $proc = ProcessBuilder::create()
+            ->add('foo')
+            ->inheritEnvironmentVariables(false)
+            ->getProcess();
+
+        $this->assertFalse($proc->areEnvironmentVariablesInherited());
     }
 
     public function testAddEnvironmentVariables()
@@ -35,12 +45,10 @@ public function testAddEnvironmentVariables()
             ->add('command')
             ->setEnv('foo', 'bar2')
             ->addEnvironmentVariables($env)
-            ->inheritEnvironmentVariables(false)
             ->getProcess()
         ;
 
         $this->assertSame($env, $proc->getEnv());
-        $this->assertFalse($proc->areEnvironmentVariablesInherited());
     }
 
     /**