remove_action on a tag within do_action for that tag can cause actions to be missed

[devesine]

Problem:

If an action (or filter) in a given priority for a given tag removes itself as part of its execution, and it was the only action/filter at that priority for that tag (or only one remaining), the next priority level is skipped.

Reproduction:

See the attached unit test.

Analysis:

This appears to possibly be a bug (or at least an undocumented feature) of PHP (tested up to 5.4.3), where next() falls over if the current array element is unset (but each() has no problem even if the /next/ array element is unset). Example non-WP code demonstrating this is included.

Proposed solution / workaround:

Switch all uses of next() in plugin.php to use each() instead; I've included a proposed patch.

Props to smathiascrowdfavorite for bringing this bug to my attention.

[devesine]

My mistake, props to ssmathias for bringing this bug to my attention.

[devesine]

Also disregard next-to-each.2.patch, it is an accidental duplicate.

[SergeyBiryukov]

Closed #17817 as a duplicate. Also related: #9968.

[scribu]

Some performance tests?

[scribu]

If it weren't for the performance consideration, we would just use a foreach.

[devesine]

A quick performance test (based on phpbench.com's iteration test code, modified to behave like the current and proposed functionality; attached) suggests that each() may in fact be a bit faster than next().

Runtimes for next():

0.00029993057250977
0.00030994415283203
0.00030922889709473
0.00030899047851562
0.00034213066101074
0.00031399726867676
0.00032401084899902
0.00030303001403809
0.00029993057250977
0.00030112266540527

Runtimes for each():

0.00015902519226074
0.0001521110534668
0.00014805793762207
0.00015783309936523
0.00016498565673828
0.00014805793762207
0.00014400482177734
0.00016307830810547
0.00014400482177734
0.00016307830810547

[devesine]

Updated the patch to properly check for each() === false.

[scribu]

Awesome.

[scribu]

Similar: #21172

[nacin]

Some thoughts:

• The is_null() checks can also go. See #21321.
• Not sure if there is a need to cast $next['value'] to an array. If we found an element in $wp_filter[ $tag ] (as in, !== false), it's going to be an array of functions tied to a priority, and not anything else.

Both changes should shave a bit of time off of this.

[nacin]

In [21287]:

Remove unnecessary is_null() checks from the worker loops inside do_action(), apply_filters(), etc. fixes #21321. see #21169.

[devesine]

next-to-each-r3.patch​ removes the (array) cast and refreshed to apply against r21293.

next-to-each-unit-r2.patch​ refreshed to apply against ​[UT930].

[devesine]

next-to-each-22075.patch​ refreshed against r22075.

next-to-each-unit-1057.patch​ refreshed against ​[UT1057].

[westi]

Replying to devesine:

next-to-each-unit-1057.patch​ refreshed against ​[UT1057].

Committed in ​[UT1096] sorry I forget to give props :(

[nacin]

This looks awesome but I am nervous about something like this for 3.5. [21287], for example, blew up in my face months ago, which is why I never came back to this. Let's try this for early 3.6.

[devesine]

Replying to nacin:

Would it be helpful to your peace of mind (regarding this ticket specifically) to increase the unit test coverage of action priority execution flow? For example, #21172 demonstrates another priority-handling edge case.

[devesine]

Both the patch and the unit test patch still apply cleanly to current trunk (r25012).

[jbrinley]

I submitted a patch over at #17817 that should also fix this issue.

Description: ​http://core.trac.wordpress.org/ticket/17817#comment:29
Patch: ​http://core.trac.wordpress.org/attachment/ticket/17817/17817.2.patch

[MikeHansenMe]

see #30284

[chriscct7]

Duplicate of #17817.