#58943 closed enhancement (fixed)
Replace `is_a` calls in core with `instanceof`
Reported by: |
|
Owned by: |
|
---|---|---|---|
Milestone: | 6.4 | Priority: | normal |
Severity: | trivial | Version: | |
Component: | General | Keywords: | has-patch commit |
Focuses: | performance, coding-standards | Cc: |
Description
Replaces all is_a
function calls with instanceof
keyword, which in theory should be faster, and provides more code clarity. The replacements use extra braces to enhance the clarity although they are technically not necessary.
Change History (16)
This ticket was mentioned in PR #4935 on WordPress/wordpress-develop by @ayeshrajans.
19 months ago
#1
#2
@
19 months ago
- Milestone changed from Awaiting Review to 6.4
- Owner set to SergeyBiryukov
- Status changed from new to accepted
This ticket was mentioned in PR #4938 on WordPress/wordpress-develop by @rajinsharwar.
19 months ago
#3
Replacing all instances of is_a to instanceOf
Trac ticket: https://core.trac.wordpress.org/ticket/58943
#4
@
19 months ago
Replaced other instances of is_a function. Do we need to update the unit tests as well @SergeyBiryukov?
#5
@
19 months ago
I wanted to note that the uses that PR #4938 by @rajinsharwar changes are more or less external libraries bundled with WordPress core.
#6
@
19 months ago
I agree with @ayeshrajans, external libraries used by WP should not be fixed here (though a patch upstream might be an idea if the library is still externally maintained).
Re: use in tests - it should be considered if any such uses should be replaced with an assertInstanceof()
assertion instead ? (not saying they should, just putting this out there as something to think about)
#7
@
19 months ago
FYI: I've opened a ticket to at some point create a sniff to detect & flag this, so we can safeguard this pattern for the future: https://github.com/PHPCSStandards/PHPCSExtra/issues/260
We may want to open a ticket in WPCS about this as well.
#8
@
19 months ago
Thanks for the suggestion @jrf @ayeshrajans. I reverted my change in the external libraries but kept the change for the http.php file (If I'm not mistaken, it's not an external library).
Thanks for the ticket in the PHPCSExtra repo, will follow on that.
#9
@
19 months ago
@rajinsharwar I don't understand why you opened a second PR instead of allowing @ayeshrajans to add that one extra change to his (already open) PR ?
#10
@
19 months ago
Sorry, I already had one opened so thought of modifying there. Closed my one. Over to you @ayeshrajans
#11
@
19 months ago
Re: use in tests - it should be considered if any such uses should be replaced with an
assertInstanceof()
assertion instead ? (not saying they should, just putting this out there as something to think about)
I agree that we should consider this. Using descriptive assertions makes reading tests and failure messages much clearer.
@ayeshrajans commented on PR #4935:
19 months ago
#12
Added one instance of missing is_a
treatment taken from @Rajinsharwar's PR #4938.
Thank you @jrfnl @SergeyBiryukov @Rajinsharwar.
#13
@
19 months ago
- Version trunk deleted
Thanks @ayeshrajans for ticket and PR.
The PR 4935 got two approval.
@SergeyBiryukov could you please take final review.
@SergeyBiryukov commented on PR #4935:
18 months ago
#16
Thanks for the PR! Merged in r56352.
Replaces all
is_a
function calls withinstanceof
keyword, which in theory should be faster, and provides more code clarity. The replacements use extra braces to enhance the clarity although they are technically not necessary.Trac ticket: https://core.trac.wordpress.org/ticket/58943#ticket