Fix the data dynamic property in WP_Term #4307
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
anton-vlasenko
force-pushed
the
fix/wp-term-remove-dynamic-property
branch
from
24f1e96
to
993587a
Compare
tests/phpunit/tests/term.php
Outdated
Comment on lines
314
to
389
| * @coversNothing | ||
| * | ||
| * @param stdClass $actual An instance of stdClass containing the values to initialize the WP_Term object. | ||
| * @param array $expected An array containing the expected values for the WP_Term object. | ||
| * | ||
| * @dataProvider data_get_raw_data_should_return_correct_values |
There was a problem hiding this comment.
Suggested change
| * @coversNothing | |
| * | |
| * @param stdClass $actual An instance of stdClass containing the values to initialize the WP_Term object. | |
| * @param array $expected An array containing the expected values for the WP_Term object. | |
| * | |
| * @dataProvider data_get_raw_data_should_return_correct_values | |
| * @dataProvider data_get_raw_data_should_return_correct_values | |
| * | |
| * @param stdClass $actual An instance of stdClass containing the values to initialize the WP_Term object. | |
| * @param array $expected An array containing the expected values for the WP_Term object. |
There was a problem hiding this comment.
Thank you for the review, @mukeshpanchal27.
I agree with the proposed changes; however, I don't think the @covers tag should be removed.
Fixed in 0e8df1a
anton-vlasenko
force-pushed
the
fix/wp-term-remove-dynamic-property
branch
from
e6e9171
to
6ac4bff
Compare
anton-vlasenko
added a commit
to anton-vlasenko/wordpress-develop
that referenced
this pull request
Sep 25, 2023
|
Thank you for the code review, @mukeshpanchal27. |
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.
Fix indentation. Co-authored-by: Mukesh Panchal <mukeshpanchal27@users.noreply.github.com>
anton-vlasenko
force-pushed
the
fix/wp-term-remove-dynamic-property
branch
from
caef2fc
to
4059886
Compare
|
I'm closing this PR as the solution needs more refinement. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR aims to solve the issue with the
WP_Term::$datadynamic property.The
WP_Termclass employs the__getmagic method to compute the object data.However, since PHP 8.2 does not support dynamic properties, it is better to eliminate this approach.
Trac ticket: https://core.trac.wordpress.org/ticket/58087
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.