Make WordPress Core

Opened 9 months ago

Closed 8 months ago

Last modified 8 months ago

#59431 closed defect (bug) (fixed)

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

Reported by: wildworks's profile wildworks Owned by: hellofromtonya's profile hellofromTonya
Milestone: 6.4 Priority: normal
Severity: minor Version: 6.4
Component: Revisions Keywords: has-patch has-testing-info commit
Focuses: Cc:

Description

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)

Change History (28)

#1 @sabernhardt
9 months ago

  • Component changed from General to Revisions

#2 @sabernhardt
9 months ago

  • Version set to trunk

#3 @kafleg
8 months ago

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

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

#4 @sabernhardt
8 months ago

  • Milestone changed from Awaiting Review to 6.4

#5 @Presskopp
8 months ago

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

Version 1, edited 8 months ago by Presskopp (previous) (next) (diff)

#6 @nicolefurlan
8 months ago

  • Keywords needs-patch added

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

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


8 months ago
#7

  • Keywords has-patch added; needs-patch removed

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

#8 @mukesh27
8 months ago

  • Keywords dev-feedback added

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

#9 @wildworks
8 months ago

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

#10 @hellofromTonya
8 months ago

  • Keywords needs-testing added; dev-feedback removed
  • Severity changed from normal to minor

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?

#11 @hellofromTonya
8 months ago

  • Owner set to hellofromTonya
  • Status changed from new to assigned

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.

#12 @wildworks
8 months ago

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


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

#14 @antonvlasenko
8 months ago

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.

#15 @ironprogrammer
8 months ago

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.
Last edited 8 months ago by ironprogrammer (previous) (diff)

@mukesh27 commented on PR #5453:


8 months ago
#16

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.

#17 @kafleg
8 months ago

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

@antonvlasenko commented on PR #5453:


8 months ago
#18

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:


8 months ago
#19

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:


8 months ago
#20

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:


8 months ago
#21

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


8 months ago
#22

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


8 months ago
#23

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:


8 months ago
#24

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.

#25 @hellofromTonya
8 months ago

  • Keywords has-testing-info commit added; needs-testing removed

#26 @hellofromTonya
8 months ago

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

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.

@hellofromTonya commented on PR #5444:


8 months ago
#28

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

Note: See TracTickets for help on using tickets.