#20776 closed enhancement (wontfix)

Improve callback verification while executing hooks

Reported by: ericlewis Owned by:
Priority: normal Milestone:
Component: Plugins Version:
Severity: normal Keywords: needs-patch
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 12 months ago.
replace is_null with is_callable
20776-1.diff (2.3 KB) - added by ericlewis 12 months 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)

replace is_null with is_callable

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.

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.

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.

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.

  • Keywords needs-patch added; has-patch removed

+1 for removing the is_null() check.

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

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