Approach 2: WP_Term: Add dynamic property support. #7231
Conversation
Test using WordPress PlaygroundThe changes in this pull request can previewed and tested using a WordPress Playground instance. WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser. Some things to be aware of
For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation. |
Dynamic properties are stored in a private internal array called 'dynamic_properties', which are used within the new full set of magic methods.
hellofromtonya
force-pushed
the
try/wp_term/support-dynamic-props
branch
from
a07dbcf to
9c01706
Compare
Restores the behavior of WP_Term::to_array() to return an associative array of all the public and dynamic properties. Type casting from object to array, e.g. `(array) $term`, is different with the full set of magic methods. It creates an associative array with: * all of the declared properties (including the new private `WP_Term::$dynamic_properties` property). * does not include the dynamic properties (as they are now stored in the `WP_Term::$dynamic_properties` property). Instead of type casting, `WP_Term::to_array()` should be used. Instances in Core are replaced to use it.
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Core Committers: Use this line as a base for the props when committing in SVN: To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
I've tried to test this PR since it's the preferred solution out of the two, but the unit tests are failing. Could you please take a look, @hellofromtonya? |
hellofromtonya
force-pushed
the
try/wp_term/support-dynamic-props
branch
from
9ce3437 to
7b2ee37
Compare
@anton-vlasenko The tests that are failing are for the |
hellofromtonya
force-pushed
the
try/wp_term/support-dynamic-props
branch
from
7b2ee37 to
c8340ae
Compare
| 'what' => 'term', | ||
| 'position' => $level, | ||
| 'supplemental' => (array) $tag, | ||
| 'supplemental' => $tag->to_array(), |
There was a problem hiding this comment.
This change is fine and is necessary to keep adding tags in wp-admin working (otherwise JavaScript just silently fails as the spinner keeps spinning on the edit-tags.php page despite the tag being added), but it seems that the Customer Reviews for WooCommerce plugin may stop working if it doesn't adopt the same change. Source: https://www.wpdirectory.net/search/01J69GH2CS5JB61HFCBF8FGTQS
As noted in https://core.trac.wordpress.org/ticket/61890#comment:13, a developer note will be necessary to inform developers of this plugin, as well as other plugins that convert WP_Term objects to arrays, about the upcoming changes.
I initially avoided this approach because I was uncertain about how it would affect many of the existing plugins, some of which are very popular.
There was a problem hiding this comment.
I tested the Customer Reviews for WooCommerce plugin with WooCommerce and this patch applied. It worked with no issues.
@anton-vlasenko did it not work for you in your testing?
I'm now thinking the adoption of $term->to_array() is not mandatory.
@adrianduffell questions made me realize, there are 2 distinct use cases for converting the term object into an array:
- Use
$term->to_array()when you need an array that includes all dynamic properties. - Use
(array) $termwhen you need only the declared properties, i.e. no dynamic properties.
And yes, both of these use cases will be in a dev note for this change.
There was a problem hiding this comment.
Wait. I see the issue.
Testing it in the Reviews > Review Tagging > Create New Tag. When adding a new review tag, the issue exists where it keeps spinning and does not show the new tag. Upon refresh, the tag is there.
hellofromtonya
changed the title
hellofromtonya
force-pushed
the
try/wp_term/support-dynamic-props
branch
2 times, most recently
from
b75d44a to
2f1ccae
Compare
Splits the tests into 1 method under test per test file/class. Adds tests for __get(), __set(), __isset(), __unset(), and to_array().
|
@adrianduffell raised a valid point:
This PR (Approach 2) does not meet that expectation. In order to continue support |
2 participants
This PR implements Approach 2 to add full support to
WP_Termfor dynamic properties.Adds dynamic property support to
WP_Term.Removes the
#[AllowDynamicProperties]attribute from the class.Adds a full set of magic methods.
Adds a new private property called
$dynamic_propertiesto hold all of the dynamic properties that interact with__get()and/or__set().The magic methods work with the new
WP_Term::$dynamic_property:WP_Term::$dynamic_property.WP_Term::$dynamic_property.isset(), it usesWP_Term::$dynamic_property.WP_Term::to_array(): Adjusts and uses public and dynamic props.Restores previous behavior of
WP_Term::to_array()to return an associative array of all the public and dynamic properties. It does not include the new privateWP_Term::$dynamic_propertyproperty in the returned array.Type casting from object to array, e.g.
(array) $term, is different with the full set of magic methods. It creates an associative array with:WP_Term::$dynamic_propertiesproperty).WP_Term::$dynamic_propertiesproperty).Instead of type casting,
WP_Term::to_array()should be used. Instances in Core are replaced to use it.TODO
WP_Term::to_array().Trac ticket: https://core.trac.wordpress.org/ticket/61890
This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.