Make apply_filters() variadic

[johnbillion]

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

https://core.trac.wordpress.org/ticket/53218

[johnbillion]

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

[jrf]

@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).

[jrf]

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

[jrf]

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.

[jrf]

P.S.: thanks for the ping 😉

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

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

[SergeyBiryukov]

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.

[johnbillion]

I'd like to get this in. Any objections?

[dingo_d]

This would be a great addition, especially from the perspective of static analysis tools like PHPStan.

Currently, you get tons of false positives when passing more than two arguments, and you need to ignore the rule altogether, which diminishes the use of the tool.

[davidbaumwald]

Took a look at the PR the PR and it looks good. Tested locally and ran unit tests. I feel like it's good to go. Spoke with @johnbillion, and he'll be committing this soon!

[peterwilsoncc]

Assigning to @johnbillion for commit.

The PR includes .github/workflows/phpbench-tests.yml, are you intending to include that as part of the commit or is it simply for generating the reports during development?

[johnbillion]

Replying to peterwilsoncc:

The PR includes .github/workflows/phpbench-tests.yml, are you intending to include that as part of the commit or is it simply for generating the reports during development?

Just for development 👍

[johnbillion]

In 52952:

Plugins: Convert apply_filters() into a proper variadic function.

This makes its signature more correct by implementing the spread operator, and adjusts the internal logic correspondingly without affecting performance.

Props jrf, SergeyBiryukov, davidbaumwald, mauriac, johnbillion

Fixes #53218

https://core.trac.wordpress.org/changeset/52952

[azouamauriac]

Replying to johnbillion:

Props jrf, SergeyBiryukov, davidbaumwald, mauriac, johnbillion

Not sure we can fix this, but just let you know, my username is ​@azouamauriac not mauriac. thanks.

[milana_cap]

This one probably needs updating usage example in code https://core.trac.wordpress.org/browser/trunk/src/wp-includes/plugin.php?rev=52952#L135,L151

And most likely reviewing user contributed notes in
​https://developer.wordpress.org/reference/functions/apply_filters/

[johnbillion]

I don't believe any of the docs need changing because the usage hasn't changed, just the internals. Anything I'm missing?