Fix WP_List_Table magic methods for PHP 8.2 dynamic properties

[antonvlasenko]

WP_List_Table has a list of compatible fields (compat_fields), which was introduced at the same time as the magic methods. The fields in compat_fields list are then not dynamic properties, as the magic methods properly handle each. But fields not in that list are dynamic properties.

What happens if a field is not in the list?

• __get() falls through the method.
• __set() nothing happens as it falls through the method.
• __isset() returns false, which is correct.
• __unset() nothing happens as it falls through the method.

The problem:
When a dynamic property is called on this class, there's no information given to the developer. The notice of _doing_it_wrong() will provide the alert to developers to modify their code.

Also notice: the compat_fields are essentially public properties through the magic methods.

Thus, there are 2 ways to fix this class to be compatible with PHP 8.2's dynamic properties:

1. Option 1: Fix the magic methods:

• Change 1: Add a notice or _doing_it_wrong() to each magic method, i.e. to alert and inform developers when attempting to get/set/isset/unset a dynamic property.

• Change 2: Add return null in __get() when attempting to get a dynamic property that is not in the compat_fields.

2. Option 2: Add each compat_fields as a public property and remove the magic methods.

Props to @jrf for identifying the issue.

References:

• These changes were ​discussed and agreed to in a livestream by @jrf @markjaquith and @hellofromTonya.
• The changes are part of #56034, which is the larger Dynamic Properties proposal and initiative.

[hellofromTonya]

@antonvlasenko can you pull WP_List_Table changes out of ​https://github.com/WordPress/wordpress-develop/pull/3535 and create a PR for this ticket?

[hellofromTonya]

More context:

These changes were made for backwards-compatibility (BC).

In #22234 (in WP 4.0.0):

• [28493] added the __get() magic method to WP_List_Table to:

  Make private properties readable for backwards compatibility
  

• [28521] added the __set() magic method

• [28524] added the __isset() and __unset() magic methods

In #30891 (in WP 4.2.0):

• [31146] added the list of compat_fields and the logic in each of the property magic methods.

This PR aims to call doint_it_wrong() when trying to use dynamic properties on the WP_List_Table class. Dynamic properties are not compatible with PHP 8.2 and above.

Trac ticket: https://core.trac.wordpress.org/ticket/58896

[antonvlasenko]

Replying to hellofromTonya:

@antonvlasenko can you pull WP_List_Table changes out of ​https://github.com/WordPress/wordpress-develop/pull/3535 and create a PR for this ticket?

I've pulled the changes out of ​https://github.com/WordPress/wordpress-develop/pull/3535 and submitted them to ​https://github.com/WordPress/wordpress-develop/pull/4905, @hellofromTonya.

[hellofromTonya]

Replying to antonvlasenko:

I've pulled the changes out of ​https://github.com/WordPress/wordpress-develop/pull/3535 and submitted them to ​https://github.com/WordPress/wordpress-develop/pull/4905

Thank you @antonvlasenko! Appreciate your time porting your original work to this ticket. Good to have option 1 here for history and contributor consideration.

[hellofromTonya]

Exploring Option 2

Let's look at option 2, which proposes to make the properties public and remove the magic methods.

Scenario 1: Accessing a defined property

Demo code: ​https://3v4l.org/jNFmL#v8.1.20

• Get: works the same ✅
• isset(): works the same ✅
• Set: works the same ✅
• unset(): works the same ✅
• After an unset():
  • Get: works the same ✅
  • isset(): works the same ✅
  • Set: works the same ✅

Functionality is unaffected

Scenario 2: Accessing an undefined (dynamic) property

Demo code: ​https://3v4l.org/nCACX#v8.1.20

• Get: ❌
  • both return same value of NULL ✅
  • Without magic method: throws Warning ❌
• isset(): works the same ✅
• Set: ❌
  • With magic method: does not set - value remains NULL
  • Without magic method: value sets ❌
• unset(): works the same ✅
• After an unset()
  • Get: ❌
    • both return same value of NULL ✅
    • Without magic method: throws Warning ❌
  • isset(): works the same ✅
  • Set: ❌
    • With magic method: does not set - value remains NULL
    • Without magic method: value sets ❌

