Skip to content

Commit

Permalink
Code Modernization: Deprecate dynamic properties in WP_List_Table mag…
Browse files Browse the repository at this point in the history 
…ic methods.

The unknown use of unknown dynamic property within the `WP_List_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_List_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.
* Removes returning the value when 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].
* Adds unit tests for happy and unhappy paths.

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 / defined as a property on the child class. When getting its value, e.g. `$list_table->data`, the `WP_List_Table::__get()` magic method is invoked, the following deprecation notice thrown, and `null` returned:

>The property `data` is not defined. Setting a dynamic (undefined) property is deprecated since version 6.4.0! Instead, define 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.

Several plugins, one of which has over 5M installs, add a property to the `$compat_fields` array. Removing the property would cause an `Undefined property` `Warning` (PHP 8) | `Notice` (PHP 7) to be thrown. Removing the associated code would change the functionality.

=== 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_List_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.]

Related to #14579, #22234, #30891.

Follow-up to [15491], [28493], [28521], [28524], [31146].

Props antonvlasenko, jrf, markjaquith, hellofromTonya, SergeyBiryukov, desrosj, peterwilsoncc, audrasjb, costdev, oglekler, jeffpaul.
Fixes #58896.
See #56034.

git-svn-id: https://develop.svn.wordpress.org/trunk@56349 602fd350-edb4-49c9-b593-d223f7449a82
  • Loading branch information
@hellofromtonya
hellofromtonya committed Aug 2, 2023
1 parent 88570fd commit c362c2a
Show file tree
Hide file tree
Showing 2 changed files with 187 additions and 2 deletions.
33 changes: 31 additions & 2 deletions src/wp-admin/includes/class-wp-list-table.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -176,6 +176,7 @@ public function __construct( $args = array() ) {
* Makes 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.
Expand All @@ -184,27 +185,42 @@ public function __get( $name ) {
if ( in_array( $name, $this->compat_fields, true ) ) {
return $this->$name;
}

trigger_error(
"The property `{$name}` is not defined. Getting a dynamic (undefined) property is " .
'deprecated since version 6.4.0! Instead, define the property on the class.',
E_USER_DEPRECATED
);
return null;
}

/**
* Makes 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 defined. Setting a dynamic (undefined) property is " .
'deprecated since version 6.4.0! Instead, define the property on the class.',
E_USER_DEPRECATED
);
}

/**
* Makes 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 a back-compat property and it is set.
Expand All @@ -214,20 +230,33 @@ public function __isset( $name ) {
return isset( $this->$name );
}

trigger_error(
"The property `{$name}` is not defined. Checking `isset()` on a dynamic (undefined) property " .
'is deprecated since version 6.4.0! Instead, define the property on the class.',
E_USER_DEPRECATED
);
return false;
}

/**
* Makes 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 defined. Unsetting a dynamic (undefined) property is " .
'deprecated since version 6.4.0! Instead, define the property on the class.',
E_USER_DEPRECATED
);
}

/**
Expand Down
156 changes: 156 additions & 0 deletions tests/phpunit/tests/admin/wpListTable.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -361,4 +361,160 @@ public function data_get_views_links_doing_it_wrong() {
),
);
}

/**
* @dataProvider data_compat_fields
* @ticket 58896
*
* @covers WP_List_Table::__get()
*
* @param string $property_name Property name to get.
* @param mixed $expected Expected value.
*/
public function test_should_get_compat_fields_defined_property( $property_name, $expected ) {
$list_table = new WP_List_Table( array( 'plural' => '_wp_tests__get' ) );

if ( 'screen' === $property_name ) {
$this->assertInstanceOf( $expected, $list_table->$property_name );
} else {
$this->assertSame( $expected, $list_table->$property_name );
}
}

/**
* @ticket 58896
*
* @covers WP_List_Table::__get()
*/
public function test_should_throw_deprecation_when_getting_dynamic_property() {
$this->expectDeprecation();
$this->expectDeprecationMessage(
'The property `undefined_property` is not defined. Getting a dynamic (undefined) property is ' .
'deprecated since version 6.4.0! Instead, define the property on the class.'
);
$this->assertNull( $this->list_table->undefined_property, 'Getting a dynamic property should return null from WP_List_Table::__get()' );
}

/**
* @dataProvider data_compat_fields
* @ticket 58896
*
* @covers WP_List_Table::__set()
*
* @param string $property_name Property name to set.
*/
public function test_should_set_compat_fields_defined_property( $property_name ) {
$value = uniqid();
$this->list_table->$property_name = $value;

$this->assertSame( $value, $this->list_table->$property_name );
}

/**
* @ticket 58896
*
* @covers WP_List_Table::__set()
*/
public function test_should_throw_deprecation_when_setting_dynamic_property() {
$this->expectDeprecation();
$this->expectDeprecationMessage(
'The property `undefined_property` is not defined. Setting a dynamic (undefined) property is ' .
'deprecated since version 6.4.0! Instead, define the property on the class.'
);
$this->list_table->undefined_property = 'some value';
}

/**
* @dataProvider data_compat_fields
* @ticket 58896
*
* @covers WP_List_Table::__isset()
*
* @param string $property_name Property name to check.
* @param mixed $expected Expected value.
*/
public function test_should_isset_compat_fields_defined_property( $property_name, $expected ) {
$actual = isset( $this->list_table->$property_name );
if ( is_null( $expected ) ) {
$this->assertFalse( $actual );
} else {
$this->assertTrue( $actual );
}
}

/**
* @ticket 58896
*
* @covers WP_List_Table::__isset()
*/
public function test_should_throw_deprecation_when_isset_of_dynamic_property() {
$this->expectDeprecation();
$this->expectDeprecationMessage(
'The property `undefined_property` is not defined. Checking `isset()` on a dynamic (undefined) property ' .
'is deprecated since version 6.4.0! Instead, define the property on the class.'
);
$this->assertFalse( isset( $this->list_table->undefined_property ), 'Checking a dyanmic property should return false from WP_List_Table::__isset()' );
}

/**
* @dataProvider data_compat_fields
* @ticket 58896
*
* @covers WP_List_Table::__unset()
*
* @param string $property_name Property name to unset.
*/
public function test_should_unset_compat_fields_defined_property( $property_name ) {
unset( $this->list_table->$property_name );
$this->assertFalse( isset( $this->list_table->$property_name ) );
}

/**
* @ticket 58896
*
* @covers WP_List_Table::__unset()
*/
public function test_should_throw_deprecation_when_unset_of_dynamic_property() {
$this->expectDeprecation();
$this->expectDeprecationMessage(
'A property `undefined_property` is not defined. Unsetting a dynamic (undefined) property is ' .
'deprecated since version 6.4.0! Instead, define the property on the class.'
);
unset( $this->list_table->undefined_property );
}

/**
* Data provider.
*
* @return array
*/
public function data_compat_fields() {
return array(
'_args' => array(
'property_name' => '_args',
'expected' => array(
'plural' => '_wp_tests__get',
'singular' => '',
'ajax' => false,
'screen' => null,
),
),
'_pagination_args' => array(
'property_name' => '_pagination_args',
'expected' => array(),
),
'screen' => array(
'property_name' => 'screen',
'expected' => WP_Screen::class,
),
'_actions' => array(
'property_name' => '_actions',
'expected' => null,
),
'_pagination' => array(
'property_name' => '_pagination',
'expected' => null,
),
);
}
}

0 comments on commit c362c2a

Please sign in to comment.