Make WordPress Core

Opened 10 years ago

Closed 6 years 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:

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 10 years ago.
next-to-each.patch (2.4 KB) - added by devesine 10 years ago.
next-to-each-php.php (1.0 KB) - added by devesine 10 years ago.
each-perf.php (287 bytes) - added by devesine 10 years ago.
next-perf.php (293 bytes) - added by devesine 10 years ago.
next-to-each-r2.patch (2.4 KB) - added by devesine 10 years ago.
next-to-each-r3.patch (2.2 KB) - added by devesine 10 years ago.
next-to-each-unit-r2.patch (1.1 KB) - added by devesine 10 years ago.
next-to-each-22075.patch (2.4 KB) - added by devesine 9 years ago.
next-to-each-unit-1057.patch (1.1 KB) - added by devesine 9 years ago.
21169-unittests.diff (1.1 KB) - added by MikeHansenMe 7 years ago.
see #30284

Download all attachments as: .zip

Change History (31)

#1 @devesine
10 years ago

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

#2 @devesine
10 years ago

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

#3 @SergeyBiryukov
10 years ago

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

#4 @scribu
10 years ago

  • Keywords needs-testing added

Some performance tests?

#5 @scribu
10 years ago

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

@devesine
10 years ago

@devesine
10 years ago

#6 @devesine
10 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 10 years ago by SergeyBiryukov (previous) (diff)

#7 @devesine
10 years ago

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

#8 @scribu
10 years ago

  • Milestone changed from Awaiting Review to 3.5

Awesome.

#9 @scribu
10 years ago

Similar: #21172

#10 @nacin
10 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
10 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
10 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
9 years ago

#14 in reply to: ↑ 13 @westi
9 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
9 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
9 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
8 years ago

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

#19 @jbrinley
8 years ago

  • Cc jonathanbrinley@… added

#20 @chriscct7
6 years 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.