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 )
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
- Normalize the object
Change History (20)
This ticket was mentioned in Slack in #core by westonruter. View the logs.
3 weeks ago
#5
@
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
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 )asdecimal-int-string, so reintroducing the bare cast is reported as areturn.typeerror from rule level 3 upwards, well inside the level 5 this project analyses at.- The
WP_Hook::$callbackskey type is narrowed fromstringtonon-decimal-int-string, along with theIteratorandArrayAccesstype parameters and theoffsetGet(),offsetSet(),current(), andnext()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 plainstring.
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.
#9
@
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
#12
@
3 weeks ago
- Keywords fixed-major dev-feedback added
- Resolution fixed
- Status closed → reopened
Re-opening for 7.1.1 merge consideration.
This ticket was mentioned in Slack in #core by adrianduffell. View the logs.
7 days ago
#15
@
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
@
4 days ago
This was discussed in last Friday’s bug scrub and we decided to include it in 7.1.1.
![(please configure the [header_logo] section in trac.ini)](/chrome/site/your_project_logo.png)
Here's an illustration of the problem: https://3v4l.org/RXD9o
Note that
spl_object_hash()returnsstringandspl_object_id()returnsint, so that is why r62408 added the(cast). Since the return value ofspl_object_id()is cast to astring, the expectation was that doingvar_export()would show string keys like this:But this is not correct, as PHP coerces numeric strings to integers, so this is the actual value:
Note in PHPStan there is a specific
decimal-int-stringtype for this kind of string:The return value of
spl_object_id()appears from the PHP source to always be apositive-int, so when casting it to astringit will end up being thisdecimal-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 adecimal-int-stringso that PHP won't coerce it to anintwhen 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
reportUnsafeArrayStringKeyCastingconfig (ref) which may suitable here.