Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace is_a calls in core with instanceof #4935

Closed
wants to merge 2 commits into from

Conversation

Ayesh
Copy link

@Ayesh Ayesh commented Jul 31, 2023

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


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.

@Ayesh Ayesh force-pushed the php-optimizations/instance-of branch from 5786196 to 3928338 Compare July 31, 2023 07:34
@@ -140,7 +140,7 @@ function twentyseventeen_edit_link() {
* @param int $id Front page section to display.
*/
function twentyseventeen_front_page_section( $partial = null, $id = 0 ) {
if ( is_a( $partial, 'WP_Customize_Partial' ) ) {
if ( $partial instanceof \WP_Customize_Partial ) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we shouldn't do the modifications for the bundled themes in this ticket.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May I ask why that is? It is part of WordPress core, and because this is not an API change, I think changing parts of core that people might extend or reuse is still fine.

If the changes were to be merged with changes in the themes, that makes we have zero is_a calls.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Ayesh Themes often support a wider range of WP versions/PHP versions, so we need to be wary of making changes to them which could cause cross-version compatibility issues, though I don't see how this change would cause such an issue as instanceof is available with the above syntax since PHP 5.0.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, apart from that, also I wanted to keep this ticket for only the changes in the core, we can do these changes in another ticket for the Bundled Themes using the Bundled Themes component, for help in tracking.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW, bundled theme files are often included in general optimizations like this, so I think it's fine to keep this change here, no need for a separate PR 👍

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.
@Ayesh Ayesh force-pushed the php-optimizations/instance-of branch from 3928338 to 6163c4e Compare July 31, 2023 11:51
Replace `is_a` with `instanceof` in `wp-includes/http.php`

Co-authored-by: Rajinsharwar <rajinsharwar@gmail.com>
@Ayesh
Copy link
Author

Ayesh commented Jul 31, 2023

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

Thank you @jrfnl @SergeyBiryukov @Rajinsharwar.

Copy link
Member

@mukeshpanchal27 mukeshpanchal27 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @Ayesh for the PR. The changes look good to me.

@SergeyBiryukov
Copy link
Member

Thanks for the PR! Merged in r56352.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants