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] Fixed escaping arguments on Windows when inheritEnvironmentVariables is set to false#22614
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
nicolas-grekas commentedMay 2, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Calling |
maryo commentedMay 7, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
OK. I actually don't use this functionality :-). I just noticed it was not working when I was debugging the failing tests and considered it a BC break. I've updated the PR to fix this. |
| $var =$uid.++$varCount; | ||
| putenv("$var=\"$value\""); | ||
| if ($env ===null) { |
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.
yoda style: null === $env
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.
updated
| if ($env ===null) { | ||
| putenv("$var=$value"); | ||
| }else { | ||
| $env[$var] =$value; |
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.
Because of line 1656 above, $value will be quoted. It shouldn't, isn't 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.
@nicolas-grekas
I thought the same at the beginning but the argument needs to be escaped even it's passed using the!var! notation (and stored inside the env vars). The tests don't pass without the quotes. BTW I am not exactly sure why it's done using the!var! workarround. Looks like a clever trick and perhaps you are it's author (due to github history/annotations). Is it somehow related to how inconsistently Windows commands handle unescaping of double quotes? (Should be mostly triple quotes and i've found an article describing it but it's not exactly specified so some commands handle it differently). I'm just curious :-)
xabbuh commentedMay 22, 2017
@maryo Can you rebase here? I have fixed the base branch given that master now is for Symfony 4.0, but the PR now contains unrelated commits. |
nicolas-grekas commentedMay 22, 2017
pleaserebase on 3.3 (there should be no merge commit it the PR history) |
…tEnvironmentVariables is set to false
nicolas-grekas left a comment
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.
👍 (understood & rebased)
nicolas-grekas commentedMay 22, 2017
Thank you@maryo. |
…EnvironmentVariables is set to false (maryo)This PR was merged into the 3.3 branch.Discussion----------[Process] Fixed escaping arguments on Windows when inheritEnvironmentVariables is set to false| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | no| Fixed tickets || License | MIT| Doc PR |I've added a FAILING testcase on Windows. It incorrectly substitutes an argument containing a quotation mark probably assuming it's an env var needed to backup when inheritEnvironmentVariables is set to false.Commits-------26032ef [Process] Fixed incorrectly escaping arguments on Windows when inheritEnvironmentVariables is set to false
I've added a FAILING testcase on Windows. It incorrectly substitutes an argument containing a quotation mark probably assuming it's an env var needed to backup when inheritEnvironmentVariables is set to false.