Plugins: Speed up hooks addition/removal by ~26%.

[bor0]

Replaces spl_object_hash() with spl_object_id() in _wp_filter_build_unique_id() and WP_Widget_Factory, and adds an early return for plain object callbacks.

Fixes https://core.trac.wordpress.org/ticket/58291

[github-actions]

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

Core Committers: Use this line as a base for the props when committing in SVN:

Props bor0, westonruter.

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

[github-actions]

Test using WordPress Playground

The changes in this pull request can previewed and tested using a WordPress Playground instance.

WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser.

Some things to be aware of

• All changes will be lost when closing a tab with a Playground instance.
• All changes will be lost when refreshing the page.
• A fresh instance is created each time the link below is clicked.
• Every time this pull request is updated, a new ZIP file containing all changes is created. If changes are not reflected in the Playground instance,
  it's possible that the most recent build failed, or has not completed. Check the list of workflow runs to be sure.

For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation.

Test this pull request with WordPress Playground.

[westonruter]

Interestingly, I thought that this line could be removed, since above $callback is excluded from being a string or an object. The only other possible value which can be callable at this point is array, so this casting would seem to be redundant. But PHPStan doesn't currently do this narrowing. Without the cast, it is seen as: mixed~(object|string) and then the is_callable() narrows it it callable(): mixed: https://phpstan.org/r/577fd6d9-b6b9-4362-9acc-0794187fa71a

[westonruter]

I would have thought PHPStan would narrow it to non-empty-list<mixed>&callable() automatically.

[westonruter]

Thank you for the iterations!

[bor0]

Benchmark results of the PR in its current state; tested on wordpress-7.0-RC3 using wp-env

┌───────────────┬───────────────────────┬─────────────────────┬─────────────┐
│ Callback type │ Old (spl_object_hash) │ New (spl_object_id) │    Diff     │
├───────────────┼───────────────────────┼─────────────────────┼─────────────┤
│ string        │ 32.19ms               │ 32.73ms             │ ~0% (noise) │
├───────────────┼───────────────────────┼─────────────────────┼─────────────┤
│ closure       │ 50.59ms               │ 37.27ms             │ −26%        │
├───────────────┼───────────────────────┼─────────────────────┼─────────────┤
│ object+method │ 48.44ms               │ 45.56ms             │ −6%         │
├───────────────┼───────────────────────┼─────────────────────┼─────────────┤
│ static        │ 39.66ms               │ 43.10ms             │ ~0% (noise) │
└───────────────┴───────────────────────┴─────────────────────┴─────────────┘

After several iterations, I noticed that static is consistently slower by 2-3 ms.

This is because they go through the new is_callable guard that wasn't there before, but we decided that the safety guard trade-off was worth it.

For completeness:

-  if ( ! is_callable( $callback, true ) ) {
+  if ( ! isset( $callback[1] ) || ! is_string( $callback[1] ) ) {

┌─────────┬─────────┬──────────────┬─────────────────┐
│         │   Old   │ is_callable  │ isset+is_string │
├─────────┼─────────┼──────────────┼─────────────────┤
│ string  │ 32.40ms │ ~32ms (0%)   │ 32.14ms (0%)    │
├─────────┼─────────┼──────────────┼─────────────────┤
│ closure │ 51.43ms │ ~37ms (−27%) │ 36.75ms (−27%)  │
├─────────┼─────────┼──────────────┼─────────────────┤
│ object  │ 49.29ms │ ~45ms (−6%)  │ 42.70ms (−13%)  │
├─────────┼─────────┼──────────────┼─────────────────┤
│ static  │ 39.84ms │ ~43ms (+9%)  │ 40.38ms (~0%)   │
└─────────┴─────────┴──────────────┴─────────────────┘

[westonruter]

This is because they go through the new is_callable guard that wasn't there before, but we decided that the safety guard trade-off was worth it.

@bor0 It looks like I was wrong. After re-testing with is_callable(), PHPStan still complains:

------ -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
  Line   src/wp-includes/plugin.php
 ------ -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
  1016   Binary operation "." between numeric-string and mixed results in an error.
         🪪  binaryOp.invalid
  1020   Binary operation "." between mixed and '::' results in an error.
         🪪  binaryOp.invalid
  1020   Binary operation "." between non-falsy-string and mixed results in an error.
         🪪  binaryOp.invalid

I reverted back to what you had before in 6f23ca4, and this is now fixed.

If we wanted to use is_callable(), then the condition would have had to be as follows for PHPStan:

if ( ! is_callable( $callback, true ) || ! is_string( $callback[1] ) ) {

As I understand, what you had before was slightly more performant anyway, so I think it's a win-win.

[westonruter]

Via 541dc90 I also added a missing null check in WP_Hook::add_filter() for the return value of _wp_filter_build_unique_id(). This fixes:

  91     Cannot access offset string|null on mixed.                                                                                        
         🪪  offsetAccess.nonOffsetAccessible                                                                                              
         at src/wp-includes/class-wp-hook.php:91  

This check was present WP_Hook::remove_filter():

wordpress-develop/src/wp-includes/class-wp-hook.php

Lines 198 to 202 in 0d0c832

	$function_key = _wp_filter_build_unique_id( $hook_name, $callback, $priority );
	$exists = isset( $function_key, $this->callbacks[ $priority ][ $function_key ] );
	if ( $exists ) {

It was also present in ::has_filter():

wordpress-develop/src/wp-includes/class-wp-hook.php

Lines 245 to 249 in 0d0c832

	$function_key = _wp_filter_build_unique_id( $hook_name, $callback, false );
	if ( ! $function_key ) {
	return false;
	}

But it was missing in ::add_filter().

[bor0]

Alright, thanks! Looks good 🙌

[github-actions]

A commit was made that fixes the Trac ticket referenced in the description of this pull request.

SVN changeset: 62408
GitHub commit: 9560811

This PR will be closed, but please confirm the accuracy of this and reopen if there is more work to be done.