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

Aljullu · 2024-10-29T09:52:01Z

This PR includes the changes that need to be backported from WordPress/gutenberg#66359 into WC core.

Trac ticket: https://core.trac.wordpress.org/ticket/62319

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.

…plate matches a default WP theme template

github-actions · 2024-10-29T09:52:22Z

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

Core Committers: Use this line as a base for the props when committing in SVN:

Props aljullu, apermo, azaozz, ntsekouras, flixos90, antonvlasenko.

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

github-actions · 2024-10-29T10:04:36Z

Test using WordPress Playground

The changes in this pull request can previewed and tested using a WordPress Playground instance.

WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser.

Some things to be aware of

The Plugin and Theme Directories cannot be accessed within Playground.
All changes will be lost when closing a tab with a Playground instance.
All changes will be lost when refreshing the page.
A fresh instance is created each time the link below is clicked.
Every time this pull request is updated, a new ZIP file containing all changes is created. If changes are not reflected in the Playground instance,
it's possible that the most recent build failed, or has not completed. Check the list of workflow runs to be sure.

For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation.

Test this pull request with WordPress Playground.

apermo

Looks good to me

…lable for all post types

…st type slugs aren't leaked by get_block_templates()

Aljullu · 2024-11-03T12:05:15Z

Thanks for the review, @apermo! Would you mind taking another look? I added a couple of commits that fix some edge cases and add unit tests.

apermo

Looking good to me besides that one suggestion, that would make things easier to read imho.

src/wp-includes/block-template-utils.php

Co-authored-by: Christoph Daum <[email protected]>

Aljullu · 2024-11-08T13:18:38Z

Thanks for the approval, @apermo! I don't have merging rights, is that something you can take care of?

Otherwise, @azaozz, wondering if this is something you can do? Thanks in advance!

apermo · 2024-11-08T13:24:20Z

Thanks for the approval, @apermo! I don't have merging rights, is that something you can take care of?

I can't merge either, this happens on the subversion by those with commit right(core commiters), I just reviewed.

apermo · 2024-11-26T13:32:33Z

src/wp-includes/block-template-utils.php

+			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 ) )
+			) {
+				$query_result[] = _build_block_template_result_from_file( $template_file, $template_type );
+			}


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

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.

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 :)

src/wp-includes/block-template-utils.php

Co-authored-by: Andrew Ozz <[email protected]>

ntsekouras · 2024-12-04T14:20:18Z

src/wp-includes/block-template-utils.php

 		foreach ( $template_files as $template_file ) {
-			$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'] ) ) {


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

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.

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?

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.

ntsekouras · 2024-12-04T14:21:33Z

src/wp-includes/block-template-utils.php

@@ -1163,14 +1163,40 @@ function get_block_templates( $query = array(), $template_type = 'wp_template' )
 	}

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


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

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

ntsekouras · 2024-12-04T14:51:30Z

This looks good, but I'd appreciate some more eyes maybe from @anton-vlasenko or @felixarntz ?

Aljullu · 2024-12-04T15:40:53Z

Thanks for the review, @ntsekouras! I answered/applied the feedback.

felixarntz

Mostly looks good to me, though one thing I'm unsure about.

felixarntz · 2024-12-04T17:45:19Z

src/wp-includes/block-template-utils.php

 		foreach ( $template_files as $template_file ) {
-			$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'] ) ) {


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?

anton-vlasenko

Looks good to me (I only have some minor notes, please see below).

anton-vlasenko · 2024-12-04T23:22:55Z

src/wp-includes/block-template-utils.php

+					isset( $query['post_type'] ) &&
+					isset( $template_file['postTypes'] ) &&
+					in_array( $query['post_type'], $template_file['postTypes'], true )


Nitpick: It seems the isset( $query['post_type'] ) check is redundant here, as $query['post_type'] should already be defined at this point.

Suggested change

isset( $query['post_type'] ) &&

isset( $template_file['postTypes'] ) &&

in_array( $query['post_type'], $template_file['postTypes'], true )

isset( $template_file['postTypes'] ) &&

in_array( $query['post_type'], $template_file['postTypes'], true )

Good catch! I removed it in 5cea08f, after switching the order of the if ... else ... clauses, which hopefully makes the code easier to read.

tests/phpunit/tests/blocks/getBlockTemplates.php

Co-authored-by: Anton Vlasenko <[email protected]>

Fix flash when clicking on template name when a plugin registered tem…

657b0a8

…plate matches a default WP theme template

Aljullu mentioned this pull request Oct 29, 2024

Fix flash when clicking template name in the editor when a plugin registered template matches a default WP theme template WordPress/gutenberg#66359

Merged

apermo approved these changes Oct 29, 2024

View reviewed changes

Aljullu added 3 commits November 3, 2024 12:26

Fix duplicate templates between database and theme

1d1af57

Make sure the custom templates with no associated post types are avai…

d32b25e

…lable for all post types

Add unit test to make sure plugin-registered template with default po…

8ab1d7a

…st type slugs aren't leaked by get_block_templates()

Aljullu force-pushed the fix/flash-clicking-template-name branch from db4ea9b to 8ab1d7a Compare November 3, 2024 11:55

apermo reviewed Nov 7, 2024

View reviewed changes

src/wp-includes/block-template-utils.php Outdated Show resolved Hide resolved

Update src/wp-includes/block-template-utils.php

1ce805e

Co-authored-by: Christoph Daum <[email protected]>

apermo approved these changes Nov 8, 2024

View reviewed changes

github-actions bot mentioned this pull request Nov 8, 2024

PHP backports for WP 6.7 WordPress/gutenberg#63827

Open

Use 'elseif' instead of two separate 'if'

8ef61a6

Aljullu force-pushed the fix/flash-clicking-template-name branch from 70f64a1 to 8ef61a6 Compare November 26, 2024 13:29

apermo approved these changes Nov 26, 2024

View reviewed changes

Add an extra comment

0cfab7c

apermo approved these changes Nov 26, 2024

View reviewed changes

azaozz reviewed Nov 26, 2024

View reviewed changes

src/wp-includes/block-template-utils.php Outdated Show resolved Hide resolved

Update src/wp-includes/block-template-utils.php

b5b24ae

Co-authored-by: Andrew Ozz <[email protected]>

ntsekouras reviewed Dec 4, 2024

View reviewed changes

Make sure '['slug__not_in']' is being updated

cd07c08

felixarntz reviewed Dec 4, 2024

View reviewed changes

anton-vlasenko reviewed Dec 5, 2024

View reviewed changes

Aljullu and others added 2 commits December 5, 2024 17:41

Refactor if ... else ...

5cea08f

Update tests/phpunit/tests/blocks/getBlockTemplates.php

7e48e6e

Co-authored-by: Anton Vlasenko <[email protected]>

felixarntz approved these changes Dec 5, 2024

View reviewed changes

Fix comment format

b8e5c50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

Aljullu commented Oct 29, 2024

github-actions bot commented Oct 29, 2024 •

edited

Loading

github-actions bot commented Oct 29, 2024

apermo left a comment

Aljullu commented Nov 3, 2024

apermo left a comment

Aljullu commented Nov 8, 2024

apermo commented Nov 8, 2024

apermo Nov 26, 2024

Aljullu Nov 26, 2024

azaozz Nov 26, 2024 •

edited

Loading

ntsekouras Dec 4, 2024 •

edited

Loading

Aljullu Dec 4, 2024

felixarntz Dec 4, 2024

Aljullu Dec 5, 2024

ntsekouras Dec 4, 2024

Aljullu Dec 4, 2024

ntsekouras commented Dec 4, 2024

Aljullu commented Dec 4, 2024

felixarntz left a comment

felixarntz Dec 4, 2024

anton-vlasenko left a comment •

edited

Loading

anton-vlasenko Dec 4, 2024

Aljullu Dec 5, 2024

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

Are you sure you want to change the base?

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

Conversation

Aljullu commented Oct 29, 2024

github-actions bot commented Oct 29, 2024 • edited Loading

github-actions bot commented Oct 29, 2024

Test using WordPress Playground

Some things to be aware of

apermo left a comment

Choose a reason for hiding this comment

Aljullu commented Nov 3, 2024

apermo left a comment

Choose a reason for hiding this comment

Aljullu commented Nov 8, 2024

apermo commented Nov 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

azaozz Nov 26, 2024 • edited Loading

Choose a reason for hiding this comment

ntsekouras Dec 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ntsekouras commented Dec 4, 2024

Aljullu commented Dec 4, 2024

felixarntz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anton-vlasenko left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Oct 29, 2024 •

edited

Loading

azaozz Nov 26, 2024 •

edited

Loading

ntsekouras Dec 4, 2024 •

edited

Loading

anton-vlasenko left a comment •

edited

Loading