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

escapeshellcmd: add warning about spaces in paths on Windows #511

Conversation

jrfnl
Copy link
Contributor

@jrfnl jrfnl commented Apr 2, 2021

It is a known issue that spaces are not escaped in shell commands, which can be especially problematic on Windows.
This adds a warning about this behaviour to the function, including a way to solve this in userland code.

Ref: https://bugs.php.net/bug.php?id=43261 (last two comments)

Also see: squizlabs/PHP_CodeSniffer#3214

It is a known issue that spaces are not escaped in shell commands, which can be especially problematic on Windows.
This adds a warning about this behaviour to the function, including a way to solve this in userland code.

Ref: https://bugs.php.net/bug.php?id=43261 (last two comments)

Also see: squizlabs/PHP_CodeSniffer#3214
@jrfnl jrfnl force-pushed the feature/escapeshellcmd-add-warning-about-spaces-vs-windows branch from ae677e1 to 1fc3c1e Compare April 2, 2021 14:48
@cmb69
Copy link
Member

cmb69 commented Apr 2, 2021

Thank you! However, that wouldn't work if the complete command line is passed, would it? I think that needs clarification.

@jrfnl
Copy link
Contributor Author

jrfnl commented Apr 2, 2021

@cmb69 Good point. I just tried with an arbitrary command I had been running anyway and Windows 10 seems to handle it fine:

image

Happy to run additional tests with more complex examples to be sure. Suggestions for commands to test are welcome.

Copy link
Member

@cmb69 cmb69 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, right, the escape character substitution happens before the command is going to be executed, so everything is fine with this PR.

@cmb69 cmb69 merged commit 1ea4e4f into php:master Apr 5, 2021
@jrfnl jrfnl deleted the feature/escapeshellcmd-add-warning-about-spaces-vs-windows branch April 5, 2021 17:10
@jrfnl
Copy link
Contributor Author

jrfnl commented Apr 5, 2021

Thanks @cmb69

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.

2 participants