Revisions: get a deprecation error about WP_Text_Diff_Renderer_Table::__isset()

[wildworks]

When I open the post revision screen, I get a deprecation error regarding WP_Text_Diff_Renderer_Table::__isset().

Here is the full error message:

Deprecated: WP_Text_Diff_Renderer_Table::__isset(): The property `_title` is not declared. Checking `isset()` on a dynamic property is deprecated since version 6.4.0! Instead, declare the property on the class. in /var/www/src/wp-includes/functions.php on line 6083
Deprecated: WP_Text_Diff_Renderer_Table::__isset(): The property `_title_left` is not declared. Checking `isset()` on a dynamic property is deprecated since version 6.4.0! Instead, declare the property on the class. in /var/www/src/wp-includes/functions.php on line 6083
Deprecated: WP_Text_Diff_Renderer_Table::__isset(): The property `_title_right` is not declared. Checking `isset()` on a dynamic property is deprecated since version 6.4.0! Instead, declare the property on the class. in /var/www/src/wp-includes/functions.php on line 6083
Deprecated: WP_Text_Diff_Renderer_Table::__isset(): The property `_title` is not declared. Checking `isset()` on a dynamic property is deprecated since version 6.4.0! Instead, declare the property on the class. in /var/www/src/wp-includes/functions.php on line 6083
Deprecated: WP_Text_Diff_Renderer_Table::__isset(): The property `_title_left` is not declared. Checking `isset()` on a dynamic property is deprecated since version 6.4.0! Instead, declare the property on the class. in /var/www/src/wp-includes/functions.php on line 6083
Deprecated: WP_Text_Diff_Renderer_Table::__isset(): The property `_title_right` is not declared. Checking `isset()` on a dynamic property is deprecated since version 6.4.0! Instead, declare the property on the class. in /var/www/src/wp-includes/functions.php on line 6083
Deprecated: WP_Text_Diff_Renderer_Table::__isset(): The property `_title` is not declared. Checking `isset()` on a dynamic property is deprecated since version 6.4.0! Instead, declare the property on the class. in /var/www/src/wp-includes/functions.php on line 6083
Deprecated: WP_Text_Diff_Renderer_Table::__isset(): The property `_title_left` is not declared. Checking `isset()` on a dynamic property is deprecated since version 6.4.0! Instead, declare the property on the class. in /var/www/src/wp-includes/functions.php on line 6083
Deprecated: WP_Text_Diff_Renderer_Table::__isset(): The property `_title_right` is not declared. Checking `isset()` on a dynamic property is deprecated since version 6.4.0! Instead, declare the property on the class. in /var/www/src/wp-includes/functions.php on line 6083
Deprecated: WP_Text_Diff_Renderer_Table::__isset(): The property `_title` is not declared. Checking `isset()` on a dynamic property is deprecated since version 6.4.0! Instead, declare the property on the class. in /var/www/src/wp-includes/functions.php on line 6083
Deprecated: WP_Text_Diff_Renderer_Table::__isset(): The property `_title_left` is not declared. Checking `isset()` on a dynamic property is deprecated since version 6.4.0! Instead, declare the property on the class. in /var/www/src/wp-includes/functions.php on line 6083
Deprecated: WP_Text_Diff_Renderer_Table::__isset(): The property `_title_right` is not declared. Checking `isset()` on a dynamic property is deprecated since version 6.4.0! Instead, declare the property on the class. in /var/www/src/wp-includes/functions.php on line 6083

Testing Instructions

• Create a post.
• Make changes and save several times.
• Click "X Revisions" in the right sidebar.

Tested environment

Latest wordpress-develop (WordPress 6.4-alpha-56267-src)

[kafleg]

This issue is still in the 6.4 Beta 2 with Gutenberg Version 16.7.0.

Error- ​https://i.rankmath.com/i/ojuMmw

[Presskopp]

I'm absoluty unsure if this is a real fix, but adding

protected $_title;
protected $_title_left;
protected $_title_right;

