Make WordPress Core

Opened 11 months ago

Last modified 11 days ago

#58087 accepted defect (bug)

Fix the 'data' dynamic property in WP_Term

Reported by: antonvlasenko's profile antonvlasenko Owned by: hellofromtonya's profile hellofromTonya
Milestone: 6.6 Priority: normal
Severity: minor Version: 6.3
Component: Taxonomy Keywords: php82 needs-testing has-patch has-unit-tests changes-requested early
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 (29)

#1 @antonvlasenko
11 months ago

I'm working on a patch for this issue.

#2 @SergeyBiryukov
11 months ago

  • Keywords php82 added

#3 @hellofromTonya
11 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
11 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 11 months ago by hellofromTonya (previous) (diff)

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


11 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
11 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 11 months ago by antonvlasenko (previous) (diff)

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


8 months ago

#8 @chaion07
8 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
7 months ago

  • Focuses php-compatibility added

Marking this ticket as a PHP 8.2 compatibility issue.

#10 @oglekler
6 months 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.


6 months ago

#12 @oglekler
6 months 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.


5 months ago

#14 @hellofromTonya
5 months 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:


5 months 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.

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


5 months ago

#17 @nicolefurlan
5 months ago

This ticket was discussed during the 6.4 bug scrub today. @hellofromTonya will compare the patch to what was discussed during the livestream, then do a historical search through the commit chain or follow-up on the previous search.

#18 @hellofromTonya
5 months ago

  • Milestone changed from 6.4 to 6.5

I don't think my initial concern has yet been addressed. I myself have not yet done the research.

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.

With today being the last scheduled beta for 6.4, moving this to 6.5. Will follow-up to do the research to get this resolved early in 6.5.

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


4 weeks ago

#20 @chaion07
4 weeks ago

  • Keywords changes-requested added

We reviewed this ticket during a recent bug-scrub session and based on the feedback received from the team we are making the following changes:

  1. Add keyword changes-requested
  2. Check in with @hellofromTonya to see if any progress has been made or if we plan to do so to keep it in the current milestone.

Cheers!

Add props to @mikeschroder @rajinsharwar & @mukesh27

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


4 weeks ago

#22 @hellofromTonya
3 weeks ago

  • Status changed from assigned to reviewing

Update:

I am still planning to land this in 6.5.

What is left to do?
First, a deeper dive into its contextual history is needed. Working on that.

Once that's done, then a direction can be selected and/or confirmed.

#23 @hellofromTonya
3 weeks ago

This issue was discussed during a live working session with @jrf @markjaquith and me.

My apologies for not summarizing the discussion earlier when this ticket was first opened.

From that session, the thinking and next steps:

  • It's constructor work.
  • data is publicly exposed through __get() though checking isset() will always return false since __isset() was not implemented.
  • Search Core for any usages.
  • Search plugins for any usages. But the results will be imprecise and possibly misleading. ✅
  • Do a deeper dive is essential to better understand the reasoning for its design. Done comment:4
  • Consider the original reasoning against the thinking of it being constructor work.

Does the original reasoning support moving it to the constructor? Thinking through that now.

#24 @hellofromTonya
3 weeks ago

Continuation of comment:4 's contextual dive to understand its current state.

The data property was originally introduced for (see [35032] and [35269] in 4.4.0):

  • caching raw database query results in get_the_terms().
    • [37573] removed it, switching the cache to term IDs.
  • saving the updated raw data back to the database in wp_update_term().
    • This still exists today.

Fast forward to today.
The data property is a container to hold the current term data / column / fields values. But it only gets set when the dynamic property is invoked, which in Core happens in wp_update_term().

Here's its usage in wp_update_term():

$term_id = (int) $term_id;

// First, get all of the original args.
$term = get_term( $term_id, $taxonomy );

if ( is_wp_error( $term ) ) {
        return $term;
}

if ( ! $term ) {
        return new WP_Error( 'invalid_term', __( 'Empty Term.' ) );
}

$term = (array) $term->data;

// Escape data pulled from DB.
$term = wp_slash( $term );

// Merge old and new args with new args overwriting old ones.
$args = array_merge( $term, $args );

$defaults    = array(
        'alias_of'    => '',
        'description' => '',
        'parent'      => 0,
        'slug'        => '',
);
$args        = wp_parse_args( $args, $defaults );
$args        = sanitize_term( $args, $taxonomy, 'db' );
$parsed_args = $args;

Notice in the above code:

  • It gets type juggled from stdClass to an array().

Is that necessary? Could $term->to_array() be used instead?

Comparing the results, the only difference I'm seeing is data does not include the filter, whereas to_array() does.

So other than the filter property, why continue using data?

  • Notice sanitize_term() is used 2x, once in WP_Term::__get() and then after that in wp_update_term().

Why do the columns need sanitizing for 'raw' and then sanitizing for 'db'? The first 'raw' seems unnecessary in the context of only wp_update_term().

Is data property necessary today?

In Core, no, as the only usage of it could be replaced.

What about out in the wild?
What about BC?

Honestly, my current thinking:

I suspect there's no or very very few instances of that dynamic property being used. Any instance in Core can be handled internally. As the property only existed internally for caching and updating, I don't see a use case for using it outside of Core.

I'm thinking there's little risk for removing it and the __get() method, similar to removing proto in [55580].

That said, if there's any risk of BC break, the other way to go is to declare the property and initialize it in the constructor.

#25 @antonvlasenko
2 weeks ago

I see an issue with removing the use of the WP_Term::$data property from Core and removing the WP_Term::__get() magic method, as there are plugins that utilize WP_Term::$data:

  • WordPress Tag and Category Manager – AI Autotagger (60 thousand installations);
  • Connections Business Directory - (8 thousand installations)

(data from wpdirectory).

I really don't know how critical it will be that about 68 thousand WordPress sites might stop working (though the actual number affected will likely be much lower).

Therefore, the most viable solution, in my opinion, is to explicitly declare the WP_Term::$data class property and initialize it in the class constructor, as suggested by @hellofromTonya in https://core.trac.wordpress.org/ticket/58087#comment:24.

P.S. Regarding the PR I submitted earlier: I have changed my mind, and I no longer believe it is necessary to use a new public method - WP_Term::get_raw_data(), as I had proposed.
Recalculating the value of WP_Term::$data due to potential changes in the values of its constituent properties does not seem to be commonly required. I wish I had provided a more detailed explanation of my reasoning in https://core.trac.wordpress.org/ticket/58087#comment:6.

#26 @hellofromTonya
11 days ago

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

Thinking more about moving the prop to the constructor ...

How are the term's properties updated such as after an edit response (from the UI)?

Handled within wp_update_term(). What's interesting is: the properties instance of WP_Term is not updated with the latest values, but instead its original values are collected in the data dynamic property and then used for merging with the new values.

But the term properties are public and thus available for direct changes. Right now, the data dynamic property gets updated with those direct changes when wp_update_term() runs.

What's the BC risk for moving data to the constructor?

The risk only exists for a highly specific use case:

  • the data property is being directly used outside of Core.
  • and the other WP_Term properties are directly changed outside of Core.

I'm thinking the overall risk to users is very minimal to none, given wp_update_term() will no longer use data.

In comparison, what's the BC risk for removing the data property and getter?

Again it's the same highly specific use case. But it cause an error when the use case exists due to the plugin or theme directly using the data property.

Thus, I agree with @antonvlasenko and the original thinking from the live working session to move it to the constructor and get rid of the getter.

Working on an updated patch. Updated the keywords to reflect the new direction.

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


11 days ago
#27

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

Fixes the dynamic property by:

  • Declaring it on the class.
  • Deprecating it.
  • Moving its code to the constructor.
  • Removing the __get().
  • Replacing the property's usage in wp_update_term().

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

#28 @hellofromTonya
11 days ago

  • Keywords changes-requested early added
  • Milestone changed from 6.5 to 6.6
  • Status changed from reviewing to accepted

In exploring the fix strategy in PR 6163, discovered a few things that need consideration:

  • The WP_Term::filter property should not be included in wp_update_term(). Notice it's not part of the properties in data.
  • The existing tests are adding and/or testing for other dynamic properties, including link and errors. Removing the getter causes these to fail.
  • The test coverage needs review to ensure there's enough coverage to validate these changes.

This work needs more time and likely a longer soak time (i.e. early alpha) to discover potentially other scenarios especially in the wild.

Moving this to 6.6 but will continue working on to land in early alpha.

@antonvlasenko commented on PR #4307:


11 days ago
#29

I'm closing this PR as the solution needs more refinement.

Note: See TracTickets for help on using tickets.