Skip to content

Commit 5b5d174

Browse files
hellofromtonyahellofromtonya
committed
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
1 parent 6fe193c commit 5b5d174

2 files changed

Lines changed: 195 additions & 3 deletions

File tree

‎src/wp-includes/class-wp-text-diff-renderer-table.php‎

Lines changed: 34 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -511,35 +511,51 @@ public function difference( $a, $b ) {
511511
* Make private properties readable for backward compatibility.
512512
*
513513
* @since 4.0.0
514+
* @since 6.4.0 Getting a dynamic property is deprecated.
514515
*
515516
* @param string $name Property to get.
516-
* @return mixed Property.
517+
* @return mixed A declared property's value, else null.
517518
*/
518519
public function __get( $name ) {
519520
if ( in_array( $name, $this->compat_fields, true ) ) {
520521
return $this->$name;
521522
}
523+
524+
trigger_error(
525+
"The property `{$name}` is not declared. Getting a dynamic property is " .
526+
'deprecated since version 6.4.0! Instead, declare the property on the class.',
527+
E_USER_DEPRECATED
528+
);
529+
return null;
522530
}
523531

524532
/**
525533
* Make private properties settable for backward compatibility.
526534
*
527535
* @since 4.0.0
536+
* @since 6.4.0 Setting a dynamic property is deprecated.
528537
*
529538
* @param string $name Property to check if set.
530539
* @param mixed $value Property value.
531-
* @return mixed Newly-set property.
532540
*/
533541
public function __set( $name, $value ) {
534542
if ( in_array( $name, $this->compat_fields, true ) ) {
535-
return $this->$name = $value;
543+
$this->$name = $value;
544+
return;
536545
}
546+
547+
trigger_error(
548+
"The property `{$name}` is not declared. Setting a dynamic property is " .
549+
'deprecated since version 6.4.0! Instead, declare the property on the class.',
550+
E_USER_DEPRECATED
551+
);
537552
}
538553

539554
/**
540555
* Make private properties checkable for backward compatibility.
541556
*
542557
* @since 4.0.0
558+
* @since 6.4.0 Checking a dynamic property is deprecated.
543559
*
544560
* @param string $name Property to check if set.
545561
* @return bool Whether the property is set.
@@ -548,18 +564,33 @@ public function __isset( $name ) {
548564
if ( in_array( $name, $this->compat_fields, true ) ) {
549565
return isset( $this->$name );
550566
}
567+
568+
trigger_error(
569+
"The property `{$name}` is not declared. Checking `isset()` on a dynamic property " .
570+
'is deprecated since version 6.4.0! Instead, declare the property on the class.',
571+
E_USER_DEPRECATED
572+
);
573+
return false;
551574
}
552575

553576
/**
554577
* Make private properties un-settable for backward compatibility.
555578
*
556579
* @since 4.0.0
580+
* @since 6.4.0 Unsetting a dynamic property is deprecated.
557581
*
558582
* @param string $name Property to unset.
559583
*/
560584
public function __unset( $name ) {
561585
if ( in_array( $name, $this->compat_fields, true ) ) {
562586
unset( $this->$name );
587+
return;
563588
}
589+
590+
trigger_error(
591+
"A property `{$name}` is not declared. Unsetting a dynamic property is " .
592+
'deprecated since version 6.4.0! Instead, declare the property on the class.',
593+
E_USER_DEPRECATED
594+
);
564595
}
565596
}

‎tests/phpunit/tests/diff/wpTextDiffRendererTable.php‎

Lines changed: 161 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,161 @@
1+
<?php
2+
3+
/**
4+
* Tests for WP_Text_Diff_Renderer_Table.
5+
*
6+
* @group diff
7+
*/
8+
class Tests_Diff_WpTextDiffRendererTable extends WP_UnitTestCase {
9+
/**
10+
* @var WP_Text_Diff_Renderer_Table
11+
*/
12+
private $diff_renderer_table;
13+
14+
public static function set_up_before_class() {
15+
parent::set_up_before_class();
16+
require_once ABSPATH . 'wp-includes/Text/Diff/Renderer.php';
17+
require_once ABSPATH . 'wp-includes/class-wp-text-diff-renderer-table.php';
18+
}
19+
20+
public function set_up() {
21+
parent::set_up();
22+
$this->diff_renderer_table = new WP_Text_Diff_Renderer_Table();
23+
}
24+
25+
/**
26+
* @dataProvider data_compat_fields
27+
* @ticket 58898
28+
*
29+
* @covers WP_Text_Diff_Renderer_Table::__get()
30+
*
31+
* @param string $property_name Property name to get.
32+
* @param mixed $expected Expected value.
33+
*/
34+
public function test_should_get_compat_fields( $property_name, $expected ) {
35+
$this->assertSame( $expected, $this->diff_renderer_table->$property_name );
36+
}
37+
38+
/**
39+
* @ticket 58898
40+
*
41+
* @covers WP_Text_Diff_Renderer_Table::__get()
42+
*/
43+
public function test_should_throw_deprecation_when_getting_dynamic_property() {
44+
$this->expectDeprecation();
45+
$this->expectDeprecationMessage(
46+
'The property `undeclared_property` is not declared. Getting a dynamic property is ' .
47+
'deprecated since version 6.4.0! Instead, declare the property on the class.'
48+
);
49+
$this->assertNull( $this->diff_renderer_table->undeclared_property, 'Getting a dynamic property should return null from WP_Text_Diff_Renderer_Table::__get()' );
50+
}
51+
52+
/**
53+
* @dataProvider data_compat_fields
54+
* @ticket 58898
55+
*
56+
* @covers WP_Text_Diff_Renderer_Table::__set()
57+
*
58+
* @param string $property_name Property name to set.
59+
*/
60+
public function test_should_set_compat_fields( $property_name ) {
61+
$value = uniqid();
62+
$this->diff_renderer_table->$property_name = $value;
63+
64+
$this->assertSame( $value, $this->diff_renderer_table->$property_name );
65+
}
66+
67+
/**
68+
* @ticket 58898
69+
*
70+
* @covers WP_Text_Diff_Renderer_Table::__set()
71+
*/
72+
public function test_should_throw_deprecation_when_setting_dynamic_property() {
73+
$this->expectDeprecation();
74+
$this->expectDeprecationMessage(
75+
'The property `undeclared_property` is not declared. Setting a dynamic property is ' .
76+
'deprecated since version 6.4.0! Instead, declare the property on the class.'
77+
);
78+
$this->diff_renderer_table->undeclared_property = 'some value';
79+
}
80+
81+
/**
82+
* @dataProvider data_compat_fields
83+
* @ticket 58898
84+
*
85+
* @covers WP_Text_Diff_Renderer_Table::__isset()
86+
*
87+
* @param string $property_name Property name to check.
88+
* @param mixed $expected Expected value.
89+
*/
90+
public function test_should_isset_compat_fields( $property_name, $expected ) {
91+
$actual = isset( $this->diff_renderer_table->$property_name );
92+
if ( is_null( $expected ) ) {
93+
$this->assertFalse( $actual );
94+
} else {
95+
$this->assertTrue( $actual );
96+
}
97+
}
98+
99+
/**
100+
* @ticket 58898
101+
*
102+
* @covers WP_Text_Diff_Renderer_Table::__isset()
103+
*/
104+
public function test_should_throw_deprecation_when_isset_of_dynamic_property() {
105+
$this->expectDeprecation();
106+
$this->expectDeprecationMessage(
107+
'The property `undeclared_property` is not declared. Checking `isset()` on a dynamic property ' .
108+
'is deprecated since version 6.4.0! Instead, declare the property on the class.'
109+
);
110+
$this->assertFalse( isset( $this->diff_renderer_table->undeclared_property ), 'Checking a dynamic property should return false from WP_Text_Diff_Renderer_Table::__isset()' );
111+
}
112+
113+
/**
114+
* @dataProvider data_compat_fields
115+
* @ticket 58898
116+
*
117+
* @covers WP_Text_Diff_Renderer_Table::__unset()
118+
*
119+
* @param string $property_name Property name to unset.
120+
*/
121+
public function test_should_unset_compat_fields( $property_name ) {
122+
unset( $this->diff_renderer_table->$property_name );
123+
$this->assertFalse( isset( $this->diff_renderer_table->$property_name ) );
124+
}
125+
126+
/**
127+
* @ticket 58898
128+
*
129+
* @covers WP_Text_Diff_Renderer_Table::__unset()
130+
*/
131+
public function test_should_throw_deprecation_when_unset_of_dynamic_property() {
132+
$this->expectDeprecation();
133+
$this->expectDeprecationMessage(
134+
'A property `undeclared_property` is not declared. Unsetting a dynamic property is ' .
135+
'deprecated since version 6.4.0! Instead, declare the property on the class.'
136+
);
137+
unset( $this->diff_renderer_table->undeclared_property );
138+
}
139+
140+
/**
141+
* Data provider.
142+
*
143+
* @return array
144+
*/
145+
public function data_compat_fields() {
146+
return array(
147+
'_show_split_view' => array(
148+
'property_name' => '_show_split_view',
149+
'expected' => true,
150+
),
151+
'inline_diff_renderer' => array(
152+
'property_name' => 'inline_diff_renderer',
153+
'expected' => 'WP_Text_Diff_Renderer_inline',
154+
),
155+
'_diff_threshold' => array(
156+
'property_name' => '_diff_threshold',
157+
'expected' => 0.6,
158+
),
159+
);
160+
}
161+
}

0 commit comments

Comments
 (0)