WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 3 years ago

Last modified 10 months ago

#18265 closed enhancement (wontfix)

Use load_template in template-loader.php

Reported by: elyobo Owned by:
Milestone: Priority: normal
Severity: normal Version:
Component: Template Keywords: has-patch
Focuses: Cc:

Description

There may be a good reason for this I'm unaware of, but the template loader (wp-includes/template-loader.php) does not use the appropriate sounding load_template() function.

The only differences are

  • A potentially smaller number of globals included, as load_templates() globalises a smaller list.
  • Query vars are extracted automatically.

In my plugin, I'm trying to make things easier for our theme folks by populating the appropriate data automatically. This is possible through the load_template() function because I can push data into $wp_query->query_vars and it will be extract()ed into local variables that are available in the template.

This is *not* possible in things like single-page.php, because template-loader.php just includes the template file it finds instead of load_template()ing it.

I have attached the one line patch to change this. If the reduced number of globals would be a problem, it is also easy to include an "extract($GLOBALS);" in load_template() as well.

Attachments (1)

load_template-in-template-loader.patch (426 bytes) - added by elyobo 3 years ago.
Patch against current trunk

Download all attachments as: .zip

Change History (27)

elyobo3 years ago

Patch against current trunk

comment:1 scribu3 years ago

I'm curious about this as well.

comment:2 nacin3 years ago

I have an immediate negative reaction to extract($GLOBALS);. Suggest wontfix.

comment:3 nacin3 years ago

  • Milestone Awaiting Review deleted
  • Resolution set to wontfix
  • Status changed from new to closed

The other problem is that extract($GLOBALS); would simply create local variables of those values -- references aren't maintained to the global variables.

While this sounds like a great idea in theory -- no more stomping the $post or $wp globals, etc. -- it would break a LOT of existing code that makes edits to core globals on the fly.

So it's more than just a negative reaction. We can't do load_template() without the globals, and we can't extract the globals, so this is a wontfix.

comment:4 elyobo3 years ago

  • Resolution wontfix deleted
  • Status changed from closed to reopened

That's not entirely true, you could do something like

foreach (array_keys($GLOBALS) as $glob) {
    global $$glob;
}

I'm not sure about the performance implications of that though, WP has a lot of globals.

If we don't do that, how about adding an extract($wp_query->query_vars) to template-loader.php; this way the behaviour is consistent for the templates at least, even though the approach is inconsistent. I expect that performance would be better than manually reglobalising all the globals.

comment:5 nacin3 years ago

I never even realized that load_template() did an extract on query_vars. Because load_template() with $load = true is never used in core, this would more or less be new behavior, and it will break things. Some query var globals are guaranteed to step over variables used in template files.

We don't want more globals, we want less. If you need a query var, use get_query_var(). Again suggesting wontfix.

comment:6 elyobo3 years ago

I completely agree on the globals front, I did a check the other day and identified 272 distinct variables called globalised with the "global" keyword in WP core :)

However, load_templates doesn't take a $load argument; it always extracts the query vars if they're there (the boolean argument is whether to include or or require_once), so it is definitely doing this in core already.

Last edited 3 years ago by elyobo (previous) (diff)

comment:7 nacin3 years ago

Sorry, I meant that it's the $load = true argument of locate_template() that triggers load_template(). That is never used in core.

