Handle spaces in PHP binary path for the wp cli update command#5855
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.
danielbachhuber
left a comment
There was a problem hiding this comment.
If we're landing code changes in addition to tests, can you make sure the pull request title and description communicate accordingly?
| Then STDOUT should be: | ||
| """ | ||
| Success: WP-CLI is at the latest version. | ||
| """ |
There was a problem hiding this comment.
Can you think of a way to add a scenario covering how the current code breaks?
wp cli update commandwp cli update command
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Pull request overview
This PR updates the wp cli update command to properly handle spaces in the PHP binary path by using Utils\esc_cmd() to escape shell arguments, and adds test coverage for the CLI update functionality.
- Replaces string interpolation with
Utils\esc_cmd()to properly escape the PHP binary path, temp file path, and allow-root flag - Adds a new feature test file with scenarios for CLI update functionality
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| php/commands/src/CLI_Command.php | Updates command construction to use Utils\esc_cmd() for proper shell escaping when verifying downloaded phar |
| features/cli-update.feature | Adds test scenarios for CLI update command, including error handling and update workflow |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Summary
Updates the
wp cli updatecommand 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.phpfile is too specifically tied to building the phar within the context of a GH workflow?