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

[Process] Fixed escaping arguments on Windows when inheritEnvironmentVariables is set to false #22614

Merged
merged 1 commit into from May 22, 2017

Conversation

Projects
None yet
4 participants
@maryo
Contributor

maryo commented May 2, 2017

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.

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas May 2, 2017

Member

Calling inheritEnvironmentVariables(false) is deprecated, which means you're mixing new features and deprecated ones. This should be avoided and is barely supported. Still, would you be able to provide a patch for this issue?

Member

nicolas-grekas commented May 2, 2017

Calling inheritEnvironmentVariables(false) is deprecated, which means you're mixing new features and deprecated ones. This should be avoided and is barely supported. Still, would you be able to provide a patch for this issue?

@nicolas-grekas nicolas-grekas added this to the 3.3 milestone May 4, 2017

@maryo maryo changed the title from [Process] Added testcase failing on Windows when inheritEnvironmentVariables is set to false to [Process] Fixed escaping arguments on Windows when inheritEnvironmentVariables is set to false May 7, 2017

@maryo

This comment has been minimized.

Show comment
Hide comment
@maryo

maryo May 7, 2017

Contributor

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.

Contributor

maryo commented May 7, 2017

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.

Show outdated Hide outdated src/Symfony/Component/Process/Process.php
$var = $uid.++$varCount;
putenv("$var=\"$value\"");
if ($env === null) {

This comment has been minimized.

@nicolas-grekas

nicolas-grekas May 15, 2017

Member

yoda style: null === $env

@nicolas-grekas

nicolas-grekas May 15, 2017

Member

yoda style: null === $env

This comment has been minimized.

@maryo

maryo May 17, 2017

Contributor

updated

@maryo

maryo May 17, 2017

Contributor

updated

Show outdated Hide outdated src/Symfony/Component/Process/Process.php
if ($env === null) {
putenv("$var=$value");
} else {
$env[$var] = $value;

This comment has been minimized.

@nicolas-grekas

nicolas-grekas May 15, 2017

Member

Because of line 1656 above, $value will be quoted. It shouldn't, isn't it?

@nicolas-grekas

nicolas-grekas May 15, 2017

Member

Because of line 1656 above, $value will be quoted. It shouldn't, isn't it?

This comment has been minimized.

@maryo

maryo May 17, 2017

Contributor

@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 :-)

@maryo

maryo May 17, 2017

Contributor

@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 xabbuh changed the base branch from master to 3.3 May 22, 2017

@xabbuh

This comment has been minimized.

Show comment
Hide comment
@xabbuh

xabbuh May 22, 2017

Member

@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.

Member

xabbuh commented May 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

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas May 22, 2017

Member

please rebase on 3.3 (there should be no merge commit it the PR history)

Member

nicolas-grekas commented May 22, 2017

please rebase on 3.3 (there should be no merge commit it the PR history)

@nicolas-grekas

👍 (understood & rebased)

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas May 22, 2017

Member

Thank you @maryo.

Member

nicolas-grekas commented May 22, 2017

Thank you @maryo.

@nicolas-grekas nicolas-grekas merged commit 26032ef into symfony:3.3 May 22, 2017

2 of 3 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details

nicolas-grekas added a commit that referenced this pull request May 22, 2017

bug #22614 [Process] Fixed escaping arguments on Windows when inherit…
…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

@fabpot fabpot referenced this pull request May 29, 2017

Merged

Release v3.3.0 #22949

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment