Ticket #4625 (closed defect: fixed)

Opened 1 year ago

Last modified 1 year ago

apply_filters is broken

Reported by: Denis-de-Bernardy Assigned to: anonymous
Priority: high Milestone: 2.3
Component: General Version: 2.2.1
Severity: normal Keywords: has-patch
Cc:

Description

should be: 

	foreach ( $wp_filter[$tag] as $filter ) {
		foreach( (array) $filter as $the_ )
			if ( !is_null($the_['function']) ){
				$args[1] = $string;
				$string = call_user_func_array($the_['function'], array_slice($args, 1, (int) $the_['accepted_args']));
			}
	}

because if you use remove_filter() at some point, you can end up with an empty array on $wp_filters[$tag]. using while ( next($wp_filters[$tag]) ) at that point ends up ignoring a whole batch of applicable filters.

Attachments

4625.diff (432 bytes) - added by Nazgul on 07/21/07 14:48:32.

Change History

07/13/07 13:59:59 changed by Otto42

A simpler change to fix this would be to change the while ( next($wp_filters[$tag]) ) into: while ( next($wp_filters[$tag]) !== FALSE)

next() returns the next entry in $wp_filters[$tag], or false when there's no more there. While it's true that the empty array could fail the boolean evaluation, it cannot fail a !== FALSE check.

07/21/07 14:48:32 changed by Nazgul

  • attachment 4625.diff added.

07/21/07 14:49:52 changed by Nazgul

  • keywords set to has-patch.
  • priority changed from highest omg bbq to high.

Added a patch based on the suggestion by Otto42.

07/31/07 17:26:11 changed by santosj

Shouldn't you also add it to the other? There are two in that file. 

do { } while ( next($wp_filters[$tag]) );

07/31/07 19:19:48 changed by Otto42

The other one is in do_action, and yeah, in theory it could be susceptible to the same issue.

08/08/07 03:57:26 changed by darkdragon

Holy cow! I was able to reproduce this and wow, that is messed up.

The patch works also. I'm going to test to see if the other one needs it also.

08/08/07 04:39:48 changed by markjaquith

  • milestone changed from 2.2.2 to 2.3 (trunk).

Is there any good reason to leave these as do...while loops? (other than making this fix a smaller patch, that is)

do...while loops have their place, but iterating through an array is not it.

08/08/07 12:44:48 changed by darkdragon

Well, it could be faster having it as a while loop, but really, if all you need is to continue and iterate over all items, foreach might also work.

It is all the same. I think the do ... while was for making sure that the first item was iterated over. It is also perhaps safer to remove items while inside the array (perhaps, but would need testing for foreach). 

while( ( $filter = next( $wp_filter[$tag] ) !== false)
{
    foreach( ... )
    {

    }
}

A good reason: Because it works and won't require more testing.

08/08/07 17:37:55 changed by markjaquith

I like that reason. :-)

08/08/07 17:41:47 changed by markjaquith

  • status changed from new to closed.
  • resolution set to fixed.

(In [5857]) explicitly check next() against FALSE when iterating through filters. Props Denis-de-Bernardy, Otto42, Nazgul, santosj (go team effort!). fixes #4625 for trunk

08/28/07 10:24:03 changed by Denis-de-Bernardy

  • status changed from closed to reopened.
  • resolution deleted.

the same problem occurs for do_action

08/28/07 10:31:09 changed by DD32

the same problem occurs for do_action

The patch here only applied to apply_filters, but the SVN commit applied the patch to apply_filters and do_action.

Allthough i notice it wasnt applied to do_action_ref_array

08/28/07 19:06:56 changed by ryan

  • status changed from reopened to closed.
  • resolution set to fixed.

(In [5958]) explicitly check next() against FALSE in do_action_ref_array(). Props Denis-de-Bernardy, Otto42, Nazgul, santosj, DD32. fixes #4625 for trunk