[hellofromTonya]

How did the original 3.1 code work?

Before the magic methods were added, how did the original WP_List_Table work with defined and undefined (dynamic) properties? This test report shows it in action, i.e. to set a baseline for considering how to fix this ticket.

Running on PHP 5.2.17

Demo: ​https://3v4l.org/GBfpi#v5.2.17 using the properties code from WP 3.1 in [15491] / #14579.

• Get:
  • Defined property: returns NULL
  • Dynamic property: returns NULL and throws Notice
• isset():
  • Defined property: false
  • Dynamic property: false
• Set:
  • Defined property: sets it
  • Dynamic property: sets it
• After an unset()
  • Get:
    • Defined property: returns NULL and throws a Notice
    • Dynamic property: returns NULL and throws a Notice
  • isset():
    • Defined property: returns false
    • Dynamic property: returns false
  • Set:
    • Defined property: sets it
    • Dynamic property: sets it
  • isset():
    • Defined property: returns true
    • Dynamic property: returns true

Running on PHP 8.1.20

Demo: ​https://3v4l.org/GBfpi#v8.1.20 using the properties code from WP 3.1 in [15491] / #14579.

• Get:
  • Defined property: null
  • Dynamic property: returns NULL and throws Warning
• isset():
  • Defined property: false
  • Dynamic property: false
• Set:
  • Defined property: sets it
  • Dynamic property: sets it
• After an unset()
  • Get:
    • Defined property: returns NULL and throws a Warning
    • Dynamic property: returns NULL and throws a Warning
  • isset():
    • Defined property: returns false
    • Dynamic property: returns false
  • Set:
    • Defined property: sets it
    • Dynamic property: sets it
  • isset():
    • Defined property: returns true
    • Dynamic property: returns true

[hellofromTonya]

Comparing the how the code worked in 3.1 to the changes made in 4.0+, the BC break happened when the magic methods were added. Removing the magic methods (option 2) restores the code to how it originally functioned, thus removing the BC break(s) that happened long ago. Hmm interesting.

Let's compare:

Demo: ​https://3v4l.org/DZnjf#v8.1.20

• Get:
  • Defined property:
    • WP 3.1: returns NULL
    • Current: returns NULL ✅
    • Option 2: returns NULL ✅
  • Dynamic property:
    • WP 3.1: returned NULL and throws Warning
    • Current: returns NULL ❌ 👈 BC Break
    • WP 3.1: returned NULL and throws Warning ✅
• isset():
  • Defined property:
    • WP 3.1: false
    • Current: false ✅
    • Option 2: false ✅
  • Dynamic property:
    • WP 3.1: false
    • Current: false ✅
    • Option 2: false ✅
• set:
  • Defined property:
    • WP 3.1: sets
    • Current: sets ✅
    • Option 2: sets ✅
  • Dynamic property:
    • WP 3.1: sets
    • Current: does not set ❌ 👈 BC Break
    • Option 2: sets ✅
• unset():
  • Get:
    • Defined property:
      • WP 3.1: returns NULL and throws Warning
      • Current: returns NULL and throws Warning ✅
      • Option 2: returns NULL and throws Warning ✅
    • Dynamic property:
      • WP 3.1: returns NULL and throws Warning
      • Current: returns NULL but does not throw Warning ❌ 👈 BC Break
      • Option 2: returns NULL and throws Warning ✅
  • isset():
    • Defined property:
      • WP 3.1: false
      • Current: false ✅
      • Option 2: false ✅
    • Dynamic property:
      • WP 3.1: false
      • Current: false ✅
      • Option 2: false ✅
  • Set:
    • Defined property:
      • WP 3.1: sets
      • Current: sets ✅
      • Option 2: sets ✅
    • Dynamic property:
      • WP 3.1: sets
      • Current: does not set ❌ 👈 BC Break
      • Option 2: sets ✅

[hellofromTonya]

When comparing how the code originally worked in 3.1 to how it worked in 4.0 after adding the magic methods to option 2, here are the findings:

1. For defined properties, the code works the same. No impacts.
2. For dynamic properties:
   • The magic methods added in 4.0 caused a BC break when getting and setting.
   • Option 1 retains the current behavior (added in 4.0 with the magic methods).
   • Option 2 fixes the BC break and restores how it originally worked in 3.1.

Would Option 2 be considered a BC break for getting or setting dynamic properties?

I'm inclined to say no. Though it will function differently than the current code for dynamic properties, the current code itself has been a BC break since first introduced in 4.0.

On the other hand, one could argue that Option 2 is itself a BC break from the long standing behavior which could impact any extenders who are attempting to get or set a dynamic property. BC break of a BC break to restore the original behavior ;)

Hmm, an interesting situation. Fix the long ago BC break (option 2). Or fix the immediate problem and retain current behavior (option 1).

My mind next goes to knowing the impact by understanding how many plugins might be affected.

Are any plugins using dynamic properties with WP_List_Table?

There are over 3200 plugins using WP_List_Table ​https://wpdirectory.net/search/01H67HCY4CABR39QEEQRNH1DG8. It is possible there are plugins using dynamic properties. And if yes, then users using those plugins would be affected.

[hellofromTonya]

After thinking about impacts on users and talking with @azaozz, I think Option 1 is the right way forward.

Though the magic methods introduced a BC break, that happened 9 years ago. Fast forward to today, plugins are (and have been) using that code. I think the current code is the yardstick for determining if the new fix is a BC break or not. Using that yardstick, Option 2 would be a BC break to the current code and thus could/would impact users.\

@antonvlasenko I'll review ​https://github.com/WordPress/wordpress-develop/pull/4905 tomorrow with the goal of committing it this week or next.

[hellofromTonya]

I think there's an Option 3 which is a combination of Option 1 and 2 with the benefits of both along with the ability to possibly remove the magic methods in the future.

Option 3:

• Properties:
  • Change their visibility keyword to public.
  • Remove the $compat_fields.
• Magic methods:
  • Remove the $compat_fields code.
  • Unknown dynamic properties in the wild: keep the magic methods (for now) but deprecate each of them. The deprecation notice will alert developers to modify their code.

The benefits:

• Performance: provides a micro-optimization when a public property is invoked outside of the object, as the magic methods and the in_array() logic within each will not be invoked.
• Maintainability: less code and complexity.
• Back-compat: no BC breaks (no change to the result of the code).
• Sets up the possibility of removing the magic methods in the future.

Implements Option 3, see explanation in the Trac ticket https://core.trac.wordpress.org/ticket/58896#comment:12.

Fixes the dynamic properties for PHP 8.2 compatibility by:

1. Defined property: restores each to public.
2. Removes the $compat_fields and logic.
3. Deprecates each property magic method.

Trac ticket: https://core.trac.wordpress.org/ticket/58896

Pinging @markjaquith and @anton-vlasenko for your review and thoughts on this implementation.

Forced pushed to rebase on trunk. Navigation Puppeteer e2e tests keep timing out. Rebased to bring in any changes that may have been committed since this PR was created.

Are any plugins extending WP_List_Table using compat_fields property? Yes.

Plugins with > 5M installs:

• ​Jetpack's `Automattic\Jetpack\Assets\Jetpack_Modules_List_Table` adds all_items dynamic property to the compat_fields property .

Plugins with <= 4k installs:

• APCu Manager
• WPJAM Basic
• DecaLog
• OPcache Manager
• oEmbed Manager
• JP Omnisearch
• MailArchiver

While Jetpack can be fixed, users upgrading to 6.4 without first upgrading the plugin will experience a Undefined property Warning on PHP 8 or Notice on PHP 7 (likely in the server logs) ​https://3v4l.org/8djK8.

