Introduce `wp cli has-command` to detect whether a command is registered by Sidsector9 · Pull Request #4349 · wp-cli/wp-cli

Sidsector9 · 2017-09-13T09:43:55Z

I've created a recursive function that traverses the array of the dump output by self::command_to_array( WP_CLI::get_root_command() )

See #4283

Tested locally
Functional tests

gitlost · 2017-09-14T10:45:31Z

php/commands/src/CLI_Command.php

+	 *
+	 * @subcommand has-command
+	 *
+	 * @when before_wp_load


Having before_wp_load is an issue as it won't detect commands added in plugins/themes. Actually, this is an issue for cmd-dump as well as the whole CLI_Command class is marked as before_wp_load. So I think the before_wp_load needs to be removed from the CLI_Command class and added to the individual commands, except for this command and cmd-dump, where after_wp_load should be used.

Actually, this is an issue for cmd-dump as well

Historically, cmd-dump has only been used for exporting bundled command definition to documentation, so it hasn't actually been an issue.

Oh, ok - I'll open an issue re individual function whens not being honoured if there's a class when as it probably needs discussion.

gitlost · 2017-09-14T10:45:37Z

php/commands/src/CLI_Command.php

+		/**
+		 * Get dump as array.
+		 */
+		$json_decoded = self::command_to_array( WP_CLI::get_root_command() );


gitlost · 2017-09-14T10:46:17Z

php/commands/src/CLI_Command.php

+		/**
+		 * If command is input as a string, then explode it into array.
+		 */
+		$command = ( 1 === count( $_ ) ) ? explode( ' ', $_[0] ) : $_;


It'd be simpler just to do $args = explode( ' ', implode( ' ', $_ ) ); which allows any old arg format.

gitlost · 2017-09-14T10:48:55Z

php/commands/src/CLI_Command.php

