Plugins: Prevent hook callback IDs from being cast to integer array keys - #13209
Plugins: Prevent hook callback IDs from being cast to integer array keys#13209westonruter wants to merge 8 commits into
Conversation
_wp_filter_build_unique_id() returns the string used as the callback key in WP_Hook::$callbacks. PHP casts an array key from string to int whenever the string is the canonical decimal representation of an integer, so a return value such as "5292" silently changes the type of the keys that consumers of that public property read back. Replace assertIsString() with an assertIsNonDecimalIntString() helper that round-trips the value through an array key, which is the runtime equivalent of PHPStan's non-decimal-int-string type. The closure case fails against trunk, where the ID for a bare object is the return value of spl_object_id() cast to a string. Also add coverage for an invokable object callback, which had none and is the other callback shape stored under a bare object ID, and add the missing @ticket annotations to the tests introduced in r62408. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
_wp_filter_build_unique_id() declares that it returns a string PHP will not cast to an integer when used as an array key, so the keys actually stored in WP_Hook::$callbacks are narrower than the array<string, Hook_Callback> these annotations claimed. Stating the real invariant turns the property into a second static analysis guard. Should the unique ID type ever widen back to a plain string, PHPStan reports the assignment in add_filter() from rule level 7 upwards, without the experimental reportUnsafeArrayStringKeyCasting option that was otherwise needed to catch it. Analysing the tree at level 8 yields an identical error set before and after, so the narrowing costs nothing. The Iterator and ArrayAccess type parameters, the offsetGet() and offsetSet() signatures, and the current() and next() return types all move with the property, since every one of them describes the same array. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Since r62408 the unique ID for a callback that is a bare object has been the return value of spl_object_id() cast to a string, such as "5292". PHP casts an array key from string to int whenever the string is the canonical decimal representation of an integer, so the cast was undone one layer down, at the point of storage: the keys of WP_Hook::$callbacks became integers for closures and __invoke instances, where every earlier release stored the 32 character hex string returned by spl_object_hash(). That property is public and is introspected by caching, profiling and debugging plugins. Any consumer running under strict_types, or passing a key to a parameter declared string, fatals on the integer. Prefix the ID with a non-numeric literal so PHP leaves it a string. This also retires the return.type baseline entry recorded when the narrower return type was first declared. PHPStan still cannot verify the result, since it has no type for a string carrying a known literal prefix and so infers only non-falsy-string. That is reported from rule level 7 upwards and is invisible at the level 5 the project analyses at. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Test using WordPress PlaygroundThe 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
For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation. |
|
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 Core Committers: Use this line as a base for the props when committing in SVN: To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
r62408 replaced spl_object_hash() with spl_object_id() in WP_Widget_Factory as well, and there without any cast, so the key for a widget registered as an instance is now an integer where every earlier release stored a 32 character hex string. WP_Widget_Factory::$widgets is public, so this is the same break as the one in WP_Hook. It also violates a contract inside core: get_widget_key() documents that it returns a string, and both the legacy widget block and the widget types REST controller pass what it returns to the_widget(), which documents its first parameter as a string and forwards it to the the_widget action, documented the same way. Nothing fatals in core because the lookups agree on the integer, but any listener declaring a string parameter under strict_types does. Prefix the key so PHP leaves it a string, and give the property a key and value type. The key type is what a bare spl_object_id() value would violate, and the value type resolves several reports of accessing a property or calling a method on mixed: analysing the file at rule level 10 goes from six errors to two. The one remaining report is that the branch registering by class name assigns an object where a WP_Widget is declared, since neither the type system nor WP_Widget itself expresses that a subclass named for registration must have a constructor callable with no arguments. It is reported from rule level 7 upwards and so is invisible at the level 5 the project analyses at. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Both prefixes fix a regression that shipped in 7.1, so the point release is the first version to carry them, not trunk's 7.2.0. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
WP_Widget_Factory::$widgets was given a plain string key type when the prefix was added, on the reasoning that a plain string already rejects both shapes the regression could take. PHPStan evaluates the two writes inline, typing a bare spl_object_id() as int and a cast one as decimal-int-string, and it models a decimal-int-string key as producing an array<int, ...>, so array<string, ...> catches either. Narrowing further appeared to buy nothing and to cost reports against correct code, since neither the prefixed key nor a class name can be proven non-numeric. That weighed the wrong thing. Neither key type produces a single report below rule level 7, so at the level this project analyses at the choice is invisible, and the one report narrowing adds is the same one already accepted for the identical expression in _wp_filter_build_unique_id(), where PHPStan equally cannot prove that a string carrying a literal prefix is not a decimal integer. Two files disagreeing about the type of a key built the same way is the larger cost, so state the invariant the prefix actually establishes. The narrower type goes in a @phpstan-var tag below the plain @var, the way WP_Hook::$callbacks already carries it, since non-decimal-int-string is not part of the documented PHPDoc vocabulary. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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 #13209. Follow-up to r62408, r62733. Props westonruter, dugi-digitaly, sergeybiryukov. See #58291. Fixes #65919. git-svn-id: https://develop.svn.wordpress.org/trunk@63334 602fd350-edb4-49c9-b593-d223f7449a82
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 WordPress/wordpress-develop#13209. Follow-up to r62408, r62733. Props westonruter, dugi-digitaly, sergeybiryukov. See #58291. Fixes #65919. Built from https://develop.svn.wordpress.org/trunk@63334 git-svn-id: http://core.svn.wordpress.org/trunk@62527 1a063a9b-81f0-0310-95a4-ce76da25c4cd
In r62408,
spl_object_hash()was replaced withspl_object_id()in two places. In both, the resulting array key is no longer a string, and in both the array holding it is a public property that plugins introspect.Committed in r63334. Core-65919 is reopened for 7.1.1 merge consideration.
WP_Hook::$callbacksThe unique ID that
_wp_filter_build_unique_id()builds for a callback that is a bare object (e.g. a closure or an instance with__invoke()) became the return value ofspl_object_id()cast to a string, for example"5292".PHP casts an array key from string to
intwhenever 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 inWP_Hook::add_filter():The keys 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(). Any consumer running understrict_types, or passing a key to a parameter declaredstring, now fatals:Prefix the ID with a non-numeric literal so PHP leaves it a string:
Only bare-object callbacks are affected.
[ $obj, 'method' ]already yields"5292method", and function names andClass::methodare returned unchanged, so no other key format changes.WP_Widget_Factory::$widgetsThe same changeset made the equivalent replacement in
WP_Widget_Factory::register()andunregister(), there without any cast at all:So the key for a widget registered as an instance is a genuine
int, andWP_Widget_Factory::$widgetsis public as well.This one also breaks a contract inside core.
get_widget_key()documents that it returns astring, and returns the array key straight out of its loop. Bothrender_block_core_legacy_widget()andWP_REST_Widget_Types_Controllerpass what it returns tothe_widget(), whose first parameter is documentedstring, and which forwards it to thethe_widgetaction, documented the same way. Nothing fatals inside core, because the lookups agree on the integer — but anythe_widgetlistener declaringfunction ( string $widget )understrict_typesdoes.The same prefix applies, held in one place so the two call sites cannot drift:
Guarding against a recurrence
The tests added in r62408 asserted the return type of
_wp_filter_build_unique_id(), which was correctlystring. They did not assert the type of the resulting array key, which is where the coercion happens, so the regression passed through them.WP_Widget_Factoryhad no key assertions at all.The tests now assert what actually matters, by round-tripping the value through an array key, the runtime equivalent of PHPStan's
non-decimal-int-string:Coverage for an invokable object callback is added, which had none, as are two tests for the widget factory keys.
Static analysis is tightened in three places:
_wp_filter_build_unique_id()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.WP_Hook::$callbackskey type is narrowed fromstringtonon-decimal-int-string, along with theIteratorandArrayAccesstype parameters and theoffsetGet(),offsetSet(),current(), andnext()signatures describing the same array. Analysing the tree at level 8 produces an identical error set before and after, so it costs nothing, and it catches the separate case of the unique ID type widening back to a plainstring.WP_Widget_Factory::$widgetsdeclares@phpstan-var array<non-decimal-int-string, WP_Widget>beneath a plain@var array<string, WP_Widget>, the wayWP_Hook::$callbacksalready carries the same type. A plainstringkey was used at first, on the grounds that PHPStan evaluates both of that class's writes inline — typing a barespl_object_id()asintand a cast one asdecimal-int-string, and modelling the latter as producing anarray<int, …>— soarray<string, …>already rejects both regression shapes without the extra report narrowing brings. That weighed the wrong thing: neither key type produces a single report below rule level 7, so at the level this project analyses at the choice is invisible, and two properties disagreeing about the type of a key built the same way costs more than the one report narrowing adds. The value type is a separate gain, retiring several reports of accessing a property or calling a method onmixed; analysing that file at level 10 goes from six errors to three.Two limitations are worth flagging for anyone raising the rule level later, both invisible at level 5:
non-falsy-stringfor either fixed expression and cannot verify it. That produces one report in each file — on the return in_wp_filter_build_unique_id()and on the instance write inWP_Widget_Factory::register()— from level 7 upwards.WP_Widget_Factory::register()that registers by class name assigns anobjectwhere aWP_Widgetis declared, since neither the type system norWP_Widgetexpresses that a subclass named for registration must have a constructor callable with no arguments. Also level 7 upwards.Testing instructions
Before this patch, the two new key assertions fail in each place:
After it:
--filter Tests_Hooks_BuildUniqueId— 9 tests, 15 assertions--filter Tests_Widgets— 130 tests, 778 assertions--group hooks— 168 testsTrac ticket: https://core.trac.wordpress.org/ticket/65919
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 fixes, and running the PHPStan and PHPUnit verification. I reviewed and take responsibility for all of it.
This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.