Handle exiting in error state on help & give suggestions. by gitlost · Pull Request #4266 · wp-cli/wp-cli

gitlost · 2017-08-03T16:22:18Z

Adds handling of help command errors on exiting, giving suggestions.

Removes the handling in the help command itself, as it's not best placed to deal with it. Also renames its find_subcommand() method to find_command_to_help() as it's very confusing when it's not a CompositeCommand.

Disables for now the auto_check_update(), as it presents the user with an odd experience when triggered. It's hard to know where to put it.

danielbachhuber · 2017-08-03T17:01:16Z

php/WP_CLI/Runner.php

 			) ) {
-			$this->auto_check_update();
-			$this->_run_command();
+			// $this->auto_check_update(); // Disable for now? If it hits the cache threshold and determines to update it exits with no recourse for the user.


Let's keep $this->auto_check_update() in please, until we discuss further. This is WP-CLI's auto-update mechanism. It's non-trivial to disable it, particularly given we're so close to a release date.

danielbachhuber · 2017-08-03T17:02:13Z

php/WP_CLI/Runner.php

 	}

-	private function _run_command() {
+	private function _run_command( $exiting = true ) {


This seems like ambiguous behavior. Is there a better way to implement it?

I'll remove the arg and rename the method _run_command_and_exit() and have the case after the 1st help attempt just use run_command( args...).

danielbachhuber · 2017-08-03T17:16:41Z

Generally, this pull request probably needs some discussion before further development. I'd be hesitant to land something like this without detailed conversation because it's a critical execution path and only has good, but not great, test coverage.

…exit.

danielbachhuber · 2017-08-03T18:46:58Z

php/WP_CLI/Runner.php

+			if ( is_string( $possible_suggestion ) ) {
+				WP_CLI::error( $possible_suggestion );
+			}
+			// Should never happen.


How can we get to 100% confidence?

Well if it does find the command then it doesn't return...if it doesn't then it does...then find_command_to_run() runs the same loop as find_command_to_help() so it will fail too and return a suggestion...100%!

Ok, then we can remove the nonsensical error message.

Removed amazingly amusing error message and now Help_Command uses Runner::find_command_to_run() so should be 100% confident.

danielbachhuber · 2017-08-03T18:47:20Z

php/commands/help.php

-		if ( function_exists( 'add_filter' ) ) {
-			$command_string = implode( ' ', $args );
-			\WP_CLI::error( sprintf( "'%s' is not a registered wp command.", $command_string ) );
-		}


Why is this removed?

Because it's a poor relation of what find_command_to_run() is doing...

Can you clarify in greater detail? I'm hesitant to remove this without a clear articulation of what this was doing before and why removing it won't cause unintended consequences.

I've changed it now to use Runner::find_command_to_help() to address #4266 (comment) fully, and removed find_command_to_help() (previously Help_Command::find_subcommand()) altogether - I'll expand below as the review commenting is getting a bit messy.

danielbachhuber · 2017-08-03T18:48:35Z

@gitlost To help me better understand the nature of these changes, can you use Licecap to provide some GIFs of the behavior before and after?

gitlost · 2017-08-03T19:42:22Z

Doesn't seem to be a Linux version of that - will come up with something...

danielbachhuber · 2017-08-09T12:49:18Z

features/help.feature

    And STDERR should be:
      """
-      Error: 'non-existent-command' is not a registered wp command.
+      Error: 'non-existent-command' is not a registered wp command. See 'wp help'.


What do you think about changing:

See 'wp help'.

To:

See 'wp help' for available commands.

I think it adds more clarity to the message.

Agree, changed.

danielbachhuber · 2017-08-09T12:54:42Z

features/help.feature

+      """
+      Warning: This does not seem to be a WordPress install.
+      If the command 'non-existent-command' is in a plugin or theme, pass --path=`path/to/wordpress`.
+      Error: 'non-existent-command' is not a registered wp command. See 'wp help'.


If we definitely know this isn't a WordPress install, we don't need to include If the command 'non-existent-command' is in a plugin or theme, pass --path=path/to/wordpress

Changed it to one line "No WordPress install found. If the command...".

The idea behind mentioning about plugin/theme is to address your comment #4265 (comment) where the command could be added in a plugin/theme.

danielbachhuber · 2017-08-09T12:55:53Z

features/help.feature

+      """
+    And STDOUT should be empty
+
+  Scenario: Suggestions for command typos in help


Does this scenario need Given an empty directory ?

danielbachhuber · 2017-08-09T12:57:46Z

features/help.feature

+      """
+    And STDOUT should be empty
+
+  Scenario: Suggestions for subcommand typos in help of specially treated commands


This seems to be missing a Given too.

danielbachhuber · 2017-08-09T12:58:48Z

php/WP_CLI/Runner.php

+			if ( is_string( $possible_suggestion ) ) {
+				WP_CLI::error( $possible_suggestion );
+			}
+			// Should never happen.


Ok, then we can remove the nonsensical error message.

danielbachhuber · 2017-08-09T13:00:33Z

php/WP_CLI/Runner.php

 		// In case the operation fails, ensure the timestamp has been updated.
 		$cache->write( $cache_key, time() );

+		// Ask before doing remote request.


Can you remove this, please, and open an issue to discuss changes to the update process before making them?

Done and will do.

danielbachhuber · 2017-08-09T13:01:09Z

php/class-wp-cli.php

+	 * @param string $question Question to display before the prompt.
+	 * @param array $assoc_args Skips prompt if 'yes' is provided.
+	 */
+	public static function ask( $question, $assoc_args = array() ) {


This can be removed for now.

danielbachhuber · 2017-08-09T13:03:27Z

php/commands/help.php

-		if ( function_exists( 'add_filter' ) ) {
-			$command_string = implode( ' ', $args );
-			\WP_CLI::error( sprintf( "'%s' is not a registered wp command.", $command_string ) );
-		}


Can you clarify in greater detail? I'm hesitant to remove this without a clear articulation of what this was doing before and why removing it won't cause unintended consequences.

``` Loading composer repositories with package information Updating dependencies Package operations: 0 installs, 1 update, 0 removals - Updating composer/composer (1.4.3 => 1.5.0): Checking out 6f3310c762 Writing lock file Generating autoload files ```

…p, use find_command_to_run.

gitlost · 2017-08-09T15:36:18Z

Here's a before/after screenshot of running this bash script https://gist.github.com/gitlost/e895eb50068aff4ca36a61de3f74a2e7 in the suggest_help branch:

Re removing the code from Help_Command, the idea is to consolidate the exiting and error messages in Runner. At the moment help will exit if it determines that WP is loaded but will just return otherwise. The error it gives on exiting is different than that that occurs when other commands don't execute (and which don't do this WP loaded check). It's confusing and inconsistent, and leads to odd behaviour at the moment, so I think it's better if help just tries to help and exit, and otherwise let Runner look after things.

danielbachhuber · 2017-08-10T13:36:25Z

Thanks @gitlost. We can land this and see if anything shakes out of the behavior changes.

gitlost · 2017-08-10T19:00:24Z

Thanks @danielbachhuber, actually was trying to come up with a proper less hand-waving explanation when I noticed that there's an issue with help if the WP install is partial, as tested for in "Help when WordPress is downloaded but not installed" in features/help.feature:49, and reproducible by:

wget https://raw.githubusercontent.com/wp-cli/builds/gh-pages/phar/wp-cli-nightly.phar -nc -q && mv wp-cli-nightly.phar wp-nightly.phar && chmod +x wp-nightly.phar
rm -rf /tmp/pull_4266_wp_install
mkdir /tmp/pull_4266_wp_install
mysql --user=wp_cli_test --password=password1 --execute='DROP DATABASE IF EXISTS wp_cli_test;'
./wp-nightly.phar core download --path=/tmp/pull_4266_wp_install
./wp-nightly.phar config create --dbname=wp_cli_test --dbuser=wp_cli_test --dbpass=password1 --path=/tmp/pull_4266_wp_install
./wp-nightly.phar help core is-installed --path=/tmp/pull_4266_wp_install
./wp-nightly.phar help --path=/tmp/pull_4266_wp_install

where the help commands give Error: Can’t select database. errors (but won't on help core install for instance).

Which probably explains why help config and help core install/multisite-install/verify-checksum/version and help db are treated specially.

I'll push a fix to a new PR which involves adding a special help_wp_die_handler to cater for this, though it's a bit hacky.

Add help_wp_die_handler. Remove special help treatment for config and db (#4266).

This reverts commit c13b881, reversing changes made to 3608f68.

Revert #4266 and #4285 because of additional bugs

Handle exiting in error state on help & give suggestions.

5052156

danielbachhuber requested changes Aug 3, 2017

View reviewed changes

Re-enable auto_check_update. Rename _run_command to _run_command_and_…

46c78f9

…exit.

danielbachhuber reviewed Aug 3, 2017

View reviewed changes

Append (not replace) command suggestion to WP install error.

9ba5347

gitlost and others added 3 commits August 3, 2017 21:17

Make them an install warning and command error rather than append.

ef4d18b

More targeted command error. Add ask to auto_check_update.

6fee4c7

Merge branch 'master' into suggest_help

fff4e52

danielbachhuber self-requested a review August 9, 2017 02:21

danielbachhuber requested changes Aug 9, 2017

View reviewed changes

gitlost and others added 4 commits August 9, 2017 16:11

Remove find_subcommand_to_help. Elaborate auto check update messaging.

ee677cb

Bump working version to v1.4.0-alpha

8f5ca3e

Remove auto_check_update changes. In help, remove find_command_to_hel…

161ac53

…p, use find_command_to_run.

Merge branch 'master' into suggest_help

e37bde8

danielbachhuber approved these changes Aug 10, 2017

View reviewed changes

danielbachhuber added this to the 1.4.0 milestone Aug 10, 2017

danielbachhuber added the scope:framework label Aug 10, 2017

danielbachhuber merged commit c13b881 into master Aug 10, 2017

danielbachhuber deleted the suggest_help branch August 10, 2017 16:52

gitlost mentioned this pull request Aug 10, 2017

Add help_wp_die_handler. Remove special help treatment for config and db (#4266). #4285

Merged

danielbachhuber added a commit that referenced this pull request Aug 10, 2017

Merge pull request #4285 from wp-cli/suggest_help

3622bcd

Add help_wp_die_handler. Remove special help treatment for config and db (#4266).

danielbachhuber added a commit that referenced this pull request Aug 15, 2017

Revert "Merge pull request #4266 from wp-cli/suggest_help"

856a2bb

This reverts commit c13b881, reversing changes made to 3608f68.

gitlost added a commit that referenced this pull request Aug 15, 2017

Merge pull request #4293 from wp-cli/4265-revert

25c4c96

Revert #4266 and #4285 because of additional bugs

This was referenced Aug 16, 2017

Inconsistency in help and suggestions for specially treated commands. #4265

Closed

Give suggestions when exiting early on wp help #4303

Merged

Conversation

gitlost commented Aug 3, 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!

danielbachhuber commented Aug 3, 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!

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!

danielbachhuber commented Aug 3, 2017

Uh oh!

gitlost commented Aug 3, 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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gitlost commented Aug 9, 2017

Uh oh!

danielbachhuber commented Aug 10, 2017

Uh oh!

gitlost commented Aug 10, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants