Make WordPress Core

Opened 3 weeks ago

Closed 2 days ago

#65919 closed defect (bug) (fixed)

Array keys in WP_Hook::$callbacks may now be integers instead of strings in 7.1 causing fatal errors with string functions

Reported by: dugi digitaly Owned by: adamsilverstein
Priority: normal Milestone: 7.1.1
Component: Plugins Version: 7.1
Severity: normal Keywords: has-patch has-unit-tests fixed-major dev-reviewed
Cc: Focuses: php-compatibility

Description (last modified by westonruter)

Since [62408] (#58291), callbacks that are a bare object (closure or an __invoke instance) are stored in WP_Hook::$callbacks under an integer array key instead of a string. Through 7.0.x these keys were always strings (spl_object_hash(), 32-char hex). This is a BC break that fatals consumers which run string operations on the callback key.

[62408] intentionally casts the object branch to string:

if ( is_object( $callback ) ) {
    return (string) spl_object_id( $callback ); // e.g. "5292"
}

The function return value is a correct string. However, the value is then used as an array key in WP_Hook::add_filter():

$this->callbacks[ $priority ][ $idx ] = array( ... );

For a bare object, $idx is a canonical decimal string ("5292"), so PHP coerces it back to int on assignment. The (string) cast therefore does not achieve key-type consistency — it is nullified one layer down, at the point of storage.

This affects bare-object callbacks only. [ $obj, 'method' ] yields "5292method", and Class::method / function-name callbacks remain strings.

Why the existing tests did not catch it

The unit tests added in [62408] assert the return type of the function (string), object uniqueness, stability, and null for malformed input. They do not assert the type of the resulting array key in WP_Hook::$callbacks, which is where the coercion happens. A regression test should read back array_keys( $wp_filter[ $hook ]->callbacks[ $priority ] ) and assert the key is a string.

Steps to reproduce

add_action( 'init', function () {} );

global $wp_filter;
var_dump( array_keys( $wp_filter['init']->callbacks[10] ) );
// 7.1:    array(1) { [0] => int(5292) }
// 7.0.x:  array(1) { [0] => string(32) "..." }

Consumer failure under declare( strict_types=1 ):

foreach ( $wp_filter['init']->callbacks[10] as $id => $cb ) {
    $prefix = substr( $id, 0, 3 );
    // TypeError: substr(): Argument #1 ($string) must be of type string, int given
}

(spl_object_id() values are not deterministic across runs; the numbers above are illustrative.)

Impact

WP_Hook::$callbacks is a public property introspected by caching, profiling, and debugging plugins. Any consumer under strict_types, or passing the key to a string-typed parameter, now fatals. Confirmed in the wild: WP Rocket (Cloudflare.php, substr() on the key) broke immediately on 7.1.

Possible resolutions

  1. Normalize the object

Change History (20)

#1 @westonruter
3 weeks ago

  • Owner set to westonruter
  • Status newaccepted

This ticket was mentioned in Slack in #core by westonruter. View the logs.


3 weeks ago

#3 @westonruter
3 weeks ago

  • Description modified (diff)

#4 @westonruter
3 weeks ago

Here's an illustration of the problem: https://3v4l.org/RXD9o

<?php
$array = array();
$array[ spl_object_hash( new stdClass() ) ] = 'spl_object_hash (WP<7.0)';
$array[ (string) spl_object_id( new stdClass() ) ]   = 'spl_object_id (WP 7.1)';

Note that spl_object_hash() returns string and spl_object_id() returns int, so that is why r62408 added the (cast). Since the return value of spl_object_id() is cast to a string, the expectation was that doing var_export() would show string keys like this:

<?php
array (
  '00000000000000010000000000000000' => 'spl_object_hash (WP<7.0)',
  '1'                                => 'spl_object_id (WP 7.1)',
)

But this is not correct, as PHP coerces numeric strings to integers, so this is the actual value:

<?php
array (
  '00000000000000010000000000000000' => 'spl_object_hash (WP<7.0)',
  1                                  => 'spl_object_id (WP 7.1)',
)

Note in PHPStan there is a specific decimal-int-string type for this kind of string:

decimal-int-string accepts strings that contain a decimal integer representation and are cast to an integer when used as an array key (e.g. '0', '123', '-1').

The return value of spl_object_id() appears from the PHP source to always be a positive-int, so when casting it to a string it will end up being this decimal-int-string.

If we want to fix, this the simplest way to fix this would be to just prefix the return value of spl_object_id() with a string like 'spl_object_id:' which would prevent it from getting typed as a decimal-int-string so that PHP won't coerce it to an int when used as an array key.

We should also make sure the PHPStan typing is added to catch this in the future for other places where core is using string array keys to prevent us from erroneously introducing int keys. There is a reportUnsafeArrayStringKeyCasting config (ref) which may suitable here.

#5 @westonruter
3 weeks ago

  • Description modified (diff)
  • Summary Since [62408] (#58291), callbacks that are a bare object (closure or an `__invoke` instance) are stored in `WP_Hook::$callbacks` under an **integer** array key instead of a string. Through 7.0.x these keys were always strings (`spl_object_hash()`, 32-char hex). This is a BC break that fatals consumers which run string operations on the callback key.Array keys in WP_Hook::$callbacks may now be integers instead of strings in 7.1 causing fatal errors with string functions

#6 @westonruter
3 weeks ago

  • Milestone Awaiting Review7.1.1

This ticket was mentioned in PR #13209 on WordPress/wordpress-develop by @westonruter.


3 weeks ago
#7

  • Keywords has-patch has-unit-tests added

Since r62408, the unique ID that _wp_filter_build_unique_id() builds for a callback that is a bare object — a closure, or an instance with __invoke() — has been the return value of spl_object_id() cast to a string, for example "5292".

PHP casts an array key from string to int whenever the string is the canonical decimal representation of an integer, so that (string) cast is undone one layer down, at the point where the ID becomes a key in WP_Hook::add_filter():

$this->callbacks[ $priority ][ $idx ] = array( ... );

The keys of WP_Hook::$callbacks therefore became integers for bare-object callbacks, where every release through 7.0.x stored the 32 character hex string returned by spl_object_hash(). That property is public and is introspected by caching, profiling, and debugging plugins, so any consumer running under strict_types, or passing a key to a parameter declared string, now fatals:

foreach ( $wp_filter['init']->callbacks[10] as $id => $cb ) {
        $prefix = substr( $id, 0, 3 );
        // TypeError: substr(): Argument #1 ($string) must be of type string, int given
}

## The fix

Prefix the ID with a non-numeric literal so PHP leaves it a string:

return 'spl_object_id:' . spl_object_id( $callback );

Only bare-object callbacks are affected. [ $obj, 'method' ] already yields "5292method", and function names and Class::method are returned unchanged, so no other key format changes.

## Guarding against a recurrence

The tests added in r62408 asserted the return type of the function, which was correctly string. They did not assert the type of the resulting array key, which is where the coercion happens, so the regression passed through them.

Two guards are added:

  • _wp_filter_build_unique_id() now declares @phpstan-return non-decimal-int-string|null. PHPStan already infers (string) spl_object_id( $x ) as decimal-int-string, so reintroducing the bare cast is reported as a return.type error from rule level 3 upwards, well inside the level 5 this project analyses at.
  • The WP_Hook::$callbacks key type is narrowed from string to non-decimal-int-string, along with the Iterator and ArrayAccess type parameters and the offsetGet(), offsetSet(), current(), and next() signatures that describe the same array. Analysing the tree at level 8 produces an identical error set before and after this narrowing, so it costs nothing, and it catches the separate case of the unique ID type widening back to a plain string.

The unit tests now assert what actually matters by round-tripping the ID through an array key, which is the runtime equivalent of PHPStan's non-decimal-int-string:

private function assertIsNonDecimalIntString( $value ): void {
        $this->assertIsString( $value, 'The unique ID is not a string.' );

        $array = array( $value => true );

        $this->assertIsString(
                array_key_first( $array ),
                sprintf( 'The unique ID "%s" was cast to an integer when used as an array key.', $value )
        );
}

Coverage for an invokable object callback is also added, which had none.

One limitation is worth flagging for anyone raising the rule level later: PHPStan has no type for a string carrying a known literal prefix, so it infers only non-falsy-string for the fixed expression and cannot verify it. That is reported from level 7 upwards and is invisible at level 5, so it does not affect this project today.

## Testing instructions

Before this patch, Tests_Hooks_BuildUniqueId fails on the closure and invokable object cases:

✘ Closure returns string
  │ The unique ID "92588" was cast to an integer when used as an array key.
✘ Invokable object returns string
  │ The unique ID "431" was cast to an integer when used as an array key.

After it, the class passes (9 tests, 15 assertions), as does --group hooks (168 tests). PHPStan reports no errors across the analysed tree.

## Use of AI Tools

AI assistance: Yes
Tool(s): Claude Code
Model(s): Claude Opus 5
Used for: Investigating whether PHPStan could be configured to catch this class of bug, drafting the test changes and the fix, and running the PHPStan and PHPUnit verification. I reviewed and take responsibility for all of it.

#8 @westonruter
3 weeks ago

  • Focuses sustainability removed

#9 @westonruter
3 weeks ago

OK, PR is ready for review: https://github.com/WordPress/wordpress-develop/pull/13209

Note that r62408 related not only to WP_Hook::$callbacks but also to WP_Widget_Factory::$widgets, so the PR ensures string keys for both.

This ticket was mentioned in Slack in #core by westonruter. View the logs.


3 weeks ago

#11 @westonruter
3 weeks ago

  • Resolutionfixed
  • Status acceptedclosed

In 63334:

Plugins: Prevent object IDs from becoming integer array keys.

To improve performance, r62408 replaced spl_object_hash() with spl_object_id() when building the array keys for WP_Hook::$callbacks and for WP_Widget_Factory::$widgets. Even when the spl_object_id() return value is cast to a string to preserve the previous string array keys, PHP automatically converts such numeric strings to int when assigning the array key. This resulted in a back-compat breakage for plugins that expected to use the array keys in string functions, including possible fatal errors when using strict_types.

To preserve the previous string key behavior, any array keys using spl_object_id() are now prefixed with a string to prevent coercion to int. This not only fixes the expected array key types when iterating over these arrays, but it also fixes the return type for WP_Widget_Factory::get_widget_key() so it actually returns a string.

In addition to the added tests, the non-decimal-int-string type in PHPStan is leveraged instead of a plain string to prevent this regression from returning.

Developed in https://github.com/WordPress/wordpress-develop/pull/13209.
Follow-up to r62408, r62733.

Props westonruter, dugi-digitaly, sergeybiryukov.
See #58291.
Fixes #65919.

#12 @westonruter
3 weeks ago

  • Keywords fixed-major dev-feedback added
  • Resolution fixed
  • Status closedreopened

Re-opening for 7.1.1 merge consideration.

This ticket was mentioned in Slack in #core by adrianduffell. View the logs.


7 days ago

#14 @adamsilverstein
7 days ago

@westonruter - PR looks good! Do you want to take backporting to 7.1 or shall I?

#15 @westonruter
7 days ago

@adamsilverstein I was waiting for dev-reviewed before proceeding, but if you want to add that and take care of the merge commit, go for it!

#16 @adrianduffell
4 days ago

This was discussed in last Friday’s bug scrub and we decided to include it in 7.1.1.

This ticket was mentioned in Slack in #core by adrianduffell. View the logs.


3 days ago

#18 @westonruter
2 days ago

  • Owner changed from westonruter to adamsilverstein
  • Status reopenedreviewing

For dev-reviewed

#19 @adamsilverstein
2 days ago

  • Keywords dev-reviewed added; dev-feedback removed

Added dev-reviewed

#20 @westonruter
2 days ago

  • Resolutionfixed
  • Status reviewingclosed

In 63576:

Plugins: Prevent object IDs from becoming integer array keys.

To improve performance, r62408 replaced spl_object_hash() with spl_object_id() when building the array keys for WP_Hook::$callbacks and for WP_Widget_Factory::$widgets. Even when the spl_object_id() return value is cast to a string to preserve the previous string array keys, PHP automatically converts such numeric strings to int when assigning the array key. This resulted in a back-compat breakage for plugins that expected to use the array keys in string functions, including possible fatal errors when using strict_types.

To preserve the previous string key behavior, any array keys using spl_object_id() are now prefixed with a string to prevent coercion to int. This not only fixes the expected array key types when iterating over these arrays, but it also fixes the return type for WP_Widget_Factory::get_widget_key() so it actually returns a string.

In addition to the added tests, the non-decimal-int-string type in PHPStan is leveraged instead of a plain string to prevent this regression from returning.

Developed in https://github.com/WordPress/wordpress-develop/pull/13209.
Follow-up to r62408, r62733.

Reviewed by adamsilverstein.
Merges r63334 to the 7.1 branch.

Props westonruter, dugi-digitaly, sergeybiryukov.
See #58291.
Fixes #65919.

Note: See TracTickets for help on using tickets.