[wp-trac] [WordPress Trac] #17817: do_action/apply_filters/etc. recursion on same filter kills underlying call

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.

 > Performance:
 >
 >  * `$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(
 $this->callbacks )`.

 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
 improvement.

 >  * 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 [17638].

 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?

 > Other:
 >
 >  * 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
 it is.

 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
 to `plugin.php`.

 >  * 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
 "benefits" section.

 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