[Process] Inherit env vars by default in PhpProcess #16288
Conversation
There was a problem hiding this comment.
this is strictly equivalent
nicolas-grekas
force-pushed
the
env-php
branch
from
ce7bcaf to
586bf84
Compare
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 |
nicolas-grekas
force-pushed
the
env-php
branch
3 times, most recently
from
495f77a to
0852e66
Compare
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 :)
nicolas-grekas
force-pushed
the
env-php
branch
from
0852e66 to
97ed91d
Compare
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)
nicolas-grekas
force-pushed
the
env-php
branch
from
97ed91d to
ab8cc29
Compare
nicolas-grekas
changed the title
|
👍 |
|
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
7 participants
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.