[wp-trac] [WordPress Trac] #17817: do_action/apply_filters/etc. recursion on same filter kills underlying call
noreply at wordpress.org
Tue Mar 10 04:46:29 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: 4.2
Component: Plugins | Version: 2.2
Severity: normal | Resolution:
Keywords: dev-feedback has-unit-tests has- | Focuses:
patch 4.2-early needs-docs |
Comment (by nacin):
A day later than promised, but here is my review.
* Please go over uses of `empty()` and `! empty()` Only use it in
conditionals when trying to avoid a notice. Otherwise use `if ( $a )` or
`if ( ! $a )`.
* false and true, not FALSE or TRUE.
* `$the_[ 'function' ]` should be `$the_['function']`, as `'function'` is
a literal. Same for `$the_['accepted_args']`.
* There is a space before `!`.
* `$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?
* 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. (I wish PHP were more amenable to linked
list manipulation, as in other languages we could do this without
resorting to a sort. It's possible )
* `$callbacks` appears to unnecessarily be used by reference when
`$this->callbacks` is iterated over in a `foreach` in a few places.
* I don't see why `has_filters()` isn't just `return ! empty(
* Should we try to avoid `array_slice()` in `apply_filters()` the same
way we do in `do_action()`?
* 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 .
* `_wp_call_all_hook()` appears like it is just an unnecessary function
call at this point and should probably stop being used.
* What would be the use case for nesting with the `all` hook?
* 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 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?)
* `class-wp-hook.php` should probably be included from `plugin.php` for
compatibility, the way `functions.php` includes `option.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.
This has taken a long time to progress to this state. I want to mention a
few notable factors:
* '''Project history:''' This API is a pillar of WordPress. It is simple
yet powerful. It's been tweaked over time, but the core of its underlying
architecture has not significantly changed since it was first written more
than a decade ago. The developers who wrote it are no longer active in the
project's development. It is particularly challenging to consider a
* '''Personal history:''' Last time I tried to touch something in
plugin.php, I broke things really badly. See #21321. My comment there is
innocuous and merely suggests there were "some side effects here". Let me
be clear: This broke wordpress.org harder than I had ever broken it
before, and ever broken it since. If the original developers can no longer
be counted on to review this, and the last time (three years ago) I tried
to go anywhere near this I blew things sky high, you can perhaps
understand the reluctancy.
Conclusions and next steps:
* I want to recognize, and I very much appreciate, @jbrinley's long march
shepherding this. Great work so far, and great improvements over time.
Thank you for continuing to stay on our case about this.
* I am not comfortable committing this without ample lead time. It is too
late for 4.2. While I should have found the time to review this earlier in
the cycle, day two of a cycle is too late. (See ''supra'' the personal and
project factors.) And this review notes a number of things to
* It could definitely use some more tests, not just coverage for the new
stuff, but to prevent regressions.
* 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.
* We probably do need one final cost-benefit analysis, where "benefits"
are what this provides to plugin developers, and "costs" are what might
break and how hard. The blog post could double as a request for comments.
* I am willing to deploy this to .org for a few hours on a weekend and
see if it breaks anything.
* I am willing to commit this to trunk after 4.2 is released.
Ticket URL: <https://core.trac.wordpress.org/ticket/17817#comment:117>
WordPress Trac <https://core.trac.wordpress.org/>
WordPress publishing platform
More information about the wp-trac