Improve performance and simplify usage of block_has_support() by supporting a string#4958
Improve performance and simplify usage of block_has_support() by supporting a string#4958kt-12 wants to merge 9 commits intoWordPress:trunkfrom
block_has_support() by supporting a string#4958Conversation
mukeshpanchal27
left a comment
There was a problem hiding this comment.
Thanks @kt-12 for the PR. Solid start. Left few suggestion.
One instance is missing here https://github.com/WordPress/wordpress-develop/blob/trunk/src/wp-includes/block-supports/border.php#L25 that need to update as well. Thanks.
src/wp-includes/blocks.php
Outdated
| } else { | ||
| $block_support = isset( $block_type->supports[ $feature ] ) ? $block_type->supports[ $feature ] : $default_value; | ||
| } |
There was a problem hiding this comment.
| } else { | |
| $block_support = isset( $block_type->supports[ $feature ] ) ? $block_type->supports[ $feature ] : $default_value; | |
| } | |
| } elseif ( isset( $block_type->supports[ $feature ] ) ) { | |
| $block_support = $block_type->supports[ $feature ]; | |
| } |
The default value already set at starting of the function so i will use elseif.
There was a problem hiding this comment.
We should also add here another condition based on @swissspidy's feedback in https://core.trac.wordpress.org/ticket/58532#comment:4.
| /** | ||
| * @ticket 58532 | ||
| */ | ||
| public function test_block_has_support_string_true() { |
There was a problem hiding this comment.
Instead of having two separate tests, I will use a single test with a data provider to test different scenarios. This approach allows me to efficiently test multiple cases using different data inputs within a single test method.
/**
* Data provider for test_block_has_support_string
*/
public function data_block_has_support_string() {
return array(
array(
array(),
'color',
false,
),
array(
array(
'supports' => array(
'align' => array( 'wide', 'full' ),
'fontSize' => true,
'color' => array(
'link' => true,
'gradient' => false,
),
),
),
'align',
true,
),
array(
array(
'supports' => array(
'align' => array( 'wide', 'full' ),
'fontSize' => true,
'color' => array(
'link' => true,
'gradient' => false,
),
),
),
'fontSize',
true,
),
array(
array(
'supports' => array(
'align' => array( 'wide', 'full' ),
'fontSize' => false,
'color' => array(
'link' => true,
'gradient' => false,
),
),
),
'fontSize',
false,
),
array(
array(
'supports' => array(
'align' => array( 'wide', 'full' ),
'fontSize' => true,
'color' => array(
'link' => true,
'gradient' => false,
),
),
),
'anchor',
false,
),
);
}
/**
* @ticket 58532
*
* @dataProvider data_block_has_support_string
*
* @param array $block_data Block data.
* @param string $support Support string to check.
* @param bool $expected Expected result.
*/
public function test_block_has_support_string( $block_data, $support, $expected ) {
$this->registry->register( 'core/example', $block_data );
$block_type = $this->registry->get_registered( 'core/example' );
$has_support = block_has_support( $block_type, $support );
$this->assertEquals( $expected, $has_support );
}
There was a problem hiding this comment.
Thanks @mukeshpanchal27. This make sense, I have updated the test cases.
| if ( is_array( $feature ) ) { | ||
| $block_support = _wp_array_get( $block_type->supports, $feature, $default_value ); | ||
| } else { | ||
| $block_support = isset( $block_type->supports[ $feature ] ) ? $block_type->supports[ $feature ] : $default_value; | ||
| } |
There was a problem hiding this comment.
I think we should also check the length of the array, e.g. something like
if ( is_array( $feature ) && count( $feature ) === 1 ) {
$feature = $feature[0];
}If someone passes an array with just 1 element in it we can also short-circuit.
This way we don't necessarily have to change all existing instances in core, but can keep them as is.
And plugins and themes doing it wrong also benefit.
There was a problem hiding this comment.
+1 to this change, that said I still think it's worth changing the instances in core since it's IMO just a bit simpler to use strings, more intuitive, and a tiny bit faster to check for.
There was a problem hiding this comment.
I have included this check. Did a small rearrangement of condition to include this. w.r.t to core I have updated all the single element array to string.
felixarntz
left a comment
There was a problem hiding this comment.
Thank you @kt-12, this looks great. Only two very minor docs suggestions other than the feedback that was already provided.
src/wp-includes/blocks.php
Outdated
| } else { | ||
| $block_support = isset( $block_type->supports[ $feature ] ) ? $block_type->supports[ $feature ] : $default_value; | ||
| } |
There was a problem hiding this comment.
We should also add here another condition based on @swissspidy's feedback in https://core.trac.wordpress.org/ticket/58532#comment:4.
| if ( is_array( $feature ) ) { | ||
| $block_support = _wp_array_get( $block_type->supports, $feature, $default_value ); | ||
| } else { | ||
| $block_support = isset( $block_type->supports[ $feature ] ) ? $block_type->supports[ $feature ] : $default_value; | ||
| } |
There was a problem hiding this comment.
+1 to this change, that said I still think it's worth changing the instances in core since it's IMO just a bit simpler to use strings, more intuitive, and a tiny bit faster to check for.
mukeshpanchal27
left a comment
There was a problem hiding this comment.
Great work @kt-12,
One instance is missing here https://github.com/WordPress/wordpress-develop/blob/trunk/src/wp-includes/block-supports/border.php#L25 that need to update as well. Thanks.
|
Committed in https://core.trac.wordpress.org/changeset/56382 |
Trac ticket: https://core.trac.wordpress.org/ticket/58532
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.