do_action/apply_filters/etc. recursion on same filter kills underlying call

[kernfel]

Affects @wp-includes/plugin.php: do_action, do_action_ref_array, apply_filters, apply_filters_ref_array, _wp_call_all_hook

When calling a specific hook from a function that was called through that same hook, the remaining hooked functions from the first iteration will be discarded.
This is due to the handling of the array of registered functions using internal array pointers instead of a more robust approach.

In my example, this problem arose when I tried to programmatically delete a menu in reaction to the removal of a category. I hooked into the delete_term action to do so, upon which another function hooked into delete_term would no longer fire.
The obvious workaround is to adjust the priorities accordingly, but it shouldn't be necessary.

The current implementation as in apply_filters is:

reset( $wp_filter[ $tag ] );

if ( empty($args) )
	$args = func_get_args();

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

} while ( next($wp_filter[$tag]) !== false );

Using the following method instead, both iterations would develop properly:

if ( empty($args) )
	$args = func_get_args();

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

Array pointers are used for performance reasons.

Related: #9968

Duplicate: #19015

Another dup: #20998

#21169 has a patch.

#21169 does not resolve this issue. The problem here is that the if we've got recursive calls on apply_filters, when we return one level up, the array pointer might not be in the expected position(because of the reset and next/each over the same array). That'd require to make a copy of $wp_filter[ $tag ], and iterate over the copy, to be fixed.

@mrubiolvn: Please provide some sample code that demonstrates the bug.

function dummyfunc($postId) {
	$post = get_post($postId);
	$childPosts = get_posts( 'post_parent='.$postId );
	foreach($childPosts as $childPost) {
		$childPost->post_title = 'whatever';
		wp_update_post($childPost);
	}
	error_log(__FUNCTION__.' triggered for post id:'.$postId);
}
function dummyfunc2($postId) {
	error_log(__FUNCTION__.' triggered for post id:'.$postId);
}

add_action('save_post','dummyfunc',10);
add_action('save_post','dummyfunc2',15);

Now the flow would be like this:

save_post is triggered, and eventually dummyfunc is executed.
It saves a child post, so save_post is triggered again, only for the child post this time.
At this point, $wp_filter [ 'save_post' ] pointer is reset. When we're done with all the filters for the child post, and any other child posts, the execution will continue right after where we left it with the initial post. Now, we do execute next(or each with #21169) over $wp_filter [ 'save_post' ]. But the pointer will likely be on the end of the array, so dummyfunc2 will not be executed for the initial post.

another solution while retaining internal pointers is to use a dummy variable (since this would duplicate the intrnal pointer , according to PHP docs)

$foo=$wp_filter[ $tag ];
reset( $foo );

if ( empty($args) )
	$args = func_get_args();

do {
	foreach( (array) current($foo) as $the_ )
		if ( !is_null($the_['function']) ){
			$args[1] = $value;
			$value = call_user_func_array($the_['function'], array_slice($args, 1, (int) $the_['accepted_args']));
		}

} while ( next($foo) !== false );

changing the type back

#23035 was marked as a duplicate.

Comparison of 3 different methods of looping through actions

foo123's solution works, but it's slow because when we execute $foo=wp_filter[$tag] the whole (often very large) array gets copied to a new variable $foo.

Instead, I suggest this version:

foreach ($wp_filter[$tag] as &$action_to_execute) {
	foreach ((array)$action_to_execute as $the_)
		if ( !is_null($the_['function']) )
			call_user_func_array($the_['function'], array_slice($args, 0, (int) $the_['accepted_args']));
}

As with foo123's solution, a new dummy variable is used - $action_to_execute - but it's created by reference, so the array isn't copied. All nested actions get executed. Furthermore, I did some benchmarking tests, and found that this is actually quicker than the current implementation. (see above attached file).

I think this really should be fixed ASAP.

It's a serious bug, because a plugin author cannot safely call wordpress functions like wp_update_post() from inside an action. (Unless he/she first reads the entire source code for wp_update_post() and all other functions that are used, to make sure that the same action isn't called).

Also, it specifically affects blogs with both WPML and ACF plugins installed (2 of the most common plugins in WP). ACF and WPML both use the 'save_posts' action, and this bug prevents ACF's 'save_post' action being called.

If there's no performance degradation, we should give this a shot.

But unit tests are essential.

I don't agree with this change. If you move to a foreach loop, you'll lose the ability to add another callback to the same hook with a later priority.

function my_first_callback( $post_id, $post ) {
  if ( $post->post_type == 'pistachio' ) {
    add_action('save_post', 'my_second_callback', 15, 2);
  }
}
function my_second_callback( $post_id, $post ) {
  // do something else
}
add_action('save_post', 'my_first_callback', 10, 2);

I had always assumed that having this ability was a conscious design decision, not a bug. Removing it would definitely break some plugins.

