-
Notifications
You must be signed in to change notification settings - Fork 958
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
Handle spaces in PHP binary path for the wp cli update
command
#5855
base: main
Are you sure you want to change the base?
Conversation
Because the `wp cli update` command needs a Phar to work, trying to mimic what the `wp-cli-bundle` repo does in their Behat steps: - https://github.com/wp-cli/wp-cli-bundle/blob/2638a2601dcbedf7b6a905a57e8761946032505a/features/cli.feature#L31C7-L31C7 While running this locally (m1 Mac), I'm getting an error that looks like maybe the various repos aren't wired up correctly so I wonder if I'll get something different when running in GH workflow.
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.
If we're landing code changes in addition to tests, can you make sure the pull request title and description communicate accordingly?
@@ -357,7 +357,7 @@ public function update( $_, $assoc_args ) { | |||
|
|||
$allow_root = WP_CLI::get_runner()->config['allow-root'] ? '--allow-root' : ''; | |||
$php_binary = Utils\get_php_binary(); | |||
$process = Process::create( "{$php_binary} $temp --info {$allow_root}" ); | |||
$process = Process::create( Utils\esc_cmd( '%s %s --info %s', $php_binary, $temp, $allow_root ) ); |
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.
I think I'm rethinking my original suggestion to use Utils\esc_cmd()
, as the originally proposed escapeshellarg( Utils\get_php_binary() );
covers less surface area.
Thoughts?
Then STDOUT should be: | ||
""" | ||
Success: WP-CLI is at the latest version. | ||
""" |
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.
Can you think of a way to add a scenario covering how the current code breaks?
wp cli update
commandwp cli update
command
Summary
Updates the
wp cli update
command to correctly handle spaces within the path to the PHP binary.Context
Within #5815 , one of the potential fixes was merged (#5818 ) and later reverted (Issue Comment Reverted commit: #5822 )
In the original issue, it was recommended to start testing this part of the code so here's a start!
Notes
I did run into some issues running this test locally, but it's passing here. Part of me wonders if the
make-phar.php
file is too specifically tied to building the phar within the context of a GH workflow?