WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 3 years ago

#28319 closed enhancement (wontfix)

Add hook to WP_Error for logging and report purposes

Reported by: ebinnion Owned by: johnbillion
Milestone: Priority: normal
Severity: normal Version: 4.0
Component: General Keywords: has-patch
Focuses: Cc:

Description

I'm currently working on a project to track errors.

There are methods to track deprecated functions, notices, and catch PHP errors. But, if I wanted to track WP_Error objects, there is currently no way to tie into the creation of these objects.

I would like to suggest adding a hook to the creation of WP_Error object which would allow developers to access a WP_Error object after it is created.

Attachments (6)

wp_error_create.diff (389 bytes) - added by ebinnion 4 years ago.
28319.diff (389 bytes) - added by ebinnion 4 years ago.
Adds hook docs to action.
28319.2.diff (539 bytes) - added by ebinnion 4 years ago.
Updated patch with hook docs. Previous update was incorrect.
28319.3.diff (512 bytes) - added by DrewAPicture 4 years ago.
28319.4.diff (905 bytes) - added by ebinnion 3 years ago.
Updated patch with action added to add method.
28319.5.diff (880 bytes) - added by ebinnion 3 years ago.
Only fire add_wp_error hook when WP_DEBUG is true.

Download all attachments as: .zip

Change History (42)

#1 @ebinnion
4 years ago

  • Keywords has-patch dev-feedback added

This ticket was mentioned in IRC in #wordpress-dev by ebinnon. View the logs.


4 years ago

#3 @mordauk
4 years ago

I support this.

#4 @DrewAPicture
4 years ago

  • Keywords needs-docs added

This will need hook docs.

@ebinnion
4 years ago

Adds hook docs to action.

@ebinnion
4 years ago

Updated patch with hook docs. Previous update was incorrect.

#5 @ebinnion
4 years ago

  • Keywords needs-docs removed

Hook docs have been added as requested. I wasn't sure which version number to use for @since, so I used 4.0.0. I will change it if we need to.

#6 @samuelsidler
4 years ago

Anything else needed here?

#7 @SergeyBiryukov
4 years ago

  • Milestone changed from Awaiting Review to 4.0

#8 @wonderboymusic
4 years ago

  • Owner set to DrewAPicture
  • Status changed from new to reviewing

@DrewAPicture
4 years ago

#9 @DrewAPicture
4 years ago

  • Keywords commit added
  • Owner DrewAPicture deleted

28319.3.diff looks good to go.

#10 @johnbillion
4 years ago

  • Owner set to johnbillion
  • Resolution set to fixed
  • Status changed from reviewing to closed

In 29033:

Add a create_wp_error action which is called when a WP_Error instance is created. Fixes #28319. Props ebinnion

#11 @kovshenin
4 years ago

  • Keywords has-patch commit removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

Don't you think this would be more useful if the action ran on add() and the class constructor used add() to set the first error, if any? That would allow to track multiple errors when they're added to the error object at a later stage:

$errors = new WP_Error();

if ( foo_condition() )
    $errors->add( 'foo', 'A foo error has occurred.' );

if ( bar_condition() )
    $errors->add( 'bar', 'A bar error has occurred.' );

// Any of the above conditions failed?
if ( ! empty( $errors->get_error_codes() ) )
    return $errors;

// Keep going

With the code above and the change in r29033, the create_wp_error action is never fired, even though two errors could have occurred.

#12 @nacin
4 years ago

  • Keywords revert added

I definitely agree with kovshenin here. This should be within add(), and we should use add() within the constructor if necessary.

I think my bigger concern here is that WP_Error objects are not exceptions — rarely are they "exceptional." Often, they are used as return values for functions that meet some kind of error condition, but that doesn't mean every usage meets the level of a deprecated function, notices, or PHP errors (see also: https://wordpress.org/plugins/log-deprecated-notices/, https://wordpress.org/plugins/debug-bar/).

This is basically the equivalent of logging every time a function returns false.

I'd go for a revert here and revisiting this in the future. What problem is actually trying to be solved here? What errors are you actually trying to track?

#13 @johnbillion
4 years ago

  • Keywords dev-feedback revert removed

I wasn't fussed by this addition but figured it was low-impact enough just to drop in. I agree with the observations above.

#15 @nacin
4 years ago

  • Keywords reporter-feedback 2nd-opinion added
  • Milestone changed from 4.0 to Future Release

#16 @ebinnion
4 years ago

I agree running the action on the add method makes much more sense.

My initial use case came came from my code wrangler trial. I was tasked with creating a message/error logging API.

I don't think this action would be useful all of the time, but in certain circumstances where a user was reporting awkward behavior, it could be convenient for debug plugins to be able to get information on WP_Error objects as well as errors/notices.

#17 follow-up: @danielbachhuber
4 years ago

This is basically the equivalent of logging every time a function returns false.

I disagree. Creating a WP_Error object is a deliberate decision of the developer to report an error upstream. Returning false is for a valid condition.

My initial use case came came from my code wrangler trial. I was tasked with creating a message/error logging API.

