Fix WP_Text_Diff_Renderer_Table magic methods for PHP 8.2 dynamic properties

[antonvlasenko]

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.

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]

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

Fix for WP_Text_Diff_Renderer_Table magic methods for PHP 8.2 dynamic properties

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

[rajinsharwar]

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

The force push does the following:

• Rebases this PR onto the latest trunk, i.e. just in case.
• Adds a few changes for code review against https://core.trac.wordpress.org/changeset/56353.
• Squashes the latest commits into 1 commit for deprecating the dynamic properties.

[hellofromTonya]

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.

[hellofromTonya]

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

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.

[hellofromTonya]

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:
  • ​`__isset()` in the PHP manual
  • ​`isset()` in the PHP manual
• 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:

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

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

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

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

[hellofromTonya]

Thank you everyone for your contributions!

[hellofromTonya]

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

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.

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.

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.

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?

@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):

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.

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

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.

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.

[hellofromTonya]

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.

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