Make WordPress Core

Opened 9 months ago

Closed 8 months ago

Last modified 6 months ago

#58898 closed defect (bug) (fixed)

Fix WP_Text_Diff_Renderer_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-patch has-unit-tests commit
Focuses: php-compatibility Cc:

Description

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

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 (23)

#1 @hellofromTonya
9 months ago

Related to #58896 and #58897 as each is proposing the same 2 options to fix the class in each of those tickets.

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


9 months ago
#2

  • Keywords has-patch added; needs-patch removed

Fix for WP_Text_Diff_Renderer_Table magic methods for PHP 8.2 dynamic properties

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

#3 @rajinsharwar
9 months ago

Similarly, in this case, went with Option 1 to fix the Magic methods and added a notice or _doing_it_wrong() to each magic method

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


9 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_Text_Diff_Renderer_Table class. Dynamic properties are not compatible with PHP 8.2 and above.

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

@hellofromTonya commented on PR #4907:


8 months ago
#5

The force push does the following:

#6 @hellofromTonya
8 months ago

Contextual history of the magic methods:

During WP 4.0.0, [28525] / #22234 and #27881:

  • Added the __get(), __set(), __isset() and __unset() magic methods.
  • Added visibility keywords for properties and methods, including replacing var with protected for the 3 internal properties: _show_split_view, inline_diff_renderer, and _diff_threshold.

During WP 4.2.0, [31135] / #30891:

  • Added the $compat_fields property and set its value to an array of the 3 internal properties.
  • Added the logic in each of the property magic methods to only act on the properties in the $compat_fields list.

The magic methods should only perform operations on a whitelisted set of properties, now specified in $compat_fields.

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. But the design intent was limited to only those 3 specific properties that are listed in the $compat_fields property. The magic methods were not designed to nor did/do work on any other private, protected, or dynamic property.

#7 @hellofromTonya
8 months ago

  • Focuses php-compatibility added
  • Keywords commit added
  • Status changed from assigned to reviewing

PR 4907 is ready for commit. It has all of the changes and follows the same strategy from WP_List_Table [56349] / #58896 and WP_User_Query [56353] / #58897.

Self-assigning for commit.

@hellofromTonya commented on PR #4904:


8 months ago
#8

Hello @Rajinsharwar 👋

