Fix PowerShell command substitution argument parsing#6227
Fix PowerShell command substitution argument parsing#6227
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.
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.
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.
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.