Reorder command arguments in Pressable_Site_WP_CLI_Command_Run class #100
Conversation
- Swapped the order of 'site' and 'wp-cli-command' arguments for clarity. - This order makes more sense and also matches a similar command for WPCOM wpcom:run-site-wp-cli-command - Also made Site a mandatory value, instead of optional
WalkthroughThe CLI command WPCOMSpecialProjects\CLI\Command\Pressable_Site_WP_CLI_Command_Run updated its configure() method to change argument order and requirements. The first argument is now site (REQUIRED), followed by wp-cli-command (REQUIRED). Previously, wp-cli-command was REQUIRED and site was OPTIONAL. No other logic, error handling, or functionality changes are indicated. Pre-merge checks (3 passed)✅ Passed Checks (3 passed)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs. ✨ Finishing Touches
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
commands/Pressable_Site_WP_CLI_Command_Run.php (3)
74-87: Warn when both --multiple and a single site are providedRight now the provided site is silently ignored when --multiple is set. Emit a clear notice to avoid confusion.
Apply:
protected function initialize( InputInterface $input, OutputInterface $output ): void { $this->skip_output = get_bool_input( $input, 'skip-output' ); $this->multiple = $input->getOption( 'multiple' ); - if ( null === $this->multiple ) { + if ( null === $this->multiple ) { // If multiple is not set, treat it as a single site operation $site = get_pressable_site_input( $input, fn() => $this->prompt_site_input( $input, $output ) ); $input->setArgument( 'site', $site ); $this->sites = array( $site ); - } elseif ( 'all' === $this->multiple ) { + } elseif ( 'all' === $this->multiple ) { + if ( $input->getArgument( 'site' ) ) { + $output->writeln( '<comment>--multiple provided; ignoring the single `site` argument.</comment>' ); + } $this->sites = get_pressable_sites(); } else { + if ( $input->getArgument( 'site' ) ) { + $output->writeln( '<comment>--multiple provided; ignoring the single `site` argument.</comment>' ); + } $this->sites = $this->get_sites_from_multiple_input(); }
89-95: Make ‘wp’ stripping saferPattern '/^wp/' can mangle commands starting with tokens like 'wpcom...'. Restrict to the standalone 'wp' token.
Apply:
- $this->wp_command = \trim( \preg_replace( '/^wp/', '', \trim( $this->wp_command ) ) ); + $this->wp_command = \trim( \preg_replace( '/^wp(\s+|$)/', '', \trim( $this->wp_command ) ) );
74-95: Optional: Back-compat shim for old argument orderTo reduce breakage, auto-detect the old order and swap with a deprecation notice (grace period).
Apply near the start of initialize(), after reading $this->multiple:
// Back-compat: if multiple not set and the first arg looks like a WP-CLI command, swap. if ( null === $this->multiple ) { $maybe_wp = (string) $input->getArgument( 'site' ); if ( preg_match( '/^wp(\s+|$)|^(plugin|theme|option|user|post|search-replace)\b/', trim( $maybe_wp ) ) ) { $output->writeln( '<comment>Detected legacy argument order. Please use: site first, then command. (Swapping args for now.)</comment>' ); $orig_cmd = $maybe_wp; $input->setArgument( 'site', get_pressable_site_input( $input, fn() => $this->prompt_site_input( $input, $output ) ) ); $input->setArgument( 'wp-cli-command', $orig_cmd ); } }
| $this->addArgument( 'site', InputArgument::REQUIRED, 'The domain or numeric Pressable ID of the site to open the shell to.' ) | ||
| ->addArgument( 'wp-cli-command', InputArgument::REQUIRED, 'The WP-CLI command to run.' ); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Argument reorder + site now REQUIRED — LGTM
Matches the PR objective and fixes the misrouting bug. This is a breaking CLI change; ensure docs/usages are updated.
Run to find and update usages/examples:
🏁 Script executed:
#!/usr/bin/env bash
# Find command invocations/usages to update argument order
rg -nC2 --pretty $'pressable:run-site-wp-cli-command\\b'
rg -nC2 --pretty $'wpcom:run-site-wp-cli-command\\b'Length of output: 531
Update documentation and examples to reflect new argument order
Search for any external docs or example scripts referencing pressable:run-site-wp-cli-command or wpcom:run-site-wp-cli-command—internal code search only found the command definitions in commands/Pressable_Site_WP_CLI_Command_Run.php and commands/WPCOM_Site_WP_CLI_Command_Run.php. Ensure all user-facing documentation (READMEs, docs/cli guides, sample scripts) are updated so the site argument comes first.
🤖 Prompt for AI Agents
In commands/Pressable_Site_WP_CLI_Command_Run.php around lines 64-65, the
argument order was changed so the 'site' argument is now first but external
documentation and sample scripts may still show the old order; search the
repository for uses of pressable:run-site-wp-cli-command and
wpcom:run-site-wp-cli-command (README, docs/, sample scripts, CI scripts) and
update any examples and docs to list the arguments with 'site' first followed by
'wp-cli-command', ensuring any displayed command usage, help text, and example
invocations reflect the new order.
| $this->addOption( 'multiple', null, InputOption::VALUE_REQUIRED, 'Determines whether the `site` argument is optional or not. Accepted values are `all` or a comma-separated list of site IDs or domains.' ) | ||
| ->addOption( 'skip-output', null, InputOption::VALUE_NONE, 'Skip outputting the response to the console.' ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update --multiple help text to reflect new semantics
It still says it “Determines whether the site argument is optional,” which is no longer true. Clarify that the site argument is ignored when --multiple is used.
Apply:
- $this->addOption( 'multiple', null, InputOption::VALUE_REQUIRED, 'Determines whether the `site` argument is optional or not. Accepted values are `all` or a comma-separated list of site IDs or domains.' )
+ $this->addOption( 'multiple', null, InputOption::VALUE_REQUIRED, 'Run on multiple sites: `all` or a comma-separated list of site IDs/domains. When set, the single `site` argument is ignored.' )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| $this->addOption( 'multiple', null, InputOption::VALUE_REQUIRED, 'Determines whether the `site` argument is optional or not. Accepted values are `all` or a comma-separated list of site IDs or domains.' ) | |
| ->addOption( 'skip-output', null, InputOption::VALUE_NONE, 'Skip outputting the response to the console.' ); | |
| $this->addOption( 'multiple', null, InputOption::VALUE_REQUIRED, 'Run on multiple sites: `all` or a comma-separated list of site IDs/domains. When set, the single `site` argument is ignored.' ) | |
| ->addOption( 'skip-output', null, InputOption::VALUE_NONE, 'Skip outputting the response to the console.' ); |
🤖 Prompt for AI Agents
In commands/Pressable_Site_WP_CLI_Command_Run.php around lines 67 to 68, the
--multiple option help text incorrectly states it "Determines whether the `site`
argument is optional"; update the option description to clarify that when
--multiple is used the `site` argument is ignored and to describe accepted
values (e.g. `all` or a comma-separated list of site IDs or domains), so the new
help text explicitly says the site argument is ignored when --multiple is
provided and lists the valid values.
|
The site value was optional because there should be a prompt for it 🤔 I think there was something that, if it's mandatory, it has to be provided and can't be added through the prompt. |
Changes proposed
wpcom:run-site-wp-cli-commandHow to reproduce issue
team51 pressable:run-site-wp-cli-command "https://mishtalk.com" "wp plugin list"wp+plugin+listas the Site ID:❌ API error (404 https://opsoasis.wpspecialprojects.com/wp-json/wpcomsp/pressable/v1/sites/wp+plugin+list): {"code":"rest_no_route","message":"No route was found matching the URL and request method.","data":{"status":404}}The command for WPCOM,
wpcom:run-site-wp-cli-command, does take as the first argument the Site URL.Summary by CodeRabbit