Make WordPress Core

Opened 10 months ago

Closed 10 months ago

Last modified 10 months ago

#58943 closed enhancement (fixed)

Replace `is_a` calls in core with `instanceof`

Reported by: ayeshrajans's profile ayeshrajans Owned by: sergeybiryukov's profile SergeyBiryukov
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.


10 months ago
#1

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.

Trac ticket: https://core.trac.wordpress.org/ticket/58943#ticket

#2 @SergeyBiryukov
10 months ago

  • Milestone changed from Awaiting Review to 6.4
  • Owner set to SergeyBiryukov
  • Status changed from new to accepted

Hi there, thanks for the ticket!

Good call, this was previously done in [31188] / #25672, but apparently some new instances were added since then.

This ticket was mentioned in PR #4938 on WordPress/wordpress-develop by @rajinsharwar.


10 months ago
#3

Replacing all instances of is_a to instanceOf

Trac ticket: https://core.trac.wordpress.org/ticket/58943

#4 @rajinsharwar
10 months ago

Replaced other instances of is_a function. Do we need to update the unit tests as well @SergeyBiryukov?

#5 @ayeshrajans
10 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 @jrf
10 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 @jrf
10 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 @rajinsharwar
10 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 @jrf
10 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 @rajinsharwar
10 months ago

Sorry, I already had one opened so thought of modifying there. Closed my one. Over to you @ayeshrajans

#11 @costdev
10 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:


10 months ago
#12

Added one instance of missing is_a treatment taken from @Rajinsharwar's PR #4938.

Thank you @jrfnl @SergeyBiryukov @Rajinsharwar.

#13 @mukesh27
10 months ago

  • Version trunk deleted

Thanks @ayeshrajans for ticket and PR.

The PR 4935 got two approval.

@SergeyBiryukov could you please take final review.

#14 @mukesh27
10 months ago

  • Keywords commit added

#15 @SergeyBiryukov
10 months ago

  • Resolution set to fixed
  • Status changed from accepted to closed

In 56352:

Coding Standards: Use instanceof keyword instead of the is_a() function.

This is a micro-optimization that removes a few unnecessary function calls.

Follow-up to [31188], [34369], [38986], [41159], [43211], [43230], [44606], [45757].

Props ayeshrajans, jrf, rajinsharwar, costdev, mukesh27, SergeyBiryukov.
Fixes #58943.

@SergeyBiryukov commented on PR #4935:


10 months ago
#16

Thanks for the PR! Merged in r56352.

Note: See TracTickets for help on using tickets.