Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
this is strictly equivalent
There was a problem hiding this comment.
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.
nicolas-grekas commentedOct 19, 2015
Seeparagonie/random_compat#68 andhttps://bugs.php.net/70742 for reasons why this is important. |
paragonie-scott commentedOct 19, 2015
stof commentedOct 19, 2015
@paragonie-scott this is a version issue for the inter-package dependencies |
495f77a to0852e66CompareThere was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
There was a problem hiding this comment.
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)
xabbuh commentedOct 23, 2015
👍 |
xabbuh commentedOct 23, 2015
Status: Reviewed |
Tobion commentedOct 23, 2015
👍 |
Tobion commentedOct 23, 2015
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.