Remove duplicate call to locate_block_template()

[dlh01]

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

---

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.

[mukeshpanchal27]

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.

[spacedmonkey]

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

[ockham]

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:

wordpress-develop/src/wp-includes/template.php

Line 103 in 88de9a9

return apply_filters( "{$type}_template", $template, $type, $templates );

The LOC directly before that line (and its comment block) is the "other" call to locate_block_template(), passing the same arguments:

wordpress-develop/src/wp-includes/template.php

Line 66 in 88de9a9

$template = locate_block_template( $template, $type, $templates );

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.

[ockham]

Nice find @dlh01 😄

As elaborated in my comment, I think this is safe to land

[felixarntz]

@dlh01 Can we add a PHPUnit test (pretty much an integration test) to ensure this doesn't break the template loading logic?

[dlh01]

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

[felixarntz]

@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]

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

[spacedmonkey]

Committed 02ea2f1