+		/**
+		 * If command doesn't start with WP, then halt script execution and exit with status 1.
+		 */
+		if ( 'wp' !== $command[0] ) {


It'd be nicer to make prefixing with wp optional by doing if ( 'wp' === $args[0] ) { array_shift( $args ); }

gitlost · 2017-09-14T10:50:16Z

php/commands/src/CLI_Command.php

+			WP_CLI::halt( 1 );
+		}
+
+		/**


Re-using Runner::find_command_to_run() would make sense here:

WP_CLI::halt( is_array( WP_CLI::get_runner()->find_command_to_run( $args ) ) ? 0 : 1 );

making the recursive_search() function below and the command_to_array() call above unnecessary.

This is just such a beautiful way to do this, I did not know about this!

gitlost · 2017-09-14T10:53:55Z

php/commands/src/CLI_Command.php

+	 */
+	public function has_command( $_, $assoc_args ) {
+
+		/**


In general in-function comments shouldn't be docblocks and if they're one liners should use // I think.

schlessera · 2017-09-14T17:27:32Z

Looks good so far.

A general note:
wp is not actually part of the command name, it is the name of the executable that was used to start up the WP-CLI framework. So, something like wp post list could just as well be php ~/wp-cli.phar post list.

As regards to testing, you could go through the following steps:

check whether the command scaffold package can be found - this should return an error, as the command is not part of the bundled commands
install the wp-cli/scaffold-package-command
check again whether the command scaffold package can be found - this time it should be positive

danielbachhuber

👍 Looking good, some minor nits.

danielbachhuber · 2017-09-15T05:38:17Z

php/commands/src/CLI_Command.php

+	 * @when after_wp_load
+	 *
+	 * @param array $_          Array of positional arguments.
+	 * @param array $assoc_args Array of associative arguments.


We should remove this argument definition, as it's not a pattern we have elsewhere.

danielbachhuber · 2017-09-15T05:38:29Z

php/commands/src/CLI_Command.php

+	 * <command_name>...
+	 * : The command
+	 *
+	 * ## EXAMPLES


Needs another space after this heading.

danielbachhuber · 2017-09-15T05:40:38Z

php/commands/src/CLI_Command.php

+	 *
+	 * ## EXAMPLES
+	 *     wp cli has-command "wp site delete"
+	 *     wp cli has-command wp site delete


Each of these examples should have some explanation:

# The "site delete" command is registered $ wp cli has-command "site delete" $ echo $? 0 # The "foo bar" command is not registered $ wp cli has-command "foo bar" $ echo $? 1

danielbachhuber · 2017-09-15T05:42:02Z

php/commands/src/CLI_Command.php

+		// If command is input as a string, then explode it into array.
+		$command = explode( ' ', implode( ' ', $_ ) );
+
+		if ( 'wp' === $command[0] ) {


I don't think we should accept "wp" to this command.

Sidsector9 · 2017-09-15T11:58:58Z

Made changes as per review

schlessera · 2017-09-16T08:12:35Z

features/cli.feature

+    When I try `{PHAR_PATH} cli has-command scaffold package`
+    Then the return code should be 1
+
+    When I run `{PHAR_PATH} package install git@github.com:wp-cli/scaffold-package-command.git`


Use a short package identifier here:
When I run {PHAR_PATH} package install wp-cli/scaffold-package-command`

@schlessera, Why didn't git clone work?

Probably just because Travis hit the git rate limiting.

With the short package identifier, in this case, you fetch the ZIP through the old package index, instead of the GH versioned source.

schlessera · 2017-09-18T06:46:06Z

php/commands/src/CLI_Command.php

+	 *
+	 * ## EXAMPLES
+	 *
+	 *     The "site delete" command is registered.


For the examples, you should follow the notation convention, as @danielbachhuber had provided:

Comments start with #, and the command starts with the "basic" prompt $. Lines without either are output.

Here's one example:

/** * ## EXAMPLES * * # The "site delete" command is registered. * $ wp cli has-command "site delete" * $ echo $? * 0 */

Here's a link to the documentation for this: https://make.wordpress.org/cli/handbook/documentation-standards/#command-annotation

danielbachhuber · 2017-09-19T16:37:39Z

features/cli.feature

+  Scenario: Ability to detect a WP-CLI registered command
+    Given an empty directory
+    And save the {SRC_DIR}/VERSION file as {TRUE_VERSION}
+    And a new Phar with version "1.3.0"


Why do you need to generate a new Phar here?

You're correct, there wasn't a need to generate a phar. I replaced it with wp @danielbachhuber

Sidsector9 added 2 commits September 13, 2017 15:09

GH#4283 Function to detect a registered WP-CLI command

cbb3450

GH#4283 Corrected white spaces

030f7e7

Sidsector9 requested review from a team, danielbachhuber and schlessera September 13, 2017 10:20

GH#4283 Corrected inline documentation

967dd89

Sidsector9 removed request for danielbachhuber and schlessera September 13, 2017 14:16

gitlost suggested changes Sep 14, 2017

View reviewed changes

GH#4283 Improvised logic by using internal function

a5adcb7

schlessera added the command:cli-has-command Related to 'cli has-command' command label Sep 14, 2017

schlessera added this to the 1.4.0 milestone Sep 14, 2017

danielbachhuber requested changes Sep 15, 2017

View reviewed changes

gitlost approved these changes Sep 15, 2017

View reviewed changes

gitlost mentioned this pull request Sep 15, 2017

A class @when overrides individual method @whens. #4350

Closed

Sidsector9 added 2 commits September 15, 2017 16:14

GH#4283 Added functional test and made minor changes

aa619e6

GH#4283 Corrected command in feature file

4a48a48

schlessera requested changes Sep 16, 2017

View reviewed changes

Sidsector9 added 2 commits September 18, 2017 11:34

GH#4283 Updated feature file

ac0cb85

GH#4283 Added short package identifier

4142212

schlessera requested changes Sep 18, 2017

View reviewed changes

GH#4283 Updated examples in doc-block

f99a682

danielbachhuber requested changes Sep 19, 2017

View reviewed changes

danielbachhuber changed the title ~~GH#4283 Function to detect a registered WP-CLI command~~ Introduce wp cli has-command to detect whether a command is registered Sep 19, 2017

GH#4283 Updated feature file

35965c1

schlessera approved these changes Sep 24, 2017

View reviewed changes

Add comment indicator before the sentence.

851c98f

danielbachhuber self-requested a review September 26, 2017 00:18

danielbachhuber approved these changes Sep 26, 2017

View reviewed changes

schlessera merged commit 4738303 into wp-cli:master Sep 26, 2017

schlessera mentioned this pull request Sep 26, 2017

New command to detect a command #4283

Closed

Conversation

Sidsector9 commented Sep 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Sidsector9 Sep 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

schlessera commented Sep 14, 2017

Uh oh!

danielbachhuber left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Sidsector9 commented Sep 15, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

schlessera Sep 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Sidsector9 commented Sep 13, 2017 •

edited

Loading

Sidsector9 Sep 14, 2017 •

edited

Loading

schlessera Sep 18, 2017 •

edited

Loading