Skip to content
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

BUGFIX: Properly escape sub process variables on windows #182

Merged
merged 1 commit into from Dec 15, 2015

Conversation

albe
Copy link
Member

@albe albe commented Dec 14, 2015

Windows SET command does not parse out quotes of the variable value but rather treats them as part of the value, which currently results in an error on windows since the fix for FLOW-381:

Flow could not create the directory
""C:/workspace/Flow/Data/Temporary"/Development/".

Note the extra quotes around the temporary base path.

This change fixes that by properly escaping the SET command arguments on windows by using escapeshellcmd instead of escapeshellarg.

FLOW-425 #close
FLOW-381 #comment Regression fix for compilation on Windows

Windows SET command does not parse out quotes of the variable value but rather treats them as part of the value,
which currently results in an error on windows since the fix for FLOW-381:

  Flow could not create the directory
  ""C:/workspace/Flow/Data/Temporary"/Development/".

Note the extra quotes around the temporary base path.

This change fixes that by properly escaping the SET command arguments on windows by using escapeshellcmd instead
of escapeshellarg.
@dlubitz
Copy link
Contributor

dlubitz commented Dec 14, 2015

👍 by reading and testing on local windows 8/php5.5
Should be merged asap, because Flow 3.0.3 does not compile on windows with this bug.

@kdambekalns
Copy link
Member

No need to test this on non-windows, because it doesn't affect that setup. And no way to test this for me, since I don't have Windows. But I guess @albe and @dlubitz can be trusted, so… 👍

kdambekalns added a commit that referenced this pull request Dec 15, 2015
BUGFIX: Properly escape sub process variables on windows

Windows SET command does not parse out quotes of the variable value but rather treats them as part of the value, which currently results in an error on windows since the fix for FLOW-381:

    Flow could not create the directory
    ""C:/workspace/Flow/Data/Temporary"/Development/".

Note the extra quotes around the temporary base path.

This change fixes that by properly escaping the SET command arguments on windows by using escapeshellcmd instead of escapeshellarg.

FLOW-381 #comment Regression fix for compilation on Windows
@kdambekalns kdambekalns merged commit 6da51aa into neos:3.0 Dec 15, 2015
@bwaidelich
Copy link
Member

Thanks a lot for this!!

@aertmann
Copy link
Collaborator

https://jira.neos.io/browse/FLOW-425 was just reported, this change fixes that right?

@dlubitz
Copy link
Contributor

dlubitz commented Dec 17, 2015

Exactly. We should create tickets for all bugs, even if we get it fixed directly. Jira should be the first point to figure out if a bug is already known to prevent multiple efforts.

----- Reply message -----
Von: "Aske Ertmann" notifications@github.com
An: "neos/flow-development-collection" flow-development-collection@noreply.github.com
Cc: "dlubitz" lubitz@vivomedia.de
Betreff: [flow-development-collection] BUGFIX: Properly escape sub process variables on windows (#182)
Datum: Do., Dez. 17, 2015 06:52
https://jira.neos.io/browse/FLOW-425 was just reported, this change fixes that right?


Reply to this email directly or view it on GitHub.

@simonschaufi
Copy link
Contributor

i spend quite a lot of time debugging this since the error output form subrequests are not very clear (and i figured out that flow calls many times itself, maybe thats the reason why its so slow on windows...)

Is it possible to do such tests with travis or does travis only support linux?

@kitsunet
Copy link
Member

No Windows Support in Travis.

@albe
Copy link
Member Author

albe commented Dec 17, 2015

Ah, my fault, should have checked jira before creating the PR so quickly. Thx @kdambekalns for adding the missing jira link

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants