Make WordPress Core

Opened 11 months ago

Closed 10 months ago

Last modified 10 months ago

#58299 closed defect (bug) (fixed)

Block template is located twice in `get_query_template()`

Reported by: dlh's profile dlh Owned by: spacedmonkey's profile spacedmonkey
Milestone: 6.3 Priority: normal
Severity: normal Version: 5.9
Component: Themes Keywords: has-patch commit
Focuses: performance Cc:

Description

get_query_template() currently calls locate_block_template() twice instead of once when a template is rendered, leading to a second call to resolve_block_template(), get_block_templates(), and so on.

The first call is hardcoded directly after locate_template() here: https://github.com/WordPress/wordpress-develop/blob/c40e0c/src/wp-includes/template.php#L66

This call was added in [51003], the original commit to add support for block templates to classic themes.

The second call occurs during the {$type}_template filter here: https://github.com/WordPress/wordpress-develop/blob/c40e0c/src/wp-includes/block-template.php#L25

This second call was added in [52316] for #54553. However, as far as I can tell, the only thing that was needed to fix the issue in #54553 was the callback added to pre_get_posts.

Adding locate_block_template() as a filter on {$type}_template would have been necessary in the Gutenberg plugin, but not in core, given the call that was added directly to get_query_template().

You can test this using the linked PR, which would remove the {$type}_template filters and leave just the pre_get_posts addition. With that PR applied, the verification steps in ticket:54553#comment:7 should still pass.

Attachments (1)

Screenshot 2023-05-12 at 14.46.49.png (205.6 KB) - added by spacedmonkey 11 months ago.

Download all attachments as: .zip

Change History (17)

This ticket was mentioned in PR #4443 on WordPress/wordpress-develop by dlh01.


11 months ago
#1

  • Keywords has-patch added

#2 @SergeyBiryukov
11 months ago

  • Milestone changed from Awaiting Review to 6.3

#3 @spacedmonkey
11 months ago

Some promising early numbers here.

Trunk PR
Response Time (median)121.83 114.41
wp-before-template (median)65.38 59.6
wp-template (median)50.68 48.99
wp-total (median)116.48 108.98

#4 @spacedmonkey
10 months ago

Wonder what @oandregal thinks about this one.

@mukesh27 commented on PR #4443:


10 months ago
#5

I ran a profiling test against the current trunk, and it showed that removing some function calls improved performance. This might be of interest to @felixarntz, @spacedmonkey, @eclarke1, and @joemcgill.

https://i0.wp.com/github.com/WordPress/wordpress-develop/assets/10103365/6b66dbc5-1d73-40c3-b1cd-731034f0c895

@spacedmonkey commented on PR #4443:


10 months ago
#6

@ockham Do you mind providing some context why this function needs to called twice?

#7 @spacedmonkey
10 months ago

@noisysocks @bernhard-reiter Can you provide some context here. Why this function is called multiple times. Does this change look good? How can we test it?

#8 in reply to: ↑ description @Bernhard Reiter
10 months ago

Replying to dlh:

Adding locate_block_template() as a filter on {$type}_template would have been necessary in the Gutenberg plugin, but not in core, given the call that was added directly to get_query_template().

This seems like a plausible explanation; we might've overlooked the filters when backporting the code from Gutenberg to Core.

I'll try and test this in a bit; I'd say that most importantly, we'll need to verify that the correct template is used when rendering a given page on the frontend, and that the site editor also continues to work as before (with a special focus on what templates are displayed as available, and that the correct template is loaded and also saved when making changes).

#9 @spacedmonkey
10 months ago

  • Owner set to Bernhard Reiter
  • Status changed from new to assigned

Thanks @Bernhard-Reiter. For now, I am going mark you as owner. After your testing and you say you are happy, then I will take over and hopefully commit.

Last edited 10 months ago by spacedmonkey (previous) (diff)

@Bernhard Reiter commented on PR #4443:


10 months ago
#10

I looked around a bit, and this looks indeed like an oversight. As I said in this comment on the corresponding Trac ticket:

Adding locate_block_template() as a filter on {$type}_template would have been necessary in the Gutenberg plugin, but not in core, given the call that was added directly to get_query_template().

This seems like a plausible explanation; we might've overlooked the filters when backporting the code from Gutenberg to Core.

To unfurl the call stack a bit: The code that this PR removes loops over all template types and adds `locate_block_template` as a filter to all their corresponding `{$type}_template` hooks (e.g. home_template, single_template, etc). The `{$type}_template` filters are only being applied in one spot: https://github.com/WordPress/wordpress-develop/blob/88de9a9d8efe13a21ebdcdfbf6f87d78875c8f0e/src/wp-includes/template.php#L103

The LOC _directly before that line_ (and its comment block) is the "other" call to locate_block_template(), passing the same arguments: https://github.com/WordPress/wordpress-develop/blob/88de9a9d8efe13a21ebdcdfbf6f87d78875c8f0e/src/wp-includes/template.php#L66

So it looks pretty safe indeed to remove the filters from the hook! I've done a bit of smoke testing, and things seem to continue to work fine.

dlh01 commented on PR #4443:


10 months ago
#11

Apologies, @felixarntz, but could you elaborate about the template-loading outcome(s) that should be tested?

@flixos90 commented on PR #4443:


10 months ago
#12

@dlh01 Fair question. Taking another look, this change is probably irrelevant to any testing.

However, as far as I can tell there are no tests at all for the block template specific bits of the get_query_template() function. This is not strictly responsibility of a PR like this, but those should have been added whenever the locate_block_template() call was added to get_query_template().

To unblock this PR, I'm happy to approve this, but it would be great if you could use a separate PR to add at least some basic test coverage to ensure that get_query_template() returns the expected result for some block templates use-cases, as otherwise we can't really make such changes safely, without test coverage.

dlh01 commented on PR #4443:


10 months ago
#13

Sure thing! I'll open a new ticket when I have those tests ready.

#14 @spacedmonkey
10 months ago

  • Keywords commit added
  • Owner changed from Bernhard Reiter to spacedmonkey

#15 @spacedmonkey
10 months ago

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

In 56060:

Themes: Block template is located twice in get_query_template().

When the function _template_loader_filters was ported to core from gutenberg, it retained the filter to load block templates. However, the function locate_block_template is called manually in get_query_template, so this filter is not needed. Calling locate_block_template twice, results in performance issue, as locate_block_template is a expensive function to run, as it does database and file lookups.

Props dlh, mukesh27, flixos90, SergeyBiryukov, bernhard-reiter, spacedmonkey.
Fixes #58299.

Note: See TracTickets for help on using tickets.