add_action('all', ...) causes unexpected behaviour

[tellyworth]

This might be intentional, but it doesn't seem to be documented. It's possible to use the 'all' tag when adding an action, not just a filter:

add_action('all', 'myaction');

However, the function will be called for all filters as well as all actions. Unless the function behaves like a filter and returns its argument, all kinds of things will go wrong as it merrily NULLs the result of every filter.

Possible solutions might be to run 'all' separately for filters and actions; prohibit 'all' actions; or keep the behaviour as-is and document it.

FWIW, an 'all' action can be useful for debugging.

[darkdragon]

Not really. The all tag is screwed up regardless. #4715 details what happens why you do so.

It is mostly by design, actions are filters, technically. You should expect that the callback will have to accept and pass though.

function mynamespace_all_debug_tag($string='')
{
    // Do debug stuff here
    return $string;
}

This is normal behavior

[darkdragon]

FYI, the all tag is to be used only for debugging.

[tellyworth]

Yup, I know it's intended for debugging.

Simple question then: is "all" intended to work with actions, or is it for filters only?

[ryan]

Once upon a time, filters and actions both returned their args. This is a holdover from then. I think we should just get rid of the merging and have do_action() and apply_filters() directly call the functions hooked onto "all", with filters and actions having separate "all" queues.

[tellyworth]

Fine by me. I also found three other "all" issues in unit testing:

