Escape spaces in PHP binary path #149
Conversation
|
@azatakmyradov I suggest you have a read through the discussion in response to this PR and then revisit your patch. |
|
@jrfnl sorry I didn't see that PR. I used |
There was a problem hiding this comment.
@jrfnl sorry I didn't see that PR. I used
escapeshellcmdto escape the string and ^ or \ for spaces depending on the os. I also extracted it to a seperate function so I can write tests for it.
@azatakmyradov No worries, I figured as much. Thanks for updating the PR and adding tests.
What is your opinion on this remark I made in the other PR ?
I'm wondering about is whether this is the right place to do the escaping. Shouldn't the escaping be done at the point when the input is actually used (not just stored and passed around).
On a practical note: I wonder whether it would be better to target the develop branch. Note: the test framework used has changed to plain PHPUnit for develop.
As the replacement function uses regexes, I'd very much would like the tests to be expanded to not just test the "happy path", but also make sure that no replacements are made when they shouldn't be made.
| */ | ||
| public static function escapeString(string $path, string $os = PHP_OS) | ||
| { | ||
| if (stripos(strtoupper($os), 'WIN') === 0) { |
There was a problem hiding this comment.
| if (stripos(strtoupper($os), 'WIN') === 0) { | |
| if (stripos($os, 'WIN') === 0) { |
stripos() is already a case-insensitive comparison, so no need for the strtoupper().
There was a problem hiding this comment.
@jrfnl Yes I agree with you that it might make sense to escape it when it's actually used. First place I encountered the error was when running php artisan insights which in turn runs php vendor/bin/parallel-lint FILE_NAME_HERE. I think it's caused by parseArguments call in Application.php line 36 at which point php executable is already set. I guess I could escape it after because I see phpExecutable property is public.
At this point I can't use the package out of the box unless I pass override the php path manually using -p flag. However, I have to change script inside vendor directory if I were to use phpinsights by Nuno Madaruno which depends on this package.
What do you think?
There was a problem hiding this comment.
seems the comment is misplaced. the discussion thread was started about not using strtoupper:
| */ | ||
| public static function escapeString(string $path, string $os = PHP_OS) | ||
| { | ||
| $path = trim($path); |
There was a problem hiding this comment.
now my program can't be in /home/user/bin . it's unrealistic, but you remove this option with this code without any good reason.
|
maybe worth checking out and having some thoughts: |
| { | ||
| $path = " path with\spaces "; | ||
|
|
||
| Assert::equal(Settings::escapeString($path), "path\\ with\\\spaces"); |
There was a problem hiding this comment.
doesn't seem to be correct, you imho \s should be encoded as \\s or even \\\\s since you enclosed it in " not '.
This PR escapes spaces in binary path.
When you have space in your PHP binary path (Ex: /**/Library/Application Support/Herd/bin/php82'). I get an error output: