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']));
		}

[scribu]

Array pointers are used for performance reasons.

Related: #9968

[SergeyBiryukov]

Duplicate: #19015

[scribu]

Another dup: #20998

[SergeyBiryukov]

#21169 has a patch.

[mrubiolvn]

#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.

[scribu]

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

[mrubiolvn]

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.

[foo123]

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 );

[foo123]

changing the type back

[SergeyBiryukov]

#23035 was marked as a duplicate.

[cheeserolls]

Comparison of 3 different methods of looping through actions

[cheeserolls]

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.

[scribu]

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

[scribu]

But unit tests are essential.

[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.

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.

[cheeserolls]

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.

[jbrinley]

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.

[cheeserolls]

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?

[Denis-de-Bernardy]

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

[SergeyBiryukov]

#24691 was marked as a duplicate.

[markjaquith]

No patch, punting out of 3.6.

[Denis-de-Bernardy]

Replying to markjaquith:

No patch, punting out of 3.6.

Can you explain how the patch in the ticket's description isn't good?

If it's a mere matter of copy/pasting it and creating a diff, I'm sure someone who is following this ticket will be happy to do it so it gets in 3.6.

If it's something else, please explain what and why (and probably close this and other tickets, since the suggested patch is the only rational fix.)

[jbrinley]

I think the correct answer to this and related tickets (#9968, #21169, and other duplicates) is to create a iterator that more accurately encapsulates the array iteration while tracking newly added/removed callbacks. The patch I'm submitting (​https://gist.github.com/jbrinley/7024059, I'll attach in just a moment) should do so in a backwards-compatible way. I had three key objectives with this:

1. Maintain backwards compatibility
2. Support recursive calls to functions that trigger hooks
3. Support manipulation of hook callbacks while the hook is running

In this patch you'll find:

• A new class, WP_Hook_Iterator, that can be used in place of a foreach loop to get each callback for an action/filter hook
• Updates to apply_filters, apply_filters_ref_array, do_action, do_action_ref_array, and _wp_call_all_hook to use the new iterator
• Unit tests asserting that you can call a hook recursively
• Unit tests asserting that you can manipulate the callbacks of a hook while that hook is running

This also passes all current unit tests, including the test added for #21169 (in tests/phpunit/tests/filters.php).

Obviously, this is a key area in WordPress, so this patch deserves careful scrutiny. Are there any edge cases we're missing in the tests? How does this affect performance? Can we accomplish the same goals more efficiently?

[jbrinley]

Implement the WP_Hook_Iterator for action/filter loops

[jbrinley]

I've run some performance benchmarking on this patch, using the code at ​https://gist.github.com/jbrinley/7518483.

Running 10000 times with 3 callbacks
Total Time (Old): 0.887964
Average Time (Old): 0.000042
Total Time (New): 1.961968
Average Time (New): 0.000149

Running 10000 times with 15 callbacks
Total Time (Old): 3.754449
Average Time (Old): 0.000160
Total Time (New): 8.169822
Average Time (New): 0.000606

Running 10000 times with 150 callbacks
Total Time (Old): 38.625702
Average Time (Old): 0.001561
Total Time (New): 184.439555
Average Time (New): 0.016029

Summary: calling do_action/apply_filters with the new iterator is less performant. With callbacks on three random priorities, it runs in about 354% of the time. With 15 random callbacks, that increases to 404%. Crank it up to 150 different priorities, and that increases to 1026%.

I would love for those numbers to be smaller, but a path thither is not clear to me. Using a global $merged_filters or similar doesn't work with nested hooks. If anyone has any suggestions, I'd appreciate a comment here.

How do those numbers translate to real-world performance impact? On a stock WP install on my test server, apply_filters increases from 66ms to 88ms. I also see a negligible gain of a couple of ms in add_filter by removing the unset on $merged_filters;

[jbrinley]

I've attached a new version, 17817.3.patch. This one is a bit more invasive, changing the way the $wp_filter global works (it's now an object containing an array of WP_Hook objects). This allows the iterators to watch for changes to filters, enabling a more efficient algorithm for iterating over the priorities.

Running the same benchmarking script (slightly modified to account for the new data structure):

Running 10000 times with 3 callbacks
Total Time (Old): 0.957014
Average Time (Old): 0.000042
Total Time (New): 2.570118
Average Time (New): 0.000155

Difference: 369%

Running 10000 times with 15 callbacks
Total Time (Old): 3.978540
Average Time (Old): 0.000155
Total Time (New): 9.040840
Average Time (New): 0.000469

Difference: 302%

Running 1000 times with 300 callbacks
Total Time (Old): 8.850893
Average Time (Old): 0.003229
Total Time (New): 17.300127
Average Time (New): 0.007968

Difference: 246%

Running 1000 times with 3000 callbacks
Total Time (Old): 88.171389
Average Time (Old): 0.027288
Total Time (New): 157.092921
Average Time (New): 0.059824

Difference: 219%

Summary: still less performant, but the difference actually decreases now as the number of callbacks increases.

On real page loads, I'm seeing about a 3% increase in total load time (increasing from ~660ms to ~680ms on my server) when profiling with xdebug. Using ab, I see an increase of about 5% (from ~190ms to ~200ms).

This still passes all unit tests, including when run with the flag "--group hooks,21169,17817,9968".

[jbrinley]

Just realized that I left in some inappropriate usages of spl_object_hash. I've updated 17817.3.patch to remove them.

[SergeyBiryukov]

#26239 was marked as a duplicate.

[rzen]

The patch attached to #26239 is a clean and lightweight solution to the issue at hand. I'm certainly not opposed to a more robust rewrite — there are many benefits to be gained no doubt — but it would be great if we could fix the issue first, then iterate and improve from there.

[jbrinley]

Replying to rzen:

The patch attached to #26239 is a clean and lightweight solution to the issue at hand. I'm certainly not opposed to a more robust rewrite — there are many benefits to be gained no doubt — but it would be great if we could fix the issue first, then iterate and improve from there.

Just like all the other solutions that make a copy of the array, ​http://core.trac.wordpress.org/attachment/ticket/26239/26239.patch is not backwards compatible. You can't modify callbacks for a filter while it is running.

[Denis-de-Bernardy]

Replying to jbrinley:

Just like all the other solutions that make a copy of the array, ​http://core.trac.wordpress.org/attachment/ticket/26239/26239.patch is not backwards compatible. You can't modify callbacks for a filter while it is running.

Expanding on my earlier point in this regard, maintaining this backward compatible behavior is of dubious merit at the very best. Particularly if maintaining it involves a plugin API that is an order of magnitude slower.

Look… Whoever writes the following code expecting 'bar' to come out deserves to see their code blow up in their face:

add_action('foo', function() {
  add_action('foo', function() {
    echo 'bar';
  }
});
do_action('foo');

Yes, it currently works in WP. But it shouldn't: you don't catch a plane that is already departed.

On the contrary, the following *should* output 'bar' and doesn't work:

function foo() {
  remove_action('foo', 'foo');
}
add_action('foo', 'foo');

function bar() {
  echo('bar');
}
add_action('foo', 'bar');

do_action('foo');

Our current approach is to crash the entire plane if anyone wants to get off.

Trying the same examples in a framework with a sane observer pattern implementation will yield the expected results. Whereas here we are haggling over keeping some manifestly broken behavior around for the sake of maintaining backwards compatibility that nobody cares about.

Can we please stop this madness?

/rant

[sc0ttkclark]

@Denis-de-Bernardy: What is your suggestion? Just wanted to be sure it was clear what you were championing, could you clarify or suggest something different?

@SergeyBiryukov: The patch ​http://core.trac.wordpress.org/attachment/ticket/17817/17817.3.patch -- is this is something that can move forward since it passes unit tests, or does this require further feedback/direction from core devs? I know it's too big of a change to make 3.8, but what are the next steps to get this ticket resolved in *some* way?

[wonderboymusic]

The code looks great, but it can't be that slow

[jbrinley]

@sc0ttkclark - I'm not quite comfortable saying that 17817.3.patch is ready for core, yet. As @wonderboymusic just said, it needs to be more performant. If core devs, or anyone else, has suggestions along those lines, they would be welcome. It's already improved from 17817.2.patch, based on feedback @nacin and @markjaquith gave at WordCamp Orlando last week.

We definitely need to get this bug fixed, and it needs to be done in a backwards-compatible way. A minor performance impact is probably worth it. I'm just not sure what the threshold is for "minor". 5% is probably too big. 1-2%, maybe even 3%, would probably be acceptable, in my opinion.

[Denis-de-Bernardy]

Replying to sc0ttkclark:

@Denis-de-Bernardy: What is your suggestion? Just wanted to be sure it was clear what you were championing, could you clarify or suggest something different?

My own opinion on this hasn't moved by one iota in over 4 years (#9968): we should replace our current mess with foreach() loops where needed, and shed a crocodile tear or two if we inconveniently break an already half-broken plugin in the process.

@SergeyBiryukov: The patch ​http://core.trac.wordpress.org/attachment/ticket/17817/17817.3.patch -- is this is something that can move forward since it passes unit tests, or does this require further feedback/direction from core devs? I know it's too big of a change to make 3.8, but what are the next steps to get this ticket resolved in *some* way?

I'll volunteer an opinion on that patch: it's nice to see some object-oriented code, but if we introduce changes of that magnitude, we might as well backport Symfony's EventDispatcher to make it php 5.2 compatible, and wrap it around the legacy API functions.

​http://symfony.com/doc/current/components/event_dispatcher/introduction.html

That way, we'd be building on top of something sound, and eventually begin paving the way towards potentially interesting behaviors such as stopping event propagation — which, incidentally, could be used to "break" the legacy API functions while keeping a perfectly functional API at its side.

[SergeyBiryukov]

Related: [4955] (for #3875).

[jbrinley]

Here's my next attempt. Executive summary: Still slower (but more reasonably so) in some cases, slightly faster in the most common case, backwards compatible, passes all unit tests. Let's ship it.

Details:

Results of benchmarking using ​https://gist.github.com/jbrinley/9458582:

Running 10000 times with 0 callbacks
Total Time (Old): 0.093368
Average Time (Old): 0.000004
Total Time (New): 0.078037
Average Time (New): 0.000004
New runs in 83.58% of the time of old.

Running 10000 times with 3 callbacks
Total Time (Old): 1.299951
Average Time (Old): 0.000064
Total Time (New): 1.822003
Average Time (New): 0.000117
New runs in 140.16% of the time of old.

Running 10000 times with 15 callbacks
Total Time (Old): 4.006840
Average Time (Old): 0.000184
Total Time (New): 6.443348
Average Time (New): 0.000390
New runs in 160.81% of the time of old.

Running 1000 times with 300 callbacks
Total Time (Old): 8.345846
Average Time (Old): 0.003690
Total Time (New): 13.177368
Average Time (New): 0.007553
New runs in 157.89% of the time of old.

Running 100 times with 3000 callbacks
Total Time (Old): 8.087688
Average Time (Old): 0.031045
Total Time (New): 10.641660
Average Time (New): 0.052878
New runs in 131.58% of the time of old.

You'll notice that with 0 callbacks, the new code is actually a bit more performant than the existing code. For the vast majority of hooks, this will be the case. In all other cases, the new code for running do_action() takes 30-60% longer than the old code.

Translating this into actual page loads using ab on a stock WP install (svn trunk@27490, twentyfourteen theme):

Existing code:

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.1      0       0
Processing:   177  190  11.0    191     209
Waiting:      177  188   8.8    191     205
Total:        177  190  11.0    192     209

17817.4.patch:

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.1      0       0
Processing:   175  186   7.3    189     196
Waiting:      175  185   7.3    189     196
Total:        175  186   7.3    189     197

A statistically insignificant 2% faster with the new code, largely due to the fact that most hooks don't have any callbacks most of the time.

[rzen]

Replying to jbrinley:

Here's my next attempt. Executive summary: Still slower (but more reasonably so) in some cases, slightly faster in the most common case, backwards compatible, passes all unit tests. Let's ship it.

I like everything about this summary. Faster than the other proposed solutions, passes all tests, backwards compatible (unlike the other solutions), AND it fixes the issue. The only way it could be better is if it were faster across the board.

Really hope we can get a core dev to bless this one :)

[mbijon]

+1 for attachment:17817.4.patch​

At the very least we should include the tests from '17817.4.patch' in the suite & mark them as skipped. No reason to not include those tests, as they'll help steer any suggested updates/enhancements

[Denis-de-Bernardy]

Likely -1 here. Won't 17814.4 break plugins that look into the $wp_filter global in an effort to remove observers added by plugins that are anonymous? Plugins and such that change $wp_filter directly, such as what occurs in some of the unit tests?

[jbrinley]

Replying to Denis-de-Bernardy:

Likely -1 here. Won't 17814.4 break plugins that look into the $wp_filter global in an effort to remove observers added by plugins that are anonymous? Plugins and such that change $wp_filter directly, such as what occurs in some of the unit tests?

@Denis-de-Bernardy, would your concerns be assuaged if WP_Hook implemented ArrayAccess, IteratorAggregate, and Countable (basically extending ArrayObject)?

In my opinion, it's unnecessary bloat for the benefit of a few edge cases where plugins take advantage of undocumented WP internals. But, my opinion aside, the solution is relatively straight forward. I'd appreciate your feedback, and maybe even a unit test or two specifying exactly what you expect from it.

[toscho]

If we are are talking about ​removing anonymous objects – this must continue to work. This solution is active on many sites, we would break too much.

WP_Hook_Iterator::__construct() should be SOLID, i.e. depend on an abstraction, not on a concrete class. There should be an interface for WP_Hook.

[SergeyBiryukov]

#27488 was marked as a duplicate.

[Denis-de-Bernardy]

Replying to jbrinley:

Replying to Denis-de-Bernardy:

Likely -1 here. Won't 17814.4 break plugins that look into the $wp_filter global in an effort to remove observers added by plugins that are anonymous? Plugins and such that change $wp_filter directly, such as what occurs in some of the unit tests?

@Denis-de-Bernardy, would your concerns be assuaged if WP_Hook implemented ArrayAccess, IteratorAggregate, and Countable (basically extending ArrayObject)?

Yeah. Look at doctrine/common, or is that collection... At any rate, there's a collection class in there that implements all three, complete with unit tests.

In my opinion, it's unnecessary bloat for the benefit of a few edge cases

It's not a few edge cases. See toscho's reply: he links to one of the highest voted questions in WP Answers.

Nor is it plugins taking advantage of undocumented internals: people make do with what they have -- including, for that matter, WP itself when it messes around with wp_filters in its own test suite.

My own use, or the WP unit test suite itself, is to pre-load wp filters prior to loading WP. So it should ideally convert any pre-existing wp_filter global as appropriate at the top of the file if any are present.

[jbrinley]

Would appreciate some feedback from @SergeyBiryukov or other core devs. Is patch 17817.4 ready to go, or do Denis-de-Bernardy's concerns merit additional consideration?

This bug has been plaguing us for years; I just need to know what it will take to get this fixed in core.

[edward_plainview]

I've recently encountered this same ... "bug" with my Broadcast plugin that hooked into the same save_post hook that another plugin at an earlier priority used. Took me hours of debugging to find the problem.

Long story short: the earlier plugin is called, calls save_post itself, broadcast is run as per save_post, and then the hook finishes, without calling the "outer" save_post loop.

This needs to be fixed since I wasn't expecting the hook looping to be broken. My patch uses foreach loops, but I see that others have suggested foreaches also.

[jbrinley]

I've attached 17817.5.patch. That should address the concern from comment 52; existing code for removing anonymous callbacks should continue to work. Also added another unit test to verify (all other unit tests still passing).

Any objections? Can we move forward with this?

[wonderboymusic]

Let's give this a good look for 4.0

[wonderboymusic]

The first time I ran unit tests, it took 1.5 minutes longer. The second time, 45 seconds. I think we need to squeeze some more performance for this to be viable.

[wonderboymusic]

See above - needs performance gains to be considered.

[Denis-de-Bernardy]

Replying to wonderboymusic:

See above - needs performance gains to be considered.

What was the objection to switching a simple foreach loop again? Isn't it time we take a rational approach to this and acknowledge that a very slight backwards compat issue in 4.0 might be fine in the interest of adding some sanity to the plugin API?

[jbrinley]

Went through a lot of iterations before arriving at the version just attached in 17817.6.patch.

Let's start with performance: this is as good as or better than what currently exists. Better with 0 callbacks or 3+ callbacks, about the same at 1-2 callbacks. Results of the test at ​https://gist.github.com/jbrinley/7518483

Running 10000 times with 0 callbacks
Total Time (Old): 0.042208
Average Time (Old): 0.000004
Total Time (New): 0.037750
Average Time (New): 0.000004
New runs in 89.44% of the time of old.

Running 10000 times with 1 callbacks
Total Time (Old): 0.212133
Average Time (Old): 0.000021
Total Time (New): 0.239030
Average Time (New): 0.000024
New runs in 112.68% of the time of old.

Running 10000 times with 2 callbacks
Total Time (Old): 0.312370
Average Time (Old): 0.000031
Total Time (New): 0.314523
Average Time (New): 0.000031
New runs in 100.69% of the time of old.

Running 10000 times with 3 callbacks
Total Time (Old): 0.400735
Average Time (Old): 0.000040
Total Time (New): 0.384914
Average Time (New): 0.000038
New runs in 96.05% of the time of old.

Running 10000 times with 5 callbacks
Total Time (Old): 0.604012
Average Time (Old): 0.000060
Total Time (New): 0.515111
Average Time (New): 0.000052
New runs in 85.28% of the time of old.

Running 10000 times with 10 callbacks
Total Time (Old): 1.133043
Average Time (Old): 0.000113
Total Time (New): 0.888408
Average Time (New): 0.000089
New runs in 78.41% of the time of old.

Running 10000 times with 15 callbacks
Total Time (Old): 1.535434
Average Time (Old): 0.000154
Total Time (New): 1.266079
Average Time (New): 0.000127
New runs in 82.46% of the time of old.

Running 1000 times with 300 callbacks
Total Time (Old): 2.447496
Average Time (Old): 0.002447
Total Time (New): 2.088821
Average Time (New): 0.002089
New runs in 85.35% of the time of old.

Running 100 times with 3000 callbacks
Total Time (Old): 0.999711
Average Time (Old): 0.009997
Total Time (New): 0.810893
Average Time (New): 0.008109
New runs in 81.11% of the time of old.

@wonderboymusic was concerned about the extra time spent running unit tests. On my system, running tests against current WP trunk takes about 3:23 (average of 4 runs). Running with this patch: 3:28 (average of 4 runs).

I think the performance concerns have been addressed. Many thanks to those of you who kept pushing for more; eventually we got it.

On to functionality: Anonymous callbacks can be removed. Plugins that setup callbacks before WordPress initializes (e.g., batcache, the WP unit test suite) will continue to work. Hooks can be called recursively with no unexpected side effects. Callbacks can be added and removed mid-run with no unexpected side effects.

I removed a unit test added in 4.0 for ticket #29070. It tested that the array pointer for the global $wp_filter didn't change when calling has_action(). Since this new implementation no longer depends on the global array pointer, that test was rendered superfluous. The array pointer does change, but it doesn't matter.

I abandoned the custom iterator that characterized my previous patches. No matter how much I optimized, it just was not performing as well as I would have liked. I was able to squeeze a lot more out of it by keeping the management of the iteration within the WP_Hook class, at the slight cost of a cleaner separation of responsibilities.

Any questions? Any concerns? Anything holding this back?

[rzen]

John, you magnificent and glorious man, these performance stats are excellent! I'm going to put it through some paces on the code where @sc0ttkclark and I first noticed the issue.

[sc0ttkclark]

Saw this come through my e-mail and was so happy!

[jbrinley]

Just realized that the patch left in a bit of debugging code. Ignore 17817.6.patch, use 17817.6.2.patch.

[leewillis77]

Great to see this being worked on - I just wasted a good two hours trying to work out why a hook wasn't being fired with this as the root cause (Not in my code thankfully).

Good news - the patch here resolves the issue I was having. However - it causes issues where there are multiple callbacks registered against a hook, and an earlier hook accepts fewer arguments than a subsequent hook, e.g.

add_action($tag, array(&$a1, 'action'), 10, 2);
add_action($tag, array(&$a2, 'action'));
add_action($tag, array(&$a3, 'action'), 10, 2);

In this situation, the third callback will only receive one argument, not two. The revised patch attached resolves this, complete with a test to prove it. I cannot for the life of me work out why though - it's as though the array_slice is affecting $args (Which it shouldn't).

In my defense, it's late in the day though ...

So - this probably needs more work (At least we'd need to verify that creating a copy of the args rather than reusing a reference doesn't impact on performance (CPU or memory)), but throwing this out for discussion.

[leewillis77]

Apparently that diff didn't include the new WP_Hook class. Updated patch attached. For info, the only real change to the code is the change from:

$func_args =& $args;

to

$func_args = $args;

in WP_Hook::do_action()

[jbrinley]

Replying to leewillis77:

In this situation, the third callback will only receive one argument, not two. The revised patch attached resolves this, complete with a test to prove it. I cannot for the life of me work out why though - it's as though the array_slice is affecting $args (Which it shouldn't).

Great catch, @leewillis77. After the assign-by-ref happens on one iteration, the reference is maintained in the next iteration when the array is sliced.

I'll run some benchmarking, but I expect the change to be inconsequential. Thanks for testing!

[leewillis77]

Question: Should do_action() and apply_filters() check whether the callback is callable before attempting to call it - thus ending the age old "PHP Warning: call_user_func_array() expects parameter 1 to be a valid callback" errors when invalid callbacks are used - or is that warning considered a "feature"?

See also the discussion on https://core.trac.wordpress.org/ticket/29803

Does anyone know from experience whether is_callable() is suitably performant - or do we need to test if we want to add it?

[jbrinley]

Replying to leewillis77:

Does anyone know from experience whether is_callable() is suitably performant - or do we need to test if we want to add it?

I did some rough testing of it at one point. I can't recall the exact numbers, but it slowed down the loop considerably.

[jbrinley]

Replying to leewillis77:

At least we'd need to verify that creating a copy of the args rather than reusing a reference doesn't impact on performance (CPU or memory)

In most cases, the difference was inconsequential. I had to bump the number of arguments up to around 100 before I started seeing a noticeable (i.e., > 5%) impact on performance. 17817.9.patch negates that impact, by avoiding the copy entirely (there will still be a copy when the callback is called, but there won't be two copies made).

[jbrinley]

Replying to leewillis77:

Does anyone know from experience whether is_callable() is suitably performant - or do we need to test if we want to add it?

Just ran some tests. is_callable() adds about 10% to the do_action execution time.

[leewillis77]

OK, so sounds like that's not worth adding - 17817.9.patch looking good to me - cheers Jonathan.

[jbrinley]

@johnbillion requested some memory statistics in IRC. Had to break the tests out into multiple files so we could isolate processes and report peak memory independently. ​https://gist.github.com/jbrinley/7518483 is updated to reflect the change. Looks like the new code uses about 3% more memory for the hook system (i.e., about 50-100 KB more memory).

$ php tests.local/test_17817.php 
Running 10000 times with 0 callbacks
Total Time (Old): 0.078679
Average Time (Old): 0.000008
Peak Memory after Old: 1755592

Total Time (New): 0.057374
Average Time (New): 0.000006
Peak Memory after New: 1800080

New runs in 72.92% of the time of old.
New runs in 102.53% of the memory of old.

$ php tests.local/test_17817.php 
Running 10000 times with 1 callbacks
Total Time (Old): 0.264266
Average Time (Old): 0.000026
Peak Memory after Old: 1758256

Total Time (New): 0.260950
Average Time (New): 0.000026
Peak Memory after New: 1804336

New runs in 98.75% of the time of old.
New runs in 102.62% of the memory of old.

$ php tests.local/test_17817.php 
Running 10000 times with 2 callbacks
Total Time (Old): 0.339878
Average Time (Old): 0.000034
Peak Memory after Old: 1759384

Total Time (New): 0.340070
Average Time (New): 0.000034
Peak Memory after New: 1805408

New runs in 100.06% of the time of old.
New runs in 102.62% of the memory of old.

$ php tests.local/test_17817.php 
Running 10000 times with 3 callbacks
Total Time (Old): 0.434463
Average Time (Old): 0.000043
Peak Memory after Old: 1760080

Total Time (New): 0.422050
Average Time (New): 0.000042
Peak Memory after New: 1806296

New runs in 97.14% of the time of old.
New runs in 102.63% of the memory of old.

$ php tests.local/test_17817.php 
Running 10000 times with 5 callbacks
Total Time (Old): 0.630719
Average Time (Old): 0.000063
Peak Memory after Old: 1761904

Total Time (New): 0.557231
Average Time (New): 0.000056
Peak Memory after New: 1808352

New runs in 88.35% of the time of old.
New runs in 102.64% of the memory of old.

$ php tests.local/test_17817.php 
Running 10000 times with 10 callbacks
Total Time (Old): 1.133310
Average Time (Old): 0.000113
Peak Memory after Old: 1766352

Total Time (New): 0.917521
Average Time (New): 0.000092
Peak Memory after New: 1813496

New runs in 80.96% of the time of old.
New runs in 102.67% of the memory of old.

$ php tests.local/test_17817.php 
Running 1000 times with 100 callbacks
Total Time (Old): 0.913040
Average Time (Old): 0.000913
Peak Memory after Old: 497704

Total Time (New): 0.748710
Average Time (New): 0.000749
Peak Memory after New: 574104

New runs in 82.00% of the time of old.
New runs in 115.35% of the memory of old.

$ php tests.local/test_17817.php 
Running 1000 times with 1000 callbacks
Total Time (Old): 6.859408
Average Time (Old): 0.006859
Peak Memory after Old: 1273632

Total Time (New): 5.400719
Average Time (New): 0.005401
Peak Memory after New: 1400144

New runs in 78.73% of the time of old.
New runs in 109.93% of the memory of old.

[jbrinley]

Per discussion with @johnbillion ​in Slack, 17817.10.patch implements the ArrayAccess interface for the WP_Hook class, adding backwards compatibility with code directly accessing the third level of the global $wp_filter array. Also adds a unit test for the new methods.

[jbrinley]

​@boonebgorges helpfully points out that we still need to avoid SPL interfaces. Fortunately, Countable is the only SPL interface implemented here, and it's actually not used for anything and can be safely removed. Therefore, 17817.11.patch.

[helgatheviking]

Now that there is a patch with good performance stats, what are the odds of this making it into a release soon?

[iseulde]

has-patch 4.2-early so moving to 4.2.

[jbrinley]

Followup to discussion in today's dev chat:

I've reviewed every instance of $wp_filter that can be found at ​https://github.com/search?q=%24wp_filter+user%3Awp-plugins&type=Code (it's a little finicky, so if no results load refresh and you may get some). Most instances are not a cause for concern; they're already compatible thanks to IteratorAggregate or ArrayAccess. I did find a handful of cases that would have compatibility issues:

Group 1 - Plugins that are messing with the $wp_filter global to work around this very issue. I only found one example here.

• ​https://github.com/wp-plugins/badgeos/blob/master/includes/rules-engine.php#L165

Group 2 - Plugins that do an is_array() check on a member of the $wp_filter global. Again, I found one example (and it's within debug tools for a plugin).

• ​https://github.com/wp-plugins/event-espresso-decaf/blob/master/core/helpers/EEH_Debug_Tools.helper.php#L81

Group 3 - Plugins that directly unset individual callbacks instead of using remove_action() / remove_filter() (this doesn't work because ArrayAccess doesn't allow indirect modification of an offset).

• ​https://github.com/wp-plugins/livepress/blob/master/php/livepress-post-format-controller.php#L243
• ​https://github.com/wp-plugins/search-by-sku-for-woocommerce/blob/master/wp-filters-extra.php#L40
• ​https://github.com/wp-plugins/contact-form-7-add-confirm/blob/master/includes/controller.php#L54-L79
• ​https://github.com/wp-plugins/wysija-newsletters/blob/master/helpers/conflicts.php#L84

Group 4 - Plugins that directly set individual callbacks, instead of using add_action() / add_filter() (same technical limitation as Group 3).

• ​https://github.com/wp-plugins/addme/blob/master/adder.php#L46
• ​https://github.com/wp-plugins/ram108-typo/blob/master/plugin.php#L42
• ​https://github.com/wp-plugins/jetpack/blob/master/class.json-api-endpoints.php#L1398

Nine examples out of 40K+ plugins, and all of them could be easily fixed. As much as I wish there were zero compatibility issues, I think this is more than reasonable.

[sc0ttkclark]

I remember having to do that BadgeOS hack :)

That's great news, in terms of your findings, eager to see this put to bed.

[DrewAPicture]

Some notes:

1. Worth making a decision about how/if we plan to support making this extensible, i.e. marking as final or not

2. Decide if there's merit in not introducing methods that directly mirror their legacy counterparts, e.g. new methods that take four parameters right out of the gate.

3. We need to do a full inline-docs pass on this before the initial commit (if it happens).

• Some missing parameter and return descriptions
• Missing @since tags
• Missing @access tags
• Inline documentation in these new methods explaining what's going on

4. I enjoy a good whitespace refactoring as much as the next developer, but I'd like to see a cleaner a patch with fewer vanilla whitespacing changes.

[nacin]

This should not land in core without lead developer approval.

[jbrinley]

17817.12.patch attached. It incorporates @wonderboymusic's formatting updates (but removes those from lines that would otherwise be untouched).

There were a few failing unit tests. Some came from #20920. That fix is now incorporated into this patch. Some came from #30936, but were really a consequence of #28535. Hook objects were backed up by reference, so new hooks added weren't removed. This updates WP_UnitTestCase to use clone when backing up and restoring $wp_filter.

Patch should now apply cleanly and all unit tests should pass.

Thanks for the feedback, @wonderboymusic and @DrewAPicture.

[toscho]

Replying to DrewAPicture:

1. Worth making a decision about how/if we plan to support making this extensible, i.e. marking as final or not

Only make it final if we have an interface. Abstract dependency injection or mocking is a nightmare already with WP_Post, let’s not repeat this mistake.

[jbrinley]

Some more statistics regarding memory usage, that may be more helpful than those above:

Peak memory loading the homepage with the 2014 theme.
Before Patch: 16.56 MB
After Patch: 16.62 MB

Peak memory loading a single post with the 2014 theme.
Before Patch: 16.74 MB
After Patch: 16.81 MB

Peak memory loading the dashboard.
Before Patch: 20.30 MB
After Patch: 21.38 MB

Peak memory loading /wp-admin/edit.php
Before Patch: 20.31 MB
After Patch: 21.05 MB

[jbrinley]

Per @boone's suggestion, I've put together draft release notes to go with this patch. The draft is available at ​https://docs.google.com/document/d/1TWd3i0GX7WsfosyCHG-EnnhANErVBsPnofvXp2qzX58/edit?usp=sharing

I would appreciate feedback.

[jbrinley]

On second though, a gist is probably better than google drive. Here it is: ​https://gist.github.com/jbrinley/eaaad00b52e1316c6904

[jbrinley]

More statistics, from today's Slack conversation:

nacin [18:16]

@jbrinley anything we can do to specifically improve upon any areas where it is slower? e.g. 12% slower for one callback.

jbrinley [22:04]

@nacin: I tried to find a way to optimize the one callback case, but failed. Anything I tried was either less performant overall, or failed to correctly handle edge cases (like a hook with a single callback, but that callback adds other callbacks while it's running).

The actual difference we're talking about is fairly minute. In a worst case scenario, where every hook that WP fires has a single callback assigned to it, 12% extra time running do_action() or apply_filters() adds approximately 0.01s to WP's total load time. But surely most of the hooks with one callback will be offset by other hooks with zero callbacks. When I load the home page of a clean install (2014 theme), apply_filters() or do_action is called 2520 times with zero callbacks vs 164 times with 1 callback.

summary: .00044 seconds lost is more than offset by .00112 seconds gained

[nacin]

A day later than promised, but here is my review.

Minor style:

• Please go over uses of empty() and ! empty() Only use it in conditionals when trying to avoid a notice. Otherwise use if ( $a ) or if ( ! $a ).

• false and true, not FALSE or TRUE.

• $the_[ 'function' ] should be $the_['function'], as 'function' is a literal. Same for $the_['accepted_args'].

• There is a space before !.

Performance:

• $this->callbacks[ $priority ][ $idx ] should store a numeric array. If appropriate, use constants such as self::FUNCTION and self::ACCEPTED_ARGS for 0 and 1 indexes. This will reduce memory usage. I have wanted to do this in core, but we couldn't for back compat (direct access of the array, of course). Can we do it now? Would this be manipulation in getIterator() (or our own iterator)? Lightweight object with ArrayAccess? Would any of these be worth it?

• Does the ksort() need to occur on add_filter()? This appears wasteful. I recognize this is a very key part of the nesting aspects of this change, but it feels heavy. (I wish PHP were more amenable to linked list manipulation, as in other languages we could do this without resorting to a sort. It's possible slicing would be faster than sorting, but I have no data to back this up.)

• $callbacks appears to unnecessarily be used by reference when $this->callbacks is iterated over in a foreach in a few places.

• I don't see why has_filters() isn't just return ! empty( $this->callbacks ).

• Should we try to avoid array_slice() in apply_filters() the same way we do in do_action()?

• The apply_filters() function now calls array_shift() in an unconditional fashion, even if there are no callbacks to apply. I recognize why it is done, but it feels like it is bringing back the same kind of thing we optimized in [17638].

• _wp_call_all_hook() appears like it is just an unnecessary function call at this point and should probably stop being used.

Other:

• What would be the use case for nesting with the all hook?

• It's possible that someone could be overwriting $wp_filter[ $tag ] with their own array. This would trigger a fatal error when a method is called. I don't know what to do about this, and I don't know how serious it is.

• It feels weird that _wp_filter_build_unique_id() was moved from the add_filter() function to the add_filter() method, but it remains in has_filter(), remove_filter(), etc. It appears that some functionality was deliberately left over in the functions -- like has_filter() when called without a function to check. Is this by design? (Is there a use case for WP_Hook used outside the context of these internal functions?)

• class-wp-hook.php should probably be included from plugin.php for compatibility, the way functions.php includes option.php.

Tests:

• This feels like it has a lot of good test coverage for what has been added, but I think we need a lot more test coverage for the existing functions and the new methods. That there's no new WP_Hook in the test suite seems to be a smell. We should also split up individual (sets of) assertions into separate tests.

This has taken a long time to progress to this state. I want to mention a few notable factors:

• Project history: This API is a pillar of WordPress. It is simple yet powerful. It's been tweaked over time, but the core of its underlying architecture has not significantly changed since it was first written more than a decade ago. The developers who wrote it are no longer active in the project's development. It is particularly challenging to consider a rewrite.

• Personal history: Last time I tried to touch something in plugin.php, I broke things really badly. See #21321. My comment there is innocuous and merely suggests there were "some side effects here". Let me be clear: This broke wordpress.org harder than I had ever broken it before, and ever broken it since. If the original developers can no longer be counted on to review this, and the last time (three years ago) I tried to go anywhere near this I blew things sky high, you can perhaps understand the reluctancy.

Conclusions and next steps:

• I want to recognize, and I very much appreciate, @jbrinley's long march shepherding this. Great work so far, and great improvements over time. Thank you for continuing to stay on our case about this.

• I am not comfortable committing this without ample lead time. It is too late for 4.2. While I should have found the time to review this earlier in the cycle, day two of a cycle is too late. (See supra the personal and project factors.) And this review notes a number of things to evaluate/study/tweak.

• It could definitely use some more tests, not just coverage for the new stuff, but to prevent regressions.

• We need a blog post written for make/core that explains to plugin authors exactly how they will benefit from this, and how code might break because of this.

• We probably do need one final cost-benefit analysis, where "benefits" are what this provides to plugin developers, and "costs" are what might break and how hard. The blog post could double as a request for comments.

• I am willing to deploy this to .org for a few hours on a weekend and see if it breaks anything.

• I am willing to commit this to trunk after 4.2 is released.

[DrewAPicture]

This is 4.3-early material. See comment:117.

[jbrinley]

Replying to nacin:

Minor style:

All should be fixed in 17817.13.patch.

Performance:

• $this->callbacks[ $priority ][ $idx ] should store a numeric array. If appropriate, use constants such as self::FUNCTION and self::ACCEPTED_ARGS for 0 and 1 indexes. This will reduce memory usage. I have wanted to do this in core, but we couldn't for back compat (direct access of the array, of course). Can we do it now? Would this be manipulation in getIterator() (or our own iterator)? Lightweight object with ArrayAccess? Would any of these be worth it?

This would probably cause some minor additional back-compat issues beyond what we've already identified. I'm not sure of the memory difference, but can run some tests.

• Does the ksort() need to occur on add_filter()? This appears wasteful. I recognize this is a very key part of the nesting aspects of this change, but it feels heavy.

As you say, it's key to supporting recursion. The sort runs when callbacks are added at a new priority to a hook that already has callbacks at other priorities. So it will generally run maybe a few dozen times (55 on the homepage of a fresh install, maybe a few dozen more depending on your plugins). The alternative is to go back to the $merged_filters approach (albeit encapsulated within each hook), and check if it's sorted every time apply_filters / do_action runs, which can come out to several hundreds of checks, and then also doing the sorting on many of those occasions (about 40 times instead of 55, on the page load above). I don't yet have much data to say if one approach is heavier than the other. My intuition is that the difference is inconsequential.

• $callbacks appears to unnecessarily be used by reference when $this->callbacks is iterated over in a foreach in a few places.

Fixed in 17817.13.patch.

• I don't see why has_filters() isn't just return ! empty( $this->callbacks ).

For backwards compatibility, if a plugin takes advantage of ArrayAccess to remove a callback but doesn't remove the priority index. Looping through the priorities ensures that we don't return a false positive. Assuming $this->callbacks is empty as it should be, the foreach won't have anything to iterate over and false will be returned. Wrapping the foreach in if ( $this->callbacks ) didn't show any performance improvement.

• Should we try to avoid array_slice() in apply_filters() the same way we do in do_action()?

Fixed in 17817.13.patch.

• The apply_filters() function now calls array_shift() in an unconditional fashion, even if there are no callbacks to apply. I recognize why it is done, but it feels like it is bringing back the same kind of thing we optimized in [17638].

Fixed in 17817.13.patch. array_shift() will only be called if there's a WP_Hook for the hook.

• _wp_call_all_hook() appears like it is just an unnecessary function call at this point and should probably stop being used.

Are you suggesting getting rid of the 'all' hook, or just inlining $wp_filter['all']->do_all_hook( $args );? I'll assume the latter. Are there any back-compat concerns for getting rid of the function?

Other:

• What would be the use case for nesting with the all hook?

I don't particularly see a use case. But if any callback on the all hook triggers another hook, all _will_ be called recursively, so it seems we should make it predictable.

• It's possible that someone could be overwriting $wp_filter[ $tag ] with their own array. This would trigger a fatal error when a method is called. I don't know what to do about this, and I don't know how serious it is.

Comment #99 above addresses everything that I found (not to say that there aren't any plugins I didn't find). Every case I found where someone was directly setting $wp_filter[ $tag ], it was to restore what was previously there. So a WP_Hook would be correctly restored. If someone does try to directly set it as an array, then yes, it would trigger a fatal, and there's nothing I can think of that we can do to stop them.

• It feels weird that _wp_filter_build_unique_id() was moved from the add_filter() function to the add_filter() method, but it remains in has_filter(), remove_filter(), etc. It appears that some functionality was deliberately left over in the functions -- like has_filter() when called without a function to check. Is this by design? (Is there a use case for WP_Hook used outside the context of these internal functions?)

Fixed in 17817.13.patch. I've moved all calls to _wp_filter_build_unique_id() inside of WP_Hook. Also changed the order of args for some functions to put $tag at the end (in anticipation of deprecating the argument in 40 years when we drop 5.2 support).

• class-wp-hook.php should probably be included from plugin.php for compatibility, the way functions.php includes option.php.

Heh. I had it there, and then Scott moved it to wp-settings.php. Now back to plugin.php.

• This feels like it has a lot of good test coverage for what has been added, but I think we need a lot more test coverage for the existing functions and the new methods. That there's no new WP_Hook in the test suite seems to be a smell. We should also split up individual (sets of) assertions into separate tests.

I'll work on this shortly.

• We need a blog post written for make/core that explains to plugin authors exactly how they will benefit from this, and how code might break because of this.

I'll work on expanding ​https://gist.github.com/jbrinley/eaaad00b52e1316c6904 to include a "benefits" section.

Thanks for the review, @nacin. I'm looking forward to seeing this in core.

[helen]

I am moving this as the first ticket into 4.3 also as recognition of the hard work and shepherding that's gone on here. It's on @nacin, though. He promised. :)

[jbrinley]

Attached 17817.14.patch. Refreshed onto 4.2, and adds a series of new unit tests for methods of the WP_Hook class (probably not yet all that @nacin wants).

[chriscct7]

#22127 was marked as a duplicate.

[jorbin]

@nacin - Still want to give this one a shot or are you going to punt again?

[wonderboymusic]

With a heavy heart

[samuelsidler]

Replying to helen:

He promised. :)

:(

[wonderboymusic]

Let's focus on this part:
Owned by: @nacin

[nacin]

Last I checked this ticket, I saw "(probably not yet all that @nacin wants)" regarding unit tests. Has anything advanced on that front?

If people feel comfortable with it, I'm game for dropping it into trunk now that 4.3 is out. Just don't be surprised if there are major explosions and we have to yank it quickly and try again after more tweaks and unit tests.

[pento]

Replying to nacin:

If people feel comfortable with it, I'm game for dropping it into trunk now that 4.3 is out.

+1, let's do that and see what explodes.

[wonderboymusic]

Replying to pento:

Replying to nacin:

If people feel comfortable with it, I'm game for dropping it into trunk now that 4.3 is out.

+1, let's do that and see what explodes.

+1 for explosion, we all know how to revert

[pento]

Needs some fixes before commit:

• 17817.14.patch​ doesn't apply cleanly against trunk.
• @since should be updated to 4.4.0.
• @DrewAPicture - should each method in WP_Hook have an @since doc, or is the one on the class enough?

[DrewAPicture]

Replying to pento:

• DrewAPicture - should each method in WP_Hook have an @since doc, or is the one on the class enough?

The class + every method, every property.

• Standards for methods (Summary, @since, @access, @param, @return): ​https://make.wordpress.org/core/handbook/best-practices/inline-documentation-standards/php/#1-functions-and-class-methods

• Standards for properties (Summary, @since, @access, @var): ​https://make.wordpress.org/core/handbook/best-practices/inline-documentation-standards/php/#2-class-properties

If we can get a refreshed patch in place, I can help with the docs.

[wonderboymusic]

For God and Country, I have refreshed this patch 17817.2.diff​

[DrewAPicture]

@wonderboymusic: Your community thanks you for your service. Next up: Docs review.

[jorbin]

I modified @wonderboymusic's patch to add a few more @ticket references in the unit tests.

Looking at this more, I think it might make sense to mark everything as final, at least initially. It's easy to remove final, but damn near impossible to add it back later on without breaking things. As hooks are the foundation of the WordPress API, I think it's important that they work consistently across WordPress installs.

[wonderboymusic]

Replying to jorbin:

Looking at this more, I think it might make sense to mark everything as final, at least initially.

methods in a class marked final inherit that sentiment, so marking the class makes sense and is a good idea

[jorbin]

Updated to include final on the class.

[DrewAPicture]

Docs audit

[DrewAPicture]

Did a docs pass in 17817.5.diff​ off of 17817.4.diff​.

Added:

• A file header
• Complete property docs
• Complete method docs
• Some inline-spacing and a few comments

Changed:

• I changed the argument order for ->add_filter() to have the two optional parameters be last (and to better follow the standalone pattern of 'hook', 'callback', 'priority', 'args'. Besides, it's always a little weird when optional parameters are in between required ones.

@wonderboymusic: Tagging you back into the ring.

[wonderboymusic]

I like these numbers

 PHPUnit #1
 2.04 mins/179.75MB
 
 PHPUnit #2
 1.9 mins/179.75MB
 
 Patch adds 618 assertions
 
 PHPUnit #1 w/ patch
 2.21min/180MB
 
 PHPUnit #2 w/ patch
 1.96min/180MB

[kitchin]

17817.5.diff is missing the /tests/phpunit/tests/hooks/* from 17817.4.diff
Also I wonder if the function/method signatures in 17817.5 are so similar that it's confusing:

function add_filter( $tag, $function_to_add, $priority = 10, $accepted_args = 1 )
public function add_filter( $function_to_add, $tag, $priority, $accepted_args )

[DrewAPicture]

Looks like I missed re-adding a couple of the tests files back on the last patch. Regenerating ...

[DrewAPicture]

Regenerated .5 with the missing tests in 17817.6.diff​

[DrewAPicture]

.5 + missing tests

[jorbin]

attachment:17817.7.diff​ updates the unit tests based on the signiture change @drewapicture proposed (and I agree with). Unit tests run clean and quick on the latest nightly of PHP 7.

I think we are ready to play ball.

[wonderboymusic]

chatted with @nacin earlier, he is doing final review

[wonderboymusic]

Replying to wonderboymusic:

chatted with @nacin earlier, he is doing final review

still in @nacin's hands, I imagine he'll update us soon

[DrewAPicture]

#21172 was marked as a duplicate.

[wonderboymusic]

organizing tickets

[johnbillion]

As per Slack chat, deadline for this to go in is Monday otherwise it's getting too late in the cycle to get a good soaking.

[chriscct7]

#21169 was marked as a duplicate.

[ocean90]

I have applied 17817.7.diff​ on my dotorg sandbox and it broke our forums which are running bbPress trunk.

I can reproduce the same with the current stable version 2.5.8 of bbPress on my local install. These are the warnings I get:

( ! ) Warning: ksort() expects parameter 1 to be array, object given in /srv/www/wp-shared/develop-plugins/bbpress/includes/core/template-functions.php on line 316
Call Stack
#	Time	Memory	Function	Location
1	0.0018	244024	{main}( )	.../index.php:0
2	0.0025	244472	require( '/srv/www/wp-develop/svn/src/wp-blog-header.php' )	.../index.php:17
3	0.0028	245368	require_once( '/srv/www/wp-develop/svn/src/wp-load.php' )	.../wp-blog-header.php:12
4	0.0038	246400	require_once( '/srv/www/wp-develop/svn/wp-config.php' )	.../wp-load.php:42
5	0.0041	249208	require_once( '/srv/www/wp-develop/svn/src/wp-settings.php' )	.../wp-config.php:102
6	0.1794	4354752	do_action( string(17), ??? )	.../wp-settings.php:336
7	0.1794	4355424	WP_Hook->do_action( array(1) )	.../plugin.php:454
8	0.1794	4356320	call_user_func_array:{/srv/www/wp-develop/svn/src/wp-includes/class-wp-hook.php:282} ( string(21), array(1) )	.../class-wp-hook.php:282
9	0.1795	4356688	bbp_after_setup_theme( string(0) )	.../class-wp-hook.php:282
10	0.1795	4356736	do_action( string(21), ??? )	.../sub-actions.php:345
11	0.1795	4357408	WP_Hook->do_action( array(1) )	.../plugin.php:454
12	0.1796	4358560	call_user_func_array:{/srv/www/wp-develop/svn/src/wp-includes/class-wp-hook.php:282} ( string(24), array(1) )	.../class-wp-hook.php:282
13	0.1796	4358936	bbp_load_theme_functions( string(0) )	.../class-wp-hook.php:282
14	0.1796	4359104	bbp_locate_template( string(21), bool, ??? )	.../template-loader.php:162
15	0.1796	4359304	bbp_get_template_stack( )	.../template-functions.php:65
16	0.1796	4359760	ksort ( object(WP_Hook)[750] )	.../template-functions.php:316

// Nine occurrences of this one:
( ! ) Notice: Undefined index: function in /srv/www/wp-shared/develop-plugins/bbpress/includes/core/template-functions.php on line 326
Call Stack
#	Time	Memory	Function	Location
1	0.0018	244024	{main}( )	.../index.php:0
2	0.0025	244472	require( '/srv/www/wp-develop/svn/src/wp-blog-header.php' )	.../index.php:17
3	0.0028	245368	require_once( '/srv/www/wp-develop/svn/src/wp-load.php' )	.../wp-blog-header.php:12
4	0.0038	246400	require_once( '/srv/www/wp-develop/svn/wp-config.php' )	.../wp-load.php:42
5	0.0041	249208	require_once( '/srv/www/wp-develop/svn/src/wp-settings.php' )	.../wp-config.php:102
6	0.1794	4354752	do_action( string(17), ??? )	.../wp-settings.php:336
7	0.1794	4355424	WP_Hook->do_action( array(1) )	.../plugin.php:454
8	0.1794	4356320	call_user_func_array:{/srv/www/wp-develop/svn/src/wp-includes/class-wp-hook.php:282} ( string(21), array(1) )	.../class-wp-hook.php:282
9	0.1795	4356688	bbp_after_setup_theme( string(0) )	.../class-wp-hook.php:282
10	0.1795	4356736	do_action( string(21), ??? )	.../sub-actions.php:345
11	0.1795	4357408	WP_Hook->do_action( array(1) )	.../plugin.php:454
12	0.1796	4358560	call_user_func_array:{/srv/www/wp-develop/svn/src/wp-includes/class-wp-hook.php:282} ( string(24), array(1) )	.../class-wp-hook.php:282
13	0.1796	4358936	bbp_load_theme_functions( string(0) )	.../class-wp-hook.php:282
14	0.1796	4359104	bbp_locate_template( string(21), bool, ??? )	.../template-loader.php:162
15	0.1796	4359304	bbp_get_template_stack( )	.../template-functions.php:65

[DrewAPicture]

Note, this would also fix the problem with priorities demonstrated in #27428.

[jbrinley]

Thanks for testing, @ocean90. Those errors are disheartening. There's really no way to be 100% backwards compatible with something that touches so deeply into the internals of $wp_filter. bbPress could be revised to work with both the old and the new $wp_filter with code such as:

function bbp_get_template_stack() {
	global $wp_filter, $merged_filters, $wp_current_filter;

	// Setup some default variables
	$tag  = 'bbp_template_stack';
	$args = $stack = array();

	// Add 'bbp_template_stack' to the current filter array
	$wp_current_filter[] = $tag;

	if ( class_exists( 'WP_Hook' ) ) {
		$filter = $wp_filter[ $tag ]->callbacks;
	} else {
		$filter = &$wp_filter[ $tag ];

		// Sort
		if ( ! isset( $merged_filters[ $tag ] ) ) {
			ksort( $filter );
			$merged_filters[ $tag ] = true;
		}
	}

	// Ensure we're always at the beginning of the filter array
	reset( $filter );

	// Loop through 'bbp_template_stack' filters, and call callback functions
	do {
		foreach ( (array) current( $filter ) as $the_ ) {
			if ( ! is_null( $the_['function'] ) ) {
				$args[1] = $stack;
				$stack[] = call_user_func_array( $the_['function'], array_slice( $args, 1, (int) $the_['accepted_args'] ) );
			}
		}
	} while ( next( $filter ) !== false );

	// Remove 'bbp_template_stack' from the current filter array
	array_pop( $wp_current_filter );

	// Remove empties and duplicates
	$stack = array_unique( array_filter( $stack ) );

	return (array) apply_filters( 'bbp_get_template_stack', $stack ) ;
}

[jbrinley]

In looking at @ocean90's issue, I realized that while IteratorAggregate is 5.2 compatible, the ArrayIterator it was returning is SPL. 17817.8.diff reworks WP_Hook to implement Iterator instead of IteratorAggregate, handling the array traversal itself rather than delegating to ArrayIterator.

[helen]

cc @jjj for the bbPress errors above.

[helen]

Sigh, didn't catch that age-old mistake fast enough. That would be @johnjamesjacoby.

[johnjamesjacoby]

Replying to helen:

Sigh, didn't catch that age-old mistake fast enough. That would be @johnjamesjacoby.

Thanks for the ping. Two thoughts:

• Both BuddyPress and bbPress will need an update to match the new approach in WordPress core
• Happy to take over the @jjj account if someone wants to bless that task for all of our conveniences

[helen]

Replying to johnjamesjacoby:

• Both BuddyPress and bbPress will need an update to match the new approach in WordPress core

More concerned about back-compat here (not taking down sites that update core before the plugin or fail to update the plugin for whatever reason) and your thoughts on the approach, since you're doing deeper things with hooks.

[johnjamesjacoby]

Replying to helen:

Replying to johnjamesjacoby:

• Both BuddyPress and bbPress will need an update to match the new approach in WordPress core

More concerned about back-compat here (not taking down sites that update core before the plugin or fail to update the plugin for whatever reason) and your thoughts on the approach, since you're doing deeper things with hooks.

IIRC, the leads in this ticket predicted a high likelihood of breakage, and deemed it necessary for the sake of progress & performance. See this comment from nacin among others here.

The bbs are a good litmus test for this type of breakage because they hug tightly to internal API's, sometimes in creative ways. Each bb'breakage is a scar on their surface; a point in time where WordPress got better because the bb's absorbed an impact.

Because WooCommerce and others have taken inspiration from this template-stack code, I'm weary of slipping it in quickly without more exposure: a post on Make, a clear description of why this break is necessary, and maybe even suggestions for alternative approaches.

All of that said, changing this code at all scares the daylights out of me. Many subsystems rely on this foundation, and the bb's may have only helped bring attention to one specific implementation.

[johnjamesjacoby]

Also, both BuddyPress & bbPress have considerations for juggling WordPress version nuances. If either project needs to move away from direct touches to the $wp_filter global (which is a known fragility) it's possible to achieve the same functionality with with a variable in their respective singletons, or a number of other ways. All about the greater good.

[boonebgorges]

The lameness of not doing this for 4.4 is extensive and profound, but I guess it's the right choice.

If we are confident that we're going to go with this in 4.5 (or whenever), we should start the education campaign today. One, developers cannot always drop everything and respond to stuff like this in the course of a few weeks or even a few months. Two, and more importantly: the sooner affected plugins can push out a fix, the more of a gap we put between the fix being available and the breaking change going into WordPress. The critical situation to avoid is one where an out-of-date version of BuddyPress (or whatever) is on a site that updates to WP_Hook; putting more time between a fixed BuddyPress and WP_Hook is going to make this less likely to happen.

[nacin]

Some of you may know that I have been playing with this patch for a few weeks with the intention of committing it. I also played with it on my WordPress.org sandbox (a self-imposed prerequisite prior to committing), but I hadn't tested all scenarios and missed that it broke the bb's. Good catch, @ocean90. I didn't notice anything else, which is great.

I want to try something here, which we've done with success before -- putting something in trunk that has a very slim chance of making it to the final release, in order to get a much better idea of what it could break. Thus, I would be comfortable committing this to trunk in the next 7 days under the following conditions:

1. We need some kind of fix or workaround (somewhere -- .org, core, or bb's) to avoid .org breakage. [ACTION NEEDED]
2. We publish ​https://gist.github.com/jbrinley/eaaad00b52e1316c6904 on make/core, with some adjustments...
3. The post should say it'll ship in 4.5, but that it is in trunk now to assess the extent of the damage. We should leave it in trunk for up to 14-21 days. This would imply its removal by beta 2 or beta 3. I will manage the fallout. Note "up to 14-21 days" -- if the breakage is particularly severe, it will have to come out quickly.

The time it spends in core will further inform how much further it has to go, whether it needs a significant shift in strategy, and whether it's possible at all.

It's up to all of you to find success on point 1. But if that happens, I will handle points 2 and 3, and the commit itself.

[netweb]

Replying to nacin:

1. We need some kind of fix or workaround (somewhere -- .org, core, or bb's) to avoid .org breakage. [ACTION NEEDED]

ACTION STARTED: ​https://bbpress.trac.wordpress.org/ticket/2871 WP_Hook

I'm yet to wrap my head around all of this or even read/test the patch fully; anyways, we'll get a start on for bbPress

[boonebgorges]

BuddyPress has been updated so that it no longer manipulates $wp_filter sub-arrays directly. ​https://buddypress.trac.wordpress.org/changeset/10279

[rotozeev]

Support of NextGen Gallery plugin have sad that this bug is a cause of my problem. There is a problem of crossposting to another blogs: shortcodes of NGG doesn't transform to html due to the apply_filters("the_content", ...) and as result I obtain the text like [singlepic id=831] instead picture. So, may be somebody knows how I can easy improve this problem?

[kitchin]

@rotozeev: We can work on it here: ​https://wordpress.org/support/topic/apply_filters-doesnt-work-with-shortcodes

[SergeyBiryukov]

Replying to kitchin:

Also I wonder if the function/method signatures in 17817.5 are so similar that it's confusing:

function add_filter( $tag, $function_to_add, $priority = 10, $accepted_args = 1 )
public function add_filter( $function_to_add, $tag, $priority, $accepted_args )

Good point, 17817.9.diff​ makes the order of arguments for WP_Hook::add_filter(), WP_Hook::remove_filter(), and WP_Hook::has_filter() consistent with the respective functions.

[netweb]

Replying to netweb:

ACTION STARTED: ​https://bbpress.trac.wordpress.org/ticket/2871 WP_Hook

Done, see ​[bbpress5944]

[DrewAPicture]

#36199 was marked as a duplicate.

[dougwollison]

Updated doc block for apply_filters; shouldn't claim $priority and $accepted_args are optional. Also removed random extra semicolon after an if block.

[dougwollison]

Correct version of previous diff; a number of the new files were missing.

[boonebgorges]

#36097 was marked as a duplicate.

[khag7]

Fixes 17817.10.2 - class-wp-hook.php has entire contents duplicated

[khag7]

17817.10.2.diff​ had an issue, 17817.10.3.diff​ fixes it.

It looks like @dougwollison copy and pasted the code for that file so it was in there twice. First 480 lines were identical to next 480 lines.

[dougwollison]

Replying to khag7:

17817.10.2.diff​ had an issue, 17817.10.3.diff​ fixes it.

It looks like @dougwollison copy and pasted the code for that file so it was in there twice. First 480 lines were identical to next 480 lines.

Thanks, legitimately not sure how I managed that.

By the way, would it be worth adding coding-standards cleanup to plugins.php while we're at this? Fixing brace-less if/foreach/while blocks mainly.

[parisholley]

any idea when this is going to make it into a release? about pulled my hair out today trying to figure out why things weren't working and this was the root cause.

[barry.hughes]

This has been a frequent source of problems for us, it would be great to see it move forward into a release!

[Compute]

Something for 4.7 early maybe?

[pento]

Apply cleanly against trunk.

[noplanman]

#37679 was marked as a duplicate.

[pento]

17817.12.diff​ fixes some unit tests, and updates the @since docs.

There was a bug I've been running into, triggered by this code. It's kind of ugly, but not insane.

<?php
/*
Plugin Name: lol
 */
add_filter( 'the_content', 'foo', 999999 );

function foo( $content ) {
        return $content . bar();
}

function bar() {
        remove_filter( 'the_content', 'foo', 999999 );
        $bar = apply_filters( 'the_content', 'bar' );
        add_filter( 'the_content', 'foo', 999999 );
        return $bar;
}

When remove_filter() is called, it eventually gets to WP_Hook::resort_active_iterations(). While the logic in this method was sound, the foreach() was resetting the array pointer for each item in $this->iterations.

In the above example, the result was that, when foo() returned to WP_Hook::apply_filters(), the internal pointer of $this->iterations[ $nesting_level ] had been reset to the first element, instead of staying where it was supposed to.

17817.13.diff​ fixes this bug by operating on a reference to each element in the WP_Hook::resort_active_iterations() loop.

It also bails on the loop early when the current() element is false, there's no need to loop through an array that we're already at the end of.

17817.13.diff​ needs unit tests to confirm that this behaviour is fixed.

[noplanman]

@pento Latest diff isn't working for me. It seems to be the issue of the pointer losing the correct position. (I think the loop on line class-wp-hook.php:293)

Interestingly though, if I remove all references of $nesting_level (or rather, keep it at 0 always), it works perfectly for my initial issue #37679.

[pento]

17817.16.diff​ adds a unit test for the filters being added and removed from the currently running hook.

@noplanman: The new test added (test_remove_and_add in tests/hooks/add_filter.php) seems to cover the remove/add behaviour you described in #37679, but I was unable to reproduce the output you described.

Could I get you to have a look at the test, and say how it differs from your code?

[noplanman]

@pento, I see your test is adding the filters with the same priority. The problem only happens if the priorities are different.

Please check #37679 again to see what I mean.

Unfortunately I can't modify your test, as PHP/PHPUnit dies on me with a Process finished with exit code 139 (interrupted by signal 11: SIGSEGV) when I change the priorities, which is very odd.
I'm trying to find a solution for this, without luck so far.
Maybe you have an idea why PHP is failing like this?

[pento]

Ah, I see what's happening.

• p11 removes itself, and so ::resort_active_iterations() causes the current $this->iterations to skip forward to priority 12.
• p11 re-adds itself, but now the current priority in $this->iterations is 12, so ::resort_active_iterations() doesn't go backwards.
• When ::apply_filters() finishes priority 11, it assumes that the current $this->iterations hasn't changed, so it grabs the next priority (after 12).

17817.17.diff​ alters ::resort_active_iterations(), to detect when a new priority level has been created, that matches the current priority according to ::apply_filters(), and moves the current $this->iterations back to that priority level.

@noplanman: Could you test 17817.17.diff​?

[noplanman]

Hi @pento

Happy to test and get this into core =)

With filters the tests work nicely. But unfortunately my problem persists using your latest code changes.

Using do_action doesn't seem to work. Here my tests for you to play with:

public function test_remove_and_add_last_action_1() {
  $this->hook = new Wp_Hook();

  $this->hook->add_filter( 'remove_and_add', '__return_empty_string', 10, 0 );

  $this->hook->add_filter( 'remove_and_add', array( $this, '_action_remove_and_add1' ), 11, 0 );
  $this->hook->add_filter( 'remove_and_add', array( $this, '_action_remove_and_add2' ), 12, 0 );

  $this->action_output = '';
  $this->hook->do_action( [] );

  // Works! First add1, then add2. ($this->action_output = '12')
  $this->assertSame( '12', $this->action_output );
}

public function test_remove_and_add_last_action_2() {
  $this->hook = new Wp_Hook();

  $this->hook->add_filter( 'remove_and_add', '__return_empty_string', 10, 0 );

  $this->hook->add_filter( 'remove_and_add', array( $this, '_action_remove_and_add1' ), 13, 0 );
  $this->hook->add_filter( 'remove_and_add', array( $this, '_action_remove_and_add2' ), 12, 0 );

  $this->action_output = '';
  $this->hook->do_action( [] );

  // Fails! First add2, but no add1. ($this->action_output = '2')
  $this->assertSame( '21', $this->action_output );
}

public function _action_remove_and_add1() {
  $this->action_output .= '1';
}

public function _action_remove_and_add2() {
  $this->hook->remove_filter( 'remove_and_add', array( $this, '_action_remove_and_add2' ), 12 );
  $this->hook->add_filter( 'remove_and_add', array( $this, '_action_remove_and_add2' ), 12, 0 );

  $this->action_output .= '2';
}

Hope this helps.

[pento]

Oh, good catch @noplanman. I totally forgot to update WP_Hook::do_action() to keep track of the current priority. :-)

17817.18.diff​ fixes this, and adds unit tests.

[noplanman]

Ok, can confirm that it's working now, great job! :-D

If you need any other testing, just let me know.

[pento]

Great news!

I've been doing more testing on W.org and WP.com, everything appears to be working smoothly. As such, the plan is to commit in the coming week. Please don't hold back on trying to break it over the next few days, I'd prefer to fix bugs now, rather than after it rolls out in the nightly. :-)

As @nacin suggested a lifetime ago, the ​draft post that @jbrinley wrote will be published on make/core, with some comments around timing for the final keep/revert decision.

[pento]

The last few patches reminded why it's super annoying that apply_filters() and do_action() are very-similar-but-different bits of code.

17817.20.diff​ merges the two, so ::do_action() now calls ::apply_filters(). I've added a unit test to check that multiple hooks against an action will still get the same value passed.

This was surprisingly easy to do, which was nice, but makes me very suspicious. Please read the code and think about ways to make it explode. :-)

[pento]

17817.21.diff​ expands the test added in 17817.20.diff​ to test across multiple priorities.

[ocean90]

@pento Patches .19 to .21 are missing the WP_Hook class. Can you please add a patch which includes everything? 😉

[pento]

Well, at least someone around here is paying attention.

17817.22.diff​ exists, for all of your WP_Hook needs.

