[Process] Added ProcessUtils::escapeArgument() to fix the bug in escapeshellarg() function on Windows #6796

Merged
merged 1 commit into from Apr 21, 2013

Conversation

Projects
None yet
3 participants
@hason
Contributor

hason commented Jan 18, 2013

Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets
License MIT
Doc PR
@vicb

View changes

src/Symfony/Component/Process/Tests/ProcessBuilderTest.php
+
+ /**
+ * @test
+ */

This comment has been minimized.

Show comment Hide comment
@vicb

vicb Jan 18, 2013

Contributor

this is not really the Sf way, I have a PR pending with a refactoring, you can take a look at it

@vicb

vicb Jan 18, 2013

Contributor

this is not really the Sf way, I have a PR pending with a refactoring, you can take a look at it

@vicb

This comment has been minimized.

Show comment Hide comment
@vicb

vicb Jan 18, 2013

Contributor

Ah windows.... Could you send the PR against 2.1. Before you might want to wait for @fabpot to comment on #6793 and rebase on it if accepted.

Contributor

vicb commented Jan 18, 2013

Ah windows.... Could you send the PR against 2.1. Before you might want to wait for @fabpot to comment on #6793 and rebase on it if accepted.

@hason

This comment has been minimized.

Show comment Hide comment
@hason

hason Mar 15, 2013

Contributor

ping @fabpot, is it really necessary to run eg. git log --format=format:%H (used in project gitonomy/gitlib)

Contributor

hason commented Mar 15, 2013

ping @fabpot, is it really necessary to run eg. git log --format=format:%H (used in project gitonomy/gitlib)

fabpot added a commit that referenced this pull request Apr 21, 2013

merged branch hason/process (PR #6796)
This PR was merged into the master branch.

Discussion
----------

[Process] Added ProcessUtils::escapeArgument() to fix the bug in escapeshellarg() function on Windows

| Q             | A
| ------------- | ---
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets |
| License       | MIT
| Doc PR        |

Commits
-------

a557b89 [Process] Added ProcessUtils::escapeArgument() to fix the bug in escapeshellarg() function on Windows

@fabpot fabpot merged commit a557b89 into symfony:master Apr 21, 2013

1 check failed

default The Travis build failed
Details

fabpot added a commit that referenced this pull request Aug 8, 2013

merged branch helmer/processutils_patch (PR #8691)
This PR was submitted for the master branch but it was merged into the 2.3 branch instead (closes #8691).

Discussion
----------

[Process] Fix empty process argument escaping on Windows

PR #6796 introduced a regression on Windows platform (symfony 2.3.0+), forgetting to escape explicitly requested empty arguments to ```""``` and return an empty string instead.

This behaviour is imo unintended and renders the component unusable for corner-cases, where you indeed want to specify an empty argument. One good example of this is assetic and compass, which (up to this day) relies on existence of empty parameters on windows.

Some panic and related links on the topic:
kriswallsmith/assetic#455
kriswallsmith/assetic#471
http://stackoverflow.com/questions/17001517/symfony2-3-with-compass ([diff](http://marker.to/KrctHM))
http://stackoverflow.com/questions/17135219/symfony-2-compass-with-assetic-on-windows-xp

Please consider a merge to master and a backport to 2.3.x

Commits
-------

a91537a Fix empty process argument escaping on Windows
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment