[wp-hackers] apply_filters, merge_filters and reset()

[Jacob]

Oh yeah, when I say "tag," I'm referring to $tag variable in each of the 
Plugin API functions and not to the taxonomy API.

Travis Snoozy wrote:
> On Wed, 07 Nov 2007 23:23:54 -0600, Jacob <wordpress at santosj.name>
> wrote:
>
>   
>> You should very well, make the bug report then for 2.3.2. It would at 
>> least fix some issues with actions that may have been edge cases.
>>     
>
> Sigh. I guess I'll do that. ;)
>
> <snip>
>   
>> I think the problem is that foreach doesn't expect for the array to
>> be updated inside of its loop. It only does the reset before it
>> starts.
>>     
>
> each()/next() does not solve this problem by itself, and there are no
> separate reset()s in the code, so foreach is the same as the current
> code in this respect.
>   
No, the while loop is handling the actual array and not a copy, 
therefore it will be updated if a new hook is added or removed. The 
foreach will not, which might be a good thing. There would need to be a 
reset before the while loop yes. Hey, if you check the bug for 
_build_hook_unique_id() (I can never remember the exact name). You'll 
find additional information that will be helpful.

If someone adds a new tag, then it should only be iterated over on the 
next call. It should not be expected to be able to call a new hook from 
within an added tag without calling that tag after the iteration is 
called first. As far as I see it, it isn't a bug in a bug in WordPress 
but in the plugin.

The situation I'm talking about is if a called hook adds or removes a 
hook within the same tag. This is a rare case, but one I've noticed 
while I've done my unit tests (however, the bug fix works for any sort 
of adding or removing of hooks no matter if it is the same tag or not).

Even through it is an edge case, it still might happen and if the 
expected result is not given (all tags are executed), then it would be a 
bug in WordPress, instead of the other way around.

Don't fix what isn't broken. The while loop is not what is broken and 
while I would like to see it a foreach, it is not a good idea.
>   
>> The second issue would be that foreach acts upon a copy of
>> the original array. Appending values to the original array will not
>> update the array referenced in the foreach construct.
>>     
>
> That's the killer, but only if the new item is, as you say, appended
> (and that's an assumption I'm willing to make, even though I haven't
> read all the code). However, then there's an issue in that calling
> add_filter or add_action will void the merge, so theoretically the
> set-check and (conditional) call to merge_filters needs to be done on
> *every iteration* -- which would cause the array pointer to be reset in
> the middle, and would in turn lead to infinite loopage.
>
> That said, voiding the merge value doesn't make sense if you just tack
> a new function onto the target -- it only makes sense to do a void if
> the hooks vs. the 'all' tag is updated, at which point you need to need
> to void the merged status on all the other tags at once. I don't think
> the code works if you add a new "all" hook in the middle (one, because
> merge_tags isn't checked/called every loop, two, because the merged
> status isn't removed on the individual tags). In fact, I don't think
> it will work if you call do_action("foo"), add_hook("all"),
> do_action("foo").
>
> This whole section of code is really unpleasant. :(
>
>
>   
Well, the all tag is bugged anyway and you shouldn't use it. You will 
lose priority information because of the array_merge function. The 
solution in 2.4 is to just call the all tag, if it exists, before 
iterating over all of the rest of the items in the current tag. Now that 
you mentioned it, there are several problems associated with 
merge_filters and it is probably for the best that it is gone.

-- 

Jacob Santos

http://www.santosj.name - blog
http://wordpress.svn.dragonu.net/unittest/ - unofficial WP unit test suite.

Also known as darkdragon and santosj on WP trac.