[ocean90]

Thanks @pento.

I run the tests with 17817.22.diff​ on PHP 7.1 RC1 and it seems like it also fixes #37295 and #37772.

But there is one failure when running phpunit -c tests/phpunit/multisite.xml:

1) Test_Nav_Menus::test_wp_get_associated_nav_menu_items
Trying to get property of non-object

/src/wp-includes/ms-blogs.php:1221
/src/wp-includes/class-wp-hook.php:294
/src/wp-includes/class-wp-hook.php:319
/src/wp-includes/plugin.php:453
/src/wp-includes/post.php:2414
/tests/phpunit/tests/post/nav-menu.php:101

I created a PR on @jorbin's GitHub mirror to run the tests on Travis CI: ​https://github.com/aaronjorbin/develop.wordpress/pull/16 / ​https://travis-ci.org/aaronjorbin/develop.wordpress/builds/157840503

[pento]

Noice. ::do_action() didn't like it when an action was called recursively. :-)

17817.23.diff​ fixes this by adding a recursion check to ::do_action(). It also updates test_do_action_doesnt_change_value to test for recursion, too.

​https://travis-ci.org/aaronjorbin/develop.wordpress/builds/157912226

[ocean90]

Replying to pento:

Noice. ::do_action() didn't like it when an action was called recursively. :-)

The Travis CI build was successful for 17817.23.diff​. I'm also running this patch on my w.org sandbox without any issues so far.

The lost five methods of WP_Hook could need some docs love, missing periods and incomplete @return tags.

[aaroncampbell]

17817.24.diff​ adds the @return for the methods that @ocean90 pointed out were missing it. It also removes the return from the rewind() method since ​it's ignored anyway. All tests still appear to pass, and I pounded on the iterator a decent bit and the rewinding seems to still work as expected.

[pento]

In 38571:

Hooks: Add the new class WP_Hook, and modify hook handling to make use of it.

Filters and actions have been the basis of WordPress' plugin functionality since time immemorial, they've always been a reliable method for acting upon the current state of WordPress, and will continue to be so.

Over the years, however, edge cases have cropped up. Particularly when it comes to recursively executing hooks, or a hook adding and removing itself, the existing implementation struggled to keep up with more complex use cases.

And so, we introduce WP_Hook. By changing $wp_filter from an array of arrays, to an array of objects, we reduce the complexity of the hook handling code, as the processing code (see ::apply_filters()) only needs to be aware of itself, rather than the state of all hooks. At the same time, we're able te handle more complex use cases, as the object can more easily keep track of its own state than an array ever could.

Props jbrinley for the original architecture and design of this patch.
Props SergeyBiryukov, cheeserolls, Denis-de-Bernardy, leewillis77, wonderboymusic, nacin, jorbin, DrewAPicture, ocean90, dougwollison, khag7, pento, noplanman and aaroncampbell for their testing, suggestions, contributions, patch maintenance, cajoling and patience as we got through this.
Fixes #17817.

[DrewAPicture]

In 38573:

Docs: Fix minor formatting for inline docs in WP_Hook following its introduction in [38571].

See #17817.

[jacobsantos]

Oh. Oh dear god. Oh you poor poor people. I am so, so very sorry.

I guess it will be okay. It should be okay. As long as everything works. Looking at the code that was committed...

I'm sorry. The design decisions that were made years and years ago were mostly compromises for PHP4. Things had to work on PHP4 you see, PHP4 had priority.

Also, the decision was that we didn't care about plugins affected by this bug, because at the time it was an edge case. I feel that it still is by the way. Looks like I'm going to maintain a separate build of WordPress that reverts this mess of a commit.

I don't care. You will either regret this commit and revert it or it won't matter and my fears will be unfounded. Either way, I think it would be a good time for a fork. Or not. It should be fine. I mean, it isn't like this patch resembles a patch that was submitted years ago back when we had a similar problem and rejected that shit faster than the fastest thing you can think that can go fast.

It is funny to me. How history repeats itself. I hope evil didn't prevail this time. Who knows? I forget why a similar solution was rejected so long ago. Performance reasons? Must have been performance reasons and PHP4. WordPress sucked back when PHP4 had to be supported. Imagine being stuck supporting a older version of PHP. What a nightmare.

I still think it is funny that this whole thing could be solved with one line and fuck it, let's just add class! I mean, you can't really solve this problem in a good way. Well, you can, you can do what you did in the commit.

You know. I'm sure it will be fine. It'll be fine. PHP5 and PHP7 are way faster than they were back in 2007. Servers are way faster than they were in 2007.

Well. Good luck. I'll make sure the door doesn't hit my ass on the way out again.

[noplanman]

@jacobsantos

I still think it is funny that this whole thing could be solved with one line

For learning sake, what fix are you referring to?
One line really isn't a big change and I'd really like to understand how it would fix this issue properly.

[dougwollison]

Replying to noplanman:

@jacobsantos

I still think it is funny that this whole thing could be solved with one line

For learning sake, what fix are you referring to?
One line really isn't a big change and I'd really like to understand how it would fix this issue properly.

