Make WordPress Core

Opened 7 years ago

Closed 3 years ago

Last modified 3 years ago

#40568 closed enhancement (fixed)

Add an action in WP_Error::add()

Reported by: shelob9's profile Shelob9 Owned by: johnbillion's profile 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 7 years ago.
Patch for discussion
40568.2.diff (642 bytes) - added by johnbillion 3 years ago.

Download all attachments as: .zip

Change History (33)

@Shelob9
7 years ago

Patch for discussion

#1 @Shelob9
7 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
7 years ago

See past conversation in #28319

#3 @Mte90
4 years 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.


4 years ago
#4

  • Keywords has-patch added; needs-patch removed

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

Patch revamped and added another filter

#5 @Mte90
4 years ago

Added a revamped patch with another filter.

#6 @Shelob9
4 years ago

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

#7 @johnbillion
4 years ago

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

#8 @johnbillion
4 years ago

  • Type changed from defect (bug) to enhancement

#9 @johnbillion
4 years 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 4 years ago by johnbillion (previous) (diff)

#10 @TimothyBlynJacobs
4 years 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
4 years 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 4 years ago by johnbillion (previous) (diff)

#12 @TimothyBlynJacobs
4 years 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
4 years 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
4 years 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
4 years ago

Added that check in the patch :-)

#16 @johnbillion
4 years 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
4 years ago

Looks like a great change to me!

#18 @johnbillion
4 years 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
4 years ago

In 49023:

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

Follow-up to [49022].

See #40568.

#20 @Mte90
4 years 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
3 years 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 3 years ago by johnjamesjacoby (previous) (diff)

#22 @alexstandiford
3 years 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
3 years 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
3 years ago

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

#25 @johnbillion
3 years ago

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

Reopening for a potential name change.

@johnbillion
3 years ago

#26 @johnbillion
3 years ago

  • Keywords has-patch added

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

#27 @Mte90
3 years 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
3 years 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.


3 years ago

#30 @SergeyBiryukov
3 years 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
3 years 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 3 years ago by giuseppe.mazzapica (previous) (diff)
Note: See TracTickets for help on using tickets.