diff --git a/src/wp-includes/class-wp-hook.php b/src/wp-includes/class-wp-hook.php index 38895c87f1349..3675070106236 100644 --- a/src/wp-includes/class-wp-hook.php +++ b/src/wp-includes/class-wp-hook.php @@ -20,8 +20,8 @@ * accepted_args: int, * } * - * @phpstan-implements Iterator> - * @phpstan-implements ArrayAccess> + * @phpstan-implements Iterator> + * @phpstan-implements ArrayAccess> */ #[AllowDynamicProperties] final class WP_Hook implements Iterator, ArrayAccess { @@ -31,7 +31,7 @@ final class WP_Hook implements Iterator, ArrayAccess { * * @since 4.7.0 * @var array - * @phpstan-var array> + * @phpstan-var array> */ public $callbacks = array(); @@ -502,7 +502,7 @@ public function offsetExists( $offset ) { * * @param int $offset The offset to retrieve. * @return array|null If set, the value at the specified offset, null otherwise. - * @phpstan-return array|null + * @phpstan-return array|null */ #[ReturnTypeWillChange] public function offsetGet( $offset ) { @@ -518,7 +518,7 @@ public function offsetGet( $offset ) { * * @param int|null $offset The offset to assign the value to. * @param array $value The value to set. - * @phpstan-param array $value + * @phpstan-param array $value */ #[ReturnTypeWillChange] public function offsetSet( $offset, $value ) { @@ -554,7 +554,7 @@ public function offsetUnset( $offset ) { * @link https://www.php.net/manual/en/iterator.current.php * * @return array|false Array of callbacks at current priority, false if there are no more elements. - * @phpstan-return array|false + * @phpstan-return array|false */ #[ReturnTypeWillChange] public function current() { @@ -569,7 +569,7 @@ public function current() { * @link https://www.php.net/manual/en/iterator.next.php * * @return array|false Array of callbacks at next priority, false if there are no more elements. - * @phpstan-return array|false + * @phpstan-return array|false */ #[ReturnTypeWillChange] public function next() { diff --git a/src/wp-includes/class-wp-widget-factory.php b/src/wp-includes/class-wp-widget-factory.php index b6233eabba002..1f6ed3a58919e 100644 --- a/src/wp-includes/class-wp-widget-factory.php +++ b/src/wp-includes/class-wp-widget-factory.php @@ -16,11 +16,25 @@ #[AllowDynamicProperties] class WP_Widget_Factory { + /** + * Prefix for the key under which a widget registered as an instance is stored. + * + * Without it the key would be the decimal representation of an integer, which PHP casts from + * string to int when it is used as an array key. + * + * @since 7.1.1 + */ + private const INSTANCE_KEY_PREFIX = 'spl_object_id:'; + /** * Widgets array. * + * Keyed by class name for a widget registered by name, and by a prefixed object ID for a widget + * registered as an instance. + * * @since 2.8.0 - * @var array + * @var array + * @phpstan-var array */ public $widgets = array(); @@ -52,12 +66,13 @@ public function WP_Widget_Factory() { * @since 2.8.0 * @since 4.6.0 Updated the `$widget` parameter to also accept a WP_Widget instance object * instead of simply a `WP_Widget` subclass name. + * @since 7.1.1 The key for an instance is prefixed so that it is never cast to an integer. * * @param string|WP_Widget $widget Either the name of a `WP_Widget` subclass or an instance of a `WP_Widget` subclass. */ public function register( $widget ) { if ( $widget instanceof WP_Widget ) { - $this->widgets[ spl_object_id( $widget ) ] = $widget; + $this->widgets[ self::INSTANCE_KEY_PREFIX . spl_object_id( $widget ) ] = $widget; } else { $this->widgets[ $widget ] = new $widget(); } @@ -69,12 +84,13 @@ public function register( $widget ) { * @since 2.8.0 * @since 4.6.0 Updated the `$widget` parameter to also accept a WP_Widget instance object * instead of simply a `WP_Widget` subclass name. + * @since 7.1.1 The key for an instance is prefixed so that it is never cast to an integer. * * @param string|WP_Widget $widget Either the name of a `WP_Widget` subclass or an instance of a `WP_Widget` subclass. */ public function unregister( $widget ) { if ( $widget instanceof WP_Widget ) { - unset( $this->widgets[ spl_object_id( $widget ) ] ); + unset( $this->widgets[ self::INSTANCE_KEY_PREFIX . spl_object_id( $widget ) ] ); } else { unset( $this->widgets[ $widget ] ); } diff --git a/src/wp-includes/plugin.php b/src/wp-includes/plugin.php index f64b584374c8e..717aebb0e5860 100644 --- a/src/wp-includes/plugin.php +++ b/src/wp-includes/plugin.php @@ -989,6 +989,7 @@ function _wp_call_all_hook( $args ) { * and no longer returns false, but can still return void for invalid callbacks. * @since 6.9.0 Returns explicit null if an invalid callback is supplied. * @since 7.1.0 Uses spl_object_id() instead of spl_object_hash() for performance. + * @since 7.1.1 The ID for an object callback is prefixed so that it is never cast to an integer array key. * * @access private * @@ -998,6 +999,8 @@ function _wp_call_all_hook( $args ) { * @param int $priority Unused. The order in which the functions * associated with a particular action are executed. * @return string|null Unique function ID for usage as array key, or null if it couldn't be determined. + * + * @phpstan-return non-decimal-int-string|null */ function _wp_filter_build_unique_id( $hook_name, $callback, $priority ): ?string { if ( is_string( $callback ) ) { @@ -1005,7 +1008,12 @@ function _wp_filter_build_unique_id( $hook_name, $callback, $priority ): ?string } if ( is_object( $callback ) ) { - return (string) spl_object_id( $callback ); + /* + * The prefix keeps the ID from being the decimal representation of an integer. PHP casts such a + * string to int when it is used as an array key, which would change the type of the keys in + * WP_Hook::$callbacks and break consumers that pass them to string functions. + */ + return 'spl_object_id:' . spl_object_id( $callback ); } if ( ! isset( $callback[1] ) || ! is_string( $callback[1] ) ) { diff --git a/tests/phpunit/tests/hooks/buildUniqueId.php b/tests/phpunit/tests/hooks/buildUniqueId.php index 387a24bbc2fd1..20ef31bdd9942 100644 --- a/tests/phpunit/tests/hooks/buildUniqueId.php +++ b/tests/phpunit/tests/hooks/buildUniqueId.php @@ -8,29 +8,63 @@ */ class Tests_Hooks_BuildUniqueId extends WP_UnitTestCase { + public static function set_up_before_class() { + parent::set_up_before_class(); + + require_once __DIR__ . '/../../includes/mock-invokable.php'; + } + + /** + * @ticket 58291 + * @ticket 65919 + */ public function test_string_callback_returns_string(): void { $result = _wp_filter_build_unique_id( '', '__return_null', 10 ); - $this->assertIsString( $result ); + $this->assertIsNonDecimalIntString( $result ); $this->assertSame( '__return_null', $result ); } + /** + * @ticket 58291 + * @ticket 65919 + */ public function test_closure_returns_string(): void { $cb = function (): void {}; $result = _wp_filter_build_unique_id( '', $cb, 10 ); - $this->assertIsString( $result ); + $this->assertIsNonDecimalIntString( $result ); + } + + /** + * @ticket 58291 + * @ticket 65919 + */ + public function test_invokable_object_returns_string(): void { + $result = _wp_filter_build_unique_id( '', new Mock_Invokable(), 10 ); + $this->assertIsNonDecimalIntString( $result ); } + /** + * @ticket 58291 + * @ticket 65919 + */ public function test_object_callback_returns_string(): void { $a = new MockAction(); $result = _wp_filter_build_unique_id( '', array( $a, 'action' ), 10 ); - $this->assertIsString( $result ); + $this->assertIsNonDecimalIntString( $result ); } + /** + * @ticket 58291 + * @ticket 65919 + */ public function test_static_callback_returns_string(): void { $result = _wp_filter_build_unique_id( '', array( 'MockAction', 'action' ), 10 ); - $this->assertIsString( $result ); + $this->assertIsNonDecimalIntString( $result ); } + /** + * @ticket 58291 + */ public function test_two_different_objects_produce_different_ids(): void { $a = new MockAction(); $b = new MockAction(); @@ -40,6 +74,9 @@ public function test_two_different_objects_produce_different_ids(): void { ); } + /** + * @ticket 58291 + */ public function test_same_object_produces_same_id(): void { $a = new MockAction(); $this->assertSame( @@ -48,15 +85,42 @@ public function test_same_object_produces_same_id(): void { ); } + /** + * @ticket 58291 + */ public function test_malformed_array_missing_method_returns_null(): void { $a = new MockAction(); $result = _wp_filter_build_unique_id( '', array( $a ), 10 ); $this->assertNull( $result ); } + /** + * @ticket 58291 + */ public function test_malformed_array_non_string_method_returns_null(): void { $a = new MockAction(); $result = _wp_filter_build_unique_id( '', array( $a, 123 ), 10 ); $this->assertNull( $result ); } + + /** + * Asserts that a value is a string which PHP does not cast to an integer when used as an array key. + * + * The return value of _wp_filter_build_unique_id() is used as the callback key in WP_Hook::$callbacks. + * PHP silently casts an array key from string to int when the string is the canonical decimal + * representation of an integer, which changes the type of the keys that consumers of that public + * property read back. This is the runtime equivalent of PHPStan's `non-decimal-int-string` type. + * + * @param mixed $value Value to check. + */ + 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 ) + ); + } } diff --git a/tests/phpunit/tests/widgets.php b/tests/phpunit/tests/widgets.php index 8eb5914c9400d..7362146fda7fc 100644 --- a/tests/phpunit/tests/widgets.php +++ b/tests/phpunit/tests/widgets.php @@ -45,6 +45,56 @@ public function test_register_and_unregister_widget_core_widget() { $this->assertArrayNotHasKey( $widget_class, $wp_widget_factory->widgets ); } + /** + * Tests that a widget registered as an instance is keyed by a string. + * + * WP_Widget_Factory::$widgets is public, and its keys were strings in every release before the + * object ID was introduced. PHP casts an array key from string to int whenever the string is the + * canonical decimal representation of an integer, so a bare spl_object_id() value would change + * the type of the keys that consumers of that property read back. + * + * @see register_widget() + * @ticket 65919 + * + * @global WP_Widget_Factory $wp_widget_factory + */ + public function test_register_widget_instance_is_keyed_by_string() { + global $wp_widget_factory; + + register_widget( new WP_Widget_Search() ); + + $this->assertCount( 1, $wp_widget_factory->widgets ); + $this->assertIsString( array_key_first( $wp_widget_factory->widgets ) ); + } + + /** + * Tests that the key returned for a widget registered as an instance is a string. + * + * WP_Widget_Factory::get_widget_key() is documented as returning a string, and core passes what + * it returns to the_widget(), which in turn passes it to the 'the_widget' action, both of which + * document the value as a string. + * + * @see WP_Widget_Factory::get_widget_key() + * @ticket 65919 + * + * @global WP_Widget_Factory $wp_widget_factory + */ + public function test_get_widget_key_for_instance_returns_string() { + global $wp_widget_factory; + + $widget = new WP_Widget_Search(); + $widget->id_base = 'better_search'; + $widget->name = 'Better Search'; + $widget->option_name = 'widget_' . $widget->id_base; + $widget->widget_options['classname'] = 'widget_' . $widget->id_base; + $widget->control_options['id_base'] = $widget->id_base; + + register_widget( $widget ); + + $this->assertIsString( $wp_widget_factory->get_widget_key( 'better_search' ) ); + $this->assertSame( $widget, $wp_widget_factory->get_widget_object( 'better_search' ) ); + } + /** * Test that registering a widget class and registering a widget instance work together. *