Make WordPress Core

Opened 10 months ago

Closed 10 months ago

Last modified 9 months ago

#58896 closed defect (bug) (fixed)

Fix WP_List_Table magic methods for PHP 8.2 dynamic properties

Reported by: antonvlasenko's profile antonvlasenko Owned by: hellofromtonya's profile hellofromTonya
Milestone: 6.4 Priority: normal
Severity: normal Version: 4.0
Component: Administration Keywords: php82 has-unit-tests commit has-patch
Focuses: php-compatibility Cc:

Description (last modified by hellofromTonya)

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.
  1. Option 2: Add each compat_fields as a public property and remove the magic methods.

Props to @jrf for identifying the issue.

References:

Change History (32)

#1 follow-up: @hellofromTonya
10 months ago

@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?

#2 @hellofromTonya
10 months ago

  • Description modified (diff)
  • Version set to 4.0

#3 @hellofromTonya
10 months ago

More context:

Original design before the magic methods

During 3.0, [15491] / #14579 added the WP_List_Table class where the "compat fields" properties were var with @access private annotation.

Changes that added the magic methods

These changes were made for backwards-compatibility (BC), as the properties were public (via var) and thus might have been used outside of WordPress Core.

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.
Last edited 10 months ago by hellofromTonya (previous) (diff)

This ticket was mentioned in PR #4905 on WordPress/wordpress-develop by @antonvlasenko.


10 months ago
#4

  • Keywords has-patch has-unit-tests added; needs-patch needs-unit-tests removed

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

#5 in reply to: ↑ 1 ; follow-up: @antonvlasenko
10 months ago

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.

Last edited 10 months ago by antonvlasenko (previous) (diff)

#6 in reply to: ↑ 5 @hellofromTonya
10 months ago

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.

#7 @hellofromTonya
10 months ago

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 ❌

#8 @hellofromTonya
10 months ago

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

#9 @hellofromTonya
10 months ago

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 ✅

#10 @hellofromTonya
10 months ago

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.

#11 @hellofromTonya
10 months ago

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.

#12 @hellofromTonya
10 months ago

  • Keywords changes-requested added

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.

This ticket was mentioned in PR #4924 on WordPress/wordpress-develop by @hellofromTonya.


10 months ago
#13

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

@hellofromTonya commented on PR #4924:


10 months ago
#14

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

@hellofromTonya commented on PR #4924:


10 months ago
#15

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.

@hellofromTonya commented on PR #4924:


10 months ago
#16

Are any plugins extending WP_List_Table using compat_fields property? Yes.

Plugins with > 5M installs:

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 :(

@hellofromTonya commented on PR #4905:


10 months ago
#17

There are 2 Core commits in this PR:

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

#18 @hellofromTonya
10 months ago

  • Focuses php-compatibility added
  • Keywords commit added; changes-requested removed

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.

#19 @hellofromTonya
10 months ago

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

#20 @hellofromTonya
10 months ago

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.

@hellofromTonya commented on PR #4905:


10 months ago
#21

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.

@hellofromTonya commented on PR #4905:


10 months ago
#22

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.

#23 @hellofromTonya
10 months ago

  • Resolution set to fixed
  • Status changed from assigned to closed

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:

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.

#25 @hellofromTonya
10 months ago

Thank you everyone for your contributions!

#26 @hellofromTonya
10 months ago

  • Keywords needs-patch added; has-patch removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

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:

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

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.

This ticket was mentioned in PR #4963 on WordPress/wordpress-develop by @hellofromTonya.


10 months ago
#27

  • Keywords has-patch added; needs-patch removed

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

#28 @hellofromTonya
10 months ago

  • Resolution set to fixed
  • Status changed from reopened to closed

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.

This ticket was mentioned in PR #5169 on WordPress/wordpress-develop by @hellofromTonya.


9 months ago
#30

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

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

#31 @hellofromTonya
9 months ago

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.

Note: See TracTickets for help on using tickets.