I wrote a blog post a couple of years back about this issue (​http://xplus3.net/2011/08/18/wordpress-action-nesting/), and the workaround is fairly simple. In my opinion, it should be up to a plugin author to take care of the housekeeping if he decides to nest action calls.

Jbrinley, I don't agree with your comment.

You said:

In my opinion, it should be up to a plugin author to take care of the housekeeping if he decides to nest action calls.

My whole point is that it usually won't be a conscious decision by a plugin author to nest action calls. They won't be directly calling do_action('something'). But they might well use another wordpress function, which they read about in the wordpress API, which inadvertently sets off some other actions.

For example, I discovered this 'bug' through some strange behaviour in the Wordpress Multilingual plugin. They need to automatically update post B when post A is saved. So of course they hooked into the 'save_post' action when post A is saved, and called wp_update_post() on post B. This seems like a completely logical and sensible approach, but it 'accidentally' caused a nested 'save_post' action, and the inner action broke the outer action.

You're saying, the plugin author should be left to clean up his own mess. But how on earth would the plugin author be expected to know he'd made a mess in the first place? You cannot expect a standard wordpress function called in an action, to prevent the rest of the actions executing. The only way a plugin author could know, would be to have read the source code of do_action, wp_update_post, and everything else that stemmed from it.

I appreciate your point that you won't be able from inside a hook, to add another callback on the same hook. I must admit that it did not occur to me. But there's an easy workaround for that too: A plugin author just has to put the conditional logic inside the second callback instead of the first. In your example:

function my_first_callback( $post_id, $post ) {
  // do something
}
function my_second_callback( $post_id, $post ) {
  if ( $post->post_type == 'pistachio' ) {
    // do something else
  }
}
add_action('save_post', 'my_first_callback', 10, 2);
add_action('save_post', 'my_second_callback', 15, 2);

In contrast the workaround you suggest for plugins in your blog post (Remembering the position of the array pointer, then manually finishing the do_action loop after you break it.) is torturous and error prone, and depends on plugin authors having huge amount of knowledge about WP's internal code. It's against the whole spirit of plugins.

It violates the 'principle of least astonishment'.

A plugin author who sets up new actions after a sequence of actions has already started would surely not be too surprised to find it didn't work. And would easily be able to fix it.

A plugin author who calls a wp function inside his action, would be astonished to find that he had inadvertently prevented all other plugins that used the same action from working.

Furthermore, if a plugin author has a problem because he can't add new callbacks to the same hook, at least he will break his own plugin, and no-one else's. And he'll therefore know that it's not working and have a chance to fix it. The current behaviour is much nastier, because a plugin author is likely to instead break other plugins, and won't even know that he's done it.

If it were a matter of creating the WP plugin API from scratch, I would agree with you. But you're talking about a backwards-incompatible change; plugins will break. We need to approach that much more cautiously.

The only way a plugin author could know, would be to have read the source code of do_action, wp_update_post, and everything else that stemmed from it.

Or we could update the codex so that users of the function know that they'll be triggering certain hooks.

If you want to go forward with this, the first step is to figure out a way to deprecate the old functionality without breaking it. E.g., throw a _deprecated_argument warning or some such when calling add_action/add_filter on the currently running hook. Keep that in there for a version or two before breaking things. Set a target version a bit in the future so plugin developers know when their code will stop working. This is a big change; people deserve the notice.

Well, again, some good points. A backwards-incompatible change which breaks plugins isn't very nice. But I find the idea of waiting a few versions to fix something which must surely be considered a bug is unappealing too.

I've just had a really good look at the plugin.php code, and have come up with the following alternative approach:

Still use current() and next() to loop through the actions/filters, thus keeping the efficiency of not copying the array, and the ability to add actions to a hook from inside the execution of that same hook. But, give the do_acttion() / apply_filter() functions responsibility for keeping track of nestings.

There is already a global var called $wp_current_filter which maintains a stack of currently executing filters. At the start of execution, the name of the filter is pushed onto it, and then popped off at the end. This variable is only used by the current_filter() function, which just returns the name of the currently executing filter.

That stack could also be used to store the POSITION of the array pointer at the currently executing filter. Then, if the same filter is nested, when we return to the outer filter, the saved position could be used to continue executing the other filters from the point that it left off.

I think this approach would not change any previous behaviour, and therefore not break existing plugins, but it would stop an inner action from breaking the outer one. And so would probably fix a lot of plugins which didn't even know they were broken.

Any thoughts?

Replying to jbrinley:

I don't agree with this change. If you move to a foreach loop, you'll lose the ability to add another callback to the same hook with a later priority.
(...)
I had always assumed that having this ability was a conscious design decision, not a bug. Removing it would definitely break some plugins.

Hold... Are you seriously arguing that we should continue to live with breaking much about every plugin that hooks after akismet in the name of backwards compatibility? :-) (see #9968, which is the exact same issue.)