Add an action in WP_Error::add()

[Shelob9]

It would be very useful if when an error was added in WP_Error.

Such an action would allow for the following:
1) Logging specific errors conditionally based on code.
2) A plugin that created a WP_Error log
3) Plugin developer could have an optional error logging feature for every time their plugin created a WP_Error object.

[Shelob9]

Patch for discussion

[Shelob9]

My patch adds the action in side of WP_Error::add() It also replaces the code that is totally the same in the constructor as that method so the action is fired by adding an error through the constructor without increasing code duplication.

[danielbachhuber]

See past conversation in #28319

[Mte90]

I was talking today with @johnbillion about it and can be handy to have 2 hooks.
One in add this ticket proposal and also inside is_wp_error so it will be possible to check if a WP_Error is checked.

Looking at the other ticket and discussions this hooks are handy for a lot of cases like expressed in this ticket or my need of today track all the WP_Error that Query Monitor don't find in a log file that I can inspect as I want.

Trac ticket: https://core.trac.wordpress.org/ticket/40568

Patch revamped and added another filter

[Mte90]

Added a revamped patch with another filter.

[Shelob9]

@Mte90 Thank you for updating the patch. Your update is an improvement.

[johnbillion]

I pushed some improvements to ​the PR after working on an integration with Query Monitor. I'm liking this and I think it's good to go.

[TimothyBlynJacobs]

What's changed since #28319?

I'm also worried about the cost of the wp_error_checked hook. Almost every function invocation in the REST API checks for is_wp_error(). This could easily wind up being called hundreds of times in a request. And once batching lands, thousands.

[johnbillion]

Hooks aren't free of course, but there are plenty of hooks that can fire hundreds of times during a request, eg. esc_html, gettext, and map_meta_cap. This isn't a reason to avoid introducing useful new hooks.

The discussion on #28319 is interesting and one that I'd forgotten about.

The primary reason I'd like to track WP_Error objects is to find ones which "disappear", eg. by being turned into boolean false as they bubble up. If the end result is boolean false, there can often be a more descriptive WP_Error further down the stack.

My plan for my Query Monitor plugin is to only log data about is_wp_error() calls when the parameter is an error, hence the $is parameter that I added. It simply won't do anything when the "thing" isn't an error.

[TimothyBlynJacobs]

Hooks aren't free of course, but there are plenty of hooks that can fire hundreds of times during a request, eg. esc_html, gettext, and map_meta_cap. This isn't a reason to avoid introducing useful new hooks.

For sure. Maybe a big banner in the hook description is enough to warn developers about potential misuse.

For numbers purposes, this is what I get in a batch REST API request to the menus endpoint.

{
  "get_post_metadata": 53118,
  "gettext": 15086,
  "gettext_default": 9366,
  "check_wp_error": 8928,
  "gettext_gutenberg": 5688,
  "query": 3754,
  "log_query_custom_data": 3754,
  "gettext_with_context": 3352,
  "gettext_with_context_default": 3317,
  "get_nav_menu": 3018,
  "get_term": 3018,
  "wp_setup_nav_menu_item": 2796,
  "wp_trim_words": 2772,
  "nav_menu_attr_title": 2772,
  "nav_menu_description": 2772,
  "rest_request_parameter_order": 2192,
  "alloptions": 1933,
  "sanitize_key": 1155,
  "pre_get_col_charset": 964,
  "set_url_scheme": 765,
  "option_home": 674,
  "pre_option_home": 674,
  "home_url": 673,
  "option_permalink_structure": 609,
  "pre_option_permalink_structure": 609,
  "rest_url_prefix": 578,
  "rest_url": 574,
  "get_object_subtype_post": 432,
  "map_meta_cap": 293,
  "update_post_metadata_cache": 265,
  "user_has_cap": 245,
  "update_post_metadata": 216,
  "added_post_meta": 216,
  "add_post_meta": 216,
  "add_post_metadata": 216,
  "register_block_type_args": 130,
  "default_post_metadata": 120,
  "clean_url": 115,
  "extra_plugin_headers": 110
}

An aside, maybe we should implement a get_metadata filter that has the meta key in it 🙈

[Mte90]

So I need to add an alert in the comment section of the hook in WP_Error? So we can move on that patch :-)

[johnbillion]

An alternative is to only trigger the wp_error_checked action when the $thing is an error. I can't think of a situation where I'd want to track that action if it's not an error.

[Mte90]

Added that check in the patch :-)

[johnbillion]

@TimothyBlynJacobs What do you think? This will drastically reduce (eliminate in most cases) the number of times the wp_error_checked action fires.

[TimothyBlynJacobs]

Looks like a great change to me!

[johnbillion]

In 49022:

General: Introduce the wp_error_added and wp_error_checked actions.

