WordPress.org

Make WordPress Core

Opened 7 months ago

Last modified 4 weeks ago

#53218 new enhancement

Make apply_filters() variadic

Reported by: johnbillion Owned by:
Milestone: 6.0 Priority: normal
Severity: normal Version: 5.3
Component: Plugins Keywords: has-patch
Focuses: Cc:

Description

Previously in #47678 most functions in core that call func_get_args() were converted to variadic functions. One exception is apply_filters() which was left as-is due to a concern about filters which pass (and expect) a value by reference:

WP_Hooks::apply_filters() can not be simplified further as - even within WP Core -, there are filter functions which expect to receive the $value to be filtered by reference.

Ref: https://core.trac.wordpress.org/ticket/47678#comment:31

It does seem however that this function can be made variadic by adding an additional parameter after $value instead of making $value variadic. The existing tests pass with this change, but the test coverage might not be sufficient.

There are two potential outcomes to this ticket:

  1. Make apply_filters() variadic by adding an additional ...$args parameter, leaving the existing $value parameter alone, or
  2. Add tests which demonstrate why this cannot be done

Change History (9)

This ticket was mentioned in PR #1259 on WordPress/wordpress-develop by johnbillion.


7 months ago

  • Keywords has-patch added; needs-patch removed

#2 @johnbillion
6 months ago

Would like to get your input @jrf when you have time.

#3 @jrf
6 months ago

@johnbillion I've had a quick look and I would strongly recommend against this patch.

The performance of a function like array_merge() is often even worse than func_get_args(), so in effect, especially with using it twice, this would make one of the most used functions in WP slower, instead of faster, decreasing the overall performance (which the variadic changes were about).

Last edited 6 months ago by jrf (previous) (diff)

#4 @jrf
6 months ago

Oh and just for reference:

WP_Hooks::apply_filters() can not be simplified further as - even within WP Core -, there are filter functions which expect to receive the $value to be filtered by reference.

This remark was about the WP_Hooks::apply_filters() class method, not about the global apply_filters() function (which the current patch is touching).

#5 @jrf
6 months ago

This is the comment which actually does apply to the global apply_filters() function and why it wasn't changed in the previous ticket:

For the remaining calls to func_get_args() I didn't see any real benefit in making this change for various reasons.

https://core.trac.wordpress.org/ticket/47678#comment:30

And for this particular case the reason was that a patch would result in what is currently proposed, which would not have a beneficial effect on performance.

Last edited 5 months ago by jrf (previous) (diff)

#6 @jrf
6 months ago

P.S.: thanks for the ping 😉

#7 follow-up: @johnbillion
3 months ago

In PR 1259 I decided to revisit this with a view to removing as much of the args array processing in apply_filters() as possible in order to introduce the variadic $args parameter without negatively affecting performance.

As Juliette noted above, using array_merge() in place of func_get_args() and array_shift() results in a slight reduction in performance. What we can do instead is move the func_get_args() call inside the all hook handling block so it's not called when all isn't in use, switch to a variadic $args, and replace array_pop( $args ) with array_unshift() to push the $value variable onto the front of $args before it's passed to WP_Hook::apply_filters(). This results in no performance change down to the microsecond.

The all hook handling now uses the same handling as in do_action(). func_get_args() is more performant than array_unshift( $args, $hook_name ).

What's the overall gain of this change? A more correct signature for apply_filters() with no negative performance impact.

Benchmarking

The standard deviations of the benchmarks for this change are quite high because the processing time is so low. With 100,000 iterations the change averages to +/- less than 1/10th of a microsecond.

Benchmark results can be seen in the action here: https://github.com/WordPress/wordpress-develop/actions/runs/1243401754

Example:

old with callback 2.240μs
new with callback 2.230μs
old without callback 0.190μs
new without callback 0.182μs

#8 in reply to: ↑ 7 @SergeyBiryukov
3 months ago

Replying to johnbillion:

In PR 1259 I decided to revisit this with a view to removing as much of the args array processing in apply_filters() as possible in order to introduce the variadic $args parameter without negatively affecting performance.

Thanks for revisiting! The PR looks good to me at a glance.

#9 @johnbillion
4 weeks ago

  • Milestone changed from Future Release to 6.0
Note: See TracTickets for help on using tickets.