#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)
Change History (33)
#1
@
8 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.
#3
@
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
#7
@
4 years ago
- Milestone changed from Awaiting Review to 5.6
- Owner set to johnbillion
- Status changed from new to reviewing
#9
@
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.
#10
@
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
@
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 don't do anything when the "thing" isn't an error.
#12
@
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
@
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
@
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.
#16
@
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.
#20
@
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
@
4 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.
#22
@
4 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
@
4 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
@
4 years ago
Also there was some discussion about this. See comment:10 onwards.
#25
@
4 years ago
- Keywords has-patch removed
- Resolution fixed deleted
- Status changed from closed to reopened
Reopening for a potential name change.
#26
@
4 years ago
- Keywords has-patch added
40568.2.diff renames the action to wp_error_seen
and improves the docs. Feedback welcome.
#27
@
4 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
@
4 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.
4 years ago
#31
@
4 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; }
Patch for discussion