2.x Improve error handling by gchtr · Pull Request #2196 · timber/timber

gchtr · 2020-02-16T11:58:28Z

Ticket: #2073 (comment), #2145

Issue

In #2073 (comment) and the following comments, we discussed whether we should use Exceptions or work with calls to Helper::doing_it_wrong().

Solution

This pull request pulls out the Helper::doing_it_wrong() function I added in #2145 and adds improvements.

I would like to go with the solution that @acobster suggested in #2073 (comment) and move the calls to doing_it_wrong_run and deprecated_function_run in front of the bailout check with WP_DEBUG.

Impact

To say it in @acobster’s words:

This gives folks an escape hatch for refactoring (which I consider the main use-case for doing_it_wrong to begin with) where they can throw exceptions, log to an external service, or whatever else to their hearts' content.

When we look at _doing_it_wrong() and _deprecated_function() in WordPress Core, the two action hooks doing_it_wrong_run and deprecated_function_run also run before a check to WP_DEBUG, so this change will make error handling work the same as in WordPress, which is a good thing.

Usage Changes

~~This will make it possible for developers to hook into their favorite logging service, be it with PHP Exceptions itself:~~

add_action( 'doing_it_wrong_run', function( $function, $message, $version ) {
    throw new \Exception( $message );
}, 10, 3 );

~~Or, for example, with a service like Bugsnag:~~

add_action( 'doing_it_wrong_run', function( $function, $message, $version ) {
    if ( ! class_exists( 'Bugsnag_Wordpress' ) ) {
        return;
    }

    /**
     * Bugsnag Instance.
     *
     * @var \Bugsnag_WordPress|\Bugsnag_Client $bugsnagWordpress
     */
    global $bugsnagWordpress;

    $bugsnagWordpress->notifyError(
        'Doing it wrong!',
        $message,
        null,
        'warning'
    );
}, 10, 3 );

Ignore the code examples above. It’s not clear yet what the best way is for developers to hook into this.

Considerations

We should probably add documentation about this. I wonder whether the Debugging Guide is the right place for this or whether

We should add a separate Error Handling / Logging Guide?
We should rename and extend the Debugging Guide with a section about Error Handling / Logging?

Testing

Not directly. But if something won’t work, it will show up in testing, because a lot of tests that run functions including calls to Helper::doing_it_wrong() and Helper::deprecated() will require these two functions to work properly.

codecov · 2020-02-16T12:10:45Z

Codecov Report

Merging #2196 into 2.x will decrease coverage by 0.28%.
The diff coverage is 45.45%.

@@             Coverage Diff              @@
##                2.x    #2196      +/-   ##
============================================
- Coverage     96.04%   95.75%   -0.29%     
- Complexity     1530     1534       +4     
============================================
  Files            50       50              
  Lines          3892     3911      +19     
============================================
+ Hits           3738     3745       +7     
- Misses          154      166      +12

