Make WordPress Core

Opened 3 years ago

Closed 2 months ago

#21169 closed defect (bug) (duplicate)

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

Reported by: devesine Owned by:
Milestone: Priority: normal
Severity: major Version:
Component: Plugins Keywords: has-patch needs-testing
Focuses: Cc:



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.


See the attached unit test.


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.

Attachments (11)

next-to-each-unit.patch (1.1 KB) - added by devesine 3 years ago.
next-to-each.patch (2.4 KB) - added by devesine 3 years ago.
next-to-each-php.php (1.0 KB) - added by devesine 3 years ago.
each-perf.php (287 bytes) - added by devesine 3 years ago.
next-perf.php (293 bytes) - added by devesine 3 years ago.
next-to-each-r2.patch (2.4 KB) - added by devesine 3 years ago.
next-to-each-r3.patch (2.2 KB) - added by devesine 3 years ago.
next-to-each-unit-r2.patch (1.1 KB) - added by devesine 3 years ago.
next-to-each-22075.patch (2.4 KB) - added by devesine 3 years ago.
next-to-each-unit-1057.patch (1.1 KB) - added by devesine 3 years ago.
21169-unittests.diff (1.1 KB) - added by MikeHansenMe 13 months ago.
see #30284

Download all attachments as: .zip

Change History (31)

#1 @devesine
3 years ago

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

#2 @devesine
3 years ago

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

#3 @SergeyBiryukov
3 years ago

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

#4 @scribu
3 years ago

  • Keywords needs-testing added

Some performance tests?

#5 @scribu
3 years ago

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

3 years ago

3 years ago

#6 @devesine
3 years ago

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():


Runtimes for each():

Last edited 3 years ago by SergeyBiryukov (previous) (diff)

#7 @devesine
3 years ago

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

#8 @scribu
3 years ago

  • Milestone changed from Awaiting Review to 3.5


#9 @scribu
3 years ago

Similar: #21172

#10 @nacin
3 years ago

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.

#11 @nacin
3 years ago

In [21287]:

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

#12 follow-up: @devesine
3 years ago

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

#13 in reply to: ↑ 12 ; follow-up: @devesine
3 years ago

#14 in reply to: ↑ 13 @westi
3 years ago

Replying to devesine:

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

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

#15 follow-up: @nacin
3 years ago

  • Keywords 3.6-early added
  • Milestone changed from 3.5 to Future Release

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.

#16 in reply to: ↑ 15 @devesine
3 years ago

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.

#17 @devesine
2 years ago

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

#19 @jbrinley
2 years ago

  • Cc jonathanbrinley@… added

#20 @chriscct7
2 months ago

  • Keywords 3.6-early removed
  • Milestone Future Release deleted
  • Resolution set to duplicate
  • Status changed from new to closed

Duplicate of #17817.

Note: See TracTickets for help on using tickets.