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 3 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
20 changes: 19 additions & 1 deletion 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,11 +1163,19 @@ 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.
*/
$template_files_query = $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 ) {
Expand All @@ -1177,6 +1185,16 @@ function get_block_templates( $query = array(), $template_type = 'wp_template' )
) {
$query_result[] = _build_block_template_result_from_file( $template_file, $template_type );
}

// The custom templates with no associated post types are available for all post types.
if ( isset( $query['post_type'] ) && ! isset( $template_file['postTypes'] ) ) {
Copy link

@ntsekouras ntsekouras Dec 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Are these changes not needed in GB? They aren't in the original GB PR.

Copy link
Author

Choose a reason for hiding this comment

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

The main difference is that in GB we are hooking into the get_block_templates filter, in other words, we are hooking into the end of this function.

In the other hand, in WP core, we can introduce the changes inside the function. While I could have simply called _get_block_templates_files() again with post_type removed (as it's done in the GB PR), it made more sense to me to reuse the existing call to _get_block_templates_files(). It requires some extra logic (basically, the new if ... else if ..., but this way we call _get_block_templates_files() only once, which I guess is better for performance.

I'm happy to change this approach if you think overcomplicates things.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for explaining. Overall this makes sense to me, though one question: Why is the isset( $query['post_type'] ) check needed in this line here specifically?

Copy link
Author

Choose a reason for hiding this comment

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

It was to avoid entering the if clause unnecessarily (as it would go through the elseif one, which is more performant). In 5cea08f I removed this isset( $query['post_type']) and switched the order of the clauses, which hopefully makes the code easier to read.

$candidate = _build_block_template_result_from_file( $template_file, $template_type );
$default_template_types = get_default_block_template_types();
$is_custom = ! isset( $default_template_types[ $candidate->slug ] );
if ( $is_custom ) {
Aljullu marked this conversation as resolved.
Show resolved Hide resolved
$query_result[] = $candidate;
}
}
}

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