Opened 6 months ago
Last modified 4 days ago
#58087 assigned defect (bug)
Fix the 'data' dynamic property in WP_Term
Reported by: |
|
Owned by: |
|
---|---|---|---|
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)
#3
@
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
@
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. MirroringWP_User
, we introduce a data property on term objects, whichget_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 withWP_Ajax_Response
:wp_ajax_add_term()
fetches a term usingget_term()
, and passes the term toWP_Ajax_Response
, which expects each of the term's properties to be scalar. Having$data
as astdClass
(meant to mimicWP_User::data
, populated by aget_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 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
@
6 months ago
- Keywords dev-feedback removed
I have considered several possible solutions to the issue with WP_Term::$data
.
- 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.
- Adding the missing magic methods, such as
__set()
,__unset()
, and__isset()
to theWP_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:
- Implementing a new
WP_Term::get_raw_data()
method, which would return the same information that theWP_Term::$data
property contained; - Deprecating the
WP_Term::$data
dynamic property in favour ofWP_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.
This ticket was mentioned in Slack in #core by chaion07. View the logs.
3 months ago
#8
@
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
@
6 weeks ago
- Focuses php-compatibility added
Marking this ticket as a PHP 8.2 compatibility issue.
#10
@
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
@
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
@
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.
I'm working on a patch for this issue.