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

Use instanceOf over property_exists. #54835

Merged
merged 2 commits into from
Sep 27, 2023
Merged

Use instanceOf over property_exists. #54835

merged 2 commits into from
Sep 27, 2023

Conversation

spacedmonkey
Copy link
Member

What?

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

instanceOf is better performance than property_exists

Why?

How?

Testing Instructions

Testing Instructions for Keyboard

Screenshots or screencast

@spacedmonkey spacedmonkey self-assigned this Sep 26, 2023
@spacedmonkey spacedmonkey added the [Type] Performance Related to performance efforts label Sep 26, 2023
@github-actions
Copy link

This pull request has changed or added PHP files. Please confirm whether these changes need to be synced to WordPress Core, and therefore featured in the next release of WordPress.

If so, it is recommended to create a new Trac ticket and submit a pull request to the WordPress Core Github repository soon after this pull request is merged.

If you're unsure, you can always ask for help in the #core-editor channel in WordPress Slack.

Thank you! ❤️

View changed files
❔ lib/block-supports/border.php
❔ lib/block-supports/colors.php
❔ lib/block-supports/typography.php
❔ lib/class-wp-duotone-gutenberg.php

@gziolo
Copy link
Member

gziolo commented Sep 26, 2023

I’d be also great to verify whether charges from WordPress/wordpress-develop@e77aaf1 got backported, too.

Copy link
Contributor

@aaronrobertshaw aaronrobertshaw left a comment

Choose a reason for hiding this comment

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

Changes LGTM 👍

One thing that I did notice was the difference in files between this PR and the core patch.

Looking closer at the class-wp-duotone-gutenberg.php file it seems to differ from core.

Was there something that should have been backported from core to Gutenberg?

@aristath
Copy link
Member

One thing that I did notice was the difference in files between this PR and the core patch.
Looking closer at the class-wp-duotone-gutenberg.php file it seems to differ from core.
Was there something that should have been backported from core to Gutenberg?

One thing that is different, is that in Gutenberg we use the null coalescing operator in many places. In Core this has not been accepted yet, so where ?? is used in Gutenberg, it was necessary to use isset() checks in Core (same logic and result, just different syntax).
The ?? decision was punted for v6.5, so until then we have 2 options:

  • Continue using ?? in Gutenberg since it will be acceptable in 6.5
  • Convert ?? to isset() checks in Gutenberg, and then re-convert them to ?? in a few months

@spacedmonkey spacedmonkey enabled auto-merge (squash) September 27, 2023 08:29
@spacedmonkey
Copy link
Member Author

Looking closer at the class-wp-duotone-gutenberg.php file it seems to differ from core.

This needs to be backported. But that is out of scope this this ticket.

@spacedmonkey spacedmonkey merged commit 98b9b26 into trunk Sep 27, 2023
49 checks passed
@spacedmonkey spacedmonkey deleted the fix/instanceof branch September 27, 2023 11:12
@github-actions github-actions bot added this to the Gutenberg 16.8 milestone Sep 27, 2023
@spacedmonkey spacedmonkey added the Needs PHP backport Needs PHP backport to Core label Sep 27, 2023
@aaronrobertshaw
Copy link
Contributor

Thanks for the extra info @spacedmonkey & @aristath 👍

For the record, what caught my eye was that we had to update the duotone class here in Gutenberg but not in core as the register_duotone_support differed. I understand it is out of scope for this PR, I was only looking to find out what was missed. Which appears to be (WordPress/wordpress-develop@fc53820) if we are looking to make future backports easier.

Copy link
Contributor

@getdave getdave left a comment

Choose a reason for hiding this comment

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

Looking through this it seems all the changes to files under block-supports/ dir are backported.

Looks like this was done in WordPress/wordpress-develop#5308.

However there is a mismatch between the class-wp-duotone-* files in Gutenberg and Core.

I'm unsure as to why this is and whether anything is outstanding in terms of backports. I'd be grateful for your input here @spacedmonkey @aaronrobertshaw.

if ( property_exists( $block_type, 'supports' ) ) {
if ( $block_type instanceof WP_Block_Type ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

At a glance, it appears that the code was refactored a little in core. The check for the block type is handled via the call to block_has_support.

Copy link
Contributor

Choose a reason for hiding this comment

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

So is there a missing backport on this PR. Should we consider it done. It's very unfortunate that the code between core and Gutenberg for block supports have diverged this much.

We should either do:

  • Have the exact same code and use a package (like the block-library) sync
  • Remove the block supports from Gutenberg (consider that they're now maintained on Core)

It's probably something to do post release though.

Copy link
Contributor

Choose a reason for hiding this comment

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

So to confirm, no further action on this and it can be marked as backported?

Should we consider it done.

Can I check whether you intended to write We should consider it done?

if ( $block_type && property_exists( $block_type, 'supports' ) ) {
if ( $block_type && $block_type instanceof WP_Block_Type ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

As with the previous code difference, I think the core version needed to be backported to Gutenberg. @spacedmonkey might have further context on why it wasn't.

@getdave
Copy link
Contributor

getdave commented Feb 1, 2024

✅ I updated this PR with the Backported to Core label to indicate that the backport has successfully merged into WP Core. I also updated the PHP Sync Tracking Issue for WP 6.5.

There was some uncertainty around the block supports element but that appears to be a wider issue relating to how Gutenberg and Core have become out of sync around these files.

@getdave getdave added Backported to WP Core Pull request that has been successfully merged into WP Core and removed Needs PHP backport Needs PHP backport to Core labels Feb 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Backported to WP Core Pull request that has been successfully merged into WP Core [Type] Performance Related to performance efforts
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants