WordPress.org

Make WordPress Core

Opened 4 months ago

Last modified 2 months ago

#53218 new enhancement

Make apply_filters() variadic

Reported by: johnbillion Owned by:
Milestone: Future Release 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 (6)

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


4 months ago

  • Keywords has-patch added; needs-patch removed

#2 @johnbillion
3 months ago

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

#3 @jrf
3 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 3 months ago by jrf (previous) (diff)

#4 @jrf
3 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
3 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 2 months ago by jrf (previous) (diff)

#6 @jrf
3 months ago

P.S.: thanks for the ping 😉

Note: See TracTickets for help on using tickets.