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

Fix flash when clicking on template name when a plugin registered template matches a default WP theme template #7676

Open
wants to merge 12 commits into
base: trunk
Choose a base branch
from
Open

Fix flash when clicking on template name when a plugin registered template matches a default WP theme template #7676

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
657b0a8
Fix flash when clicking on template name when a plugin registered tem…
Aljullu Oct 29, 2024
1d1af57
Fix duplicate templates between database and theme
Aljullu Nov 3, 2024
d32b25e
Make sure the custom templates with no associated post types are avai…
Aljullu Nov 3, 2024
8ab1d7a
Add unit test to make sure plugin-registered template with default po…
Aljullu Nov 3, 2024
1ce805e
Update src/wp-includes/block-template-utils.php
Aljullu Nov 8, 2024
8ef61a6
Use 'elseif' instead of two separate 'if'
Aljullu Nov 26, 2024
0cfab7c
Add an extra comment
Aljullu Nov 26, 2024
b5b24ae
Update src/wp-includes/block-template-utils.php
Aljullu Nov 27, 2024
cd07c08
Make sure '['slug__not_in']' is being updated
Aljullu Dec 4, 2024
5cea08f
Refactor if ... else ...
Aljullu Dec 5, 2024
7e48e6e
Update tests/phpunit/tests/blocks/getBlockTemplates.php
Aljullu Dec 5, 2024
b8e5c50
Fix comment format
Aljullu Dec 11, 2024
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
26 changes: 23 additions & 3 deletions src/wp-includes/block-template-utils.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1163,14 +1163,34 @@ function get_block_templates( $query = array(), $template_type = 'wp_template' )
}

if ( ! isset( $query['wp_id'] ) ) {
$template_files_query = $query;
Copy link

Choose a reason for hiding this comment

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

Is this not important for the get_block_templates filter below? Before we were updating the same instance.

Copy link
Author

Choose a reason for hiding this comment

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

Good call. I added an extra commit (cd07c08) to make sure $query['slug__not_in'] stays consistent with what we had until now.

/*
* If the query has found some user templates, those have priority
* over the theme-provided ones, so we skip querying and building them.
*/
$query['slug__not_in'] = wp_list_pluck( $query_result, 'slug' );
$template_files = _get_block_templates_files( $template_type, $query );
$template_files_query['slug__not_in'] = wp_list_pluck( $query_result, 'slug' );
/*
* We need to unset the post_type query param because some templates
* would be excluded otherwise, like `page.html` when looking for
* `page` templates. We need all templates so we can exclude duplicates
* from plugin-registered templates.
* See: https://github.com/WordPress/gutenberg/issues/65584
*/
unset( $template_files_query['post_type'] );
$template_files = _get_block_templates_files( $template_type, $template_files_query );
foreach ( $template_files as $template_file ) {
$query_result[] = _build_block_template_result_from_file( $template_file, $template_type );
if ( isset( $query['post_type'] ) && ! isset( $template_file['postTypes'] ) ) { // The custom templates with no associated post types are available for all post types.
$candidate = _build_block_template_result_from_file( $template_file, $template_type );
$default_template_types = get_default_block_template_types();
if ( ! isset( $default_template_types[ $candidate->slug ] ) ) {
$query_result[] = $candidate;
}
} elseif (
! isset( $query['post_type'] ) ||
( isset( $query['post_type'] ) && isset( $template_file['postTypes'] ) && in_array( $query['post_type'], $template_file['postTypes'], true ) )
Aljullu marked this conversation as resolved.
Show resolved Hide resolved
) {
$query_result[] = _build_block_template_result_from_file( $template_file, $template_type );
}
Copy link

Choose a reason for hiding this comment

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

This is definitely better than the one you posted minutes ago.

After checking, I agree that this is equivalent, but I'm unsure wether this is really an improvement. Because the readability in my eyes was better before with single ifs.

Adding a continue; to the first if would likely have the same result. But I'm not convinced about that either.

Maybe moving the comments, and having those better in sight will do the trick here? This would also ask for a comment at the elseif

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for the feedback! Yeah, I don't have a strong opinion, I prefer the elseif as it makes it clear there is no overlap between the two paths. I added an extra comment in 0cfab7c as you suggested. Please let me know how it looks.

Copy link
Contributor

@azaozz azaozz Nov 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

the readability in my eyes was better before with single ifs

I know this is probably getting into a "personal preference" territory, but I tend to agree with @Aljullu that if(){...} elseif(){...} makes it clearer that the two conditionals don't overlap. Also makes this code a very tiny bit faster as the elseif() may not need to be evaluated :)

}

if ( 'wp_template' === $template_type ) {
Expand Down
58 changes: 58 additions & 0 deletions tests/phpunit/tests/blocks/getBlockTemplates.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -228,4 +228,62 @@ public function data_get_block_templates_should_respect_posttypes_property() {
),
);
}

/**
* @dataProvider data_get_block_templates_should_not_leak_plugin_registered_templates_with_default_post_type_slugs
* @ticket 62319
Aljullu marked this conversation as resolved.
Show resolved Hide resolved
*
* @param string $template_slug Default slug for the post type.
* @param string $post_type Post type for query.
* @param array $expected Expected template IDs.
*/
public function test_get_block_templates_should_not_leak_plugin_registered_templates_with_default_post_type_slugs( $template_slug, $post_type, $expected ) {
$template_name = 'test-plugin//' . $template_slug;
$template_args = array(
'content' => 'Template content',
'title' => 'Test Template for ' . $post_type,
'description' => 'Description of test template',
'post_types' => array( $post_type ),
);
register_block_template( $template_name, $template_args );

$templates = get_block_templates( array( 'post_type' => $post_type ) );

$this->assertSameSets(
$expected,
$this->get_template_ids( $templates )
);

unregister_block_template( $template_name );
}

/**
* Data provider.
*
* Make sure that plugin-registered templates with default post type slugs (ie: `single` or `page`)
* don't leak into `get_block_templates()`.
* See: https://core.trac.wordpress.org/ticket/62319.
*
* @return array
*/
public function data_get_block_templates_should_not_leak_plugin_registered_templates_with_default_post_type_slugs() {
return array(
'post' => array(
'template_slug' => 'single',
'post_type' => 'post',
'expected' => array(
'block-theme//custom-hero-template',
'block-theme//custom-single-post-template',
),
),
'page' => array(
'template_slug' => 'page',
'post_type' => 'page',
'expected' => array(
'block-theme//custom-hero-template',
'block-theme//page-home',
),
),
);
}
}
Loading