WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 10 days ago

Last modified 10 days ago

#40568 closed enhancement (fixed)

Add an action in WP_Error::add()

Reported by: Shelob9 Owned by: johnbillion
Milestone: 5.6 Priority: normal
Severity: normal Version: 2.1
Component: General Keywords: dev-feedback has-patch
Focuses: Cc:

Description

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.

Attachments (2)

40568.diff (1.1 KB) - added by Shelob9 4 years ago.
Patch for discussion
40568.2.diff (642 bytes) - added by johnbillion 11 days ago.

Download all attachments as: .zip

Change History (33)

@Shelob9
4 years ago

Patch for discussion

#1 @Shelob9
4 years ago

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.

#2 @danielbachhuber
4 years ago

See past conversation in #28319

#3 @Mte90
3 months ago

  • Keywords needs-patch added

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.

This ticket was mentioned in PR #522 on WordPress/wordpress-develop by Mte90.


3 months ago

  • Keywords has-patch added; needs-patch removed

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

Patch revamped and added another filter

#5 @Mte90
3 months ago

Added a revamped patch with another filter.

#6 @Shelob9
3 months ago

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

#7 @johnbillion
3 months ago

  • Milestone changed from Awaiting Review to 5.6
  • Owner set to johnbillion
  • Status changed from new to reviewing

#8 @johnbillion
3 months ago

  • Type changed from defect (bug) to enhancement

#9 @johnbillion
3 months ago

  • Keywords commit added

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.

Last edited 3 months ago by johnbillion (previous) (diff)

#10 @TimothyBlynJacobs
3 months ago

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.

#11 @johnbillion
3 months ago

  • Keywords dev-feedback added; commit removed

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.

Last edited 3 months ago by johnbillion (previous) (diff)

#12 @TimothyBlynJacobs
3 months ago

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 🙈

#13 @Mte90
2 months ago

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

#14 @johnbillion
2 months ago

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.

#15 @Mte90
2 months ago

Added that check in the patch :-)

#16 @johnbillion
2 months ago

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

#17 @TimothyBlynJacobs
2 months ago

Looks like a great change to me!

#18 @johnbillion
2 months ago

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

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

#19 @SergeyBiryukov
2 months ago

In 49023:

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

Follow-up to [49022].

See #40568.

#20 @Mte90
2 months ago

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).

#21 @johnjamesjacoby
2 weeks ago

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.

Last edited 2 weeks ago by johnjamesjacoby (previous) (diff)

#22 @alexstandiford
2 weeks ago

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.

#23 @johnbillion
2 weeks ago

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?

#24 @johnbillion
2 weeks ago

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

#25 @johnbillion
2 weeks ago

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

Reopening for a potential name change.

@johnbillion
11 days ago

#26 @johnbillion
11 days ago

  • Keywords has-patch added

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

#27 @Mte90
11 days ago

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.

#28 @johnbillion
11 days ago

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?

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


10 days ago

#30 @SergeyBiryukov
10 days ago

  • Resolution set to fixed
  • Status changed from reopened to closed

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.

#31 @giuseppe.mazzapica
10 days ago

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( 'wp_error_seen', $thing );
    }

    return $is_wp_error;
}
Last edited 10 days ago by giuseppe.mazzapica (previous) (diff)
Note: See TracTickets for help on using tickets.