Impacted Files	Coverage Δ	Complexity Δ
lib/Post.php	`98% <100%> (ø)`	`200 <0> (ø)`	⬇️
lib/Helper.php	`87.42% <40%> (-6.17%)`	`82 <4> (+4)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update eb8a495...6d0f250. Read the comment docs.

jarednova · 2020-02-17T13:56:45Z

@gchtr Looks good! I pulled it down to add an application and test coverage. Let me know if I got all that right.

I think while this is open and fresh we should go ahead and add the info to the docs. Of these, I think this is the way to go:

We should rename and extend the Debugging Guide with a section about Error Handling / Logging?

Perhaps "Debugging & Error Logging"? This is something I do with internal documentation a lot at Upstatement. Rather than a section/folder/document be one thing, making something "This & That" communicates that it's both "this", "that', and everything in between.

gchtr · 2020-02-26T19:29:15Z

Perhaps "Debugging & Error Logging"? This is something I do with internal documentation a lot at Upstatement. Rather than a section/folder/document be one thing, making something "This & That" communicates that it's both "this", "that', and everything in between.

@jarednova Sounds good! I’ll keep working on this.

gchtr

As always when I work on documentation, I take a close look at how stuff works and learn tons in the process of it.

@jarednova and @acobster, before I make any changes to this pull request, I wanted to get your opinion on some of my thoughts. Please see the review comments. I have the updated guide ready locally, but depending on what we decide here, I’ll have to work on it some more.

I figured that we use both error_log() and trigger_error(). Until now, I never really looked at what the difference is. Apparently, when we use error_log(), we log directly to the log. But when we use trigger_error(), it first runs through error handlers set through set_error_handler().

The set_error_handler() function is what plugins like Query Monitor or Bugsnag use to catch errors and handle them their way. For example, Bugsnag will send you emails with the error, which is very helpful. If we use error_log(), these plugins can’t catch the errors. Errors might be lost in a log that nobody looks at.

I think it might help if we:

Deprecate the Helper::error_log() function and introduce a Helper::trigger_error() function.
Update the Helper::warn() function to use the Helper::trigger_error() function, too.
Introduce the same functionality to Helper::trigger_error() that we have in Helper::doing_it_wrong() and Helper::deprecated(). This would mean something like the following.

public static function trigger_error( $message, $type = E_USER_NOTICE ) {
	do_action( 'timber/trigger_error_run', $message, $type );

	if ( ! WP_DEBUG ) {
		return;
	}

	trigger_error( '[ Timber ] ' . $message, $type );
}

I think that would make error handling in Timber more flexible and also be in line with @acobster’s comment in #2073 (comment).

lib/Helper.php

acobster · 2020-03-01T17:50:41Z

do_action( 'timber/trigger_error_run', $message, $type );

This is a confusing hook name IMO. If we're going to introduce a new hook, why not simplify to, say, timber/error?

Deprecate the Helper::error_log() function and introduce a Helper::trigger_error() function.

The difference being that the former is not affected by set_error_handler, while the latter is? Wouldn't that mean that if you have $LoggingService hooked into timber/trigger_error_run (or whatever) and set_error_handlers, you'd get log $LoggingService entries for both? I don't think that's what I'd want. :)

Or is that the only way that WP_DEBUG can have the intended effect?? PHP is weird.

jarednova · 2020-03-01T19:10:39Z

Or is that the only way that WP_DEBUG can have the intended effect?? PHP is weird.

... and WordPress makes it weirder. My take is two-fold. First, I'm persuaded that trigger_error is the proper place to send errors as error_log is sending errors to a specific place (thus, perhaps bypassing potential error/QA/monitoring workflows).

My only pushback is in the scope of this ticket and PR. Does the current code satisfy what was needed by #2073 (comment), #2145 with the doing_it_wrong stuff applied? My impulse is to merge the work and then isolate the error_log vs. trigger_error solve in its own issue/PR so we don't go too deep down a rabbit hole (esp. b/c the 2.x code isn't deployed yet, thus no backwards compatibility concerns).

Of course, if you really see it as just as a few tweaks away from resolving, go for it! But if there's a risk that it opens up additional rounds of Qs/considerations, I think it better to isolate.

gchtr · 2020-03-01T19:46:08Z

The difference being that the former is not affected by set_error_handler, while the latter is? Wouldn't that mean that if you have $LoggingService hooked into timber/trigger_error_run (or whatever) and set_error_handlers, you'd get log $LoggingService entries for both? I don't think that's what I'd want. :)

Hm... 🤔 yes. That’s indeed not good. Damn …

The question then is, do we advise developers to use the doing_it_wrong_run action to add their custom error handling, or do we advise to use set_error_handler() to add a custom error handler?

From what I saw doing_it_wrong_run is usually only used by testing suites to make the special tags like @expectedIncorrectUsage or @exptectedDeprecated work.

But then, if we advise to use set_error_handler() and also advise to not set WP_DEBUG to true in production, these errors would still never be logged.

My only pushback is in the scope of this ticket and PR. Does the current code satisfy what was needed by #2073 (comment), #2145 with the doing_it_wrong stuff applied? My impulse is to merge the work and then isolate the error_log vs. trigger_error solve in its own issue/PR so we don't go too deep down a rabbit hole (esp. b/c the 2.x code isn't deployed yet, thus no backwards compatibility concerns).

Of course, if you really see it as just as a few tweaks away from resolving, go for it! But if there's a risk that it opens up additional rounds of Qs/considerations, I think it better to isolate.

Well, if we leave this pull request at the current state, we have consistency with how WordPress does it. But we can’t add documentation for Advanced Error Logging then, because we would first need to decide how to do it right.

I guess it is a rabbit hole. We should take the time to explore how it’s properly done in the WordPress context.

I’ll clean this up next week for a final review and open a separate issue to work on in the future.

gchtr · 2020-03-04T20:42:11Z

@jarednova @acobster Back to you for review.

jarednova

I like this all very much — let's do it!

acobster · 2020-03-06T02:09:56Z

If @jarednova is happy, I'm happy!

gchtr added 2 commits February 16, 2020 12:03

Add Helper::doing_it_wrong()

740dc30

Move action calls in front of bailouts

d7b6031

gchtr added the 2.0 label Feb 16, 2020

gchtr requested review from acobster and jarednova February 16, 2020 11:58

gchtr requested a review from pascalknecht as a code owner February 16, 2020 11:58

gchtr assigned acobster Feb 16, 2020

gchtr mentioned this pull request Feb 16, 2020

2.x Documentation for API #2073

Merged

24 tasks

Add test to cover doing_it_wrong helper

7375688

Fixed other test covering the same deprecated notice

6d0f250

gchtr self-assigned this Feb 26, 2020

gchtr unassigned acobster Feb 26, 2020

gchtr commented Feb 29, 2020

View reviewed changes

lib/Helper.php Outdated Show resolved Hide resolved

lib/Helper.php Outdated Show resolved Hide resolved

gchtr mentioned this pull request Mar 4, 2020

Define and document the best way to handle errors #2210

Open

gchtr added 3 commits March 4, 2020 21:33

Apply suggestions from code review

e430526

Remove more HTML from debugging output

d70d845

Prefix errors with "[ Timber ]"

83a0616

gchtr removed the request for review from pascalknecht March 4, 2020 20:41

jarednova approved these changes Mar 6, 2020

View reviewed changes

gchtr merged commit 6827d70 into 2.x Mar 9, 2020

gchtr deleted the 2.x-error-handling branch March 9, 2020 16:44

gchtr mentioned this pull request Mar 9, 2020

2.x Update function signatures for ::get_post() and ::get_posts() #2145

Closed

3 tasks

gchtr mentioned this pull request Aug 14, 2020

2.x Deprecate Timber\PostClassMap filter and rename it to timber/post/classmap #2293

Merged

rubas mentioned this pull request Apr 3, 2024

Error Handling -> E_USER_WARNING instead of E_USER_NOTICE #2961

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

2.x Improve error handling#2196

2.x Improve error handling#2196
gchtr merged 7 commits into2.xfrom
2.x-error-handling

gchtr commented Feb 16, 2020 •

edited

Loading

Uh oh!

codecov bot commented Feb 16, 2020 •

edited

Loading

Uh oh!

jarednova commented Feb 17, 2020

Uh oh!

gchtr commented Feb 26, 2020

Uh oh!

gchtr left a comment

Uh oh!

Uh oh!

Uh oh!

acobster commented Mar 1, 2020

Uh oh!

jarednova commented Mar 1, 2020

Uh oh!

gchtr commented Mar 1, 2020

Uh oh!

gchtr commented Mar 4, 2020

Uh oh!

jarednova left a comment

Uh oh!

acobster commented Mar 6, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

gchtr commented Feb 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Issue

Solution

Impact

Usage Changes

Considerations

Testing

Uh oh!

codecov bot commented Feb 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

jarednova commented Feb 17, 2020

Uh oh!

gchtr commented Feb 26, 2020

Uh oh!

gchtr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

acobster commented Mar 1, 2020

Uh oh!

jarednova commented Mar 1, 2020

Uh oh!

gchtr commented Mar 1, 2020

Uh oh!

gchtr commented Mar 4, 2020

Uh oh!

jarednova left a comment

Choose a reason for hiding this comment

Uh oh!

acobster commented Mar 6, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

gchtr commented Feb 16, 2020 •

edited

Loading

codecov bot commented Feb 16, 2020 •

edited

Loading