Fix the 'data' dynamic property in WP_Term

[antonvlasenko]

The WP_Term class employs the __get magic method to compute the object data.
However, since PHP 8.2 does not support dynamic properties, it is better to eliminate this approach and explicitly declare the WP_Term::data class property to store the object's data instead.

[antonvlasenko]

I'm working on a patch for this issue.

[hellofromTonya]

Some contextual information to help with determining next steps for this ticket:

• WP_Term was introduced in 4.4.0 via [34997] / #14162.

• The declared $data property and the __get magic method were also introduced in 4.4.0 via [35032] / #34262. Why? As a technique to:

  get raw data from a WP_Term object. Mirroring WP_User, we introduce a data property on term objects, which get_the_terms() uses to fetch cacheable term info.
  

• The declared data property was later removed in 4.4.0 via [35269] / #34348 to fix a data type incompatibility with WP_Ajax_Response:

  wp_ajax_add_term() fetches a term using get_term(), and passes the term to WP_Ajax_Response, which expects each of the term's properties to be scalar. Having $data as a stdClass (meant to mimic WP_User::data, populated by a get_row() database query) violated this expectation, causing fatal string conversion errors. As a workaround, $term->data is converted so that it is no longer an actual property of the term object, but is assembled only when requested in the magic __get() method.
  

Notice the following:

• Its design was introduced back in 4.4.0.
• Removing the declared property and doing the conversion in the magic method were "a workaround". A longer strategy was shared here to make WP_Ajax_Response "do something more intelligent".

Given the current design is an intentional workaround to deal with "non-scalar properties", a deeper dive is needed to better understand what's happened since 4.4.0 in the usage of the data property and the non-scalar issue. For example:

• Does this non-scalar issue still exist?
• Could the property be computed in the constructor (per its original design)?
• How is the data property being used today in Core? How does this differ from 4.4.0's design intent?

Removing the magic method and/or adding the data property onto the class should not happen until the deep dive is completed and side-effects and impacts understood.

Automated tests should also be added to document the current behavior, especially in the areas of concern. Doing so could also provide a better understanding.

This PR aims to solve the issue with the WP_Term::$data dynamic property. It has not been implemented correctly, especially considering that support for dynamic properties has been removed in PHP 8.2.

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

[antonvlasenko]

I have considered several possible solutions to the issue with WP_Term::$data.

1. Explicitly declaring the $data property and initializing it in the class constructor.

This will cause the $data property to be returned when calling foreach ($wp_term as $property_value) and (array) $wp_term (please see ​this code example).
However, adding new categories and tags on the wp-admin/edit-tags.php page still leads to fatal errors as of March 2023.

As @hellofromTonya clearly pointed out in her detailed note above, it was decided to abandon the explicit declaration of the WP_Term::$data property earlier for this very reason.

One may argue that this problem can be solved by removing the data key from the array wherever the WP_Term object is converted to an array. However, this is almost certain to introduce bugs in plugins.

Moreover, it is ​virtually impossible to track where this object-to-array conversion is used in plugins, let alone fix the associated bugs.

2. Adding the missing magic methods, such as __set(), __unset(), and __isset() to the WP_Term class to ensure proper operation of the class with this dynamic property.

This may seem like a quick fix, but it is not the best solution from an architectural point of view.

Dynamic properties should not be relied upon to obtain data about the object, and having a class method instead would be a more appropriate approach. This would avoid potential issues with dynamic properties in the future and would improve the overall design of the WP_Term class.

In light of this, I propose:

1. Implementing a new WP_Term::get_raw_data() method, which would return the same information that the WP_Term::$data property contained;
2. Deprecating the WP_Term::$data dynamic property in favour of WP_Term::get_raw_data() and removing it in one of the future WordPress releases.

​The PR I've just submitted implements the proposed changes.

Update: With that being said, I am not entirely opposed to adding the missing magic methods. If the community decides to proceed with this approach, I can submit a pull request that would implement this alternative solution.

[chaion07]

Thanks @antonvlasenko for reporting this. We reviewed this ticket during a recent bug-scrub and based on the feedback received we are updating the milestone from 6.3 to 6.4. Additionally we would urge for more discussion and examples supporting the new approach. Perhaps other volunteers and contributors can work on it to at their convenience. Cheers!

Props to @oglekler, @mikeschroder and @audrasjb

[hellofromTonya]

Marking this ticket as a PHP 8.2 compatibility issue.

[oglekler]

I tried to apply the patch, and it turned out that it needs to be refreshed (rebased).
We have about 9 days until the Beta 1, so, let's move it forward. Thank you 🙏

[oglekler]

This ticket was discussed during bug scrub.

The PR needs testing.

Add props to @mukesh27

[hellofromTonya]

I think the scope of work is a bugfix rather than an enhancement, as it's fixing an incompatibility with PHP 8.2 and beyond.

Thank you for the code review, @mukeshpanchal27.
I have resolved the merge conflict and addressed the feedback from the code review.
The PR is now ready for consideration.