I think Option 3 is not the best option :(

There are 2 Core commits in this PR:

1. Start the tests from a known starting state.
2. Tests added for the magic methods.

[hellofromTonya]

​PR 4905 is ready for commit.

It's Option 1 but deprecates instead of using _doing_it_wrong(), i.e. to keep parity with the PHP 8.2 deprecation and provide feedback to extenders to define properties on their child class instead of using dynamic properties.

I'll prepare the commit shortly.

Also added php-compatibility focus to identify this ticket resolves an incompatibility that PHP 8.2 first introduced.

[hellofromTonya]

Committer note: This ticket was originally part of #56876. The props list needs to also include contributors from #56876.

[hellofromTonya]

In 56348:

Tests: Fix leakage in WP_List_Table tests.

Fixes WP_List_table tests leaking into other tests by:

• Restores the original $hook_suffix global value.

Rather than modifying the global for all tests, it now restores the original value between tests. Why? To ensure each test starts at a known state.

• Uses a new instance of WP_List_Table for each test.

A test may modify the $list_table object. If it does, it could impact tests yet to run. By instantiating a new instance in the set_up() test fixture, each test is isolated from the others.

Follow-up to [53868], [54215].

Props hellofromTonya, antonvlasenko.
See #58955, #58896.

I committed the test leakage fix in https://core.trac.wordpress.org/changeset/56348. Need to rebase and then fix merge conflicts for this PR (i.e. as it also fixed the problem). Doing that now.

There are 2 Core commits in this PR:

1. Start the tests from a known starting state ​https://github.com/WordPress/wordpress-develop/pull/4905/commits/4e6fa3653e43f0323a330b75112f03bd1458c2e8.
2. Tests added for the magic methods.

[hellofromTonya]

In 56349:

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:

• 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 ​live open public working session where these changes were discussed and agreed to.
• ​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.

Committed via https://core.trac.wordpress.org/changeset/56349.

[hellofromTonya]

Thank you everyone for your contributions!

[hellofromTonya]

Reopening to improve the deprecation message. I think the word "define" should be changed to "declare".

Why is "declare" better?
It aligns well to the topic of dynamic properties and the act of explicitly listing the variable as a property on the class.

The goal of this message is guide developers to change their code. Changing the term to "declare" hopefully will aid in the understanding of what is being asked of developers when this deprecation is thrown.

For example, if the developer does "dynamic properties" search, they'll likely be lead to these resources:

• The ​RFC for deprecating dynamic properties, which defines a dynamic property in PHP as:

“Dynamic property” here refers to a property that has not been declared in the class.

• ​php.watch's explanation of the dynamic properties deprecation.

It's also worth noting that both terms are used in the ​PHP manual's OOP Property page.

I'll follow-up with PR and commit shortly.

Improves the magic method deprecation message by changing from "define" to "declare".

Reasoning provided here in Trac ticket.

Trac ticket: https://core.trac.wordpress.org/ticket/58896

[hellofromTonya]

In 56356:

Code Modernization: Use "declare" in WP_List_Table magic methods deprecation message

Changes "define" to "declare" in the deprecation message in WP_List_Table magic methods.

Why is "declare" better?
It aligns well to:

• the topic of and published information about dynamic properties.
• the act of explicitly listing the variable as a property on the class.

The goal of this message is guide developers to change their code. Changing the term to "declare" hopefully will aid in the understanding of what is being asked of developers when this deprecation is thrown.

Follow-up [56349].

Props hellofromTonya, antonvlasenko.
Fixes #58896.

Committed via https://core.trac.wordpress.org/changeset/56356.

Replaces trigger_error() with wp_trigger_error() for the deprecation notices of dynamic properties in magic methods.

• WP_List_Table added by changeset 56349 / Trac 58896

• WP_User_Query added by changeset 56353 / Trac 58897

• WP_Text_Diff_Renderer_Table added by changeset 56354 / Trac 58898

Trac ticket: https://core.trac.wordpress.org/ticket/57686

[hellofromTonya]

In 56542:

Code Modernization: Use wp_trigger_error() in WP_List_Table magic methods.

Replaces trigger_error() with wp_trigger_error() in each of the WP_List_Table magic methods.

[56349] added the dynamic properties deprecation messages to the __get(), __set(), __isset(), __unset() magic methods. Since that commit, wp_trigger_error() was introduced (see [56530]) as a wrapper for trigger_error().

Follow-up to [56349], [56356], [56530].

See #58896, #57686.

Committed via:

• WP_List_Table https://core.trac.wordpress.org/changeset/56542
• WP_User_Query https://core.trac.wordpress.org/changeset/56543
• WP_Text_Diff_Renderer_Table https://core.trac.wordpress.org/changeset/56544