Fix WP_User_Query magic methods for PHP 8.2 dynamic property

[antonvlasenko]

WP_User_Query 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.

Proposed Solutions

As proposed in #58896, 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.

Fixing the magic methods of the WP_User_Query class

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

[rajinsharwar]

Tried to follow with Option 1; added a notice or _doing_it_wrong() to each magic method, added the return null in get() when attempting to get a dynamic property that is not in the compat_fields.

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/58897

[hellofromTonya]

Historical context:

Original design before the magic methods

During 3.1, [15491] / #14579:

• Added the WP_User_Query class.
• Marked its var $results and var $total_users as @access private, meaning intended for only internal usage within the object.

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.

During 4.0.0, [28528] / #27881:

• Added the _get(), __set(), __isset() and __unset() magic methods
• Added visibility keywords for properties and methods, including replacing var with private for the 2 properties.

During 4.2.0, [31144] / #30891:

• Added the list of compat_fields and the logic in each of the property magic methods.

[hellofromTonya]

The history and design intent in WP_User_Query properties and magic methods is the same as WP_List_Table, see #58896. After extensive and careful review and consideration, Option 1 with deprecations was selected and committed [56349].

I think the same approach should be used in WP_User_Query.

To expedite, let's combine the work into ​https://github.com/WordPress/wordpress-develop/pull/4906, which @antonvlasenko is in the process of updating for the changes made in [56349].

Hello @Rajinsharwar 👋 Thank you for contributing by creating this PR.

Instead of using _doing_it_wrong(), let's use the same strategy done in WP_List_Table changeset https://core.trac.wordpress.org/changeset/56349 that deprecates the magic methods for dynamic properties and fixes a couple of issues in the magic methods. It also adds happy and unhappy tests for each of the magic methods.

I hope you don't mind, but let's consolidate this work into 1 PR and collaborate together. I'll close this in favor of ​https://github.com/WordPress/wordpress-develop/pull/4906.

As we discussed, @anton-vlasenko please update this PR to use the same code strategy done in https://core.trac.wordpress.org/changeset/56349.

@hellofromtonya Could you please review this PR?
I've updated it.
e2e test are failing, but it seems to be unrelated to the changes in this PR.

[hellofromTonya]

​PR 4906 is ready for commit.

Self-assigning for commit.

[hellofromTonya]

In 56353:

Code Modernization: Deprecate dynamic properties in WP_User_Query magic methods.

The unknown use of unknown dynamic property within the WP_User_Query 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_User_Query.

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 ​per the PHP handbook.
• Fixes __isset() to return false if not in the $compat_fields, as isset() and __isset() should always return bool:
  • ​`__isset()` in the PHP manual
  • ​`isset()` in the PHP manual
• 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 as a property on the child class. When getting its value, e.g. $user_query->data, the WP_User_Query::__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_User_Query. 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, #27881, #30891.

Follow-up to [15491], [28528], [31144].

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

[hellofromTonya]

Thank you everyone for your contributions!

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

[hellofromTonya]

This ticket addressed a PHP 8.2 compatibility issue.

[hellofromTonya]

In 56543:

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

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

[56353] 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 [56353], [56530].

See #58897, #57686.