Make WordPress Core

Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#55881 closed defect (bug) (fixed)

Block templates with specific postTypes that have been edited by the user can be selected for any other post type

Reported by: aljullu's profile aljullu Owned by: hellofromtonya's profile hellofromTonya
Milestone: 6.1 Priority: normal
Severity: normal Version:
Component: Editor Keywords: has-patch
Focuses: Cc:

Description

When a block template provided by a theme has been edited by the user, the postTypes argument is ignored, so the template is available for the wrong post types.

Steps to reproduce

  1. With a block theme (I used Twenty Twenty Two), create two templates and add them to theme.json:
{
	"version": 2,
	"customTemplates": [
		....
		{
			"name": "custom-single-post-template",
			"title": "Custom Single Post template",
			"postTypes": [
				"post"
			]
		},
		{
			"name": "custom-single-post-template-not-modified",
			"title": "Custom Single Post template (not modified)",
			"postTypes": [
				"post"
			]
		}
	],
	...
}
  1. Go to Appearance > Editor > Templates and make a modification to Custom Single Post template.
  2. Now, create a new page.
  3. Notice in the Template panel in the sidebar, Custom Single Post template is available while Custom Single Post template (not modified) isn't.

Expected behavior

If the user modifies a template in the Site Editor, that shouldn't affect which post types that template is available for.

More context

I think the issue is caused by `_build_block_template_result_from_post()` not setting a post_types attribute to templates returned. _build_block_template_result_from_post() is used by `get_block_template()`.

We identified that issue when working on WooCommerce templates. See woocommerce/woocommerce-blocks#6490.

Change History (18)

This ticket was mentioned in PR #3131 on WordPress/wordpress-develop by Aljullu.


3 years ago
#1

  • Keywords has-patch added

When loading a block template from post (ie: the user had made some modifications to it), WP was not setting the post_types attribute for that template. That caused templates specific to some post types to show available for all.

Trac ticket: https://core.trac.wordpress.org/ticket/55881#ticket

## Steps to test

  1. With a block theme, create two templates and add them to theme.json:

{{{JSON
{

"version": 2,
"customTemplates": [

....
{

"name": "custom-single-post-template",
"title": "Custom Single Post template",
"postTypes": [

"post"

]

},
{

"name": "custom-single-post-template-not-modified",
"title": "Custom Single Post template (not modified)",
"postTypes": [

"post"

]

}

],
...

}
}}}

  1. Go to Appearance > Editor > Templates and make a modification to _Custom Single Post template_.
  2. Now, create a new page.
  3. Verify in the Template panel in the sidebar neither _Custom Single Post template_ nor _Custom Single Post template (not modified)_ are available.

Before | After
--- | ---
https://i0.wp.com/user-images.githubusercontent.com/3616980/186490258-915e7e9e-2465-40a7-8f64-91b0b3cec3b4.png | https://i0.wp.com/user-images.githubusercontent.com/3616980/186490105-a144a1ea-27d6-4e6f-9bc2-470298431a83.png

ockham commented on PR #3131:


3 years ago
#2

Thanks a lot for your backport PR, @Aljullu!

Could you maybe add a bit of test coverage for the updated behavior to `tests/phpunit/tests/block-template-utils.php`? 😊

We already have some coverage for _build_block_template_result_from_post and get_block_templates there, so it should hopefully be fairly straight-forward to do 😄

ockham commented on PR #3131:


3 years ago
#3

Thanks a lot for your backport PR, @Aljullu!

Could you maybe add a bit of test coverage for the updated behavior to `tests/phpunit/tests/block-template-utils.php`? 😊

We already have some coverage for _build_block_template_result_from_post and get_block_templates there, so it should hopefully be fairly straight-forward to do 😄

FWIW, I've now tried the following:

{{{diff
diff --git a/tests/phpunit/data/themedir1/block-theme/theme.json b/tests/phpunit/data/themedir1/block-theme/theme.json
index 38fcb1d9dd..666340d5ab 100644
--- a/tests/phpunit/data/themedir1/block-theme/theme.json
+++ b/tests/phpunit/data/themedir1/block-theme/theme.json
@@ -59,6 +59,16 @@

{

"name": "page-home",
"title": "Homepage template"

+ },
+ {
+ "name": "custom-single-post-template",
+ "title": "Custom Single Post template",
+ "postTypes": post?
+ },
+ {
+ "name": "custom-single-post-template-not-modified",
+ "title": "Custom Single Post template (not modified)",
+ "postTypes": post?

}

],
"templateParts": [

diff --git a/tests/phpunit/tests/block-template-utils.php b/tests/phpunit/tests/block-template-utils.php
index 666edd020a..8c930eeb30 100644
--- a/tests/phpunit/tests/block-template-utils.php
+++ b/tests/phpunit/tests/block-template-utils.php
@@ -319,6 +319,11 @@ class Tests_Block_Template_Utils extends WP_UnitTestCase {

$template_ids

);
*/

+
+ Filter by post type.
+ $templates = get_block_templates( array( 'post_type' => 'post' ), 'wp_template' );
+ $template_ids = get_template_ids( $templates );
+ $this->assertSame( array( get_stylesheet() . '
' . 'custom-single-post-template' ), $template_ids );

}


/

}}}

Unfortunately, this is currently failing:

There was 1 failure:

1) Tests_Block_Template_Utils::test_get_block_templates
Failed asserting that two arrays are identical.
--- Expected
+++ Actual
@@ @@
 Array &0 (
-    0 => 'block-theme//custom-single-post-template'
+    0 => 'block-theme//my_template'
 )

I have a few theories why that might be happening; will elaborate in a follow-up comment.

ockham commented on PR #3131:


3 years ago
#4

Thanks a lot for your backport PR, @Aljullu!

Could you maybe add a bit of test coverage for the updated behavior to `tests/phpunit/tests/block-template-utils.php`? 😊

We already have some coverage for _build_block_template_result_from_post and get_block_templates there, so it should hopefully be fairly straight-forward to do 😄

FWIW, I've now tried the following:

{{{diff
diff --git a/tests/phpunit/data/themedir1/block-theme/theme.json b/tests/phpunit/data/themedir1/block-theme/theme.json
index 38fcb1d9dd..666340d5ab 100644
--- a/tests/phpunit/data/themedir1/block-theme/theme.json
+++ b/tests/phpunit/data/themedir1/block-theme/theme.json
@@ -59,6 +59,16 @@

{

"name": "page-home",
"title": "Homepage template"

+ },
+ {
+ "name": "custom-single-post-template",
+ "title": "Custom Single Post template",
+ "postTypes": post?
+ },
+ {
+ "name": "custom-single-post-template-not-modified",
+ "title": "Custom Single Post template (not modified)",
+ "postTypes": post?

}

],
"templateParts": [

diff --git a/tests/phpunit/tests/block-template-utils.php b/tests/phpunit/tests/block-template-utils.php
index 666edd020a..8c930eeb30 100644
--- a/tests/phpunit/tests/block-template-utils.php
+++ b/tests/phpunit/tests/block-template-utils.php
@@ -319,6 +319,11 @@ class Tests_Block_Template_Utils extends WP_UnitTestCase {

$template_ids

);
*/

+
+ Filter by post type.
+ $templates = get_block_templates( array( 'post_type' => 'post' ), 'wp_template' );
+ $template_ids = get_template_ids( $templates );
+ $this->assertSame( array( get_stylesheet() . '
' . 'custom-single-post-template' ), $template_ids );

}


/

}}}

Unfortunately, npm run test:php -- --group=block-templates is currently failing:

There was 1 failure:

1) Tests_Block_Template_Utils::test_get_block_templates
Failed asserting that two arrays are identical.
--- Expected
+++ Actual
@@ @@
 Array &0 (
-    0 => 'block-theme//custom-single-post-template'
+    0 => 'block-theme//my_template'
 )

I have a few theories why that might be happening; will elaborate in a follow-up comment.

ockham commented on PR #3131:


3 years ago
#5

Here's some basic unit test coverage:

{{{diff
commit 4ea8873d670bfd785e51d5414938ae02b63d960c
Author: Bernie Reiter <ockham@…>
Date: Thu Sep 8 15:44:59 2022 +0200

Add basic test coverage

diff --git a/tests/phpunit/data/themedir1/block-theme/templates/custom-single-post-template.html b/tests/phpunit/data/themedir1/block-theme/templates/custom-single-post-template.html
new file mode 100644
index 0000000000..fc0a5f8a4b
--- /dev/null
+++ b/tests/phpunit/data/themedir1/block-theme/templates/custom-single-post-template.html
@@ -0,0 +1,3 @@
+
+<p>Custom Single Post template</p>
+
diff --git a/tests/phpunit/data/themedir1/block-theme/theme.json b/tests/phpunit/data/themedir1/block-theme/theme.json
index 38fcb1d9dd..b4fc0500cc 100644
--- a/tests/phpunit/data/themedir1/block-theme/theme.json
+++ b/tests/phpunit/data/themedir1/block-theme/theme.json
@@ -59,6 +59,11 @@

{

"name": "page-home",
"title": "Homepage template"

+ },
+ {
+ "name": "custom-single-post-template",
+ "title": "Custom Single Post template",
+ "postTypes": post?

}

],
"templateParts": [

diff --git a/tests/phpunit/tests/block-template-utils.php b/tests/phpunit/tests/block-template-utils.php
index 666edd020a..1c4adc66ff 100644
--- a/tests/phpunit/tests/block-template-utils.php
+++ b/tests/phpunit/tests/block-template-utils.php
@@ -319,6 +319,27 @@ class Tests_Block_Template_Utils extends WP_UnitTestCase {

$template_ids

);
*/

+
+ Filter by post type.
+ $templates = get_block_templates( array( 'post_type' => 'post' ), 'wp_template' );
+ $template_ids = get_template_ids( $templates );
+ $this->assertSame(
+ array(
+ get_stylesheet() . '
' . 'my_template',
+ get_stylesheet() . '' . 'custom-single-post-template'
+ ),
+ $template_ids
+ );
+
+ $templates = get_block_templates( array( 'post_type' => 'page' ), 'wp_template' );
+ $template_ids = get_template_ids( $templates );
+ $this->assertSame(
+ array(
+ get_stylesheet() . '
' . 'my_template',
+ get_stylesheet() . '' . 'page-home'
+ ),
+ $template_ids
+ );

}


/

}}}

However, this is currently passing both on this branch, _and_ on trunk. This means that something is off -- it shouldn't pass on trunk.

ockham commented on PR #3131:


3 years ago
#6

The following test fails on trunk but passes on this branch:

