Make WordPress Core

Opened 16 years ago

Closed 16 years ago

#5231 closed defect (bug) (fixed)

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

Reported by: tellyworth's profile tellyworth Owned by:
Milestone: 2.5 Priority: normal
Severity: normal Version:
Component: General Keywords: has-patch needs-testing
Focuses: Cc:

Description

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.

Attachments (10)

plugin.#5231.r6311.diff (3.9 KB) - added by darkdragon 16 years ago.
Patch based on r6311, adds patch to fix #4715 and add functionality for both action and filter
plugin.#5231.r6312.diff (4.0 KB) - added by darkdragon 16 years ago.
Checks to make sure the actions aren't merged first.
plugin.#5231.r6312.take2.diff (4.0 KB) - added by darkdragon 16 years ago.
Speed enhancement to removing filters.
plugin.#5231.r6312.take3.diff (8.7 KB) - added by darkdragon 16 years ago.
Separate action and filter hooks out into their own globals.
action_filter.diff (8.8 KB) - added by ryan 16 years ago.
5231.diff (919 bytes) - added by mdawaffe 16 years ago.
has_filter.diff (2.7 KB) - added by ryan 16 years ago.
has_filter_with_arg.diff (5.8 KB) - added by mdawaffe 16 years ago.
all_action_fix_r6322.patch (437 bytes) - added by tellyworth 16 years ago.
filtaction.diff (9.2 KB) - added by ryan 16 years ago.

Download all attachments as: .zip

Change History (54)

#1 @darkdragon
16 years ago

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

#2 @darkdragon
16 years ago

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

#3 @tellyworth
16 years ago

Yup, I know it's intended for debugging.

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

#4 @ryan
16 years ago

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.

#5 @tellyworth
16 years ago

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

#6 @darkdragon
16 years ago

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.

#7 @ryan
16 years ago

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

@darkdragon
16 years ago

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

#8 @darkdragon
16 years ago

  • Keywords has-patch needs-testing added

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

#9 follow-up: @ryan
16 years ago

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.

#10 @ryan
16 years ago

  • Milestone changed from 2.5 to 2.4

#11 @darkdragon
16 years ago

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
16 years ago

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

@darkdragon
16 years ago

Speed enhancement to removing filters.

#12 @darkdragon
16 years ago

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.

#13 @darkdragon
16 years ago

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.

#14 in reply to: ↑ 9 @darkdragon
16 years ago

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.

#15 @ryan
16 years ago

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

#16 @darkdragon
16 years ago

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.

#17 @ryan
16 years ago

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.

#18 @darkdragon
16 years ago

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.

#19 @darkdragon
16 years ago

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
16 years ago

Separate action and filter hooks out into their own globals.

#20 @darkdragon
16 years ago

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

#21 @ryan
16 years ago

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
16 years ago

#22 @ryan
16 years ago

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.

#23 @ryan
16 years ago

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

#24 @ryan
16 years ago

  • Resolution set to fixed
  • Status changed from new to closed

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

#25 @mdawaffe
16 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

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

@mdawaffe
16 years ago

#26 @ryan
16 years ago

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

#27 @santosj
16 years ago

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

#28 @ryan
16 years ago

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

@ryan
16 years ago

#29 @ryan
16 years ago

Patch implements has_filter and has_action and uses them where needed.

#30 @mdawaffe
16 years ago

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.

#31 @ryan
16 years ago

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

Could that be:

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

#32 @mdawaffe
16 years ago

Yes, should do.

#33 @ryan
16 years ago

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

#35 in reply to: ↑ 34 @mdawaffe
16 years ago

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?

#36 @ryan
16 years ago

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.

#37 @ryan
16 years ago

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.

#38 @santosj
16 years ago

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

@ryan
16 years ago

#39 @ryan
16 years ago

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.

#40 @ryan
16 years ago

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.

#41 @ryan
16 years ago

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

#42 @tellyworth
16 years ago

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

#43 @darkdragon
16 years ago

Does this mean this ticket is fixed?

#44 @darkdragon
16 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

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

Note: See TracTickets for help on using tickets.