Make WordPress Core

Opened 10 months ago

Closed 10 months ago

Last modified 9 months ago

#58897 closed defect (bug) (fixed)

Fix WP_User_Query magic methods for PHP 8.2 dynamic property

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

Description (last modified by hellofromTonya)

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.
  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 (15)

#1 @hellofromTonya
10 months ago

  • Description modified (diff)

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


10 months ago
#2

  • Keywords has-patch added; needs-patch removed

Fixing the magic methods of the WP_User_Query class

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

#3 @rajinsharwar
10 months ago

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 ticket was mentioned in PR #4906 on WordPress/wordpress-develop by @antonvlasenko.


10 months ago
#4

  • Keywords has-unit-tests added; 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/58897

#5 @hellofromTonya
10 months ago

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.

#6 @hellofromTonya
10 months ago

  • Keywords changes-requested added

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

@hellofromTonya commented on PR #4903:


10 months ago
#7

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.

@hellofromTonya commented on PR #4906:


10 months ago
#8

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

@antonvlasenko commented on PR #4906:


10 months ago
#9

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

#10 @hellofromTonya
10 months ago

  • Keywords commit added; changes-requested removed
  • Status changed from assigned to reviewing

PR 4906 is ready for commit.

Self-assigning for commit.

#11 @hellofromTonya
10 months ago

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

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

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.

#12 @hellofromTonya
10 months ago

Thank you everyone for your contributions!

#14 @hellofromTonya
9 months ago

  • Focuses php-compatibility added

This ticket addressed a PHP 8.2 compatibility issue.

#15 @hellofromTonya
9 months ago

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.

Note: See TracTickets for help on using tickets.