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.

Sign up for GitHub

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

Jump to bottom

Use instanceOf over property_exists. #54835

Merged
merged 2 commits into from
Sep 27, 2023
Merged

Use instanceOf over property_exists. #54835

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5aad847
Use instanceOf over property_exists.
spacedmonkey Sep 26, 2023
76b3a87
Merge branch 'trunk' into fix/instanceof
spacedmonkey Sep 27, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion lib/block-supports/border.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,7 @@ function gutenberg_apply_border_support( $block_type, $block_attributes ) {
*/
function gutenberg_has_border_feature_support( $block_type, $feature, $default_value = false ) {
// Check if all border support features have been opted into via `"__experimentalBorder": true`.
if ( property_exists( $block_type, 'supports' ) ) {
if ( $block_type instanceof WP_Block_Type ) {
$block_type_supports_border = $block_type->supports['__experimentalBorder'] ?? $default_value;
if ( true === $block_type_supports_border ) {
return true;
Expand Down
2 changes: 1 addition & 1 deletion lib/block-supports/colors.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
*/
function gutenberg_register_colors_support( $block_type ) {
$color_support = false;
if ( property_exists( $block_type, 'supports' ) ) {
if ( $block_type instanceof WP_Block_Type ) {
$color_support = $block_type->supports['color'] ?? false;
}
$has_text_colors_support = true === $color_support ||
Expand Down
4 changes: 2 additions & 2 deletions lib/block-supports/typography.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
* @param WP_Block_Type $block_type Block Type.
*/
function gutenberg_register_typography_support( $block_type ) {
if ( ! property_exists( $block_type, 'supports' ) ) {
if ( ! $block_type instanceof WP_Block_Type ) {
return;
}

Expand Down Expand Up @@ -76,7 +76,7 @@ function gutenberg_register_typography_support( $block_type ) {
* @return array Typography CSS classes and inline styles.
*/
function gutenberg_apply_typography_support( $block_type, $block_attributes ) {
if ( ! property_exists( $block_type, 'supports' ) ) {
if ( ! $block_type instanceof WP_Block_Type ) {
return array();
}

Expand Down
4 changes: 2 additions & 2 deletions lib/class-wp-duotone-gutenberg.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -649,7 +649,7 @@ private static function get_global_styles_presets( $sources ) {
private static function get_selector( $block_name ) {
$block_type = WP_Block_Type_Registry::get_instance()->get_registered( $block_name );

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.

The code does not match in WP Core

https://github.com/WordPress/wordpress-develop/blob/e83f5a1a602b86dbecbf8ee58d6b8761d53b6197/src/wp-includes/class-wp-duotone.php#L956-L958

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.

// Backwards compatibility with `supports.color.__experimentalDuotone`
// is provided via the `block_type_metadata_settings` filter. If
// `supports.filter.duotone` has not been set and the experimental
Expand Down Expand Up @@ -747,7 +747,7 @@ private static function enqueue_global_styles_preset( $filter_id, $duotone_selec
*/
public static function register_duotone_support( $block_type ) {
$has_duotone_support = false;
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.

This line doesn't seem to appear at all in WP Core

https://github.com/WordPress/wordpress-develop/blob/e83f5a1a602b86dbecbf8ee58d6b8761d53b6197/src/wp-includes/class-wp-duotone.php#L926

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?

// Previous `color.__experimentalDuotone` support flag is migrated
// to `filter.duotone` via `block_type_metadata_settings` filter.
$has_duotone_support = $block_type->supports['filter']['duotone'] ?? null;
Expand Down
Loading