Plugins: Don't skip the next priority's callbacks when a callback empties its bucket mid-iteration#11715
Conversation
…ties its bucket mid-iteration. When a callback running inside apply_filters() is the only one at its priority and removes itself (or another callback empties that bucket), WP_Hook::resort_active_iterations() walks the iterator forward to the first remaining priority greater than $current. The trailing next() in apply_filters() then overshoots that priority and silently skips its callbacks. The bug only manifests when at least one earlier-priority callback has already run; otherwise the existing array_unshift fast-path handles it. This adds the symmetric counterpart to the existing add-side handling: if $current's bucket was emptied during iteration, step the iterator back one so that apply_filters()'s trailing next() lands on the right priority instead of skipping past it. Fixes #65167.
|
Hi @obenland! 👋 Thank you for your contribution to WordPress! 💖 It looks like this is your first pull request to No one monitors this repository for new pull requests. Pull requests must be attached to a Trac ticket to be considered for inclusion in WordPress Core. To attach a pull request to a Trac ticket, please include the ticket's full URL in your pull request description. Pull requests are never merged on GitHub. The WordPress codebase continues to be managed through the SVN repository that this GitHub repository mirrors. Please feel free to open pull requests to work on any contribution you are making. More information about how GitHub pull requests can be used to contribute to WordPress can be found in the Core Handbook. Please include automated tests. Including tests in your pull request is one way to help your patch be considered faster. To learn about WordPress' test suites, visit the Automated Testing page in the handbook. If you have not had a chance, please review the Contribute with Code page in the WordPress Core Handbook. The Developer Hub also documents the various coding standards that are followed:
Thank you, |
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Core Committers: Use this line as a base for the props when committing in SVN: To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
Test using WordPress PlaygroundThe changes in this pull request can previewed and tested using a WordPress Playground instance. WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser. Some things to be aware of
For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation. |
There was a problem hiding this comment.
Pull request overview
This PR fixes an edge case in WP_Hook iteration where removing the last callback at the currently-executing priority can cause the next remaining priority’s callbacks to be skipped during apply_filters()/do_action() execution.
Changes:
- Adjusts
WP_Hook::resort_active_iterations()to compensate when the current priority bucket is emptied mid-iteration, preventingapply_filters()’s trailingnext()from overshooting the next remaining priority. - Adds a PHPUnit regression test to verify that callbacks at the next remaining priority still run after a self-removing callback empties its own priority bucket.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/wp-includes/class-wp-hook.php |
Updates active-iteration pointer handling when the current priority bucket is removed mid-iteration. |
tests/phpunit/tests/hooks/removeFilter.php |
Adds a regression test for the “emptied bucket skips next priority” iteration bug. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…lf-removing closure.
Inside a closure, __FUNCTION__ resolves to the closure's synthetic name (e.g. {closure:Tests_Hooks_RemoveFilter::...}), not the test method name used at the call site. As a result the closure's remove_filter() targeted a hook that was never registered, the priority bucket never emptied, and the regression test passed for the wrong reason.
Capture the hook name in a local variable in the test method and use it consistently for add_filter() and remove_filter(). Verified that the test now fails on trunk without the WP_Hook fix and passes with it.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
When a callback running inside
apply_filters()is the only one at its priority and removes itself (or another callback empties that bucket),WP_Hooksilently skips the callbacks at the next remaining priority — but only when at least one earlier-priority callback has already run.WP_Hook::resort_active_iterations()already has symmetric handling for the add path (a callback added at the current priority during iteration). The remove path was missing the equivalent fix; the trailingnext()inapply_filters()overshoots the first remaining priority greater than$currentand skips its callbacks.This patch adds the symmetric counterpart: when
$current's bucket has been emptied during iteration, step the iterator back one so thatapply_filters()'s trailingnext()lands on the right priority.A regression test is added to
tests/phpunit/tests/hooks/removeFilter.php. It fails on trunk without the patch and passes with it.Trac ticket: https://core.trac.wordpress.org/ticket/64653
Use of AI Tools
AI assistance: Yes
Tool(s): Claude Code
Model(s): Claude Opus 4.7
Used for: Investigation of the bug, drafting the patch and the regression test, and writing this PR description. The diagnosis, patch shape, and tests were reviewed and edited by me before submission.
This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request.