8000 [Process] Inherit env vars by default in PhpProcess by nicolas-grekas · Pull Request #16288 · symfony/symfony · GitHub
[go: up one dir, main page]

Skip to content

[Process] Inherit env vars by default in PhpProcess #16288

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.

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
Oct 23, 2015
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
3 changes: 1 addition & 2 deletions src/Symfony/Component/BrowserKit/Client.php
Original file line number Diff line number Diff line change
Expand Up @@ -334,8 +334,7 @@ public function request($method, $uri, array $parameters = array(), array $files
*/
protected function doRequestInProcess($request)
{
// We set the TMPDIR (for Macs) and TEMP (for Windows), because on these platforms the temp directory changes based on the user.
$process = new PhpProcess($this->getScript($request), null, array('TMPDIR' => sys_get_temp_dir(), 'TEMP' => sys_get_temp_dir()));
$process = new PhpProcess($this->getScript($request), null, null);
Copy link
Member

Choose a reason for hiding this comment

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

Don't we need to keep the TMPDIR/TEMPDIR? We need to see when and why it was added, but I suppose it was there for a reason.

Copy link
Member

Choose a reason for hiding this comment

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

@fabpot I guess it is because the environment was not inherited by default, and so we forced inheriting part of it.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, this seemed to be the reason (see 1cec45c and #1785).

Copy link
Member Author

Choose a reason for hiding this comment

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

yep, this was just a workaround against a bug fixed a few lines below (PhpProcess defaulting to not inheriting the env)

$process->run();

if (!$process->isSuccessful() || !preg_match('/^O\:\d+\:/', $process->getOutput())) {
Expand Down
2 changes: 1 addition & 1 deletion src/Symfony/Component/BrowserKit/composer.json
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
"symfony/dom-crawler": "~2.0,>=2.0.5"
},
"require-dev": {
"symfony/process": "~2.0,>=2.0.5",
"symfony/process": "~2.3.34|~2.7,>=2.7.6",
Copy link
Member

Choose a reason for hiding this comment

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

We should really start to use the caret operator.

Copy link
Member Author

Choose a reason for hiding this comment

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

someone should open a PR :)

"symfony/css-selector": "~2.0,>=2.0.5"
},
"suggest": {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -283,7 +283,7 @@ private static function getClassHierarchy(\ReflectionClass $class)

$traits = array();

if (function_exists('get_declared_traits')) {
if (method_exists('ReflectionClass', 'getTraits')) {
Copy link
Member Author

Choose a reason for hiding this comment

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

not related but still worth cleaning: get_declared_traits is not used after this check; ReflectionClass::getTraits is.

foreach ($classes as $c) {
foreach (self::resolveDependencies(self::computeTraitDeps($c), $c) as $trait) {
if ($trait !== $c) {
Expand Down
12 changes: 6 additions & 6 deletions src/Symfony/Component/Process/PhpProcess.php
Original file line number Diff line number Diff line change
Expand Up @@ -27,13 +27,13 @@ class PhpProcess extends Process
/**
* Constructor.
*
* @param string $script The PHP script to run (as a string)
* @param string $cwd The working directory
* @param array $env The environment variables
* @param int $timeout The timeout in seconds
* @param array $options An array of options for proc_open
* @param string $script The PHP script to run (as a string)
* @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 $options An array of options for proc_open
*/
public function __construct($script, $cwd = null, array $env = array(), $timeout = 60, array $options = array())
public function __construct($script, $cwd = null, array $env = null, $timeout = 60, array $options = array())
{
$executableFinder = new PhpExecutableFinder();
if (false === $php = $executableFinder->find()) {
Expand Down
4 changes: 2 additions & 2 deletions src/Symfony/Component/Process/Process.php
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,7 @@ class Process
*
* @param string $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 inherit
* @param array|null $env The environment variables or null to use the same environment as the current PHP process
* @param string|null $stdin The STDIN content
* @param int|float|null $timeout The timeout in seconds or null to disable
* @param array $options An array of options for proc_open
Expand Down Expand Up @@ -851,7 +851,7 @@ public function setEnv(array $env)

$this->env = array();
foreach ($env as $key => $value) {
$this->env[(binary) $key] = (binary) $value;
$this->env[$key] = (string) $value;
Copy link
Member Author

Choose a reason for hiding this comment

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

this is strictly equivalent

}

return $this;
Expand Down
0