{{{diff
commit d526a41f1cfb994ca1d2876e985b94936f83f5f7
Author: Bernie Reiter <ockham@…>
Date: Thu Sep 8 15:44:59 2022 +0200

Add basic test coverage

diff --git a/tests/phpunit/data/themedir1/block-theme/templates/custom-single-post-template.html b/tests/phpunit/data/themedir1/block-theme/templates/custom-single-post-template.html
new file mode 100644
index 0000000000..fc0a5f8a4b
--- /dev/null
+++ b/tests/phpunit/data/themedir1/block-theme/templates/custom-single-post-template.html
@@ -0,0 +1,3 @@
+
+<p>Custom Single Post template</p>
+
diff --git a/tests/phpunit/data/themedir1/block-theme/theme.json b/tests/phpunit/data/themedir1/block-theme/theme.json
index 7676d642b2..4961a620a2 100644
--- a/tests/phpunit/data/themedir1/block-theme/theme.json
+++ b/tests/phpunit/data/themedir1/block-theme/theme.json
@@ -58,6 +58,11 @@

{

"name": "page-home",
"title": "Homepage template"

+ },
+ {
+ "name": "custom-single-post-template",
+ "title": "Custom Single Post template",
+ "postTypes": post?

}

],
"templateParts": [

diff --git a/tests/phpunit/tests/block-template-utils.php b/tests/phpunit/tests/block-template-utils.php
index 666edd020a..6d16cc7260 100644
--- a/tests/phpunit/tests/block-template-utils.php
+++ b/tests/phpunit/tests/block-template-utils.php
@@ -12,6 +12,7 @@

*/

class Tests_Block_Template_Utils extends WP_UnitTestCase {

private static $post;

+ private static $custom_single_post_template_post;

private static $template_part_post;
private static $test_theme = 'block-theme';


@@ -50,6 +51,22 @@ class Tests_Block_Template_Utils extends WP_UnitTestCase {

self::$post = self::factory()->post->create_and_get( $args );
wp_set_post_terms( self::$post->ID, self::$test_theme, 'wp_theme' );


+ Set up template post.
+ $args = array(
+ 'post_type' => 'wp_template',
+ 'post_name' => 'custom-single-post-template',
+ 'post_title' => 'Custom Single Post template (modified)',
+ 'post_content' => 'Content',
+ 'post_excerpt' => 'Description of custom single post template',
+ 'tax_input' => array(
+ 'wp_theme' => array(
+ self::$test_theme,
+ ),
+ ),
+ );
+ self::$custom_single_post_template_post = self::factory()->post->create_and_get( $args );
+ wp_set_post_terms( self::$custom_single_post_template_post->ID, self::$test_theme, 'wp_theme' );
+

Set up template part post.
$template_part_args = array(

'post_type' => 'wp_template_part',

@@ -319,6 +336,27 @@ class Tests_Block_Template_Utils extends WP_UnitTestCase {

$template_ids

);
*/

+
+ Filter by post type.
+ $templates = get_block_templates( array( 'post_type' => 'post' ), 'wp_template' );
+ $template_ids = get_template_ids( $templates );
+ $this->assertSame(
+ array(
+ get_stylesheet() . '
' . 'my_template',
+ get_stylesheet() . '' . 'custom-single-post-template'
+ ),
+ $template_ids
+ );
+
+ $templates = get_block_templates( array( 'post_type' => 'page' ), 'wp_template' );
+ $template_ids = get_template_ids( $templates );
+ $this->assertSame(
+ array(
+ get_stylesheet() . '
' . 'my_template',
+ get_stylesheet() . '' . 'page-home'
+ ),
+ $template_ids
+ );

}


/

}}}

(Formatting might still be off, phpcbf isn't working for me right now.)

Run with npm run test:php -- --group=block-templates 🙁

ockham commented on PR #3131:


3 years ago
#7

Thanks for looking into this, @ockham! Do you want to push the commit yourself, or do you want me to do it? (Both work for me, just want to make sure you get proper attribution for your work 🙂 )

@Aljullu I don't think I have write access to your wordpress-develop fork, so it'd be great if you push the commit 😊 (Thanks for considering attribution, but no worries, I don't mind.)

Aljullu commented on PR #3131:


3 years ago
#8

Thanks @ockham! I applied your patch and rebased this PR. You will notice I had to make some small changes in some other tests as well, so I would appreciate if you can take a look.

ockham commented on PR #3131:


3 years ago
#9

Thanks @ockham! I applied your patch and rebased this PR. You will notice I had to make some small changes in some other tests as well, so I would appreciate if you can take a look.

Ah, thank you -- I should've run _all tests_ at least once I was done updating the block template utils one 😅

It's a bit unfortunate that we have to add the new template to the tests in [https://github.com/WordPress/wordpress-develop/pull/3131/files#diff-8d71d763873e0209e6b7435705f6b9c87390afe4ec5a2a7002c72fe0dd75b179 wpThemeJsonResolver.php], since that template isn't really of any concern to what we're testing there (translation of template title and description). I'll have a quick look if I can find a solution where we don't have to include it.

ockham commented on PR #3131:


3 years ago
#10

Here's a new patch. It keeps the check for custom-single-post-template in test_merges_child_theme_json_into_parent_theme_json where it seemed to make some sense, but removes it from test_translations_are_applied where it doesn't (and changes the assertions accordingly).

Finally, I'd forgotten to remove the template post object during teardown in block-template-utils.php, so I've also added that.

{{{diff
diff --git a/tests/phpunit/tests/block-template-utils.php b/tests/phpunit/tests/block-template-utils.php
index 1f034fdc78..340bbfff4f 100644
--- a/tests/phpunit/tests/block-template-utils.php
+++ b/tests/phpunit/tests/block-template-utils.php
@@ -95,6 +95,7 @@ class Tests_Block_Template_Utils extends WP_UnitTestCase {

public static function wpTearDownAfterClass() {

wp_delete_post( self::$post->ID );

+ wp_delete_post( self::$custom_single_post_template_post->ID );

}


public function test_build_block_template_result_from_post() {

diff --git a/tests/phpunit/tests/theme/wpThemeJsonResolver.php b/tests/phpunit/tests/theme/wpThemeJsonResolver.php
index 7aabc1c599..a8a93f9b23 100644
--- a/tests/phpunit/tests/theme/wpThemeJsonResolver.php
+++ b/tests/phpunit/tests/theme/wpThemeJsonResolver.php
@@ -154,18 +154,15 @@ class Tests_Theme_wpThemeJsonResolver extends WP_UnitTestCase {

),
$theme_data->get_settings()

);

  • $this->assertSameSets(

+
+ $custom_templates = $theme_data->get_custom_templates();
+ $this->assertArrayHasKey( 'page-home', $custom_templates );
+ $this->assertSame(
+ $custom_templatespage-home?,

array(

  • 'page-home' => array(
  • 'title' => 'Szablon strony głównej',
  • 'postTypes' => array( 'page' ),
  • ),
  • 'custom-single-post-template' => array(
  • 'title' => 'Custom Single Post template',
  • 'postTypes' => array( 'post' ),
  • ),
  • ),
  • $theme_data->get_custom_templates()

+ 'title' => 'Szablon strony głównej',
+ 'postTypes' => array( 'page' ),
+ )

);
$this->assertSameSets(

array(

}}}

Aljullu commented on PR #3131:


3 years ago
#11

New patch applied. Thanks for following-up on this, @ockham!

#12 @hellofromTonya
3 years ago

  • Milestone changed from Awaiting Review to 6.1
  • Owner set to hellofromTonya
  • Status changed from new to reviewing

I can reproduce the reported issue. Moving the ticket into 6.1.

There is a patch ready for review, testing, and commit consideration. I'll work on that next.

hellofromtonya commented on PR #3131:


3 years ago
#13

I can confirm that the PR fixes the reported issue. However, the tests are not validating the issue. Why? Without the fix applied, the tests should fail. Then applying the fix, the tests should pass. However, currently the tests are passing without the fix applied.

I'm looking at the tests now.

hellofromtonya commented on PR #3131:


3 years ago
#14

I can confirm that the PR fixes the reported issue. However, the tests are not validating the issue. Why? Without the fix applied, the tests should fail. Then applying the fix, the tests should pass. However, currently the tests are passing without the fix applied.

I'm looking at the tests now.

#15 @hellofromTonya
3 years ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

In 54184:

Editor: Fix get_block_templates() to get templates for a post type.

When a post type is passed to get_block_templates() in the query, return only the templates that match that post type.

Fixes an issue where:

  • when a block template provided by a theme has been edited by the user
  • and that template has specific defined postTypes
  • but after editing, the template was available for all post types.

Follow-up to [52062].

Props aljullu, bernhard-reiter, hellofromTonya.
Fixes #55881.

hellofromtonya commented on PR #3131:


3 years ago
#16

Great job @Aljullu and @ockham 👏 Thank you for your contributions! This PR has been committed via changeset https://core.trac.wordpress.org/changeset/54184.

#17 @SergeyBiryukov
3 years ago

In 54187:

Tests: Update the test for respecting the post type in get_block_templates().

  • Use assertSameSets() to avoid a failure when array items are returned in different order (example).
  • Move the test to a more appropriate place now that the function has its own test class.
  • Rename the test method to match the function name.

Follow-up to [52062], [53927], [54184].

See #55881.

#18 @desrosj
3 years ago

In 54210:

Coding Standards: Various alignment fixes from composer format.

Follow up to [53874], [54097], [54110], [54155], [54162], [54184].

See #39210, #55443, #56288, #56092, #56408, #56467, #55881.

Note: See TracTickets for help on using tickets.