[wp-trac] [WordPress Trac] #17817: do_action/apply_filters/etc. recursion on same filter kills underlying call
noreply at wordpress.org
Thu Apr 9 17:56:01 UTC 2015
#17817: do_action/apply_filters/etc. recursion on same filter kills underlying call
Reporter: kernfel | Owner:
Type: defect (bug) | Status: reopened
Priority: normal | Milestone: Future
Component: Plugins | Release
Severity: normal | Version: 2.2
Keywords: dev-feedback has-unit-tests has- | Resolution:
patch needs-docs 4.3-early | Focuses:
Comment (by jbrinley):
Replying to [comment:117 nacin]:
> Minor style:
All should be fixed in 17817.13.patch.
> * `$this->callbacks[ $priority ][ $idx ]` should store a numeric array.
If appropriate, use constants such as self::FUNCTION and
self::ACCEPTED_ARGS for 0 and 1 indexes. This will reduce memory usage. I
have wanted to do this in core, but we couldn't for back compat (direct
access of the array, of course). Can we do it now? Would this be
manipulation in `getIterator()` (or our own iterator)? Lightweight object
with ArrayAccess? Would any of these be worth it?
This would probably cause some minor additional back-compat issues beyond
what we've already identified. I'm not sure of the memory difference, but
can run some tests.
> * Does the ksort() need to occur on `add_filter()`? This appears
wasteful. I recognize this is a very key part of the nesting aspects of
this change, but it feels heavy.
As you say, it's key to supporting recursion. The sort runs when callbacks
are added at a new priority to a hook that already has callbacks at other
priorities. So it will generally run maybe a few dozen times (55 on the
homepage of a fresh install, maybe a few dozen more depending on your
plugins). The alternative is to go back to the `$merged_filters` approach
(albeit encapsulated within each hook), and check if it's sorted every
time `apply_filters` / `do_action` runs, which can come out to several
hundreds of checks, and then also doing the sorting on many of those
occasions (about 40 times instead of 55, on the page load above). I don't
yet have much data to say if one approach is heavier than the other. My
intuition is that the difference is inconsequential.
> * `$callbacks` appears to unnecessarily be used by reference when
`$this->callbacks` is iterated over in a `foreach` in a few places.
Fixed in 17817.13.patch.
> * I don't see why `has_filters()` isn't just `return ! empty(
For backwards compatibility, if a plugin takes advantage of `ArrayAccess`
to remove a callback but doesn't remove the priority index. Looping
through the priorities ensures that we don't return a false positive.
Assuming `$this->callbacks` is empty as it should be, the `foreach` won't
have anything to iterate over and `false` will be returned. Wrapping the
`foreach` in `if ( $this->callbacks )` didn't show any performance
> * Should we try to avoid `array_slice()` in `apply_filters()` the same
way we do in `do_action()`?
Fixed in 17817.13.patch.
> * The `apply_filters()` function now calls `array_shift()` in an
unconditional fashion, even if there are no callbacks to apply. I
recognize why it is done, but it feels like it is bringing back the same
kind of thing we optimized in .
Fixed in 17817.13.patch. `array_shift()` will only be called if there's a
`WP_Hook` for the hook.
> * `_wp_call_all_hook()` appears like it is just an unnecessary function
call at this point and should probably stop being used.
Are you suggesting getting rid of the 'all' hook, or just inlining
`$wp_filter['all']->do_all_hook( $args );`? I'll assume the latter. Are
there any back-compat concerns for getting rid of the function?
> * What would be the use case for nesting with the `all` hook?
I don't particularly see a use case. But if any callback on the `all` hook
triggers another hook, `all` _will_ be called recursively, so it seems we
should make it predictable.
> * It's possible that someone could be overwriting `$wp_filter[ $tag ]`
with their own array. This would trigger a fatal error when a method is
called. I don't know what to do about this, and I don't know how serious
Comment #99 above addresses everything that I found (not to say that there
aren't any plugins I didn't find). Every case I found where someone was
directly setting `$wp_filter[ $tag ]`, it was to restore what was
previously there. So a `WP_Hook` would be correctly restored. If someone
does try to directly set it as an array, then yes, it would trigger a
fatal, and there's nothing I can think of that we can do to stop them.
> * It feels weird that _wp_filter_build_unique_id() was moved from the
`add_filter()` function to the `add_filter()` method, but it remains in
`has_filter()`, `remove_filter()`, etc. It appears that some functionality
was deliberately left over in the functions -- like `has_filter()` when
called without a function to check. Is this by design? (Is there a use
case for WP_Hook used outside the context of these internal functions?)
Fixed in 17817.13.patch. I've moved all calls to
`_wp_filter_build_unique_id()` inside of `WP_Hook`. Also changed the order
of args for some functions to put `$tag` at the end (in anticipation of
deprecating the argument in 40 years when we drop 5.2 support).
> * `class-wp-hook.php` should probably be included from `plugin.php` for
compatibility, the way `functions.php` includes `option.php`.
Heh. I had it there, and then Scott moved it to wp-settings.php. Now back
> * This feels like it has a lot of good test coverage for what has been
added, but I think we need a lot more test coverage for the existing
functions and the new methods. That there's no `new WP_Hook` in the test
suite seems to be a smell. We should also split up individual (sets of)
assertions into separate tests.
I'll work on this shortly.
> * We need a blog post written for make/core that explains to plugin
authors exactly how they will ''benefit'' from this, and how code might
break because of this.
I'll work on expanding
https://gist.github.com/jbrinley/eaaad00b52e1316c6904 to include a
Thanks for the review, @nacin. I'm looking forward to seeing this in core.
Ticket URL: <https://core.trac.wordpress.org/ticket/17817#comment:119>
WordPress Trac <https://core.trac.wordpress.org/>
WordPress publishing platform
More information about the wp-trac