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

[WordPress Trac]

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

 Minor style:

  * 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 `!`.

 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?

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

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

  * `_wp_call_all_hook()` appears like it is just an unnecessary function
 call at this point and should probably stop being used.

 Other:

  * 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 is.

  * 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`.

 Tests:

  * 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
 rewrite.

  * '''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
 evaluate/study/tweak.

  * 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