-
Notifications
You must be signed in to change notification settings - Fork 23
Escape spaces in PHP binary path #149
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
Changes from all commits
079eb1a
8a8ac6d
9498c76
d0060cc
66fadde
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -130,7 +130,7 @@ public static function parseArguments(array $arguments) | |||||
|
||||||
// Use the currently invoked php as the default if possible | ||||||
if (defined('PHP_BINARY')) { | ||||||
$settings->phpExecutable = PHP_BINARY; | ||||||
$settings->phpExecutable = self::escapeString(PHP_BINARY); | ||||||
} | ||||||
|
||||||
foreach ($arguments as $argument) { | ||||||
|
@@ -235,6 +235,24 @@ public static function getPathsFromStdIn() | |||||
$lines = explode("\n", rtrim($content)); | ||||||
return array_map('rtrim', $lines); | ||||||
} | ||||||
|
||||||
/** | ||||||
* @param string $path | ||||||
* @param string $os | ||||||
* @return string | ||||||
*/ | ||||||
public static function escapeString(string $path, string $os = PHP_OS) | ||||||
{ | ||||||
$path = trim($path); | ||||||
|
||||||
if (stripos(strtoupper($os), 'WIN') === 0) { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @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 At this point I can't use the package out of the box unless I pass override the php path manually using What do you think? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. seems the comment is misplaced. the discussion thread was started about not using strtoupper: |
||||||
$path = str_replace('^\\', '\\', preg_replace('`(?<!^) `', '^ ', escapeshellcmd($path))); | ||||||
} else { | ||||||
$path = preg_replace('`(?<!\\\) `', '\ ', escapeshellcmd($path)); | ||||||
} | ||||||
|
||||||
return $path; | ||||||
} | ||||||
} | ||||||
|
||||||
class ArrayIterator extends \ArrayIterator | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -144,6 +144,41 @@ class SettingsParseArgumentsTest extends Tester\TestCase | |
|
||
Assert::equal($expectedSettings->syntaxErrorCallbackFile, $settings->syntaxErrorCallbackFile); | ||
} | ||
|
||
public function testEscapingPathForUnixSystems() | ||
{ | ||
Settings::escapeString('path with spaces'); | ||
|
||
$php_path = "path with/spaces between/php.exe"; | ||
Assert::equal(Settings::escapeString($php_path, 'Darwin'), "path\\ with/spaces\\ between/php.exe"); | ||
} | ||
|
||
public function testEscapingPathForWindows() | ||
{ | ||
Settings::escapeString('path with spaces'); | ||
|
||
$php_path = "path with\spaces between\php.exe"; | ||
Assert::equal(Settings::escapeString($php_path, 'Windows'), 'path^ with\\\spaces^ between\\\php.exe'); | ||
} | ||
|
||
public function testEscapingEmptyPath() | ||
{ | ||
Assert::equal(Settings::escapeString(''), ''); | ||
} | ||
|
||
public function testEscapingPathWithSpecialChars() | ||
{ | ||
$path = "path/with/special&characters"; | ||
|
||
Assert::equal(Settings::escapeString($path), "path/with/special\&characters"); | ||
} | ||
|
||
public function testEscapingPathWithLeadingTrailingSpaces() | ||
{ | ||
$path = " path with\spaces "; | ||
|
||
Assert::equal(Settings::escapeString($path), "path\\ with\\\spaces"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. doesn't seem to be correct, you imho |
||
} | ||
} | ||
|
||
$testCase = new SettingsParseArgumentsTest; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.