WordPress.org

Make WordPress Core

Opened 9 years ago

Closed 9 years ago

#20776 closed enhancement (wontfix)

Improve callback verification while executing hooks

Reported by: ericlewis Owned by:
Milestone: Priority: normal
Severity: normal Version:
Component: Plugins Keywords: needs-patch
Focuses: Cc:

Description

When executing hooks, we currently have a simple callback verification:

if ( !is_null($the_['function']) )
				call_user_func_array($the_['function'], array_slice($args, 0, (int) $the_['accepted_args']));

This works in most cases, if everything is coded properly in the theme / plugins. However, there are cases when a bad callback makes it through this validation, such as dropping this in a theme or plugin:

add_action('init', array( NULL, 'method') );

Will cause an error you may be familiar with:

Warning: call_user_func_array() [function.call-user-func-array]: First argument is expected to be a valid callback, 'Array' was given in /path/to/wp/wp-includes/plugin.php on line 405.

Which can be a tricky issue to debug.

I suggest using the is_callable() php function for verification instead, which should remove this warning from ever showing up.

Attachments (2)

20776.diff (501 bytes) - added by ericlewis 9 years ago.
replace is_null with is_callable
20776-1.diff (2.3 KB) - added by ericlewis 9 years ago.
remove is_null check from apply_filters, apply_filters_ref_array, do_action, do_action_ref_array, _wp_call_all_hook. These functions can use more dev-review for possible performance gains (remove array casting suggested).

Download all attachments as: .zip

Change History (8)

@ericlewis
9 years ago

replace is_null with is_callable

#1 @nacin
9 years ago

I disagree with this. One of the best things we can do here is offer immediate and loud feedback to a developer in these cases. This is a developer error, not a user error. Issuing a warning should raise enough alarms without actually terminating execution flow.

We've all made a typo in a callback, only to spend quite a bit of time figuring out what was going wrong. It's worse when the issue is not reflected in the logs.

I'm actually not sure why the NULL check is there. I would rather remove it for both meager performance reasons and developer feedback reasons.

#2 @nacin
9 years ago

The check for NULL was done in [1394].

You can see the full reasoning here: http://wordpress.org/support/topic/bug-in-remove_filterapply_filters?replies=3.

#3 @nacin
9 years ago

As $wp_filter's structure has changed over the years, so has the need for this is_null() check. In [6320], some code went into remove_filter() which caused empty priority arrays to be unset(). I can no longer reproduce the original issue that caused [1394].

I would not be surprised if we could remove a few other checks as well, such as the array cast on the foreach's. I did not investigate that, however.

#4 @ericlewis
9 years ago

Yeah, after thinking about it, I agree. No validation at all would be better to create the proper PHP errors, while silent invalidation could lead to even more hard to track issues.

It looks like the array cast on $wp_filter[$tag] came from #4394, marked as code clean-up. I'd imagine that's good for taking out as well.

#5 @scribu
9 years ago

  • Keywords needs-patch added; has-patch removed

+1 for removing the is_null() check.

@ericlewis
9 years ago

remove is_null check from apply_filters, apply_filters_ref_array, do_action, do_action_ref_array, _wp_call_all_hook. These functions can use more dev-review for possible performance gains (remove array casting suggested).

#6 @nacin
9 years ago

  • Milestone Awaiting Review deleted
  • Resolution set to wontfix
  • Status changed from new to closed

20776-1.diff ended up making it in via #21321. I forgot there was a ticket for it; I thought it came up in another medium.

Note: See TracTickets for help on using tickets.