Funny enough, I set about doing a similar thing on WordPress.com — track instances in which users run into application errors. You might find some thread via MGS about it. I didn't end up producing anything because there isn't an action in WP_Error.

+1 generally to the idea of adding an action.

#18 @ebinnion
4 years ago

  • Keywords reporter-feedback removed

@ebinnion
3 years ago

Updated patch with action added to add method.

#19 @ebinnion
3 years ago

  • Keywords has-patch added

I have updated the patch to move the action to the add method as well as having the constructor call the add method.

Because the action now runs on add, I changed the action name to add_wp_error.

I used the following in a plugin to test:

add_action( 'init', 'test_something' );

function test_something() {
	if ( isset( $_GET['test_error'] ) ) {
	    $errors = new WP_Error();

	    $errors->add( 'foo', 'A foo error has occurred.' );

	    $errors->add( 'bar', 'A bar error has occurred.' );

	    exit;
	}
	
}

add_action( 'add_wp_error', 'print_something' );
function print_something( $error ) {
	echo '<pre>';
	print_r( $error );
	echo '</pre>';
}

And I received the following output:

WP_Error Object
(
    [errors:WP_Error:private] => Array
        (
            [foo] => Array
                (
                    [0] => A foo error has occurred.
                )

        )

    [error_data:WP_Error:private] => Array
        (
        )

)
WP_Error Object
(
    [errors:WP_Error:private] => Array
        (
            [foo] => Array
                (
                    [0] => A foo error has occurred.
                )

            [bar] => Array
                (
                    [0] => A bar error has occurred.
                )

        )

    [error_data:WP_Error:private] => Array
        (
        )

)

I'm not sure if it makes more sense to just pass the last error message or to pass the whole WP_Error object.

#20 in reply to: ↑ 17 @rmccue
3 years ago

  • Keywords close added

Replying to danielbachhuber:

I disagree. Creating a WP_Error object is a deliberate decision of the developer to report an error upstream. Returning false is for a valid condition.

FWIW, I agree with nacin. The REST API will make pretty liberal use of WP_Error (intentionally), and logging all of those alone would be problematic. In addition, there's the possibility for a DoS attack using it if errors are logged to a DB.

Recommend close as wontfix.

#21 @pento
3 years ago

  • Keywords close removed

A use for this has come up again on WP.com - I think 28319.4.diff is a reasonable method for allowing external access to all errors, at least until we can put proper error logging in place.

In addition, there's the possibility for a DoS attack using it if errors are logged to a DB.

It's possible to write code that will do a lot worse with pretty much everything everywhere in core. This is not a reason to close.

#22 @sboisvert
3 years ago

I strongly support this patch and we currently have a version of this running on WP.com.

There are some instances in the code where it's impossible to catch WP_Errors and they will cause fatals (because a string is expected upstream) and WP_Errors are returned before filtering.

In this specific case get_terms_to_edit() returns a WP_Error for a taxonomy that Schrödinger exists, it's then used as a string causing a fatal in the admin.

#23 follow-up: @carlalexander
3 years ago

I think that a solid set of hooks for errors is a necessary need for WordPress in the future. Your goals and ambitions with WordPress aren't slowing down. There's an API, more JavaScript client side, more ways for things to get handled silently by WordPress and hidden from developers (on top of overwriting PHP error logging settings).

To have a great application platform, you need to have solid error handling. You need a way for developers to assess problems when they don't have access to debugging tools.

I think my bigger concern here is that WP_Error objects are not exceptions — rarely are they "exceptional."

Right. You also have no exception to intercept either. This is an intentional design though. I understand that. The larger issue with WP_Errors is that there's no way to triage them. It's hard to filter relevant WP_Errors (they exist) from the noise. The WP_Error carries no weight, but it does when handled by the code in core.