load_template() is used directly in only one situation: feed templates. (Also theme-compat files, but those don't really count.)

Otherwise, load_template() is just used by plugins.

comment:8 scribu3 years ago

It would have been nice if load_template() was used (so that there wouldn't be an inconsistency between templates loaded by plugins vs. templates loaded by Core), but as nacin said, we can't do that, due to backwards compatibility.

I can push data into $wp_query->query_vars and it will be extract()ed into local variables that are available in the template.

That doesn't sound like a very good practice. The way WP handles this is by using template tags (i.e. functions) like the_title(). Your plugin should define it's own template tags that access globals in the background, as necessary.

comment:9 follow-up: nacin3 years ago

A better WP implementation would have been bbPress's version of load_template():

function bb_load_template( $files, $globals = false ... );

Pretty nifty, allows things like this:

bb_load_template( 'login.php', array( 'user_exists', 'user_login', 'remember_checked', 'redirect_to', 're', 'bb_login_error' ) );

The bbPress code is full of code like this -- WordPress-inspired, but improved in the slightest of ways.

comment:10 elyobo3 years ago

nacin, yeah, that makes more sense.

That said, get_header(), get_footer() and get_sidebar() all call locate_template with $load = true. While these are called from templates, so may technically not be core, every site must have a template, so every single site is seeing this behaviour already in their sidebars, headers and footers... but not in their pages, errors, 404s etc which are loaded by template-loader.php.

scribu, I'm not sure how extracting the query vars would break backwards compatibility; if EXTR_SKIP is used it will not stomp on any other variables and if the globalising approach was used all the expected globals would be present - either solution should be backwards compatible.

nacin, the bbPress's version sounds like exactly what I was looking for when I went digging in to find out how to inject my variables into theme templates :) It seemed like the query_vars approach worked, but then I discovered it only worked some of the time (as above) and not the rest, which just seemed inconsistent.

comment:11 scribu3 years ago

scribu, I'm not sure how extracting the query vars would break backwards compatibility

That's not what I meant. I was trying to say that using query_vars to pass arbitrary values to a template is not an elegant solution.

comment:12 scribu3 years ago

As such, I don't think we should be encouraging it by using load_template() in template-loader.php (even if it's done in a backward-compatible way).

Last edited 3 years ago by scribu (previous) (diff)

comment:13 elyobo3 years ago

Sorry scribu, I misunderstood then.

It seems a much more elegant solution (to me) than making my front end guys learn new functions to access bits of my data, rather than the $variables that they already know and love. Given that this data is already extracted into header, footer and sidebar templates, it seems like it would also improve consistency and usability.

You're right that there should be a proper way to do this; but there currently isn't one.

comment:14 in reply to: ↑ 9 ; follow-up: scribu3 years ago

Replying to nacin:

A better WP implementation would have been bbPress's version of load_template():

function bb_load_template( $files, $globals = false ... );

Even more flexible would be this:

function load_template( $files, $data = array() );

Then you could pass any data you want, including globals:

load_template( 'login.php', wp_array_slice_assoc( $GLOBALS, array( 'user_exists', 'user_login', 'remember_checked', 'redirect_to', 're', 'bb_login_error' ) ) );
Last edited 3 years ago by scribu (previous) (diff)

comment:15 in reply to: ↑ 14 elyobo3 years ago

Replying to scribu:

From the look of his example, I think that's how the bbPress version works as well.

comment:16 scribu3 years ago

No, bb_load_template() only makes available existing globals, like global $$key, whereas my example would extract( $data ).

comment:17 elyobo3 years ago

Ah, my wordpress ignorance showing through :) Should have looked up wp_array_slice_assoc.

Anyhow, I have to sleep. Thanks for the discussion folks. I guess we'll have to wait for an elegant solution to the problem to turn up. Would there be any interest in me opening a feature request to make it possible to do what I want (injecting variables into templates)?

In the meantime, I think I'll just add something like the following line at the top of my templates and use this to inject the data I want into each template; it requires me to modify each template, but it prevents the front end guys from having to learn stuff about the internals of how the data is generated, so it's most of the way there.

<?php extract(apply_filters('template_data', array()), EXTR_SKIP); ?>

comment:18 scribu3 years ago

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

Since in your own theme you don't have to worry about backwards-compatibility, you could force template-loader.php to call load_template():

function force_load_template( $template ) {
	load_template( $template );
	die;
}
add_filter( 'template_include', 'force_load_template', 999 );

Re-closing as wontfix. Feel free to leave further comments, though.

Version 0, edited 3 years ago by scribu (next)

comment:19 nacin3 years ago

You could also extract your variables into the global namespace on the template_redirect or template_include hooks.

comment:20 scribu3 years ago

You could also extract your variables into the global namespace on the template_redirect or template_include hooks.

That wouldn't work, since you would be in the callback's scope, not in load_template()'s.

Last edited 3 years ago by scribu (previous) (diff)

comment:21 scribu3 years ago

Nevermind, yes, it would work, since the template is loaded in the global scope.

Last edited 3 years ago by scribu (previous) (diff)

comment:22 scribu3 years ago

Thinking more about the ideal load_template() function, even if it did accept a $data arg, it would still be akward to use, since it's called from WP Core, not your own code.

But with the 'template_include' filter you can do pretty much whatever you want: check basename( $template ), use conditional tags like is_single(), use your own version of load_template() etc.

Last edited 3 years ago by scribu (previous) (diff)

comment:23 elyobo3 years ago

Globalising variables would work, and is effectively doing the same as what extracting the query vars in the template loader would have done, but nacin is right that we don't need need even more globals floating around.

I feel pain every time I'm forced to use an exit or die statement; it makes unit testing a pain and feels like a hack. That said, it looks like simply returning a value that evaluates to false will stop the original load from happening, without having to resort to a die statement, so it seems like a workable approach; all that needs to be done is to remove the die statement from scribu's suggestion.

Thanks for the help.

comment:24 scribu3 years ago

Hey, you're right. Updated my code.

comment:25 elyobo3 years ago

Well, I'm right so long as nothing else further down in the filter chain returns a non-false value, which calling die guarantees won't happen ;) But as this is an internal project, I think I can make sure that's the case.

comment:26 lkraav10 months ago

  • Cc leho@… added
Note: See TracTickets for help on using tickets.