WordPress.org

Make WordPress Core

Opened 3 years ago

Last modified 19 months ago

#21169 new defect (bug)

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

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

Description

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.

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 7 months ago.
see #30284

Download all attachments as: .zip

Change History (30)

@devesine3 years ago

comment:1 @devesine3 years ago

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

@devesine3 years ago

comment:2 @devesine3 years ago

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

comment:3 @SergeyBiryukov3 years ago

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

comment:4 @scribu3 years ago

  • Keywords needs-testing added

Some performance tests?

comment:5 @scribu3 years ago

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

@devesine3 years ago

@devesine3 years ago

comment:6 @devesine3 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():

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
Last edited 3 years ago by SergeyBiryukov (previous) (diff)

@devesine3 years ago

comment:7 @devesine3 years ago

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

comment:8 @scribu3 years ago

  • Milestone changed from Awaiting Review to 3.5

Awesome.

comment:9 @scribu3 years ago

Similar: #21172

comment:10 @nacin3 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.

comment:11 @nacin3 years ago

In [21287]:

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

@devesine3 years ago

comment:12 follow-up: @devesine3 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].

comment:13 in reply to: ↑ 12 ; follow-up: @devesine3 years ago

comment:14 in reply to: ↑ 13 @westi3 years ago

Replying to devesine:

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

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

comment:15 follow-up: @nacin3 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.

comment:16 in reply to: ↑ 15 @devesine3 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.

comment:17 @devesine22 months ago

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

comment:19 @jbrinley19 months ago

  • Cc jonathanbrinley@… added
Note: See TracTickets for help on using tickets.