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

Do not look for block variants, if not supporting block-templates #45362

Merged
merged 3 commits into from Nov 8, 2022

Conversation

spacedmonkey
Copy link
Member

@spacedmonkey spacedmonkey commented Oct 27, 2022

What?

Do not look for block variants if theme does not support them. Saving a database query.

Original ticket here.

Why?

How?

Testing Instructions

Screenshots or screencast

@spacedmonkey spacedmonkey self-assigned this Oct 27, 2022
@codesandbox
Copy link

codesandbox bot commented Oct 27, 2022

CodeSandbox logoCodeSandbox logo  Open in CodeSandbox Web Editor | VS Code | VS Code Insiders

@t-hamano
Copy link
Contributor

I don't know the details of how it works, but I am concerned that it may affect the classic theme that supports template parts. In the Classic theme, opt-in the Template Parts Editor as follows:

function add_block_template_part_support() {
    add_theme_support( 'block-template-parts' );
}
add_action( 'after_setup_theme', 'add_block_template_part_support' );

Then, when we try to load other template parts within the template parts editor, the behavior changes.

On trunk

trunk

On this branch

pr

peterwilsoncc
peterwilsoncc previously approved these changes Oct 28, 2022
Copy link
Contributor

@peterwilsoncc peterwilsoncc left a comment

Choose a reason for hiding this comment

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

This looks good to me based on manual testing.

Note:

On my Gutenberg development box, I needed to modify the equivelant path in WordPress, /wp-includes/blocks/template-part.php, to confirm this change.

For whatever reason, the plugin wasn't using its version of the PHP files.

@peterwilsoncc
Copy link
Contributor

Having dug in to this following @t-hamano 's comment above, I think it needs to return early with the following:

if (
	! current_theme_supports( 'block-templates' )
	&& ! current_theme_supports( 'block-template-parts' )
) {
	return [];
}

block-templates support implies block-template-parts support but this doesn't appear to be reflected when calling current_theme_supports().

@talldan
Copy link
Contributor

talldan commented Oct 28, 2022

This is a performance issue? The ticket/PR is a bit light on details.

This idea behind this PR seems like an ok patch for a short term improvement in 6.1.1, but doesn't really get to the core of the issue. Other code can still call get_block_templates and reintroduce the problem.

The two questions I have:

  • Why does get_block_templates go as far as making a DB query if templates/template parts are not supported? Could the early return be added to that function?
  • Why is the template part block registered for themes that don't support template parts? Not registering it would also avoid this code being triggered and avoid a bunch of unnecessary work.

It might be good to look at a different fix for 6.2.

@talldan talldan added [Type] Performance Related to performance efforts [Block] Template Part Affects the Template Parts Block labels Oct 28, 2022
@Mamaduka
Copy link
Member

block-templates support implies block-template-parts support

@peterwilsoncc, I don't think that's true. Those are two separate "flags" for classic themes.

@peterwilsoncc
Copy link
Contributor

@Mamaduka This is what I see in TT3, which includes template parts but doesn't indicate block-template-parts support:

$ wp shell
wp> get_stylesheet_uri();
=> string(69) "http://vagrant.local/wp/wp-content/themes/twentytwentythree/style.css"
wp> current_theme_supports( 'block-templates' )
=> bool(true)
wp> current_theme_supports( 'block-template-parts' )
=> bool(false) 

Maybe there is some nuance that I am missing re the purpose the theme support flags serve.

@talldan Yeah, a perf issue to avoid unneeded DB queries. Thanks for tagging appropriately.

I think you're right, there is room to improve further in 6.2 by basically nooping any of the database queries template blocks make.

@Mamaduka
Copy link
Member

@peterwilsoncc, the block-template-parts is a new flag that partially enables Site Editor for classic themes. Since block themes already have full access to Site Editor, there was no need to set the theme support for them.

Internally the flag controls what pieces of the UI are hidden/inaccessible.

Here's a dev note for more details: https://make.wordpress.org/core/2022/10/04/block-based-template-parts-in-traditional-themes/.

@spacedmonkey
Copy link
Member Author

Can you pull and tests again @t-hamano

@t-hamano
Copy link
Contributor

I have checked the behavior again and it is working as expected.

I also checked again the support variations in the classic theme, and I believe they are as follows:

Support block-templates only

We can use the template editor in the post editor. The template parts editor menu is not accessible. However, we can call template parts defined in the theme from within the template editor.

Support block-template-parts only

We cannot use the template editor in the post editor. we can access the template parts editor menu, and in the template parts editor we can also call template parts defined in your theme.

Support block-templates and block-template-parts

We can use the template editor in the post editor. We can access the template parts editor menu, and in the template parts editor, we can also call template parts defined in the theme.

Therefore, I think it would be appropriate not to query only for template parts unless both block-templates and block-template-parts are supported.

Co-authored-by: Aki Hamano <54422211+t-hamano@users.noreply.github.com>
@spacedmonkey
Copy link
Member Author

I guess the question is, do we even need to register this block on themes that do not support them anyway. Seems like it would never be used.

@spacedmonkey spacedmonkey added the Backport to WP Minor Release Pull request that needs to be backported to a WordPress minor release label Nov 7, 2022
@spacedmonkey
Copy link
Member Author

I would love some context here from @talldan.

Copy link
Member

@Mamaduka Mamaduka left a comment

Choose a reason for hiding this comment

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

The changes here look good to me. Thank you, @spacedmonkey!

I agree with @talldan that we should explore a more general solution for WP 6.2.

Even for the block themes, variations are only needed by the editors. There's no use in querying them when a user visits a page.

One option we can explore for the next major release is the lazy initialization of block variables.

@spacedmonkey spacedmonkey merged commit 49b6ad8 into trunk Nov 8, 2022
@spacedmonkey spacedmonkey deleted the fix/template-loading branch November 8, 2022 11:23
@github-actions github-actions bot added this to the Gutenberg 14.6 milestone Nov 8, 2022
@spacedmonkey
Copy link
Member Author

One option we can explore for the next major release is the lazy initialization of block variables.

Created a ticket #45601.

@sybrew
Copy link

sybrew commented Nov 8, 2022

Could the concerns of #45362 (comment) above also be addressed, please?

I think no database queries for theme support, and especially not anything via WP Query, should be performed before a template can be determined (template_redirect).

Mamaduka pushed a commit that referenced this pull request Nov 10, 2022
…5362)

* Do not look for block varients, if not supporting block-templates

* Apply suggestions from code review

* Apply suggestions from code review

Co-authored-by: Aki Hamano <54422211+t-hamano@users.noreply.github.com>

Co-authored-by: Aki Hamano <54422211+t-hamano@users.noreply.github.com>
Mamaduka pushed a commit that referenced this pull request Nov 10, 2022
…5362)

* Do not look for block varients, if not supporting block-templates

* Apply suggestions from code review

* Apply suggestions from code review

Co-authored-by: Aki Hamano <54422211+t-hamano@users.noreply.github.com>

Co-authored-by: Aki Hamano <54422211+t-hamano@users.noreply.github.com>
@Mamaduka Mamaduka removed the Backport to WP Minor Release Pull request that needs to be backported to a WordPress minor release label Nov 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Template Part Affects the Template Parts Block [Type] Performance Related to performance efforts
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

6 participants