[Process] Inherit env vars by default in PhpProcess #16288
Conversation
| @@ -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; | |||
There was a problem hiding this comment.
this is strictly equivalent
ce7bcaf to
586bf84
Compare
| @@ -283,7 +283,7 @@ private static function getClassHierarchy(\ReflectionClass $class) | |||
|
|
|||
| $traits = array(); | |||
|
|
|||
| if (function_exists('get_declared_traits')) { | |||
| if (method_exists('ReflectionClass', 'getTraits')) { | |||
There was a problem hiding this comment.
not related but still worth cleaning: get_declared_traits is not used after this check; ReflectionClass::getTraits is.
|
See paragonie/random_compat#68 and https://bugs.php.net/70742 for reasons why this is important. |
|
@paragonie-scott this is a version issue for the inter-package dependencies |
495f77a to
0852e66
Compare
| @@ -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", | |||
There was a problem hiding this comment.
We should really start to use the caret operator.
There was a problem hiding this comment.
someone should open a PR :)
0852e66 to
97ed91d
Compare
| @@ -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); | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@fabpot I guess it is because the environment was not inherited by default, and so we forced inheriting part of it.
There was a problem hiding this comment.
yep, this was just a workaround against a bug fixed a few lines below (PhpProcess defaulting to not inheriting the env)
97ed91d to
ab8cc29
Compare
|
👍 |
|
Status: Reviewed |
|
👍 |
|
Thank you @nicolas-grekas. |
…as-grekas) This PR was merged into the 2.3 branch. Discussion ---------- [Process] Inherit env vars by default in PhpProcess | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - This is the cause of our failures on Windows, where the SYSTEMROOT env var is mandatory for mcrypt_create_iv to work. I don't know why the browserkit client is run with no env inheritance and this looks like a bug. Same for PhpProcess emptying the env by default, this looks like a bug, esp. since the parent `Process` class defaults to inheriting the env. Tests are not broken by this change. Commits ------- ab8cc29 [Process] Inherit env vars by default in PhpProcess
This is the cause of our failures on Windows, where the SYSTEMROOT env var is mandatory for mcrypt_create_iv to work.
I don't know why the browserkit client is run with no env inheritance and this looks like a bug.
Same for PhpProcess emptying the env by default, this looks like a bug, esp. since the parent
Processclass defaults to inheriting the env.Tests are not broken by this change.