under class WP_Text_Diff_Renderer_Table extends Text_Diff_Renderer {
in \wp-includes\class-wp-text-diff-renderer-table.php

at least makes the messages disappear

[nicolefurlan]

@Presskopp would you like to create a PR or patch with the fix you mentioned in #comment:5?

added 3 protected variables, so at least the warnings go away

[mukesh27]

Hi there! Thanks for ticket and PR.

@wildworks could you please check which PHP version you have used in your testing?

[56354] is it side effect of this changes? cc. @antonvlasenko @hellofromTonya

[wildworks]

I tested it again using the nightly build WordPress version (WordPress 6.4-beta2-56809).

It may not be a PHP version issue, as I have confirmed that it occurs with at least the following PHP versions.

• PHP 8.1.23
• PHP 8.0.30
• PHP 7.4.30
• PHP 7.3.5

[hellofromTonya]

Thanks for the ping.

Yes, the deprecation notices are intentional (by design) to alert of dynamic (undeclared) properties being accessed (see [56354]). These deprecation messages will not show in production, only when WP_DEBUG is turned on as of [56544].

Before declaring these properties on the class, I suggest first researching where in the source code they are being requested? Is the request coming from within Core or one of its default themes? Or is the request coming from a plugin or non-default theme?

[hellofromTonya]

I'll set myself as owner to do the research to find if / where these properties are being used within Core itself or one of the default themes.

[wildworks]

I added the debug_backtrace() function ​here and tried to find out where this method was called from. Then it showed me this line.

​https://github.com/WordPress/wordpress-develop/blob/a9bb470f8b20fb10848666942ac55b796cf6f9bc/src/wp-includes/Text/Diff/Renderer.php#L40

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

[antonvlasenko]

Test Report

Patch tested: ​https://github.com/WordPress/wordpress-develop/pull/5453

Steps to Reproduce or Test

1. Turn on WP_DEBUG and WP_SCRIPT_DEBUG.
2. Create a post.
3. Make changes and save several times.
4. Click "X Revisions" in the right sidebar.
5. 🐞Observe several errors on the page:

   Deprecated: WP_Text_Diff_Renderer_Table::__isset(): The property `_title` is not declared. Checking `isset()` on a dynamic property is deprecated since version 6.4.0! Instead, declare the property on the class.
   Deprecated: WP_Text_Diff_Renderer_Table::__isset(): The property `_title_left` is not declared. Checking `isset()` on a dynamic property is deprecated since version 6.4.0! Instead, declare the property on the class.
   Deprecated: WP_Text_Diff_Renderer_Table::__isset(): The property `_title_right` is not declared. Checking `isset()` on a dynamic property is deprecated since version 6.4.0! Instead, declare the property on the class.
   

Expected Results

When testing a patch to validate it works as expected:

• ✅ There should be no errors on the revisions page.

When reproducing a bug:

• ❌ Observe the errors mentioned above.

Environment

• WordPress: 6.4-beta2-56769-src
• PHP: 7.3.33
• Server: Apache/2.4.57 (Unix) PHP/7.3.33
• Database: mysqli (Server: 5.7.43 / Client: Unavailable)
• Browser: Safari 17.0 (macOS)
• Theme: Twenty Twenty-Three 1.2
• MU-Plugins: None activated
• Plugins: None

Actual Results

When reproducing a bug/defect:

• ❌ Errors observed on the revisions page.

When testing the bugfix patch:

• ✅ No errors on the revisions page.

[ironprogrammer]

Thank you for the patch, @antonvlasenko!

Test Report

Patch tested: ​https://github.com/WordPress/wordpress-develop/pull/5453 👍🏻

Environment

• Hardware: MacBook Pro Apple M1 Pro
• OS: macOS 13.6
• Browser: Safari 16.6
• Server: nginx/1.25.2
• PHP: 7.4.33, 8.2.11
• WordPress: 6.4-beta3-56822-src
• Theme: twentytwentythree v1.2

Actual Results

• ✅ WP_Text_Diff_Renderer_Table::__isset() deprecation notices are no longer triggered/logged.

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.

[kafleg]

Thank you for creating PR. I just checked this PR and it is working fine to me.

At least, the errors are disappeared.

Tested with WP 6.4 Beta 3

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]

Marking ​https://github.com/WordPress/wordpress-develop/pull/5453 for commit.

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

Closing in favor of ​PR 5453, which was committed via https://core.trac.wordpress.org/changeset/56938.