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.

[nicolefurlan]

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.

[hellofromTonya]

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.

[chaion07]

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

[hellofromTonya]

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.

[hellofromTonya]

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.

[hellofromTonya]

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.

[antonvlasenko]

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.

[hellofromTonya]

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.

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

[hellofromTonya]

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.

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

[hellofromTonya]

WP_Term's data property's design is on purpose. By design, it's value is initially initialized / set each time when getting the data property.

This design for late / lazily init / set of known, named dynamic properties exist elsewhere in Core, such as in WP_Block::attributes.

As it's by design, a new strategy is needed. It can't be initialized in the constructor. While adding a new getter method could work too, a more generic approach is needed to ensure the code continues to work as it has while also fixing the isset() and getting rid of the dynamic property deprecation (and later fatals).

I opened #60875 to lay out an architectural strategy for all instances of this type of named known late init dynamic property (long name right lol).

@antonvlasenko and I have discussed this design and impacts. He's planning to adjust his PR to the new strategy. I'd suggest this new approach to be committed early in 6.6 to give a long soak time for any unknowns for further adjustments (to the strategy) if needed.

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

This PR needs additional investigation since the unit tests are failing.

[antonvlasenko]

I've submitted a new ​PR that partially implements @hellofromTonya 's proposal.
However, I decided to not add a private property to the WP_Term.
The rationale for this decision is outlined in https://core.trac.wordpress.org/ticket/60875#comment:1.
Another reason for not adding a private property was risk of breaking plugins that cast an WP_Term object to an array: ​https://www.wpdirectory.net/search/01HWR887XBHDYRN6G0E7N0SVZA
FYI, this new ​PR incorporates the suggestion mentioned in https://core.trac.wordpress.org/ticket/56034#comment:70.

I'd be grateful for any kind of feedback.

[oglekler]

This is an early ticket and at is too late for it in this cycle, so, I am moving it to the next milestone.

[hellofromTonya]

After reviewing the challenges of the proposed #60875 and what is needed to implement it, I'm of the opinion it's too complicated in relationship to handling the data dynamic property.

In the journey to figure out how to handle this particular dynamic property, we've explored various approaches and each came with a set of challenges.

I'm now of the opinion that ​PR 4307 is the right approach.

This approach:

• Adds a new public method intended for usage outside of the object within Core and extenders implementations
• Deprecates the usage of the __get() magic method to alert extenders to update their plugins and themes to use the new public method.

It does not introduce a BC break. And there are no identified issues or side-effects. After this journey, I now see it as the most simplistic approach that achieves the goals for resolving not only the dynamic properties but also removing incomplete magic methods.

I'm reopening that PR with the intent to move forward with commit during 6.7 alpha phases. Committing it early enough will give a longer soak time for testing and feedback.

Reopening this PR as IMO it's the most simplistic approach to resolve the issue and goals (see my reasonings here).

[hellofromTonya]

Discovered today: Core adds a lot of dynamic properties to WP_Term. For example:

• wp_tag_cloud() adds link and id dynamic properties.
• _make_cat_compat() adds cat_ID, category_count, category_description, cat_name, category_nicename, category_parent.

This ticket is only dealing with the data dynamic property. That no longer makes sense, given there are many other dynamic properties added and used within Core itself.

While this ticket could be repurposed, I'm thinking it might be better to close it. Why? The ticket is extensively all about the data property. I'm thinking a new ticket needs to be opened for how to deal with all of the dynamic properties Core adds / uses for WP_Term.

Reclosing this PR. Why? Discovered today that Core adds/uses a lot of dynamic properties for WP_Term. Thus, it no longer makes sense to only deal with the data property. A broader solution will be needed for all of the dynamic properties.

[hellofromTonya]

Closing this ticket in favor of #61890.

As noted in comment:41, a solution needs to consider all of the known, named dynamic properties, and not just WP_Term::$data. The scope changes. Work will continue in #61890.