Make WordPress Core

Opened 6 months ago

Last modified 4 days ago

#58087 assigned defect (bug)

Fix the 'data' dynamic property in WP_Term

Reported by: antonvlasenko's profile antonvlasenko Owned by: hellofromtonya's profile hellofromTonya
Milestone: 6.4 Priority: normal
Severity: minor Version: 6.3
Component: Taxonomy Keywords: php82 has-patch has-unit-tests needs-testing
Focuses: coding-standards, php-compatibility Cc:

Description

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.

Change History (15)

#1 @antonvlasenko
6 months ago

I'm working on a patch for this issue.

#2 @SergeyBiryukov
6 months ago

  • Keywords php82 added

#3 @hellofromTonya
6 months ago

  • Keywords needs-unit-tests added
  • Milestone changed from Awaiting Review to 6.3
  • Owner set to hellofromTonya
  • Status changed from new to assigned
  • Summary changed from Refactor WP_Term to ensure better compatibility with PHP 8.2 to Fix the 'data' dynamic property in WP_Term

#4 @hellofromTonya
6 months ago

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

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

Last edited 6 months ago by hellofromTonya (previous) (diff)

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


6 months ago
#5

  • Keywords has-patch has-unit-tests added; needs-unit-tests removed

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

#6 @antonvlasenko
6 months ago

  • Keywords dev-feedback removed

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.

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

Last edited 6 months ago by antonvlasenko (previous) (diff)

This ticket was mentioned in Slack in #core by chaion07. View the logs.


3 months ago

#8 @chaion07
3 months ago

  • Milestone changed from 6.3 to 6.4

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

#9 @hellofromTonya
6 weeks ago

  • Focuses php-compatibility added

Marking this ticket as a PHP 8.2 compatibility issue.

#10 @oglekler
12 days ago

  • Keywords needs-refresh added

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 🙏

This ticket was mentioned in Slack in #core by oglekler. View the logs.


9 days ago

#12 @oglekler
9 days ago

  • Keywords needs-testing added; needs-refresh removed

This ticket was discussed during bug scrub.

The PR needs testing.

Add props to @mukesh27

This ticket was mentioned in Slack in #core by oglekler. View the logs.


4 days ago

#14 @hellofromTonya
4 days ago

  • Type changed from enhancement to defect (bug)

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.

@antonvlasenko commented on PR #4307:


4 days ago
#15

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.

Note: See TracTickets for help on using tickets.