Thank you for contribution! The changes shifted, following the same changes done in `WP_List_Table` (https://core.trac.wordpress.org/ticket/58896) and [WP_User_Query].

I hope you don't mind, but I'm closing your PR in favor PR https://github.com/WordPress/wordpress-develop/pull/4907, as it captured and consolidated the work in one place.

#9 @hellofromTonya
8 months ago

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

In 56354:

Code Modernization: Deprecate dynamic properties in WP_Text_Diff_Renderer_Table magic methods.

The unknown use of unknown dynamic property within the WP_Text_Diff_Renderer_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_Text_Diff_Renderer_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.
  • 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 a test class with happy and unhappy paths for these changes.

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_Text_Diff_Renderer_Table::__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_Text_Diff_Renderer_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:

Follow-up to [28525], [31135].

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

#11 @hellofromTonya
8 months ago

Thank you everyone for your contributions!

#12 @hellofromTonya
7 months ago

In 56544:

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

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

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

See #58898, #57686.

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


6 months ago
#13

This PR aims to add missing class properties to ensure WP_Text_Diff_Renderer_Table is PHP 8.2 compatible.

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

@mukesh27 commented on PR #5453:


6 months ago
#14

Thank you, @anton-vlasenko, for the PR. Could you also include a brief description indicating that we added these variables for backward compatibility (BC)? This way, we can prevent someone from raising a ticket in the future to remove these variables, claiming they are unused.

@antonvlasenko commented on PR #5453:


6 months ago
#15

Thank you, @anton-vlasenko, for the PR. Could you also include a brief description indicating that we added these variables for backward compatibility (BC)?

Thank you for checking this PR, @mukeshpanchal27!
I've added more detailed descriptions to the class properties in 73ebb738b7c1c30485472604051627cdc5d7a9b6 to clarify their purpose.

This way, we can prevent someone from raising a ticket in the future to remove these variables, claiming they are unused.

If I understand you correctly, you're referring to this comment. These class properties weren't just declared to maintain BC. They were introduced to ensure compatibility with PHP 8.2. What I meant is that these properties are not assigned any value to ensure BC. In my opinion, this doesn't need to be explained in the code. But that's my opinion.

@hellofromTonya commented on PR #5453:


6 months ago
#16

To follow-up on the discussion of these properties:

Anton is right - they are not being declared on the class for BC. Rather, this patch is necessary to explicitly declare the properties that are being used in Core.

Why? Prior to this patch, the properties were dynamic and thus were accessible only through the magic methods. r56354 modified the magic methods for all WP supported PHP versions (not just PHP 8.2+) to raise a deprecation for all dynamic properties.

Core itself though should not be raising this new deprecation as it's properties should all be declared.

@hellofromTonya commented on PR #5453:


6 months ago
#17

I first reviewed this patch thinking - yes, let's add the properties as declared. But then looking at r56354 I remember the changes implemented put all declared properties into the $compat_fields property which then is used in the magic methods.

I wondering .. should these 3 properties also be added to the $compat_fields array of compatible fields, rather than declaring them on the class? In this way, the current behavior is maintained, i.e. these 3 will still use the magic methods as they always have.

@anton-vlasenko @mukeshpanchal27 what do you think?

@antonvlasenko commented on PR #5453:


6 months ago
#18

@hellofromtonya I'm looking at https://core.trac.wordpress.org/ticket/56034 and, as far as I understand, the recommended way of dealing with known, named dynamic properties is to declare them on the class.
I'm referring to this part of the ticket (highlighted text):
https://i0.wp.com/github.com/WordPress/wordpress-develop/assets/43744263/bd16bdf4-20b6-403a-a4ce-cd6798ba94aa
However, I agree that there is less risk in adding these properties to the $compat_fields array than in declaring them on the class. I need to investigate potential BC risks.

@hellofromTonya commented on PR #5453:


6 months ago
#19

@anton-vlasenko you're right. That is the recommended approach. Thanks for reminding me!

I'm curious of the previous behavior, i.e. before r56354. Let's double check before moving forward with committing this change.

@antonvlasenko commented on PR #5453:


6 months ago
#20

Thank you, @hellofromtonya.
Before https://core.trac.wordpress.org/changeset/56354, these properties were dynamic and were never set in the Text_Diff_Renderer's constructor because __isset returned false. Now, the behavior remains the same. The only difference is that these properties are now declared, so the __isset() magic method is no longer called for them.
IMO, having them declared on the class is fine. But I'm open to hearing alternative opinions.

@hellofromTonya commented on PR #5453:


6 months ago
#21

Thanks @anton-vlasenko. Testing before and after for these Core specific dynamic properties:

  • Before 6.4: they always returned null and could not be changed / set https://3v4l.org/FrRJL
  • But this PR changes that behavior by allowing them to now be set, but ... that is the plan per proposal on how to deal with "known named dynamic properties".

Known, named, dynamic property | Declare the property on the (parent) class

So the PR as it is now aligns to that plan.

#22 @hellofromTonya
6 months ago

In 56938:

Code Modernization: Declare dynamic properties on WP_Text_Diff_Renderer_Table.

Core uses 3 known, named, dynamic properties on the WP_Text_Diff_Renderer_Table class:

  • _title
  • _title_left
  • _title_right

When reviewing revisions (within the admin UI), wp_text_diff() passes the arguments (without the leading _) to a new instance, which raised deprecation notices (see [56354]).

Note: the parent class adds the leading _ to each of these properties (see [7747]).

The deprecation notices are resolved by declaring each of these known, named, dynamic properties on the class, per the #56034 mitigation strategy. These new properties are not initialized to retain their previous null behavior.

Follow-up to [56354], [23506], [7747].

Props wildworks, antonvlasenko, hellofromTonya, ironprogrammer, kafleg, mukesh27, nicolefurlan, presskopp, sabernhardt.
Fixes #59431.
See #58898, #56034.

Note: See TracTickets for help on using tickets.