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

Plugin Block Template editor inconsistency #65584

Closed
2 tasks done
widoz opened this issue Sep 23, 2024 · 21 comments · Fixed by #66359
Closed
2 tasks done

Plugin Block Template editor inconsistency #65584

widoz opened this issue Sep 23, 2024 · 21 comments · Fixed by #66359
Assignees
Labels
[Feature] Templates API Related to API powering block template functionality in the Site Editor [Status] In Progress Tracking issues with work in progress [Type] Bug An existing feature does not function as intended

Comments

@widoz
Copy link
Contributor

widoz commented Sep 23, 2024

Description

I've noticed that registering a block template with a slug matching exactly page even if not applied (so the meta _wp_page_template does not exist) will be applied if and only if I click on the Template within the editor.

The template is applied only to the Editor.

Below is the code I've used

namespace Plugin\Templates;

function load_template(string $templateName): string {
	ob_start();
	include plugin_dir_path(__FILE__) . '/templates/' . $templateName . '.php';
	return ob_get_clean();
}

add_action( 'init', static function() {
	wp_register_block_template(
		'templates//page',
		[
			'title' => 'My Single Page',
			'description' => 'This is my single template',
			'content' => load_template('page'),
			'post_types' => [ 'page' ],
		]
	);
} );

Step-by-step reproduction instructions

  1. Register a new block template using a slug including '//page' as part of the template name
  2. Open the Sample Page Edit page
  3. On Page Panel, click Pages near the right of the Template label

Screenshots, screen recording, code snippet

Screencast.from.2024-09-23.23-41-27.webm

Environment info

WordPress Version: 6.7-alpha-59076
Gutenberg Version: 19.3.0-rc.1

Please confirm that you have searched existing issues in the repo.

  • Yes

Please confirm that you have tested with all plugins deactivated except Gutenberg.

  • Yes
@widoz widoz added the [Type] Bug An existing feature does not function as intended label Sep 23, 2024
@ndiego ndiego added the [Feature] Templates API Related to API powering block template functionality in the Site Editor label Sep 25, 2024
@colorful-tones
Copy link
Member

Thanks for reporting this issue. However, I'm not certain that it is a bug, or merely how the API is designed?

With the example code above it seems you would be trying to overwrite the page.php. However, the wp_register_block_template API is to allow plugins to register custom templates, and not overwrite existing or default WP template files.

If we look at this check: https://github.com/WordPress/wordpress-develop/blob/292769dde95d7fec3b4f335af5b1c5cf04054db4/src/wp-includes/class-wp-block-templates-registry.php#L89

It is doing a lookup for get_default_block_template_types() (see: wp-includes/block-template-utils.php#L141), which checks against 'page' (see: wp-includes/block-template-utils.php#L163).

If my understanding is correct then maybe we could look to introduce some additional error boundary for checking against get_default_block_template_types() and consider updating the dev note for 6.7. But, I would like @Aljullu to give some perspective. I believe he helped sheperd some of this API into being.

I may be misunderstanding @widoz use case, and please forgive me if so. 😃

@colorful-tones
Copy link
Member

I meant to also tag @getdave and @kevin940726 for insight please.

@widoz
Copy link
Contributor Author

widoz commented Oct 10, 2024

With the example code above it seems you would be trying to overwrite the page.php. However, the wp_register_block_template API is to allow plugins to register custom templates, and not overwrite existing or default WP template files.

Hi @colorful-tones, They do not if the template exists in the theme but it is allowed if the theme does not provide one of them.
See #61577

@ndiego
Copy link
Member

ndiego commented Oct 11, 2024

@Aljullu since you have worked on the template registration API, would you be able to provide any insight here?

@ndiego
Copy link
Member

ndiego commented Oct 14, 2024

Since RC1 is occurring later today and there is no active PR, I think we should punt this to 6.8 or 6.7.1. Thoughts @colorful-tones @getdave @kevin940726?

@colorful-tones
Copy link
Member

Since RC1 is occurring later today and there is no active PR, I think we should punt this to 6.8 or 6.7.1.

Agree 👍

@Aljullu
Copy link
Contributor

Aljullu commented Oct 14, 2024

Thanks for opening this issue, @widoz. I'm not able to reproduce and I'm not 100% sure to understand what's going on here. A couple of questions:

  • Does the theme you use include a page template?
  • Does enabling/disabling the Gutenberg plugin makes a difference? Asking because the backend code in WP core and in Gutenberg is slightly different, so the issue might only be reproducible in GB or in WP. Never mind, the code from WP core takes precedence over the code from Gutenberg, so the issue might be there. 🤔

Also wondering if you can reproduce the issue by hardcoding the content in the code snippet, that would allow us to have simpler steps. Something like this: 🙂

add_action( 'init', static function() {
	wp_register_block_template(
		'templates//page',
		[
			'title' => 'My Single Page',
			'description' => 'This is my single template',
			'content' => '<!-- wp:paragraph --><p>This is a plugin-registered template.</p><!-- /wp:paragraph -->',
			'post_types' => [ 'page' ],
		]
	);
} );

However, the wp_register_block_template API is to allow plugins to register custom templates, and not overwrite existing or default WP template files.

In fact, the goal of the API is to allow registering any template, even default WP ones. However, as @widoz mentions, theme templates take priority over plugin-registered templates. So if the theme includes a page.html template, the plugin-registered one shouldn't be available anywhere, as the theme template is considered to "override" the plugin one.

@colorful-tones
Copy link
Member

Also wondering if you can reproduce the issue by hardcoding the content in the code snippet

I wondered about this too. Good call out for simplifying the recreation steps. 👍

@ndiego ndiego moved this from 📥 Todo to 🗣️ In Discussion / Needs Decision in WordPress 6.7 Editor Tasks Oct 14, 2024
@colorful-tones
Copy link
Member

@widoz do you mind clarifying these items please:

  • Does the theme you use include a page template (page.html)?
  • Can reproduce the issue with the following simplified code?
add_action( 'init', static function() {
	wp_register_block_template(
		'templates//page',
		[
			'title' => 'My Single Page',
			'description' => 'This is my single template',
			'content' => '<!-- wp:paragraph --><p>This is a plugin-registered template.</p><!-- /wp:paragraph -->',
			'post_types' => [ 'page' ],
		]
	);
} );

@widoz
Copy link
Contributor Author

widoz commented Oct 18, 2024

Hi everyone, sorry about the late reply, I'll try to answer as best as I can

@Aljullu
Here is my Gutenberg fork I use for study, the templates plugin is located at https://github.com/widoz/gutenberg/tree/my-trunk/development/plugins/templates . I use twentytwentyfour as a theme for my local env.

Does the theme you use include a page template?

Yes

Also wondering if you can reproduce the issue by hardcoding the content in the code snippet

Unfortunately, I cannot replicate the problem anymore, at least not the way I was able to replicate it the first time (see first video). Now as you can see from the video below, the Template name appears for less than a second. It's not persistent anymore.

No differences in using a load_template or a static content.

Screencast.from.2024-10-18.23-31-44.webm

Unfortunately I had problems with my local setup, and I had to destroy everything and I've also updated to the latest changes in the plugin, therefore my current installation is no longer the same of the one I had when I was experiencing the issue.

cc @colorful-tones

@ndiego
Copy link
Member

ndiego commented Oct 21, 2024

Thanks for the additional info @widoz. I was finally able to replicate this. As you mentioned, the issue is no longer persistent, but I was able to capture the flash in the video below.

custom-template-bug.mp4

Steps to replicate:

  • Set up a local site with 6.7 Beta/RC installed
  • Add the following code via a plugin for the functions.php of the current theme
add_action( 'init', function() {
	register_block_template(
		'templates//page',
		[
			'title' => 'My Single Page',
			'description' => 'This is my single template',
			'content' => '<!-- wp:paragraph --><p>This is a plugin-registered template.</p><!-- /wp:paragraph -->',
			'post_types' => [ 'page' ],
		]
	);
} );
  • Navigate to a new page
  • The Template setting should be set to "Pages"
  • Click on "Pages" and notice the flash.
  • Update the code by replacing templates//page with something like templates//testing
  • Confirm that the flash no longer occurs when clicking "Pages"

cc @Aljullu

@ndiego ndiego moved this from 🗣️ In Discussion / Needs Decision to 📥 Todo in WordPress 6.7 Editor Tasks Oct 21, 2024
@getdave
Copy link
Contributor

getdave commented Oct 22, 2024

Screenshots to show the difference:

Before flash

Image

After flash

Image

@ndiego
Copy link
Member

ndiego commented Oct 22, 2024

The fact that "Swap template" takes a second to load in is not an issue. It happens with or without a registered template.

The issue is that the name of the registered template "My Single Page" displays briefly. Here's a screenshot from the video above.

Image

My understanding is that if you register a template that is already part of the theme, the theme template takes precedence. At least that's what's noted in the PR. In the case of the example, the //page template is already included in TT5, so the one registered with the code snippet should not appear. It seems like the Editor defaults to the newly registered version, then realizes the theme template should be used instead, hence the flash.

If this is just a visual bug, I don't think it's high priority. However, I do want to ensure it is just a visual bug and not something more.

@Aljullu
Copy link
Contributor

Aljullu commented Oct 22, 2024

Thanks for your reporting the issue and your investigation trying to reproduce this, I was able to reproduce too.

My understanding is that if you register a template that is already part of the theme, the theme template takes precedence. At least that's what's noted in the PR. In the case of the example, the //page template is already included in TT5, so the one registered with the code snippet should not appear. It seems like the Editor defaults to the newly registered version, then realizes the theme template should be used instead, hence the flash.

If this is just a visual bug, I don't think it's high priority. However, I do want to ensure it is just a visual bug and not something more.

Yes, this is my perception as well. There are two requests to the /templates endpoint and for some reason the first one isn't applying the priorities correctly: it returns the page template registered by the plugin instead of the one by the theme. Given that there is a second call right after which overrides the first one and returns the correct values, I think this is only a visual issue.

I will investigate it a bit more this week and hopefully come up with a fix, but I agree that this seems to be only a visual bug, it doesn't seem to affect the functionality of the editor or the frontend.

@ndiego
Copy link
Member

ndiego commented Oct 22, 2024

Fantastic, thanks for the confirmation @Aljullu 🙏

Given that there is a second call right after which overrides the first one and returns the correct values, I think this is only a visual issue.

Given this, I recommend we punt to 6.7.1 or 6.8. Thoughts @getdave @colorful-tones?

@Aljullu
Copy link
Contributor

Aljullu commented Oct 23, 2024

I have been investigating this a bit more, the issue seems to be that _get_block_templates_files() (_gutenberg_get_block_templates_files() in Gutenberg) doesn't return the page template from the theme when $query['post_type'] is page. I guess the reason is because themes don't need to specify the page post type for templates which already have the page slug, and that's why the template is missed by the function. To me this looks like a bug in _get_block_templates_files(), but I acknowledge I might be missing some context.

In any case, I created a PR to fix this in Gutenberg: #66359. @ndiego I would appreciate it if you can take a look or ping somebody to take a look. I can then create the PR in WordPress core, but I want to make sure this is the correct approach, as I'm not very familiar with this logic.

@ndiego
Copy link
Member

ndiego commented Oct 23, 2024

Thanks for the ping @Aljullu, I am not familiar with this either 😅 @getdave would you be able to take a look or know who to ping?

@widoz
Copy link
Contributor Author

widoz commented Oct 23, 2024

Hi @Aljullu and @ndiego,

I was trying to debug a bit the issue based on the @Aljullu comment, I can say the problem is due to one of the two rest calls made when clicking on the BlockThemeControl dropdown button, the call including the post type will include the template//page because due to the query the _gutenberg_get_block_templates_files do not return a collection including the theme page template.

The solution used in #66359 might not be practicable, for two main reasons,

  1. The issue is not only related to the page post type. Indeed, I can replicate it with post too (see example below).
register_block_template(
	'templates//single',
	[
		'title' => 'My Single Page',
		'description' => 'This is template 1',
		'content' => load_template('page'),
		'post_types' => [ 'post' ],
	]
);
  1. Shouldn't the _add_block_template_info include the template information for those default post-type templates like page and single? During the query where we pass the post_type=page the call to wp_get_theme_data_custom_templates obviously only returns the custom templates but we are in a context where we are merging at least "virtually" both default and custom templates.

Furthermore, I've registered a new post type, and I've noticed that having the following configuration, the template info (title) is retrieved from the one registered by the plugin while the content comes from the theme (might not be related).

register_post_type(
	'production', 
	[
		'public' => true, 
		'label' => 'Production', 
		'supports' => ['title', 'editor', 'custom-fields'], 
		'show_in_rest' => true
	]
);

register_block_template(
	'templates//single-production',
	[
		'title' => 'My Single Page',
		'description' => 'This is template 1',
		'content' => load_template('page'),
		'post_types' => [ 'production' ],
	]
);

I don't have a final thought on this to be honest, I'm still trying to understand, specifically about point two.

Hope this help a bit.

@Aljullu
Copy link
Contributor

Aljullu commented Oct 24, 2024

You're right, @widoz, the issue was not only in pages, but any other post type. Thanks for pointing it out!

I tried a different approach in #66359 which I think might be simpler and has the benefit of containing the changes in the Template registration API code, so we don't need to modify existing functions from core.

@widoz
Copy link
Contributor Author

widoz commented Oct 24, 2024

Hi @Aljullu

I see, so not passing the query will make the function _gutenberg_get_block_templates_files retrieve all templates including the non-custom ones. Cool.

Have you had a chance to look at the custom post type issue where the title gets changed with the one registered from the plugin? I've not checked yet and I was curious to know if the fix does also solve that issue. It should right? As far as I understanding the following condition is the one that will include the candidate in both cases.

if (
	! $post_type ||
	( $post_type && isset( $candidate['postTypes'] ) && in_array( $post_type, $candidate['postTypes'], true ) )
) {
	$template_files[ $template_slug ] = $candidate;
}

@Aljullu
Copy link
Contributor

Aljullu commented Oct 28, 2024

Hey there, @widoz,

I see, so not passing the query will make the function _gutenberg_get_block_templates_files retrieve all templates including the non-custom ones. Cool.

Yes, exactly!

Have you had a chance to look at the custom post type issue where the title gets changed with the one registered from the plugin?

Do you have some steps to reproduce? I'm not 100% sure to understand what you mean.

If in the theme, I create a single-production.html template and add a title to it via theme.json, the title from the theme template is honored.

For example, with this code snippet in a plugin:

add_action( 'init', function() {
	register_post_type(
		'production', 
		[
			'public' => true, 
			'label' => 'Production', 
			'supports' => ['title', 'editor', 'custom-fields'], 
			'show_in_rest' => true,
		]
	);

	register_block_template(
		'templates//single-production',
		[
			'title' => 'My Single Production Template',
			'description' => 'This is template 3',
			'content' => '<!-- wp:paragraph --><p>This is a plugin-registered template.</p><!-- /wp:paragraph -->',
			'post_types' => [ 'production' ],
		]
	);
} );

If the theme has single-production.html and in the theme.json adds a custom title:

{
	...
	"customTemplates": [
		...
		{
			"name": "single-production",
			"postTypes": ["production"],
			"title": "This is the theme single production template"
		}
	]
}

The theme title will take priority.

On the other hand, if the theme doesn't provide a title, we use the one from the plugin, but that's intentional. It means that themes can override plugin templates without the need to declare the template title in theme.json. One benefit of this is that it makes i18n easier, as the template name only needs to be translated in the plugin, and themes can simply fall back to it.

Does it make sense? I recognize it's a bit confusing, so I might have missed something. 😅

@github-project-automation github-project-automation bot moved this from 🏈 Punted to 6.8 to ✅ Done in WordPress 6.7 Editor Tasks Nov 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Templates API Related to API powering block template functionality in the Site Editor [Status] In Progress Tracking issues with work in progress [Type] Bug An existing feature does not function as intended
Projects
Status: Done
5 participants