1. An "all" filter will mess up the priorities of existing filters when merged (already reported in #4715, I can confirm this too)

2. An "all" action (not filter) causes duplicates: after the "all" action has been triggered for action "foo", it exists in two places in wp_filters. Subsequent calls to do_action cause additional duplicates.

3. Removing an "all" action doesn't work.

The unit tests will help refactoring, since they cover most of the basic behaviour. They're here: ​http://svn.automattic.com/wordpress-tests/wp-testcase/test_filters.php and
​http://svn.automattic.com/wordpress-tests/wp-testcase/test_actions.php

[darkdragon]

I'll submit a patch ryan, if no one else does. I have to go to work for the next 9 1/2 hours (includes driving). Seems trivial.

[ryan]

Anyone take a crack at this yet? I'll give it a look next week if no one jumps in.

[darkdragon]

Patch based on r6311, adds patch to fix #4715 and add functionality for both action and filter

[darkdragon]

Yeah, the patch fixes several other bug problems.

Combines bug fixes for #4715, #5261, and fixes a notice based in _wp_build...().

Has extra @param documentation

[ryan]

Is there any reason to have both filters and actions in $wp_filter? How about we separate actions into $wp_action with a merge_actions() function. Also, do_action() looks like it is always calling merge_filters() instead of checking to see if that tag is already merged.

[darkdragon]

For the sake of KISS, I just kept it as is. I did not realize that it was supposed to check to see if it is merged. Well, that would probably fix your issue.

[darkdragon]

Checks to make sure the actions aren't merged first.

[darkdragon]

Speed enhancement to removing filters.

[darkdragon]

I added two more patches that fix the issue ryan bought up. I also realized that the assignment in remove_filter() would actually decrease time with creating and allocating memory for what would amount to temporary variables.

I used if statements instead. If the array portion exist, then it will be deleted and on then. This is a lot less time allocated towards checking whether it exists and remove it, then the worse case which is check that it exists, does not exist, create it, destroy it.

[darkdragon]

Not to mention the speed boost from not doing the merging each time for action. Although, I haven't done any speed benchmarks, I don't think it would be all that terrible.

[darkdragon]

Replying to ryan:

Is there any reason to have both filters and actions in $wp_filter? How about we separate actions into $wp_action with a merge_actions() function. Also, do_action() looks like it is always calling merge_filters() instead of checking to see if that tag is already merged.

After some thought, it would make sense to separate the two, in order to keep any overriding nature from happening.

[ryan]

Indeed. Add the separation of filters and actions to your patch and I think we're good.

[darkdragon]

Well, what you have to realize is that I'm insane and that is a no can do, unless you want me to rewrite the entire Plugin Library into an Object Oriented approach. I'll let someone more sane do what you wish.

Unless you just want me to append '_action' to all actions and '_filter' to all filters for removing, adding, and calling.

[ryan]

Perhaps I'm insane because I don't see why you can't just separate add_action from add_filter and use separate arrays for filters and actions. Full separation of the two. I'll write it, just want to make sure I'm not missing something.

[darkdragon]

Duplicate code? Why would you have similar code in two different places when you have code in one place and called in two different places. I can't do that, but if you're willing then I think it would work.

Dude, I cringe whenever I look at the similar code branches in do_action, apply_filters, and do_action_ref_array. About lose my sanity I do. Although, I have no non-drastic solution. Just purely maddening.

The classes I threw together would require a couple more hours. I just don't see myself finishing it when your solution would only take about 30 minutes to an hour (testing included), if even that.

[darkdragon]

Well, to clarify, I did think about separation, but I do rather like having one function do all of the work. In the class, basically, I had one method that would take the $tag, $function_to_add, $priority, and $accepted_args and build the array, and then return it.

Damn it! I just realized that the code I wrote wouldn't work! Damn you coordinate system! You have to duplicate the addition code! I was thinking an assignment from an function would work, but it wouldn't. If you did that you would overwrite everything that was in the array and would only store the new tag system.

You also can't easily assign it to a variable and then use it in the array (tried that, pretty worthless).

An extra three lines in add_action won't hurt anything. It would completely decouple filters from actions, which would require that the Plugin API documentation at the codex be updated.

[darkdragon]

Separate action and filter hooks out into their own globals.

[darkdragon]

Well, you know, the patch failed. Sigh. Oh well, I'll just let you do it then.

[ryan]

Using the array union operators means that duplicated keys won't be overwritten. I think we should just forget about giving all a priority and just fire the all hooks either before or after the regular hooks. No merge needed.

[ryan]

Patch gets rid of the merge entirely. 'all' filters and actions are done first, followed by the filter or action being invoked. Filters and actions are separated completely. The xdebug profiler shows notable speedup in add_action() and add_filter() with apply_filters() and do_action() being about the same speed as before.

[ryan]

tellyworth ran unit tests and said it looks alright. Committing.

[ryan]

(In [6316]) Separate action and filter arrays. Get rid of filter merging. fixes #5231

[mdawaffe]

With separation of $wp_filter from $wp_action, we need to change some of the hardcoded references to $wp_filter.

Attached adds $wp_action to unsets in wp-settings.php (necessary?) and fixes get_plugin_page_hook().

[ryan]

mdawaffe suggested wrapping these checks in some API. has_filter() and has_action().

[santosj]

Could it be combined with the functionality to get current hook executed?

[ryan]

(In [6319]) Add missing type arg on call to _wp_filter_build_unique_id(). see #5231

[ryan]

Patch implements has_filter and has_action and uses them where needed.

[mdawaffe]

has_filter_with_arg.diff = has_filter.diff plus the following.

1. has_filter( $tag ) improved to return false if $wp_filter[$tag] is empty (not just if not set).
2. remove_filter( $tag, $func, $priority ) improved to unset $wp_filter[$tag][$priority] as necessary (so that has_filter() can work).
3. new has_filter( $tag, $function ) syntax. Returns $priority if $function is attached to $tag, false otherwise.
4. tweak to internally used _wp_filter_build_unique_id() to make has_filter( $tag, $function ) work.
5. same for *_action.

[ryan]

if ( !count($GLOBALS['wp_filter'][$tag][$priority]) ) 

Could that be:

if ( empty($GLOBALS['wp_filter'][$tag][$priority]) ) 

[mdawaffe]

Yes, should do.

[ryan]

(In [6320]) has_action and has_filter. see #5231

[tellyworth]

all_action_fix_r6322.patch fixes a bug where the all action only ran once.

Unit tests are here:

​http://svn.automattic.com/wordpress-tests/wp-testcase/test_actions.php

​http://svn.automattic.com/wordpress-tests/wp-testcase/test_filters.php

[mdawaffe]

Replying to tellyworth:

all_action_fix_r6322.patch fixes a bug where the all action only ran once.

Does do_action_ref_array need a similar fix?

[ryan]

I'm finding that several plugins are doing add_action where they mean add_filter, and vice versa. Now that we've split filters and plugins, these plugins break.

[ryan]

I think we need to revert back to one array for both filters and actions. 'all' support can be simplified to pass the name of the hook and an array of all the args to functions hooked to 'all'. An 'all' function would not be expected to return a value. 'all' functions won't filter anything. They just get notification of the hook being processed and the args passed to that hook and that's it.

[santosj]

Can you add type of hook (filter|action) or is that what you mean?

[ryan]

Patch brings actions and filters back together in $wp_filter. 'all' functions are passed the name of the action/filter and all arguments. This means that 'all' hooks don't need to use current_filter() to get the current action/filter. We could also pass the hook type (filter or action), although I don't know how practically useful that is to the plugins that would be using 'all' hooks.

Functions attached to 'all' are not expected to return anything. This avoids needing a filter versus action distinction, allowing us to use one array to store both.

[ryan]

I also incorporated tellyworth's fix and moved the argument processing in do_action() down below the check to see if the action is set. This avoids unnecessary func_get_arg() calls.

[ryan]

(In [6324]) Change 'all' hook calling. Bring filters and actions back into one array for back-compat. see #5231

[tellyworth]

Unit tests pass against [6324]. There are no tests for has_filter/has_action yet though.

[darkdragon]

Does this mean this ticket is fixed?

[darkdragon]

Closing as fixed, since it appears that the original issue was solved quite some time ago.