Skip to content

[Process] Allow inheriting env vars instead of replacing them #19053

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
Jun 15, 2016
Merged

[Process] Allow inheriting env vars instead of replacing them #19053

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
46 changes: 43 additions & 3 deletions src/Symfony/Component/Process/Process.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,7 @@ class Process implements \IteratorAggregate
private $incrementalErrorOutputOffset = 0;
private $tty;
private $pty;
private $inheritEnv = false;

private $useFileHandles = false;
/** @var PipesInterface */
Expand Down Expand Up @@ -267,9 +268,22 @@ 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"));
Copy link
Member

@xabbuh xabbuh Jun 14, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Don't we need one space after a value if it is followed by another key?

Never mind, you use one export per environment variable.

}
$env = null;
} else {
$env = $this->env;
}
if ('\\' === DIRECTORY_SEPARATOR && $this->enhanceWindowsCompatibility) {
$commandline = 'cmd /V:ON /E:ON /D /C "('.$commandline.')';
$commandline = 'cmd /V:ON /E:ON /D /C "('.$envline.$commandline.')';
foreach ($this->processPipes->getFiles() as $offset => $filename) {
$commandline .= ' '.$offset.'>'.ProcessUtils::escapeArgument($filename);
}
Expand All @@ -283,15 +297,17 @@ public function start(callable $callback = null)
$descriptors[3] = array('pipe', 'w');

// See https://unix.stackexchange.com/questions/71205/background-process-pipe-input
$commandline = '{ ('.$this->commandline.') <&3 3<&- 3>/dev/null & } 3<&0;';
$commandline = $envline.'{ ('.$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, $this->env, $this->options);
$this->process = proc_open($commandline, $descriptors, $this->processPipes->pipes, $this->cwd, $env, $this->options);

if (!is_resource($this->process)) {
throw new RuntimeException('Unable to launch a new process.');
Expand Down Expand Up @@ -1197,6 +1213,30 @@ public function setEnhanceSigchildCompatibility($enhance)
return $this;
}

/**
* Sets whether environment variables will be inherited or not.
*
* @param bool $inheritEnv
*
* @return self The current Process instance
*/
public function inheritEnvironmentVariables($inheritEnv = true)
Copy link
Member Author

Choose a reason for hiding this comment

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

I'd personally prefer inheritEnv() here, but I borrowed this name from ProcessBuilder, consistency first :)

{
$this->inheritEnv = (bool) $inheritEnv;

return $this;
}

/**
* Returns whether environment variables will be inherited or not.
*
* @return bool
*/
public function areEnvironmentVariablesInherited()
{
return $this->inheritEnv;
}

/**
* Performs a check between the timeout definition and the time the process started.
*
Expand Down
9 changes: 3 additions & 6 deletions src/Symfony/Component/Process/ProcessBuilder.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -267,14 +267,11 @@ public function getProcess()
$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);

if ($this->inheritEnv) {
Copy link
Member

Choose a reason for hiding this comment

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

you should probably remove the old merging of the environment then

$env = array_replace($_ENV, $_SERVER, $this->env);
} else {
$env = $this->env;
$process->inheritEnvironmentVariables();
}

$process = new Process($script, $this->cwd, $env, $this->input, $this->timeout, $options);

if ($this->outputDisabled) {
$process->disableOutput();
}
Expand Down
25 changes: 2 additions & 23 deletions src/Symfony/Component/Process/Tests/ProcessBuilderTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,17 +17,11 @@ class ProcessBuilderTest extends \PHPUnit_Framework_TestCase
{
public function testInheritEnvironmentVars()
{
$_ENV['MY_VAR_1'] = 'foo';

$proc = ProcessBuilder::create()
->add('foo')
->getProcess();

unset($_ENV['MY_VAR_1']);

$env = $proc->getEnv();
$this->assertArrayHasKey('MY_VAR_1', $env);
$this->assertEquals('foo', $env['MY_VAR_1']);
$this->assertTrue($proc->areEnvironmentVariablesInherited());
}

public function testAddEnvironmentVariables()
Expand All @@ -46,22 +40,7 @@ public function testAddEnvironmentVariables()
;

$this->assertSame($env, $proc->getEnv());
}

public function testProcessShouldInheritAndOverrideEnvironmentVars()
{
$_ENV['MY_VAR_1'] = 'foo';

$proc = ProcessBuilder::create()
->setEnv('MY_VAR_1', 'bar')
->add('foo')
->getProcess();

unset($_ENV['MY_VAR_1']);

$env = $proc->getEnv();
$this->assertArrayHasKey('MY_VAR_1', $env);
$this->assertEquals('bar', $env['MY_VAR_1']);
$this->assertFalse($proc->areEnvironmentVariablesInherited());
}

/**
Expand Down
34 changes: 34 additions & 0 deletions src/Symfony/Component/Process/Tests/ProcessTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1357,6 +1357,40 @@ public function testChainedProcesses()
$this->assertSame('456', $p2->getOutput());
}

public function testInheritEnvEnabled()
{
$process = $this->getProcess(self::$phpBin.' -r '.escapeshellarg('echo serialize($_SERVER);'), null, array('BAR' => 'BAZ'));

putenv('FOO=BAR');

$this->assertSame($process, $process->inheritEnvironmentVariables(1));
$this->assertTrue($process->areEnvironmentVariablesInherited());

$process->run();

$expected = array('BAR' => 'BAZ', 'FOO' => 'BAR');
$env = array_intersect_key(unserialize($process->getOutput()), $expected);

$this->assertSame($expected, $env);
}

public function testInheritEnvDisabled()
{
$process = $this->getProcess(self::$phpBin.' -r '.escapeshellarg('echo serialize($_SERVER);'), null, array('BAR' => 'BAZ'));

putenv('FOO=BAR');

$this->assertFalse($process->areEnvironmentVariablesInherited());

$process->run();

$expected = array('BAR' => 'BAZ', 'FOO' => 'BAR');
$env = array_intersect_key(unserialize($process->getOutput()), $expected);
unset($expected['FOO']);

$this->assertSame($expected, $env);
}

/**
* @param string $commandline
* @param null|string $cwd
Expand Down