Make WordPress Core

Opened 2 years ago

Closed 21 months ago

Last modified 21 months ago

#54541 closed enhancement (fixed)

Add actions to template loading to assist with collecting debug info

Reported by: rmccue's profile rmccue Owned by: johnbillion's profile johnbillion
Milestone: 6.1 Priority: normal
Severity: normal Version:
Component: Themes Keywords: needs-dev-note has-patch commit
Focuses: performance Cc:

Description

locate_template() is the low-level workhorse that handles finding template parts; with the second parameter ($load = true), it also handles actually loading those templates. This is used throughout the template hierarchy loading system; get_template_part() calls it, as well as get_header(), get_footer(), etc.

In order to provide timing data to debug tools like Query Monitor, having actions before and after the template itself loads would be super useful. We could do this within each usage of locate_template(), or inside it.

Potentially, this could be a shortcircuit filter, allowing for alternative loading patterns (#13239) or automatic fragment caching. However I don't want to derail this ticket, as there are a lot of strong opinions on the template hierarchy, so I would suggest we do not provide a filter at this stage; right now, just having the timing data would be useful.

(I offer no opinions at this time on the relation to the Gutenberg FSE templating.)

Change History (20)

This ticket was mentioned in PR #2462 on WordPress/wordpress-develop by Tabrisrp.


2 years ago
#1

  • Keywords has-patch added; needs-patch removed

This patch adds new actions before and after loading the located template in locate_template().

The template filename is passed as an additional parameter to the action, as that can be useful for callbacks on these hooks.

The hooks name and the @since value are suggestions, I couldn't find if there is a standard for hooks names in core, as there is some prefixed with wp_, and others that are not.

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

#2 @johnbillion
2 years ago

  • Milestone changed from Awaiting Review to 6.0
  • Owner set to johnbillion
  • Status changed from new to reviewing
  • Type changed from defect (bug) to enhancement

#3 @costdev
2 years ago

  • Keywords needs-unit-tests needs-dev-note added
  • Milestone changed from 6.0 to 6.1

With 6.0 Beta 1 released on Tuesday, we are now past Feature Freeze for the 6.0 cycle. Moving to the 6.1 milestone.

  • This ticket should have unit tests accompanying it. Adding needs-unit-tests.
  • This ticket should be included in a dev note to alert extenders to the new action hooks. Adding needs-dev-note.

martinkrcho commented on PR #2462:


2 years ago
#4

@Tabrisrp , would you be able to apply the changes suggested by @peterwilsoncc? Then we could get @johnbillion to review and I would be more than happy to contribute with some unit tests.

Tabrisrp commented on PR #2462:


2 years ago
#5

@martinkrcho Done!

#6 @peterwilsoncc
2 years ago

  • Keywords commit added

Marking the linked pull request as ready for commit as of 5bc0c5a.

#7 @johnbillion
2 years ago

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

In 53560:

Themes: Add actions to template loading to assist with collecting debug info.

This introduces the following new actions which wrap the process of loading a template file:

  • wp_before_load_template
  • wp_after_load_template

Props rmccue, tabrisrp, peterwilsoncc

Fixes #54541

#9 @johnjamesjacoby
23 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

If I understand the intentions of this ticket and its hooks correctly, locate_template() may be the wrong function location for these hooks, or these hooks would perhaps benefit from a slightly different naming convention.


Is it possible that load_template() is where these exact hooks belong (with the $template_names param removed)? Obviously, that is the function responsible for requiring the located file with the intended variables available to it, and the measurable code execution for Query Monitor is the extraction of the query_vars and the rendering/inclusion of the template file.

In my research & experience, load_template() is used by many of the most popular WordPress plugins – not locate_template() – which means their template parts will never see these hooks fire and be unable to benefit from any future fragment caching implementation that may want to use them.

Scopecreep

The Feeds feature in WordPress also uses load_template() via the do_feed() function (through do_action( "do_feed_{$feed}" )) which kind of seems to me like it should have its own special case inside of locate_template() the way that theme-compat does so that Feeds can use locate_template() while also enabling Query Monitor to use the same hooks.

Plugins

Mostly, the plugins below use load_template() to get around the fact that locate_template() is strictly locked down to the parent/child theme hierarchy, so they write their own "locators" with extra paths to add to the core hierarchy under certain controlled conditions.

  • bbPress & BuddyPress for theme compatibility
  • EDD for loading template parts
  • WooCommerce for loading template parts
  • WPForms for loading template parts
  • Events Calendar for loading template parts
  • Jetpack for loading inline-help-template.php
  • A bunch more... 😅

Those (any) custom locator will not benefit from the actions being added here.

In total, there are around 10,085 usages in the Plugins repository (according to wpdirectory.net) that will not know what is happening before or after their own calls to load_template() because they are not using locate_template().

Asking respectfully for your friendly additional thoughts & feedbacks 💫🤝

(Edit: reworded some stuff for improved tone 💟)

Last edited 21 months ago by johnjamesjacoby (previous) (diff)

#10 @mukesh27
21 months ago

  • Keywords dev-feedback added

#11 @desrosj
21 months ago

  • Keywords commit removed

@johnbillion Are you able to add some thoughts in response to ticket:54541#comment:9?

Would be great to get clarification prior to beta 1.

This ticket was mentioned in Slack in #core by jeffpaul. View the logs.


21 months ago

#13 @JeffPaul
21 months ago

As discussed ahead of today's scheduled 6.1 Beta 1 release party (see the Slack link above), it was agreed that @johnbillion would review @johnjamesjacoby's comment#9. With no action on this ahead of the updated 6.1 Beta 1 release party tomorrow, this will be punted to Future Release.

#14 @johnbillion
21 months ago

  • Keywords needs-patch added; has-patch needs-unit-tests dev-feedback removed
  • Status changed from reopened to accepted

@johnjamesjacoby I agree that moving these actions into load_template() makes sense. We do lose the $template_names context but the existing get_template_part action includes this and I'm planning on proposing a new action that fires after locate_template() is called inside get_template_part() which provides an extra debugging opportunity that's specific to template parts.

I'll open a PR with your suggestion.

@JeffPaul This should remain in the 6.1 milestone as it already has a commit.

This ticket was mentioned in PR #3298 on WordPress/wordpress-develop by johnbillion.


21 months ago
#15

  • Keywords has-patch added; needs-patch removed

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

Per feedback from J³ on the ticket, moving these new actions inside load_template() makes sense.

johnbillion commented on PR #3298:


21 months ago
#16

This amends the change introduced in #2462. cc @Tabrisrp

#17 @peterwilsoncc
21 months ago

  • Keywords commit added

Follow up pull request addressing @johnjamesjacoby's concerns approved for commit as of c70fe21d9585e.

#18 @peterwilsoncc
21 months ago

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

In 54270:

Themes: Relocate actions firing prior to and after template loading.

This relocates the actions wp_before_load_template and wp_after_load_template to fire within the load_template() function.

Prior to this change the actions fired in the locate_template() function.

Follow up to [53560].

Props johnjamesjacoby, johnbillion.
Fixes #54541.

peterwilsoncc commented on PR #3298:


21 months ago
#19

Merged in https://core.trac.wordpress.org/changeset/54270 / 77e65eace541663ac1875aa00688943d5d8bd446

JJJ commented on PR #3298:


21 months ago
#20

Thank you, kindly, friends 💪

Note: See TracTickets for help on using tickets.