From 5b5d174a30c3057eb5870256e10ff0861df6eb09 Mon Sep 17 00:00:00 2001 From: Tonya Mork Date: Thu, 3 Aug 2023 18:11:36 +0000 Subject: [PATCH] Code Modernization: Deprecate dynamic properties in WP_Text_Diff_Renderer_Table magic methods. The unknown use of unknown dynamic property within the `WP_Text_Diff_Renderer_Table` property magic methods is now deprecated. A descriptive deprecation notice is provided to alert developers to declare the property on the child class extending `WP_Text_Diff_Renderer_Table`. Changes in this commit: * Adds a deprecation notice to the `__get()`, `__set()`, `__isset()`, `__unset()` magic methods, i.e. to alert and inform developers when attempting to get/set/isset/unset a dynamic property. * Fixes `__get()` to explicitly returns `null` when attempting to get a dynamic property. * Fixes `__set()` by removing the value return after setting a declared property, as (a) unnecessary and (b) `__set()` should return `void` [https://www.php.net/manual/en/language.oop5.overloading.php#object.set per the PHP handbook]. * Fixes `__isset()` to return `false` if not in the `$compat_fields`, as `isset()` and `__isset()` should always return `bool`: * [https://www.php.net/manual/en/language.oop5.overloading.php#object.isset `__isset()` in the PHP manual] * [https://www.php.net/manual/en/function.isset.php `isset()` in the PHP manual] * Adds a test class with happy and unhappy paths for these changes. For backward compatibility, no changes are made to the internal declared properties listed in `$compat_fields` and accessed through the magic methods. For example: A child class uses a property named `$data` that is not declared as a property on the child class. When getting its value, e.g. `$user_query->data`, the `WP_Text_Diff_Renderer_Table::__get()` magic method is invoked, the following deprecation notice thrown, and `null` returned: >The property `data` is not declared. Setting a dynamic property is deprecated since version 6.4.0! Instead, declare the property on the class. === Why not remove the magic methods, remove the `$compat_fields` property, and restore the properties `public`? tl;dr Backward compatibility. If a plugin adds a property to `$compat_fields` array, then sites using that plugin would experience (a) an `Undefined property` `Warning` (PHP 8) | `Notice` (PHP 7) and (b) a possible change in behavior. === Why not limit the deprecation for PHP versions >= 8.2? tl;dr original design intent and inform The magic methods and `$compat_fields` property were added for one purpose: to continue providing external access to internal properties declared on `WP_Text_Diff_Renderer_Table`. They were not intended to be used for dynamic properties. Deprecating that unintended usage both alerts developers a change is needed in their child class and informs them what to change. References: * Dynamic (non-explicitly declared) properties are deprecated as of PHP 8.2 and are expected to become a fatal error in PHP 9.0. * A [https://www.youtube.com/live/vDZWepDQQVE?feature=share&t=10097 live open public working session] where these changes were discussed and agreed to. * [https://wiki.php.net/rfc/deprecate_dynamic_properties PHP RFC: Deprecate dynamic properties.] Follow-up to [28525], [31135]. Props antonvlasenko, rajinsharwar, jrf, markjaquith, hellofromTonya, SergeyBiryukov, desrosj, peterwilsoncc, audrasjb, costdev, oglekler, jeffpaul. Fixes #58898. See #56034. git-svn-id: https://develop.svn.wordpress.org/trunk@56354 602fd350-edb4-49c9-b593-d223f7449a82 --- .../class-wp-text-diff-renderer-table.php | 37 +++- .../tests/diff/wpTextDiffRendererTable.php | 161 ++++++++++++++++++ 2 files changed, 195 insertions(+), 3 deletions(-) create mode 100644 tests/phpunit/tests/diff/wpTextDiffRendererTable.php diff --git a/src/wp-includes/class-wp-text-diff-renderer-table.php b/src/wp-includes/class-wp-text-diff-renderer-table.php index 0174fed359..b54aa6c368 100644 --- a/src/wp-includes/class-wp-text-diff-renderer-table.php +++ b/src/wp-includes/class-wp-text-diff-renderer-table.php @@ -511,35 +511,51 @@ class WP_Text_Diff_Renderer_Table extends Text_Diff_Renderer { * Make private properties readable for backward compatibility. * * @since 4.0.0 + * @since 6.4.0 Getting a dynamic property is deprecated. * * @param string $name Property to get. - * @return mixed Property. + * @return mixed A declared property's value, else null. */ public function __get( $name ) { if ( in_array( $name, $this->compat_fields, true ) ) { return $this->$name; } + + trigger_error( + "The property `{$name}` is not declared. Getting a dynamic property is " . + 'deprecated since version 6.4.0! Instead, declare the property on the class.', + E_USER_DEPRECATED + ); + return null; } /** * Make private properties settable for backward compatibility. * * @since 4.0.0 + * @since 6.4.0 Setting a dynamic property is deprecated. * * @param string $name Property to check if set. * @param mixed $value Property value. - * @return mixed Newly-set property. */ public function __set( $name, $value ) { if ( in_array( $name, $this->compat_fields, true ) ) { - return $this->$name = $value; + $this->$name = $value; + return; } + + trigger_error( + "The property `{$name}` is not declared. Setting a dynamic property is " . + 'deprecated since version 6.4.0! Instead, declare the property on the class.', + E_USER_DEPRECATED + ); } /** * Make private properties checkable for backward compatibility. * * @since 4.0.0 + * @since 6.4.0 Checking a dynamic property is deprecated. * * @param string $name Property to check if set. * @return bool Whether the property is set. @@ -548,18 +564,33 @@ class WP_Text_Diff_Renderer_Table extends Text_Diff_Renderer { if ( in_array( $name, $this->compat_fields, true ) ) { return isset( $this->$name ); } + + trigger_error( + "The property `{$name}` is not declared. Checking `isset()` on a dynamic property " . + 'is deprecated since version 6.4.0! Instead, declare the property on the class.', + E_USER_DEPRECATED + ); + return false; } /** * Make private properties un-settable for backward compatibility. * * @since 4.0.0 + * @since 6.4.0 Unsetting a dynamic property is deprecated. * * @param string $name Property to unset. */ public function __unset( $name ) { if ( in_array( $name, $this->compat_fields, true ) ) { unset( $this->$name ); + return; } + + trigger_error( + "A property `{$name}` is not declared. Unsetting a dynamic property is " . + 'deprecated since version 6.4.0! Instead, declare the property on the class.', + E_USER_DEPRECATED + ); } } diff --git a/tests/phpunit/tests/diff/wpTextDiffRendererTable.php b/tests/phpunit/tests/diff/wpTextDiffRendererTable.php new file mode 100644 index 0000000000..4dbbdbbefe --- /dev/null +++ b/tests/phpunit/tests/diff/wpTextDiffRendererTable.php @@ -0,0 +1,161 @@ +diff_renderer_table = new WP_Text_Diff_Renderer_Table(); + } + + /** + * @dataProvider data_compat_fields + * @ticket 58898 + * + * @covers WP_Text_Diff_Renderer_Table::__get() + * + * @param string $property_name Property name to get. + * @param mixed $expected Expected value. + */ + public function test_should_get_compat_fields( $property_name, $expected ) { + $this->assertSame( $expected, $this->diff_renderer_table->$property_name ); + } + + /** + * @ticket 58898 + * + * @covers WP_Text_Diff_Renderer_Table::__get() + */ + public function test_should_throw_deprecation_when_getting_dynamic_property() { + $this->expectDeprecation(); + $this->expectDeprecationMessage( + 'The property `undeclared_property` is not declared. Getting a dynamic property is ' . + 'deprecated since version 6.4.0! Instead, declare the property on the class.' + ); + $this->assertNull( $this->diff_renderer_table->undeclared_property, 'Getting a dynamic property should return null from WP_Text_Diff_Renderer_Table::__get()' ); + } + + /** + * @dataProvider data_compat_fields + * @ticket 58898 + * + * @covers WP_Text_Diff_Renderer_Table::__set() + * + * @param string $property_name Property name to set. + */ + public function test_should_set_compat_fields( $property_name ) { + $value = uniqid(); + $this->diff_renderer_table->$property_name = $value; + + $this->assertSame( $value, $this->diff_renderer_table->$property_name ); + } + + /** + * @ticket 58898 + * + * @covers WP_Text_Diff_Renderer_Table::__set() + */ + public function test_should_throw_deprecation_when_setting_dynamic_property() { + $this->expectDeprecation(); + $this->expectDeprecationMessage( + 'The property `undeclared_property` is not declared. Setting a dynamic property is ' . + 'deprecated since version 6.4.0! Instead, declare the property on the class.' + ); + $this->diff_renderer_table->undeclared_property = 'some value'; + } + + /** + * @dataProvider data_compat_fields + * @ticket 58898 + * + * @covers WP_Text_Diff_Renderer_Table::__isset() + * + * @param string $property_name Property name to check. + * @param mixed $expected Expected value. + */ + public function test_should_isset_compat_fields( $property_name, $expected ) { + $actual = isset( $this->diff_renderer_table->$property_name ); + if ( is_null( $expected ) ) { + $this->assertFalse( $actual ); + } else { + $this->assertTrue( $actual ); + } + } + + /** + * @ticket 58898 + * + * @covers WP_Text_Diff_Renderer_Table::__isset() + */ + public function test_should_throw_deprecation_when_isset_of_dynamic_property() { + $this->expectDeprecation(); + $this->expectDeprecationMessage( + 'The property `undeclared_property` is not declared. Checking `isset()` on a dynamic property ' . + 'is deprecated since version 6.4.0! Instead, declare the property on the class.' + ); + $this->assertFalse( isset( $this->diff_renderer_table->undeclared_property ), 'Checking a dynamic property should return false from WP_Text_Diff_Renderer_Table::__isset()' ); + } + + /** + * @dataProvider data_compat_fields + * @ticket 58898 + * + * @covers WP_Text_Diff_Renderer_Table::__unset() + * + * @param string $property_name Property name to unset. + */ + public function test_should_unset_compat_fields( $property_name ) { + unset( $this->diff_renderer_table->$property_name ); + $this->assertFalse( isset( $this->diff_renderer_table->$property_name ) ); + } + + /** + * @ticket 58898 + * + * @covers WP_Text_Diff_Renderer_Table::__unset() + */ + public function test_should_throw_deprecation_when_unset_of_dynamic_property() { + $this->expectDeprecation(); + $this->expectDeprecationMessage( + 'A property `undeclared_property` is not declared. Unsetting a dynamic property is ' . + 'deprecated since version 6.4.0! Instead, declare the property on the class.' + ); + unset( $this->diff_renderer_table->undeclared_property ); + } + + /** + * Data provider. + * + * @return array + */ + public function data_compat_fields() { + return array( + '_show_split_view' => array( + 'property_name' => '_show_split_view', + 'expected' => true, + ), + 'inline_diff_renderer' => array( + 'property_name' => 'inline_diff_renderer', + 'expected' => 'WP_Text_Diff_Renderer_inline', + ), + '_diff_threshold' => array( + 'property_name' => '_diff_threshold', + 'expected' => 0.6, + ), + ); + } +}