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

Handle exiting in error state on help & give suggestions. #4266

Merged
merged 11 commits into from Aug 10, 2017

Conversation

2 participants
@gitlost
Contributor

gitlost commented Aug 3, 2017

Issue #4265

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.

Show outdated Hide outdated php/WP_CLI/Runner.php Outdated
Show outdated Hide outdated php/WP_CLI/Runner.php Outdated
@danielbachhuber

This comment has been minimized.

Show comment
Hide comment
@danielbachhuber

danielbachhuber Aug 3, 2017

Member

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.

Member

danielbachhuber commented Aug 3, 2017

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.

Show outdated Hide outdated php/WP_CLI/Runner.php Outdated
if ( function_exists( 'add_filter' ) ) {
$command_string = implode( ' ', $args );
\WP_CLI::error( sprintf( "'%s' is not a registered wp command.", $command_string ) );
}

This comment has been minimized.

@danielbachhuber

danielbachhuber Aug 3, 2017

Member

Why is this removed?

@danielbachhuber

danielbachhuber Aug 3, 2017

Member

Why is this removed?

This comment has been minimized.

@gitlost

gitlost Aug 3, 2017

Contributor

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

@gitlost

gitlost Aug 3, 2017

Contributor

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

This comment has been minimized.

@danielbachhuber

danielbachhuber Aug 9, 2017

Member

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.

@danielbachhuber

danielbachhuber Aug 9, 2017

Member

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.

This comment has been minimized.

@gitlost

gitlost Aug 9, 2017

Contributor

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.

@gitlost

gitlost Aug 9, 2017

Contributor

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

This comment has been minimized.

Show comment
Hide comment
@danielbachhuber

danielbachhuber Aug 3, 2017

Member

@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?

Member

danielbachhuber commented Aug 3, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@gitlost

gitlost Aug 3, 2017

Contributor

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

Contributor

gitlost commented Aug 3, 2017

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

@danielbachhuber danielbachhuber self-requested a review Aug 9, 2017

Show outdated Hide outdated features/help.feature Outdated
Show outdated Hide outdated features/help.feature Outdated
Show outdated Hide outdated features/help.feature Outdated
"""
And STDOUT should be empty
Scenario: Suggestions for subcommand typos in help of specially treated commands

This comment has been minimized.

@danielbachhuber

danielbachhuber Aug 9, 2017

Member

This seems to be missing a Given too.

@danielbachhuber

danielbachhuber Aug 9, 2017

Member

This seems to be missing a Given too.

This comment has been minimized.

@gitlost

gitlost Aug 9, 2017

Contributor

Fixed.

@gitlost

gitlost Aug 9, 2017

Contributor

Fixed.

Show outdated Hide outdated php/WP_CLI/Runner.php Outdated
Show outdated Hide outdated php/WP_CLI/Runner.php Outdated
Show outdated Hide outdated php/class-wp-cli.php Outdated
if ( function_exists( 'add_filter' ) ) {
$command_string = implode( ' ', $args );
\WP_CLI::error( sprintf( "'%s' is not a registered wp command.", $command_string ) );
}

This comment has been minimized.

@danielbachhuber

danielbachhuber Aug 9, 2017

Member

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.

@danielbachhuber

danielbachhuber Aug 9, 2017

Member

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.

gitlost and others added some commits Aug 8, 2017

Update Composer dependencies (2017-08-09)
```
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
```
@gitlost

This comment has been minimized.

Show comment
Hide comment
@gitlost

gitlost Aug 9, 2017

Contributor

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

screenshot from 2017-08-09 15-52-38

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.

Contributor

gitlost commented Aug 9, 2017

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

screenshot from 2017-08-09 15-52-38

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 danielbachhuber added this to the 1.4.0 milestone Aug 10, 2017

@danielbachhuber

This comment has been minimized.

Show comment
Hide comment
@danielbachhuber

danielbachhuber Aug 10, 2017

Member

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

Member

danielbachhuber commented Aug 10, 2017

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

@danielbachhuber danielbachhuber merged commit c13b881 into master Aug 10, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@danielbachhuber danielbachhuber deleted the suggest_help branch Aug 10, 2017

@gitlost

This comment has been minimized.

Show comment
Hide comment
@gitlost

gitlost Aug 10, 2017

Contributor

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.

Contributor

gitlost commented Aug 10, 2017

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.

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

Merge pull request #4285 from wp-cli/suggest_help
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"
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
Revert #4266 and #4285 because of additional bugs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment