Make WordPress Core

Changeset 56349


Ignore:
Timestamp:
08/02/2023 06:35:24 PM (10 months ago)
Author:
hellofromTonya
Message:

Code Modernization: Deprecate dynamic properties in WP_List_Table magic 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 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:

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.

Location:
trunk
Files:
2 edited

Legend:

Unmodified
Added
Removed
  • trunk/src/wp-admin/includes/class-wp-list-table.php

    r56261 r56349  
    177177     *
    178178     * @since 4.0.0
     179     * @since 6.4.0 Getting a dynamic property is deprecated.
    179180     *
    180181     * @param string $name Property to get.
     
    185186            return $this->$name;
    186187        }
     188
     189        trigger_error(
     190            "The property `{$name}` is not defined. Getting a dynamic (undefined) property is " .
     191            'deprecated since version 6.4.0! Instead, define the property on the class.',
     192            E_USER_DEPRECATED
     193        );
     194        return null;
    187195    }
    188196
     
    191199     *
    192200     * @since 4.0.0
     201     * @since 6.4.0 Setting a dynamic property is deprecated.
    193202     *
    194203     * @param string $name  Property to check if set.
    195204     * @param mixed  $value Property value.
    196      * @return mixed Newly-set property.
    197205     */
    198206    public function __set( $name, $value ) {
    199207        if ( in_array( $name, $this->compat_fields, true ) ) {
    200             return $this->$name = $value;
    201         }
     208            $this->$name = $value;
     209            return;
     210        }
     211
     212        trigger_error(
     213            "The property `{$name}` is not defined. Setting a dynamic (undefined) property is " .
     214            'deprecated since version 6.4.0! Instead, define the property on the class.',
     215            E_USER_DEPRECATED
     216        );
    202217    }
    203218
     
    206221     *
    207222     * @since 4.0.0
     223     * @since 6.4.0 Checking a dynamic property is deprecated.
    208224     *
    209225     * @param string $name Property to check if set.
     
    215231        }
    216232
     233        trigger_error(
     234            "The property `{$name}` is not defined. Checking `isset()` on a dynamic (undefined) property " .
     235            'is deprecated since version 6.4.0! Instead, define the property on the class.',
     236            E_USER_DEPRECATED
     237        );
    217238        return false;
    218239    }
     
    222243     *
    223244     * @since 4.0.0
     245     * @since 6.4.0 Unsetting a dynamic property is deprecated.
    224246     *
    225247     * @param string $name Property to unset.
     
    228250        if ( in_array( $name, $this->compat_fields, true ) ) {
    229251            unset( $this->$name );
    230         }
     252            return;
     253        }
     254
     255        trigger_error(
     256            "A property `{$name}` is not defined. Unsetting a dynamic (undefined) property is " .
     257            'deprecated since version 6.4.0! Instead, define the property on the class.',
     258            E_USER_DEPRECATED
     259        );
    231260    }
    232261
  • trunk/tests/phpunit/tests/admin/wpListTable.php

    r56348 r56349  
    362362        );
    363363    }
     364
     365    /**
     366     * @dataProvider data_compat_fields
     367     * @ticket 58896
     368     *
     369     * @covers WP_List_Table::__get()
     370     *
     371     * @param string $property_name Property name to get.
     372     * @param mixed $expected       Expected value.
     373     */
     374    public function test_should_get_compat_fields_defined_property( $property_name, $expected ) {
     375        $list_table = new WP_List_Table( array( 'plural' => '_wp_tests__get' ) );
     376
     377        if ( 'screen' === $property_name ) {
     378            $this->assertInstanceOf( $expected, $list_table->$property_name );
     379        } else {
     380            $this->assertSame( $expected, $list_table->$property_name );
     381        }
     382    }
     383
     384    /**
     385     * @ticket 58896
     386     *
     387     * @covers WP_List_Table::__get()
     388     */
     389    public function test_should_throw_deprecation_when_getting_dynamic_property() {
     390        $this->expectDeprecation();
     391        $this->expectDeprecationMessage(
     392            'The property `undefined_property` is not defined. Getting a dynamic (undefined) property is ' .
     393            'deprecated since version 6.4.0! Instead, define the property on the class.'
     394        );
     395        $this->assertNull( $this->list_table->undefined_property, 'Getting a dynamic property should return null from WP_List_Table::__get()' );
     396    }
     397
     398    /**
     399     * @dataProvider data_compat_fields
     400     * @ticket 58896
     401     *
     402     * @covers WP_List_Table::__set()
     403     *
     404     * @param string $property_name Property name to set.
     405     */
     406    public function test_should_set_compat_fields_defined_property( $property_name ) {
     407        $value                            = uniqid();
     408        $this->list_table->$property_name = $value;
     409
     410        $this->assertSame( $value, $this->list_table->$property_name );
     411    }
     412
     413    /**
     414     * @ticket 58896
     415     *
     416     * @covers WP_List_Table::__set()
     417     */
     418    public function test_should_throw_deprecation_when_setting_dynamic_property() {
     419        $this->expectDeprecation();
     420        $this->expectDeprecationMessage(
     421            'The property `undefined_property` is not defined. Setting a dynamic (undefined) property is ' .
     422            'deprecated since version 6.4.0! Instead, define the property on the class.'
     423        );
     424        $this->list_table->undefined_property = 'some value';
     425    }
     426
     427    /**
     428     * @dataProvider data_compat_fields
     429     * @ticket 58896
     430     *
     431     * @covers WP_List_Table::__isset()
     432     *
     433     * @param string $property_name Property name to check.
     434     * @param mixed $expected       Expected value.
     435     */
     436    public function test_should_isset_compat_fields_defined_property( $property_name, $expected ) {
     437        $actual = isset( $this->list_table->$property_name );
     438        if ( is_null( $expected ) ) {
     439            $this->assertFalse( $actual );
     440        } else {
     441            $this->assertTrue( $actual );
     442        }
     443    }
     444
     445    /**
     446     * @ticket 58896
     447     *
     448     * @covers WP_List_Table::__isset()
     449     */
     450    public function test_should_throw_deprecation_when_isset_of_dynamic_property() {
     451        $this->expectDeprecation();
     452        $this->expectDeprecationMessage(
     453            'The property `undefined_property` is not defined. Checking `isset()` on a dynamic (undefined) property ' .
     454            'is deprecated since version 6.4.0! Instead, define the property on the class.'
     455        );
     456        $this->assertFalse( isset( $this->list_table->undefined_property ), 'Checking a dyanmic property should return false from WP_List_Table::__isset()' );
     457    }
     458
     459    /**
     460     * @dataProvider data_compat_fields
     461     * @ticket 58896
     462     *
     463     * @covers WP_List_Table::__unset()
     464     *
     465     * @param string $property_name Property name to unset.
     466     */
     467    public function test_should_unset_compat_fields_defined_property( $property_name ) {
     468        unset( $this->list_table->$property_name );
     469        $this->assertFalse( isset( $this->list_table->$property_name ) );
     470    }
     471
     472    /**
     473     * @ticket 58896
     474     *
     475     * @covers WP_List_Table::__unset()
     476     */
     477    public function test_should_throw_deprecation_when_unset_of_dynamic_property() {
     478        $this->expectDeprecation();
     479        $this->expectDeprecationMessage(
     480            'A property `undefined_property` is not defined. Unsetting a dynamic (undefined) property is ' .
     481            'deprecated since version 6.4.0! Instead, define the property on the class.'
     482        );
     483        unset( $this->list_table->undefined_property );
     484    }
     485
     486    /**
     487     * Data provider.
     488     *
     489     * @return array
     490     */
     491    public function data_compat_fields() {
     492        return array(
     493            '_args'            => array(
     494                'property_name' => '_args',
     495                'expected'      => array(
     496                    'plural'   => '_wp_tests__get',
     497                    'singular' => '',
     498                    'ajax'     => false,
     499                    'screen'   => null,
     500                ),
     501            ),
     502            '_pagination_args' => array(
     503                'property_name' => '_pagination_args',
     504                'expected'      => array(),
     505            ),
     506            'screen'           => array(
     507                'property_name' => 'screen',
     508                'expected'      => WP_Screen::class,
     509            ),
     510            '_actions'         => array(
     511                'property_name' => '_actions',
     512                'expected'      => null,
     513            ),
     514            '_pagination'      => array(
     515                'property_name' => '_pagination',
     516                'expected'      => null,
     517            ),
     518        );
     519    }
    364520}
Note: See TracChangeset for help on using the changeset viewer.