These actions allow debugging tools to track WP_Error instances as they're created and subsequently passed between functions which check for error objects.

Props Shelob9, Mte90, TimothyBlynJacobs, johnbillion

Fixes #40568

[SergeyBiryukov]

In 49023:

General: Give the $is variable in is_wp_error() a more descriptive name.

Follow-up to [49022].

See #40568.

[Mte90]

Talking with the Italian community we discussed how that function is a pure function.
So doesn't should handle other things otherside a proposal was to execute the hook if WP_DEBUG is set as true to minimize the effect (after all is just for development).

[johnjamesjacoby]

I'm just seeing this for the first time, and I find it odd that the wp_error_checked action only fires when $thing is an error. This means there is no way to know via this action if something was error-checked and it was not an error.

If I call is_wp_error( $thing ) and it's not an instance of WP_Error, how will I ever know it was checked?

Edit: in addition, if this hook ever changes to always firing, I think the ideal parameter order...

// would be this...
do_action( 'wp_error_checked', $is_wp_error, $thing );

// ...instead of this
do_action( 'wp_error_checked', $thing, $is_wp_error );

If we think there will ever be a future need to have this always fire, now is the time to get that right.

I also feel like wp_error_checked is not an obvious hook name for what this function is historically used for. I use it all over the place to make sure something is or is not a WP_Error and to take appropriate action either way. I have never used this function with the intent of denoting a single instance of having checked for an error. That means I may check the same object multiple times looking for multiple failures, because a single WP_Error object may contain multiple errors. See: ​bbPress.

[alexstandiford]

I love this update!

Since wp_error_checked only fires when the thing is a WP_Error, wp_error_checked seems a bit confusing to me. Perhaps it would be better if the name was something like wp_error_found, or something?

Is there any reason why we wouldn't want to add an action to has_errors, as well? There are many circumstances in which this is used instead of is_wp_error for checking for errors, and I think it would be just as beneficial.

[johnbillion]

Personally, my use case for this is to track a WP_Error object as it moves through a call stack. I'm not interested in when a function performs a check for a WP_Error, I'm interested in when a WP_Error object passes through that function.

What's the use case for needing to know when a function checks for a WP_Error but the variable it checks is not one?

[johnbillion]

Also there was some discussion about this. See comment:10 onwards.

[johnbillion]

Reopening for a potential name change.

[johnbillion]

40568.2.diff​ renames the action to wp_error_seen and improves the docs. Feedback welcome.

[Mte90]

My only feedback if it is the case to add the support for the WP_Debug variable and execute the filter if it is flagged so any change we do is not performance aggressive in production for this filter.

[johnbillion]

What's the use case for needing to know when a function checks for a WP_Error but the variable it checks is not one?

[SergeyBiryukov]

In 49635:

General: Rename the wp_error_checked action to is_wp_error_instance for clarity.

Follow-up to [49022], [49023].

Props johnbillion, helen, johnjamesjacoby, Mte90, alexstandiford, hellofromTonya, jnylen0, SergeyBiryukov.
Fixes #40568.

[giuseppe.mazzapica]

I saw this reopened and wanted to comment, but I was too slow for @SergeyBiryukov

I'll comment anyway.

is_wp_error has always been a pure function, that has been used in pure logic without consequences.

Since I saw this ticket, I changed all occurrences of is_wp_error with $thing instanceof WP_Error and we're discussing to add is_wp_error to the list of discouraged functions in our code style.

Checking a type of a variable should not be impure, nor trigger actions.

I understand the reasoning in the ticket, but maybe could you consider to add a parameter to is_wp_error that make the function trigger the hook?

shortcode_atts do something similar. If you pass the third argument to it, it fires an action, otherwise it is a quite useful pure function (​https://github.com/WordPress/WordPress/blob/master/wp-includes/shortcodes.php#L582-L597).

Could you consider to do the same for is_wp_error?

That would mean that in all places where WP itself uses it, and it makes sense, that parameter would need to be passed. That could be a lot of work, I checked and there are ~420 occurrences of is_wp_error in WP, even if I think most of them would not need to checked.

For example, in core the same error can be checked multiple times and triggering that hook multiple times for same error seems overkill to me.

Alternatively, could you add a parameter to opt-out from the hook triggering?

So the code would become either the following or the same but $do_action default to true.

<?php
function is_wp_error( $thing, $do_action = false ) {
    
    $is_wp_error = ( $thing instanceof WP_Error );

    if ( $do_action && $is_wp_error ) {
        /**
        * Fires when `is_wp_error()` is called and its parameter is an instance of `WP_Error`.
        *
        * @since 5.6.0
        *
        * @param WP_Error $thing The error object passed to `is_wp_error()`.
        */
        do_action( 'is_wp_error_instance', $thing );
    }

    return $is_wp_error;
}