Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Support exceptions in WP_CLI::error_to_string() method#5331

Merged
schlessera merged 3 commits into
wp-cli:masterwp-cli/wp-cli:masterfrom
chesio:issue-5267chesio/wp-cli:issue-5267Copy head branch name to clipboard
Feb 18, 2020
Merged

Support exceptions in WP_CLI::error_to_string() method#5331
schlessera merged 3 commits into
wp-cli:masterwp-cli/wp-cli:masterfrom
chesio:issue-5267chesio/wp-cli:issue-5267Copy head branch name to clipboard

Conversation

@chesio

@chesio chesio commented Dec 16, 2019

Copy link
Copy Markdown
Contributor

Fixes #5267.

Note: I don't know if returning $e->getMessage() or converting exception to string is preferred. PR does the former.

@chesio chesio requested a review from a team as a code owner December 16, 2019 14:16
Comment thread php/class-wp-cli.php Outdated
}

if ( $errors instanceof \Exception ) {
return $errors->getMessage();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might make sense to also print the concrete class of the exception, as that is often the most important distinction amongst exceptions.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefixed the error message with a class name.

Comment thread php/class-wp-cli.php Outdated
}
}

if ( $errors instanceof \Exception ) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should check for Throwable as well, as that is the base interface to catch starting with PHP 7+.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch, I learned something new :)

@chesio chesio requested a review from schlessera January 30, 2020 12:01
@chesio

chesio commented Jan 30, 2020

Copy link
Copy Markdown
Contributor Author

Hi @schlessera, sorry it took me so long - I've made the changes as requested/suggested.

@schlessera schlessera merged commit 6f5e81f into wp-cli:master Feb 18, 2020
@schlessera schlessera changed the title Support exceptions in WP_CLI::error_to_string method Support exceptions in WP_CLI::error_to_string() method Feb 18, 2020
@schlessera schlessera added this to the 2.5.0 milestone Feb 18, 2020
@schlessera

Copy link
Copy Markdown
Member

Thanks for the PR, @chesio !

@schlessera

Copy link
Copy Markdown
Member

Created new issue #5356 as a result of seeing the implementation here.

@chesio chesio deleted the issue-5267 branch February 18, 2020 20:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

error_to_string() should support exceptions too

2 participants

Morty Proxy This is a proxified and sanitized view of the page, visit original site.