WordPress.org

Make WordPress Core

Opened 22 months ago

Last modified 6 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 (10)

next-to-each-unit.patch (1.1 KB) - added by devesine 22 months ago.
next-to-each.patch (2.4 KB) - added by devesine 22 months ago.
next-to-each-php.php (1.0 KB) - added by devesine 22 months ago.
each-perf.php (287 bytes) - added by devesine 22 months ago.
next-perf.php (293 bytes) - added by devesine 22 months ago.
next-to-each-r2.patch (2.4 KB) - added by devesine 22 months ago.
next-to-each-r3.patch (2.2 KB) - added by devesine 21 months ago.
next-to-each-unit-r2.patch (1.1 KB) - added by devesine 21 months ago.
next-to-each-22075.patch (2.4 KB) - added by devesine 19 months ago.
next-to-each-unit-1057.patch (1.1 KB) - added by devesine 19 months ago.

Download all attachments as: .zip

Change History (29)

devesine22 months ago

comment:1 devesine22 months ago

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

devesine22 months ago

comment:2 devesine22 months ago

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

comment:3 SergeyBiryukov22 months ago

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

comment:4 scribu22 months ago

  • Keywords needs-testing added

Some performance tests?

comment:5 scribu22 months ago

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

devesine22 months ago

devesine22 months ago

comment:6 devesine22 months 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

Version 0, edited 22 months ago by devesine (next)

devesine22 months ago

comment:7 devesine22 months ago

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

comment:8 scribu22 months ago

  • Milestone changed from Awaiting Review to 3.5

Awesome.

comment:9 scribu22 months ago

Similar: #21172

comment:10 nacin21 months 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 nacin21 months ago

In [21287]:

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

devesine21 months ago

comment:12 follow-up: devesine21 months 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: devesine19 months ago

comment:14 in reply to: ↑ 13 westi18 months 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: nacin18 months 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 devesine18 months 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 devesine9 months ago

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

comment:19 jbrinley6 months ago

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