This means that not all WP_Errors are equal. I took a stab at trying to figure that out last year (http://carlalexander.ca/diving-into-wordpress-errors/). The main conclusion was that there was no easy way to track them down without checking every hook WordPress. You'd then need data after to see which ones are relevant or not.

On top of that, WP_Errors are not always self contained. The most specific example is the http_api_debug hook. The WP_Error sent by the hook isn't as useful without other variables passed in the hook. That means that any hook in the WP_Error object (on add or construct) isn't very useful besides for tracking a message. You lack contextual information.

The REST API will make pretty liberal use of WP_Error (intentionally), and logging all of those alone would be problematic.

I use WP_Errors a lot with the custom work I do with the API. It's great, but the API once rolled out will highlight this issue more. Anyone building an API based app will need to know what's going on their server. WordPress won't be able to handle errors in the same way anymore.

In addition, there's the possibility for a DoS attack using it if errors are logged to a DB.

You could collect them during the whole execution and write everything out after the WordPress has terminated on the shutdown hook. There's definitely ways to minimize the impact otherwise people wouldn't use logging.

Last edited 3 years ago by carlalexander (previous) (diff)

#24 follow-ups: @dd32
3 years ago

In this specific case get_terms_to_edit() returns a WP_Error for a taxonomy that Schrödinger exists, it's then used as a string causing a fatal in the admin.

Then WordPress needs to add a is_wp_error() to the result of get_terms_to_edit() and it's friends before attempting to use it as a string then. I've created #30472 for that.

I'm not against this patch, but I do wonder if it'll actually be as useful as expected, or if it's just a hack to fix that one situation.
A logging interface (PSR-3 style) seems to be a far better option IMHO.

Last edited 3 years ago by dd32 (previous) (diff)

#25 in reply to: ↑ 23 ; follow-up: @rmccue
3 years ago

Replying to carlalexander:

The REST API will make pretty liberal use of WP_Error (intentionally), and logging all of those alone would be problematic.

I use WP_Errors a lot with the custom work I do with the API. It's great, but the API once rolled out will highlight this issue more. Anyone building an API based app will need to know what's going on their server. WordPress won't be able to handle errors in the same way anymore.

My point was more that it could be totally intentional to have multiple WP_Errors created in a single call to WP. In some of the services I maintain, an individual request might have 20 error objects created as-is, it's no different to returning false or null from a callback (except that you can have extra data). We don't log every time we return null.

You could collect them during the whole execution and write everything out after the WordPress has terminated on the shutdown hook. There's definitely ways to minimize the impact otherwise people wouldn't use logging.

My reasoning was unclear here; I meant if we ever added actual logging for this by default.

#26 in reply to: ↑ 25 @carlalexander
3 years ago

My point was more that it could be totally intentional to have multiple WP_Errors created in a single call to WP. In some of the services I maintain, an individual request might have 20 error objects created as-is, it's no different to returning false or null from a callback (except that you can have extra data). We don't log every time we return null.

No, you're right. That's why I mentioned that one issue is that WP_Error doesn't carry any weight. Out of those 20 objects some could be relevant. That's the larger issue with the WP_Error class design. You need a way to filter the noise.

A logging interface (PSR-3 style) seems to be a far better option IMHO.

That would be great. You still need a way to get messages to that logging interface though.

Last edited 3 years ago by carlalexander (previous) (diff)

#27 @sboisvert
3 years ago

Then WordPress needs to add a is_wp_check() to the result of get_terms_to_edit() and it's friends before attempting to use it as a string then. I've created #30472 for that.

That was one example and while that patch will solve the fatal it won't address the underlying issue. It will just make it fail silently instead of having it work as expected. I think the overall idea of being able to hook into WP_Errors is still very important.

My reasoning was unclear here; I meant if we ever added actual logging for this by default.

I agree it would be far too much noise. I think the only time this would be done is in a debugging plugin (or potentially with WP_DEBUG enabled in a dev environment).

#28 @ericlewis
3 years ago

#30913 was marked as a duplicate.

#29 in reply to: ↑ 24 ; follow-up: @ericlewis
3 years ago

Replying to dd32:

A logging interface (PSR-3 style) seems to be a far better option IMHO.

How would we support a PSR-3 style logging interface without PHP namespace support?

#30 in reply to: ↑ 29 @dd32
3 years ago

Replying to ericlewis:

Replying to dd32:

A logging interface (PSR-3 style) seems to be a far better option IMHO.

How would we support a PSR-3 style logging interface without PHP namespace support?

We'd have to skip anything in the spec that requires something we can't support. That would mean we'd skip the Namespaces and Traits, and probably just offer up a singleton which could be accessed from anywhere/assigned to a class property.
So when I said PSR3-style, I guess I meant something based off that design, but WPised into something we can actually use flexibly.

#31 in reply to: ↑ 24 @sboisvert
3 years ago

A logging interface (PSR-3 style) seems to be a far better option IMHO.

Just so I understand correctly the WP_Errors would basically be routed thru there as well as doing_it_wrong, deprecated etc and we'd hook in there instead of WP_Error? Would we still be able to change what's returned? If not I feel like these might be 2 different and great features and that doing one would not stop us from doing the other.

#32 @ericlewis
3 years ago

We should probably close this ticket and open a separate one to discuss a PSR-3-inspired logger for WordPress application debugging.

In summary, WP_Error is intended as a standardized error return value from a function. It is not intended as an application error. See nacin's comment, rmccue's two comments.

Last edited 3 years ago by ericlewis (previous) (diff)

#33 @ericlewis
3 years ago

Say hi to #30934, "Create a new API to standardize application logging"

@ebinnion
3 years ago

Only fire add_wp_error hook when WP_DEBUG is true.

#34 @ebinnion
3 years ago

I went ahead and created a new patch that only fires the add_wp_error hook when WP_DEBUG is defined and true to minimize some of the noise and DOS attack concerns.

While this isn't a logger, I still think this is a valid approach for developers to be able to get at WP_Error objects.

Last edited 3 years ago by ebinnion (previous) (diff)

This ticket was mentioned in Slack in #core by pento. View the logs.


3 years ago

#36 @pento
3 years ago

  • Keywords 2nd-opinion removed
  • Milestone Future Release deleted
  • Resolution set to wontfix
  • Status changed from reopened to closed

Per Slack discussion, this is really only being driven by WP.com usage, which would be better handled by a proper logging system. We'll keep using this internally for now, then switch to logging when it appears.

Closing this ticket in favour of #30934.

Note: See TracTickets for help on using tickets.