WordPress.org

Make WordPress Core

Opened 3 years ago

Last modified 103 minutes ago

#40568 reviewing enhancement

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: has-patch dev-feedback
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 (1)

40568.diff (1.1 KB) - added by Shelob9 3 years ago.
Patch for discussion

Download all attachments as: .zip

Change History (18)

@Shelob9
3 years ago

Patch for discussion

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

See past conversation in #28319

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


10 days 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
10 days ago

Added a revamped patch with another filter.

#6 @Shelob9
10 days ago

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

#7 @johnbillion
10 days ago

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

#8 @johnbillion
9 days ago

  • Type changed from defect (bug) to enhancement

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

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

#12 @TimothyBlynJacobs
7 days 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 days 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
28 hours 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
27 hours ago

Added that check in the patch :-)

#16 @johnbillion
3 hours 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
103 minutes ago

Looks like a great change to me!

Note: See TracTickets for help on using tickets.