Ensure WP_Text_Diff_Renderer_Table is PHP 8.2 compatible

[anton-vlasenko]

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

---

This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

[anton-vlasenko]

I intentionally didn't initialize these class properties because they used to be set to NULL in the previous implementation. This is to maintain BC.

[hellofromtonya]

Yes, I agree, the original default value of null is being maintained.

[mukeshpanchal27]

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.

[anton-vlasenko]

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 73ebb73 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]

Core is using these 3 properties, which prior to this patch, were dynamic properties. With the changes to the magic methods, dynamic properties are deprecated for all PHP versions. Explicitly declaring these properties on the class is a follow-up to that scope of work and is needed.

Changes LTGM 👍 Ready for commit.

[hellofromtonya]

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]

I first reviewed this patch thinking - yes, let's add the properties as declared. But then looking at r56354 I remember the changes implemented uses the $compat_fields property to access these "declared" properties (compatible fields) through 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?

[anton-vlasenko]

@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 probably 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]

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

[anton-vlasenko]

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]

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]

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