#58299 closed defect (bug) (fixed)
Block template is located twice in `get_query_template()`
Reported by: |
|
Owned by: |
|
---|---|---|---|
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)
Change History (17)
This ticket was mentioned in PR #4443 on WordPress/wordpress-develop by dlh01.
22 months ago
#1
- Keywords has-patch added
#3
@
21 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 |
@mukesh27 commented on PR #4443:
20 months ago
#5
@spacedmonkey commented on PR #4443:
20 months ago
#6
@ockham Do you mind providing some context why this function needs to called twice?
#7
@
20 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
@
20 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 toget_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
@
20 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.
@Bernhard Reiter commented on PR #4443:
20 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 toget_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.
20 months ago
#11
Apologies, @felixarntz, but could you elaborate about the template-loading outcome(s) that should be tested?
@flixos90 commented on PR #4443:
20 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.
Trac ticket: https://core.trac.wordpress.org/ticket/58299