-
Notifications
You must be signed in to change notification settings - Fork 1k
Fix PowerShell command substitution argument parsing #6227
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
base: main
Are you sure you want to change the base?
Conversation
…umeric IDs Co-authored-by: swissspidy <841956+swissspidy@users.noreply.github.com>
Co-authored-by: swissspidy <841956+swissspidy@users.noreply.github.com>
Co-authored-by: swissspidy <841956+swissspidy@users.noreply.github.com>
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
/gemini review |
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.
Code Review
This pull request effectively addresses a command substitution issue on Windows PowerShell where multiple numeric IDs are passed as a single string. The fix is well-targeted, splitting arguments only on Windows and only when they match a strict pattern of space-separated numbers. The changes are accompanied by comprehensive unit and feature tests, ensuring correctness and preventing regressions. I have one minor suggestion to improve performance in the argument splitting logic. Overall, this is a great improvement for Windows users.
| // Split on whitespace and add each ID as a separate argument | ||
| $ids = preg_split( '/\s+/', $arg, -1, PREG_SPLIT_NO_EMPTY ); | ||
| if ( false !== $ids ) { | ||
| $split_args = array_merge( $split_args, $ids ); |
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.
For better performance, especially when dealing with a large number of IDs, consider using array_push() with the splat operator (...) instead of array_merge(). array_merge() creates a new array in each iteration inside this loop, which can be inefficient. array_push() modifies the array in place and is generally more performant for appending elements.
array_push( $split_args, ...$ids );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.
Pull request overview
This pull request fixes a Windows PowerShell-specific issue where command substitution (e.g., $(wp post list --format=ids)) returns space-separated IDs as a single string instead of individual arguments, breaking commands that expect multiple ID parameters.
Changes:
- Added Windows-specific argument splitting logic in
Runner.phpto detect and split space-separated numeric IDs - Added comprehensive unit tests in
WindowsArgsTest.phpto validate splitting behavior across various scenarios - Added Behat integration tests in
command.featureto test real-world command execution
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| php/WP_CLI/Runner.php | Adds Windows-specific logic in back_compat_conversions() to split arguments matching the pattern of space-separated numeric IDs (2+ numbers), using a conservative regex that only affects the intended use case |
| tests/WindowsArgsTest.php | Adds unit tests covering all edge cases including single IDs, non-numeric strings, mixed arguments, tabs/spaces, and leading/trailing spaces, following established codebase testing patterns |
| features/command.feature | Adds Behat integration tests that simulate Windows behavior using WP_CLI_TEST_IS_WINDOWS environment variable, testing both Windows and non-Windows scenarios |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
On Windows PowerShell, command substitution returns space-separated values as a single string rather than individual arguments. This breaks commands expecting multiple IDs:
Changes
Runner.php: Added Windows-specific argument splitting in
back_compat_conversions()/^\d+(\s+\d+)+$/Tests: Added unit tests covering splitting behavior and edge cases
Implementation
This mirrors Unix shell argument expansion behavior without affecting other platforms or non-ID arguments.
Warning
Firewall rules blocked me from connecting to one or more addresses (expand for details)
I tried to connect to the following addresses, but was blocked by firewall rules:
example.com/usr/bin/php php vendor/bin/phpunit --color=always --bootstrap ./vendor/wp-cli/wp-cli-tests/tests/bootstrap.php k/wp-cli/wp-cli/php/commands/src/CLI_Cache_Command.php k/wp-cli/wp-cli/php/commands/src/Help_Command.php ndor/bin/git k/wp-cli/wp-cli/git on.php k/wp-cli/wp-cli/-a Extension.php nFin�� ionsIterator.php-v nArrayIterator.php sh iner/Specificatigit nIterator.php ificationLocator-v grep(dns block)/usr/bin/php php vendor/bin/phpunit --color=always --bootstrap ./vendor/wp-cli/wp-cli-tests/tests/bootstrap.php(dns block)nosuchhost_asdf_asdf_asdf.com/usr/bin/php php vendor/bin/phpunit --color=always --bootstrap ./vendor/wp-cli/wp-cli-tests/tests/bootstrap.php k/wp-cli/wp-cli/php/commands/src/CLI_Cache_Command.php k/wp-cli/wp-cli/php/commands/src/Help_Command.php ndor/bin/git k/wp-cli/wp-cli/git on.php k/wp-cli/wp-cli/-a Extension.php nFin�� ionsIterator.php-v nArrayIterator.php sh iner/Specificatigit nIterator.php ificationLocator-v grep(dns block)/usr/bin/php php vendor/bin/phpunit --color=always --bootstrap ./vendor/wp-cli/wp-cli-tests/tests/bootstrap.php(dns block)If you need me to access, download, or install something from one of these locations, you can either:
Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.