PHP 8.2: Fix property typo in IXR_Message class#3777
PHP 8.2: Fix property typo in IXR_Message class#3777anomiex wants to merge 4 commits intoWordPress:trunkfrom
Conversation
The IXR_Message class declares a property `_customTag`, which is never assigned or used. It does assign to `customTag`, which outside of that one assignment is never used either. A search on wpdirectory didn't turn up anything using either property (but lots of results for other classes having a property of the same name). Since there are various other underscore-prefixed properties declared on the class, including one named `_currentTagContents` which is used in several places, it seems likely the declared property is correct and the assignment is a typo. Trac ticket: https://core.trac.wordpress.org/ticket/56033 Trac ticket: https://core.trac.wordpress.org/ticket/56009
There was a problem hiding this comment.
I agree with @anomiex's conclusions here. Looking back, this typo came in with the original inclusion of xmlrpc via r1346 https://core.trac.wordpress.org/changeset/1346
I don't see either version of the property being used anywhere, so the change is functionally trivial.
jrfnl
left a comment
jrfnl
left a comment
There was a problem hiding this comment.
Good catch! I concur with the analysis and the change.
IIRC, this was originally an external library, which also explains the lack of tests. Would it be possible to add a perfunctory test to safeguard this change ?
I.e. a minimal test which triggers the tag_open() method and which would throw the "dynamic properties are deprecated" notice on PHP 8.2 without this change and no longer does so after this change.
|
Added a test, as requested. |
jrfnl
left a comment
jrfnl
left a comment
There was a problem hiding this comment.
Thanks for adding the test @anomiex !
✅ I've checked out the branch and I have confirmed that the test covers the originally identified issue. Without the change a "Creation of dynamic property IXR_Message::$currentTag is deprecated" notice is thrown and this is fixed with the change included.
I've left a suggestion for the test name and docblock. I'm not terribly sure about the current test class/file naming rules, @SergeyBiryukov should be able to provide guidance on that.
This patch can be committed under Trac ticket #56790.
Co-authored-by: Juliette <[email protected]>
mukeshpanchal27
left a comment
mukeshpanchal27
left a comment
There was a problem hiding this comment.
Thanks @anomiex Changes looks good to me.
|
So several people have approved this, what needs to be done for it to be committed? |
|
Thanks for the PR! Merged in r55105. |
5 participants
The IXR_Message class declares a property
_currentTag, which is never assigned or used. It does assign tocurrentTag, which outside of that one assignment is never used either. A search on wpdirectory didn't turn up anything using either property (but lots of results for other classes having a property of the same name).Since there are various other underscore-prefixed properties declared on the class, including one named
_currentTagContentswhich is used in several places, it seems likely the declared property is correct and the assignment is a typo.Trac ticket: https://core.trac.wordpress.org/ticket/56790
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.