If I had to guess, something like what me and others have proposed over the years in our duplicate tickets. Namely, copying the hooks list for a tag to a new variable and iterating over that. But it's been pointed out there are apparently performance issues with that solution.

[noplanman]

Ok, I see what you mean.

I'm just curious to know if recursive adding/removing of hooks within a hook would work properly then, as the array would be changing during the loop.

At the moment I'm just happy that @pento has put in the effort to get this fixed.

[jacobsantos]

Replying to noplanman:

Ok, I see what you mean.

I'm just curious to know if recursive adding/removing of hooks within a hook would work properly then, as the array would be changing during the loop.

At the moment I'm just happy that @pento has put in the effort to get this fixed.

Yeah, that was it exactly. The problem is what do you want to happen? If you add a callback to a hook that is already running, then what do you want to happen?

1. The callback is called at the next time the hook is called.
2. The callback is called later within the same hook execution. So if you add a callback to hook 'something' running with priority 10, while running at priority 10, that same callback is called at some point soon after.

The solution can't have both. Or realistically, it could have both, but solutions for both are different and counter each other. I am sure this was mentioned at some point above. With the patch, great, I am guessing the patch is the option 2.

The problem is what do people mean when they add to the same hook they are running on? So if the programmer starts adding callbacks to the 'wp_init' hook, do they mean they want it to run soon after? I understand there is a chicken and the egg problem. That is why there is an early process startup execution and late process startup execution hook. So you can manage what goes into the latter in the former.

The point is more so that if you want to remove a callback from a hook, you may do so. What logical reason does a person have for adding more hooks to 'wp_init'? Do you mean you want to execute the callbacks? Why not just execute the callbacks?

---

For me, the problem is not that the problem should or should not have been fixed. Incorrect behavior is incorrect behavior and should be fixed. The problem is that the solution required is hideous. Not the code provided and the code required to accurately and completely fix the problem. The problem is fundamental with how PHP handles arrays and references and pointers. Requiring code that breaks this behavior in some way to provide consistency.

This is code that when it breaks, it will be damn near impossible to fix. It will be damn near impossible to optimize. The original patch like the one committed was beautiful in that only one person knew how it worked. Only one or a few people would ever know how it worked. You do not want code like that in any project. You may think you understand how this code works, but you don't. Or at least, if it was like the original patch. The code that was committed appears to be a bit more straightforward, so I am not sure if that just means I know more or that the author is a better code artist.

I only got to the point where what appears to be an incredibly expensive method is called, each time a callback is added to a hook. Sure, optimization is sometimes counter-intuitive, but when has sorting ever been a fast process? For each addition? Insanity. I would buy a hat and eat it, if sorting each time didn't have a huge performance hit.

It is said that copying the array would have performance issues. How does what was committed not have huge performance issues?

The effort is awesome, sure, but you just used C4 to clean a window. I guess technically, the window is clean, albeit scattered to the wind.

[noplanman]

Incorrect behavior is incorrect behavior and should be fixed.

Absolutely, though utopian. Especially in a large project, it's extremely difficult to "force" developers/users to use it properly. Because what does "properly" even mean. This is where flexibility comes in and allowing developers to express and create amazing things. Of course there are boundaries, but isn't software here to try to break those?

I totally agree with you, that in the scenario that you described, there are different way of interpreting what the developer wanted to achieve. That's a tough boundary to crack, as it's something a machine can't (maybe yet) know. Edge cases like that are clearly an exception, but should still have their place, motivating more developers to come up with unique and gorgeous solutions.
The same will be the case with this fix. Some time down the line, there will probably be new changes, optimisations, maybe a whole rewrite, who knows!

But hey, we're all here to learn together, right? Striving to become better and write smarter code :-)

I think we can put this discussion on pause here, as it isn't really contributing to this current issue any more, but thanks a lot for elaborating on your point!

[flixos90]

A small improvement idea: I think it would make sense to separate the WP_Hook::has_filter() and WP_Hook::has_filters() methods completely so that both have a single responsibility: The $function_to_check parameter would become required for WP_Hook::has_filter() and we would no longer call WP_Hook::has_filters() from WP_Hook::has_filter(). The logic to be backwards compatible could then happen in the regular has_filter() function - this way at least the new class methods would only have one specific purpose.

Another thing I observed: This might have a reason I'm overlooking right now, but why aren't we storing the tag (hook name) in the class as property (and pass it to the constructor)? This would allow us to get rid of passing the $tag parameter on all the methods. Sorry to come up with this now, I haven't really paid attention to this movement before :(

[danielbachhuber]

FYI - Because WP-CLI directly modifies the $wp_filter global, this WP_Hook thing breaks the implementation we adopted in #34936. See ​https://travis-ci.org/wp-cli/wp-cli/jobs/158420911

If r38282 stays in, I can get around this by requiring plugins.php early in WP-CLI (#37707) and using add_action() and add_filter().

[jorbin]

I don't see any reason that r38282 won't be staying in.

[nerrad]

So we were getting failing unit tests in our builds after this was deployed and so far it looks like it traces to our usage of the tests_add_filter() function found in /tests/phpunit/functions.php. That function overrides $wp_filter and doesn't implement the new changes.

[jorbin]

Replying to nerrad:

So we were getting failing unit tests in our builds after this was deployed and so far it looks like it traces to our usage of the tests_add_filter() function found in /tests/phpunit/functions.php. That function overrides $wp_filter and doesn't implement the new changes.

Can you provide a sample test that demonstrates this failure please.

[nerrad]

Here's the code I see for tests_add_filter (to be clear, this is pulled directly from the WordPress Core UnitTest files):

// For adding hooks before loading WP
function tests_add_filter($tag, $function_to_add, $priority = 10, $accepted_args = 1) {
	global $wp_filter;

	$idx = _test_filter_build_unique_id($tag, $function_to_add, $priority);
	$wp_filter[$tag][$priority][$idx] = array('function' => $function_to_add, 'accepted_args' => $accepted_args);
	return true;
}

All we have in our tests is implementation of that filter in our plugin unit tests bootstrap. So basically we do something like:

tests_add_filter( 'some_filter_we_want_to_hook_into', '__return_false' )

[danielbachhuber]

Replying to danielbachhuber:

FYI - Because WP-CLI directly modifies the $wp_filter global, this WP_Hook thing breaks the implementation we adopted in #34936.

As it turns out, WP-CLI broke because of poor coding on my part. My bad. See ​https://github.com/wp-cli/wp-cli/pull/3384 for the fix.

However, I discovered this breaks the intent of #37707 because it depends on WPINC, which is only available after wp-settings.php has begun loading.

[boonebgorges]

Replying to nerrad:

So we were getting failing unit tests in our builds after this was deployed and so far it looks like it traces to our usage of the tests_add_filter() function found in /tests/phpunit/functions.php. That function overrides $wp_filter and doesn't implement the new changes.

I can confirm that there's an issue here. tests_add_filter() still works OK before WP has loaded - WP_Hook::build_preinitialized_hooks() takes care of the manually-set $wp_filter keys. But if you run tests_add_filter() after these pre-bootstrap filters are normalized, they never get converted to WP_Hook objects. 17817.25.diff​ is a simple fix, though the class_exists() check might break once WP_Hook is autoloaded; we may want a global flag or a constant or somesuch to keep track of whether preexisting hooks have been converted.

[jorbin]

Replying to danielbachhuber:

Replying to danielbachhuber:

FYI - Because WP-CLI directly modifies the $wp_filter global, this WP_Hook thing breaks the implementation we adopted in #34936.

As it turns out, WP-CLI broke because of poor coding on my part. My bad. See ​https://github.com/wp-cli/wp-cli/pull/3384 for the fix.

However, I discovered this breaks the intent of #37707 because it depends on WPINC, which is only available after wp-settings.php has begun loading.

The above patch fixes this issue, though the more I think about it, the more I wonder if WP_Hook should even be in a separate file. Is there ever a time you would want one and not the other?

[dd32]

Replying to boonebgorges:

I can confirm that there's an issue here. tests_add_filter() still works OK before WP has loaded - WP_Hook::build_preinitialized_hooks() takes care of the manually-set $wp_filter keys. But if you run tests_add_filter() after these pre-bootstrap filters are normalized, they never get converted to WP_Hook objects. 17817.25.diff​ is a simple fix, though the class_exists() check might break once WP_Hook is autoloaded; we may want a global flag or a constant or somesuch to keep track of whether preexisting hooks have been converted.

Once @jorbin's 17817.26.diff​ is in, can't 17817.25.diff​ just be a check on function_exists( 'add_filter' ) and use the API when available, falling back to directly modifying when it's called before initialisation?

[pento]

Replying to jorbin:

The above patch fixes this issue, though the more I think about it, the more I wonder if WP_Hook should even be in a separate file. Is there ever a time you would want one and not the other?

17817.26.diff​ will break things in weird ways - if something loads plugin.php without loading class-wp-hook.php, it still won't be able to use add_filter(), etc. And once plugin.php is loaded, it can't be relied on to process $wp_filter as it does at the top of the file.

Discussion of the correct fix for that can continue in #37707.

[pento]

Replying to dd32:

Once @jorbin's 17817.26.diff​ is in, can't 17817.25.diff​ just be a check on function_exists( 'add_filter' ) and use the API when available, falling back to directly modifying when it's called before initialisation?

It doesn't need to wait for 17817.26.diff​. In the test suite, phpunit/includes/functions.php is loaded, it plays some funky music, then wp-settings.php is loaded - WP_Hook will be loaded the same before or after whatever the fix for #37707 is.

[pento]

In 38582:

Tests: Use add_filter() when it's available.

The tests_add_filter() helper function directly manipulates the $wp_filter global, instead of using add_filter(). We can use add_filter() when it's available, and fall back to manipulating $wp_filter when it isn't, relying on the $wp_filter bootstrap code at the top of plugin.php to handle conversion.

Props boonebgorges, dd32 and pento: WordPress Thought Leadership Triumvirate.
Fixes #17817.

[kraftbj]

This change introduced #38011 which changes the behavior when a function called by a hook adds another function onto the same hook with an earlier priority. The new behavior is logical, but different than previously.

[dd32]

Upgrade filters added manually in advanced-cache.php