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

Give suggestions when exiting early on `wp help` #4303

Merged
merged 3 commits into from Aug 16, 2017

Conversation

2 participants
@gitlost
Contributor

gitlost commented Aug 16, 2017

Issue #4265 and PR #4291

This is the same PR as the original PRs #4266 and #4285 but defining WP_DEBUG (and WP_DEBUG_DISPLAY) instead of WP_ADMIN on help to get WP to use wp_die() so its error message can be trapped.

It differs from the original try 571de94 in that the define is only done when WP's "wp-settings.php" is loaded instead of "wp-settings-cli.php", as the latter checks for a db error immediately after require_wp_db() and uses wp_die() - added in #352 and for the same reason i.e. that dead_db() uses die() when WP_ADMIN is not defined. This avoids the PHP Deprecated notices being triggered for old PHPs/WPs which caused the original try to fail.

Having WP_DEBUG and WP_DEBUG_DISPLAY defined causes wpdb::__construct() to set wpdb::$show_errors which in turn causes wpdb::bail() to use wp_die() rather than return, so dead_db() is by-passed.

This makes the hack less robust than defining WP_ADMIN, as a wp-config.php could define WP_DEBUG or WP_DEBUG_DISPLAY to be falsey and thus trigger the die() if the database is unavailable, but still much preferable in view of #674 as mentioned in #4291 (comment).

With reference to #4291 (comment)

It's also another hack that's likely to have unintended consequences.

Although it's regrettable that a hack like this is needed, defining WP_DEBUG is unlikely to have unintended consequences as all reasonable WP code should run the same whether it's defined or not. Also this execution path is only for help and only to get any packages/plugins/themes to add their commands (not run them).

(As an aside I think a case could be made for defining WP_DEBUG always when running under behat, at least for modern PHPs and WPs, as PHP notices are currently invisible in Travis.)

@@ -1043,6 +1058,15 @@ public function load_wordpress() {
// Load Core, mu-plugins, plugins, themes etc.
if ( Utils\wp_version_compare( '4.6-alpha-37575', '>=' ) ) {
if ( $this->cmd_starts_with( array( 'help' ) ) ) {
// Hack: define `WP_DEBUG` and `WP_DEBUG_DISPLAY` to get `wpdb::bail()` to `wp_die()`.

This comment has been minimized.

@danielbachhuber

danielbachhuber Aug 16, 2017

Member

@gitlost Just to confirm, this is only a problem for WP 4.6 and lower?

@danielbachhuber

danielbachhuber Aug 16, 2017

Member

@gitlost Just to confirm, this is only a problem for WP 4.6 and lower?

This comment has been minimized.

@gitlost

gitlost Aug 16, 2017

Contributor

WP 4.6 and higher, as "wp-settings-cli.php" has the wbdb->error check and wp_die() after require_wp_db().

@gitlost

gitlost Aug 16, 2017

Contributor

WP 4.6 and higher, as "wp-settings-cli.php" has the wbdb->error check and wp_die() after require_wp_db().

This comment has been minimized.

@danielbachhuber

danielbachhuber Aug 16, 2017

Member

Ok. Can you open a core Trac ticket to see about getting this resolved upstream, and link to it here?

@danielbachhuber

danielbachhuber Aug 16, 2017

Member

Ok. Can you open a core Trac ticket to see about getting this resolved upstream, and link to it here?

This comment has been minimized.

@gitlost

gitlost Aug 16, 2017

Contributor

Will do...

@gitlost

gitlost Aug 16, 2017

Contributor

Will do...

@danielbachhuber danielbachhuber changed the title from Issue 4265 to Give suggestions when exiting early on `wp help` Aug 16, 2017

@danielbachhuber danielbachhuber added this to the 1.4.0 milestone Aug 16, 2017

@danielbachhuber danielbachhuber merged commit cb78577 into wp-cli:master Aug 16, 2017

1 check passed

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

This comment has been minimized.

Show comment
Hide comment
@gitlost
Contributor

gitlost commented Aug 17, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment