WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#11242 closed enhancement (fixed)

Add "template_include" action to be run just before templates are included

Reported by: holizz Owned by: westi
Milestone: 3.0 Priority: normal
Severity: normal Version: 2.9
Component: Plugins Keywords: has-patch dev-feedback
Focuses: Cc:

Description

While working on a template-orientated plugin, we found ourselves replicating a large chunk of wp-includes/template-loader.php in our template_redirect action, so we thought there should be a filter to modify the template path before it gets included.

Attachments (8)

diff (3.2 KB) - added by holizz 4 years ago.
Patch to add template_include filter
r12283.diff (3.0 KB) - added by holizz 4 years ago.
revised patch
r12294.diff (3.0 KB) - added by holizz 4 years ago.
Latest patch, without != null
11242.patch (5.5 KB) - added by hakre 4 years ago.
Updated and Improved Patch
ticket_11242-patch.diff (5.6 KB) - added by ptahdunbar 4 years ago.
Minor updates based off hakre's 11242.patch
ticket_11242_patch2.diff (1.2 KB) - added by ptahdunbar 4 years ago.
adds a template_loaded action to load_template()
ticket_11242_patch3.diff (4.3 KB) - added by ptahdunbar 4 years ago.
11242.diff (4.1 KB) - added by nacin 4 years ago.

Download all attachments as: .zip

Change History (34)

holizz4 years ago

Patch to add template_include filter

comment:1 Denis-de-Bernardy4 years ago

  • Keywords close added
  • Milestone changed from Unassigned to Future Release

you should probably be using the wp hook.

comment:2 harrym4 years ago

Hmm. We're trying to intercept the path for a template and change it before it's included.

How's the WP hook helpful? Doesn't look like it would allow us to do that.

comment:3 scribu4 years ago

  • Keywords has-patch added; has_patch removed
  • Milestone Future Release deleted
  • Resolution set to worksforme
  • Status changed from new to closed

I also don't see how filtering the full path is better than calling the 'template_redirect' hook.

You have a filter for each get_*_template() function. You should use those instead.

Also, take a look at the Carrington Framework and see how they go about it.

comment:4 harrym4 years ago

  • Resolution worksforme deleted
  • Status changed from closed to reopened

Well, I can answer those!

template_redirect is a hook, not a filter, so it doesn't give you access to the template path that's been chosen or give you a chance to change it. It's called before wordpress even decides what template to load.

There are *_template filters, but it's not possible to enumerate a complete set of them and hook them all: the attachment filter, for example, generates the filter tag by looking at the MIME type of the attachment. Since plugins can add whatever mime types they like, it's not possible to generate a complete set of *_template filters to hook into.

The Carrington template appears to use the is_* functions to figure out what template it's in -- which is fine. We're not trying to do that. We're trying to change the default template loader's behaviour, so executing code after the template has already been chosen and included won't work.

What's the harm in adding this as a new filter?

comment:5 nacin4 years ago

A filter is a type of hook. (An action, such as template_redirect, is another type.)

Every $template is run through a filter before being included. get_attachment_template uses get_query_template, which in turn invokes a filter. Having a filter to run through $template in template-loader.php would not allow for any context, hence why it's best to use the individual filters or utilize template_redirect, or utilize a combination of both.

The various *_template filters include category_template, comments_popup_template,
comments_template, home_template, page_template, tag_template, taxonomy_template, {$type}_template. You could tie the same function to all of them if you'd like.

comment:6 Denis-de-Bernardy4 years ago

  • Resolution set to wontfix
  • Status changed from reopened to closed

hook into template_redirect with an extremely low priority (a zillion). rewrite template_redirect the way you want it. and finish your function with an exit statement.

comment:7 follow-up: harrym4 years ago

  • Resolution wontfix deleted
  • Status changed from closed to reopened

@nacin:

As you've noted with {$type}_template: some of the filter names are generated on the fly. There's no way for us to know what all the possible filter names are, so we can't hook into every *_template filter that might be called. So that doesn't work for us.

The filter we've added with this patch doesn't need any additional context. It's simply a filter on the path to a template that executes before the template is included.

@Denis:

I'm afraid that doesn't work either. template_redirect isn't called when templates are included using load_template() in wp-include/theme.php.

We really have thought about this!

comment:8 Denis-de-Bernardy4 years ago

  • Keywords dev-feedback added; close removed
  • Milestone set to Future Release

comment:9 scribu4 years ago

harrym, could you post a usage example of this new filter?

comment:10 harrym4 years ago

Sure. We're using this filter to make a plugin that allows us to write Wordpress templates using HAML. We find HAML much easier to maintain than embedded HTML/PHP.

So, we do:

add_filter('template_include', 'wphaml_template_include');
function wphaml_template_include($template)
{
   // Globalise the Wordpress environment
   global $posts, $post, $wp_did_header, $wp_did_template_redirect, $wp_query, $wp_rewrite, $wpdb, $wp_version, $wp, $id, $comment, $user_ID;
   
   // Is there a haml template?
   $haml_template = str_replace(".php", ".haml.php", $template);
      
   if(file_exists($haml_template))
   {
      // Process the HAML template to turn it into HTML
      $haml->parse_and_execute($haml_template);
        
      // The current HAML parser requires templates to execute in its own context (suck) 
      // so return null to prevent WP from including the template. If this parser was
      // better, we'd just return the path to the compiled template here.
      return null;
   }
   
   // No haml template -- use Wordpress's
   return $template;
}

I've cut lots out to make it simpler, but that's the general idea.

comment:11 in reply to: ↑ 7 ; follow-up: Denis-de-Bernardy4 years ago

Replying to harrym:

@Denis:

I'm afraid that doesn't work either. template_redirect isn't called when templates are included using load_template() in wp-include/theme.php.

We really have thought about this!

the again, those (when used) have stuff like:

do_action( 'get_header', $name );

comment:12 in reply to: ↑ 11 ; follow-up: harrym4 years ago

Replying to Denis-de-Bernardy:

Replying to harrym:

@Denis:

I'm afraid that doesn't work either. template_redirect isn't called when templates are included using load_template() in wp-include/theme.php.

We really have thought about this!

the again, those (when used) have stuff like:

do_action( 'get_header', $name );

That's true, but none of these hooks allow us to *override* the template handling. Once get_header's been called, we can't stop the header from being included, or modify the template's path.

comment:13 in reply to: ↑ 12 ; follow-ups: Denis-de-Bernardy4 years ago

Replying to harrym:

That's true, but none of these hooks allow us to *override* the template handling. Once get_header's been called, we can't stop the header from being included, or modify the template's path.

not so.

function my_header($name) {
  // do stuff
  die;
}
add_action('get_header', 'my_header');

itching to re-close, personally.

comment:14 dd324 years ago

  • Version set to 2.9

I can see where just die()'ing and the current hooks may not provide a full plugin-development-friendly environment.

Rather than looking at how it can be done, look at how it can be done better.

comment:15 in reply to: ↑ 13 harrym4 years ago

Replying to Denis-de-Bernardy:

Replying to harrym:

That's true, but none of these hooks allow us to *override* the template handling. Once get_header's been called, we can't stop the header from being included, or modify the template's path.

not so.

function my_header($name) {
  // do stuff
  die;
}
add_action('get_header', 'my_header');

itching to re-close, personally.

*confused*

If you die in a get_header action, you kill the script before most of the page is even created. In our case, when get_sidebar is called, we'd want to grab the template, compile it to HAML and have Wordpress include the compiled HAML instead of its own template.

The rest of the page still needs to be executed, and we need to be able to tell Wordpress not to include its template if we've included ours.

comment:16 Denis-de-Bernardy4 years ago

@harrym: my bad, yes. you're very right for that one.

comment:17 in reply to: ↑ 13 scribu4 years ago

Replying to Denis-de-Bernardy:

function my_header($name) {
  // do stuff
  die;
}
add_action('get_header', 'my_header');

I was never confortable with this approach.

Maybe it's time to finally add a dedicated filter.

comment:18 nacin4 years ago

In that case, let's see if we can get this patch solid.

What I notice:

  • The return; should be left in for is_robots and is_feed. If you need to intercept those, you should use template_redirect.
  • The end of template-loader.php should probably just be:
    if ( $template = apply_filters( 'template_include', $template ) )
      include $template;
    
  • The changes to theme.php should probably be:
    $ if ( $_template_file = apply_filters( 'template_include', $_template_file ) )
      require_once $_template_file;
    
  • Do we continue using include and require_once respectively? Should we run file_exists and/or suppress errors?

holizz4 years ago

revised patch

comment:19 holizz4 years ago

Thanks for the feedback, nacin. I revised my patch. (I didn't add any file_exists checking or error supression.)

comment:20 nacin4 years ago

I also meant that you should get rid of the "$template != null" check near the end of template-loader.php, and allow the filter to handle an empty $template as well. That's all I got.

holizz4 years ago

Latest patch, without != null

comment:21 nacin4 years ago

  • Milestone changed from Future Release to 3.0

comment:22 Denis-de-Bernardy4 years ago

current patch seems to make sense if we really want a filter.

comment:23 hakre4 years ago

I reviewed the patch and like it. This will add some filters that are often needed. I found some places which actually benefit from a touchup:

1.) the template-loader can be made configure-able. That does not only clean up the code, it also offers the possibility to add an additional filter: template_include_config.
2.) the default (non-existant) template is not NULL but an empty string (). that is what all the get_for_whatever_query_template() is returning in case of error. the patch should be aligned to those.
3.) an action after the actual include does make sense in my eyes. you smell what this well open? I named it
template_included in my patch.
4.) load_template() did contain a require_once statement. I do not see that this makes sense. It will prevent to load a sub-template twice. I changed it to include (as we do in the template loader as well).
5.) since that function is not named include_template() but load_template() I suggest to change the filter to
template_load in there. In accordance to 3.) I introduced the template_loaded action into there as well.

My patch is based on the last one provided (r12294.diff).

comment:24 hakre4 years ago

I reviewed the patch and like it. This will add some filters that are often needed. I found some places which actually benefit from a touchup:

  1. the template-loader can be made configure-able. That does not only clean up the code, it also offers the possibility to add an additional filter: template_include_config.
  2. the default (non-existant) template is not NULL but an empty string (). that is what all the get_for_whatever_query_template() is returning in case of error. the patch should be aligned to those.
  3. an action after the actual include does make sense in my eyes. you smell what this well open? I named it template_included in my patch.
  4. load_template() did contain a require_once statement. I do not see that this makes sense. It will prevent to load a sub-template twice. I changed it to include (as we do in the template loader as well).
  5. since that function is not named include_template() but load_template() I suggest to change the filter to template_load in there. In accordance to 3.) I introduced the template_loaded action into there as well.

My patch is based on the last one provided (r12294.diff). Comment-Update: better to read

hakre4 years ago

Updated and Improved Patch

ptahdunbar4 years ago

Minor updates based off hakre's 11242.patch

ptahdunbar4 years ago

adds a template_loaded action to load_template()

nacin4 years ago

comment:25 hakre4 years ago

I wanted to give it a testdrive but the latest patch didn't apply clean any longer.

Some stats so far (pre patch)

Tests: 489, Assertions: 2919, Failures: 135, Errors: 32, Skipped: 18. (pthdnbr)
Tests: 489, Assertions: 2879, Failures: 128, Errors: 54, Skipped: 18. (my run)

my system is WINNT, pthdnbr some *NIX if I remember correctly.

comment:26 nacin4 years ago

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

(In [13252]) Add template_include filter, to modify the template path before it is included. Fixes #11242

Note: See TracTickets for help on using tickets.