get_template_part() should let you specify a directory

[aaroncampbell]

IT would be nice for get_template_part() to allow you to specify a directory to look for a file in. Right now you actually *can* do this, but it requires passing a 'slug' to the function like directory/slug. Since everywhere else in the code slugs are sanitized, this seems like an unexpected way to allow this functionality (I didn't realize this worked until @nacin pointed it out). Since this slug isn't actually sanitized at all, you can currently do get_template_part( '../../../test' ); which seems rather unsafe (get_template_part should be able to include from outside the themes directory).

I suggest sanitizing $slug and adding a third [optional] parameter that allows you to specify the directory to look in. The directory parameter should be sanitized enough to not allow it to start with a . or a / (although this more likely belongs in locate_template() as something done to $template_name inside the foreach).

What does everyone think about this approach?

How many themes do we think are currently using the $slug parameter to specify a directory?

Right now the optional $name parameter is set up as a fall through, so if $slug-$name.php doesn't exist $slug.php is used. Should $directory be set up similarly ($directory/$slug-$name.php -> $directory/$slug.php -> $slug-$name.php -> $slug.php)?

[aaroncampbell]

Other tickets to consider:

• #11216 - Support page templates located in a subdirectory of the theme
• #12877 - Modular themes: Apply template hierarchy to folders within a theme
• #15061 - Allow either array or string to be passed to locate_template
• #13691 - Make get_template_part() accept multiple template names

[scribu]

get_template_part() should not be alowed to look outside the current theme directory.

Maybe we should just sanitize it so that it allows 'directory/slug', but not '../slug'.

[aaroncampbell]

If we do that it would fix the main issue, but then I think we should at least change what we call it to something other than $slug. Everywhere else in our code $slug is a sanitized string (I'm pretty sure they're all letters, numbers, underscores, and hyphens only). Also, doing it that way saves some backward compatibility, but it also means that you can't fall back through to files in the root theme directory. I checked some of the popular themes (like themehybrid and Genesis) and haven't seen any that use get_template_part() that way yet, but I've only got to check a few so far.It would be nice if someone with access to the themes repository could run something like this grep -inR --color "get_template_part\s*([^/]*/.*)" * and let us know how often this use of get_template_part() is.

Scribu did find a few themes using it this way by ​using Google, but it's hard to tell how many would actually be affected.

[nacin]

I don't see the full slug and sanitization correlation but we can probably change the args to $name and $context.

There are more than a few themes using get_template_part to traverse directories.

[aaroncampbell]

Grrr. I understand it's necessary, but it's times like this when I really hate backwards compatibility. Anyway, I suppose we have to stick to the current setup.

class-simplepie.php has remove_dot_segments() which I think is exactly what should be done to $slug.

And definitely +1 for renaming the arg to something other than slug, since it seems really inconsistent with what we use that term for everywhere else.

[westi]

Personally, I never intended for get_template_part() to look in directories.

The fact that it does is a bug IMHO.

But I guess some theme authors appear to like creating lots of directories within the theme.

It looks like @Viper007Bond added the directory example to the codex :-(
​http://codex.wordpress.org/index.php?title=Function_Reference/get_template_part&diff=92384&oldid=90074

The phpdoc was always clear about the functions intentions.

Putting the files in directories makes it harder for child themes to override as they now have to mimic the directory structure even if they want to override a single part.

I would be tempted to sanitise out everything that isn't legal in a filename!

[Viper007Bond]

Replying to westi:

It looks like @Viper007Bond added the directory example to the codex :-(
​http://codex.wordpress.org/index.php?title=Function_Reference/get_template_part&diff=92384&oldid=90074

I added it as a WP.com VIP did it and I thought it was a clever organizational method. I didn't realize it wasn't intentional to allow such a thing. :)

Personally I really like the idea of it though. Say for example you have a bunch of ad template parts. Rather than having to clutter up the main directory, you can just make an "ads" folder and stick all your ad templates in there.

We shouldn't break backwards compatibility by not allowing slashes in my opinion. Perhaps we can deprecate it though in favor of an additional argument so that get_template_part( 'banner', false, 'ads' ) will look for /ads/banner.php and then /ads-banner.php or something.

Or maybe we just leave it as is with some better security (probably the best).

[westi]

Replying to Viper007Bond:

Replying to westi:

It looks like @Viper007Bond added the directory example to the codex :-(
​http://codex.wordpress.org/index.php?title=Function_Reference/get_template_part&diff=92384&oldid=90074

I added it as a WP.com VIP did it and I thought it was a clever organizational method. I didn't realize it wasn't intentional to allow such a thing. :)

Personally I really like the idea of it though. Say for example you have a bunch of ad template parts. Rather than having to clutter up the main directory, you can just make an "ads" folder and stick all your ad templates in there.

We shouldn't break backwards compatibility by not allowing slashes in my opinion. Perhaps we can deprecate it though in favor of an additional argument so that get_template_part( 'banner', false, 'ads' ) will look for /ads/banner.php and then /ads-banner.php or something.

Or maybe we just leave it as is with some better security (probably the best).

Thank you for illustrating the use case.

I guess it would be nice to switch to encouraging the directory to be a separate argument but preserving support for the old method.

Whatever a bit of an extra security blanket would be worth having although I hope people are not taking user submitted data and passing it to get_template_part ;-)

I like the idea of switching to adding a folder argument so that child themes can override simply but dropping in a file in the root and not have to worry about recreating the folder structure.

[nacin]

I think folder hierarchy is best considered in the context of #12877.

[westi]

Replying to nacin:

I think folder hierarchy is best considered in the context of #12877.

Still no convinced we need to rewrite the world like that ticket might imply ;-)

[aaroncampbell]

What if we did something like this:

1. Add the third parameter for $directory
2. Explode $slug on the last DIRECTORY_SEPARATOR
3. If $directory is empty but we got a directory from exploding $slug, use that
4. Sanitize $slug as we usually do
5. Sanitize $directory using something like remove_dot_segments() to remove any . or .. segments
6. Evangelize and tell people that the proper way to use directories is it to use the third parameter...update docs, etc.
7. Some day down the road remove the excess code.

That way would keep backwards compatibility, while still moving us in the direction that seems best.

[westi]

Replying to aaroncampbell:

What if we did something like this:

1. Add the third parameter for $directory
2. Explode $slug on the last DIRECTORY_SEPARATOR
3. If $directory is empty but we got a directory from exploding $slug, use that
4. Sanitize $slug as we usually do
5. Sanitize $directory using something like remove_dot_segments() to remove any . or .. segments
6. Evangelize and tell people that the proper way to use directories is it to use the third parameter...update docs, etc.
7. Some day down the road remove the excess code.

That way would keep backwards compatibility, while still moving us in the direction that seems best.

This sounds great.

[Viper007Bond]

Replying to aaroncampbell:

What if we did something like this:

+1, although switch 2 and 3 obviously for performance (no need to explode if we got a third parameter). ;)

[aaroncampbell]

The patch is a quick and dirty example of what I mean. The real question I still have is how we should go about using remove_dot_segments(). It's currently part of Simple Pie, so I'm wondering if we should just pull it out and put it in wp-includes/formatting.php (I think we're keeping up Simple Pie ourselves now right?), or if there's a better way.

Also, @Viper007Bond, the reason to still split $slug is to remove the directory from it. It makes it so you have to put the directory as the third parameter OR as part of the slug, but not both. Also, I ended up using substr to grab the parts of the string I need. Maybe explode would be better, but I'm not sure.

[Viper007Bond]

If you're passing the third argument, it's safe to assume that you aren't passing a folder in the first argument too.

[aaroncampbell]

So what do you think should happen if someone calls it like this:

get_template_part( 'something/somewhere/somefile', 'somename', 'somedir/anotherdir' );

Should it look for somedir/anotherdir/somethingsomewheresomefile-somename.php or should it look for somedir/anotherdir/somefile-somename.php. My patch does the latter, and not splitting up $slug (just sanitizing it with sanitize_file_name) would accomplish the former.

Either way, that's pretty minor (I can easily do it either way). The real question is whether or not we're keeping up Simple Pie (I thought that we were, but I want to make sure). If we are, I'll pull out remove_dot_segments(), put it in wp-includes/formatting.php, and start using it. If not, maybe we roll it into the start of our own version of this function?

[aaroncampbell]

This new patch pulls remove_dot_segments() out of simple pie, puts it into formatting.php, and uses it in get_template_part()`

[nacin]

I'd rather not remove part of SimplePie like that. That's a method in an external package which we keep sync'd. People might be using SimplePie apart from formatting.php and such, or be expecting the method to exist.

We should have path sanitization elsewhere, no?

[aaroncampbell]

I don't see where else we have it, but I could be missing it. I've been asking in this ticket and on IRC all week trying to get someone to weigh in on that. It was my understanding that we were now keeping up Simple Pie ourselves (I though that's what happened after the ​2010-10-01 dev meeting).

Since I thought we were maintaining it, I thought it would be nice to reduce our codebase by better integrating it with WP. However, if it's still an external package, would it make more sense to add that function to formatting.php WITHOUT removing it from simplepie? IT would accomplish the same thing, it just wouldn't shrink simplepie by 100+ lines.

[nacin]

That's a year old. Ryan McCue now maintains it at ​http://github.com/simplepie/simplepie.

I don't know how much active development is going on, but he's handling bugs and such. He's a WordPress core contributor, so we do sometimes fix stuff and send it upstream to him. Whenever that happens we usually pull his fixes downstream. So perhaps stagnated, not dead, is a better way to define the project.

[aaroncampbell]

The new patch doesn't modify class-simplepie.php at all. Unfortunately, that means that we have the exact same code in 3 places (that function is in two places in class-simplepie.php), but everything works and simplepie remains unchanged.

[rmccue]

Thanks for picking up that it was defined twice Aaron. If there's anything else anyone notices with SimplePie, ping me on IRC (rmccue), email me[at]ryanmccue.info or CC the same address.

On a sidenote, the one-dot-two branch is what WP should be using if you're pulling it downstream. That'll also include a number of backported fixes, such as a fix for bad decoding of URLs. It'll be packaged up as 1.2.1 some time soon, but there are some things (non-code-related) stopping us.

[aaroncampbell]

The 004 patch is just 003 refreshed to work on the current trunk. It gives us a remove_dot_segments() function in wp-includes/formatting.php and it allows for you to specify a third directory parameter for get_template_part while still supporting the directory being added as part of the first slug parameter (should be completely backwards compatible).

[aaroncampbell]

I just re-did the 004 patch for coding standards on the new remove_dot_segments().

[scribu]

Similar: #11216

[scribu]

Meh, I see you already linked to it at the beginning.

I don't think the $directory parameter is worth it at this point, since we can't get rid of the old way, but the sanitization is definetly a good idea.

[aaroncampbell]

Well, I personally am still in favor of doing it right and eventually (it'll be a while, I know) completely sanitizing the $slug as a slug. However, I'm fine with whatever gets decided upon. Maybe we can talk about it in an upcoming devchat

[aaroncampbell]

I'd like to try to get this into 3.3. Anyone against that?

[scribu]

Wouldn't it be better to use basename() and dirname() instead of the substr() trickery?

[aaroncampbell]

I'm honestly not sure why I did it that way. I'm uploading a refreshed patch that uses basename() and dirname() and works on current trunk.

[westi]

Replying to aaroncampbell:

I'd like to try to get this into 3.3. Anyone against that?

Seems reasonable.

Could you open a ​Unit Test trac ticket and attach some tests for the new code - some for the functionality of get_template_part (we can add another dummy theme to the unit tests for this) and for the functionality of remove_dot_segments

[aaroncampbell]

I opened ​#UT22 because I'm running into an issue with the tests for get_template_part().

[scribu]

Marked #18567 as dup.

[Viper007Bond]

Made a ticket for get_(header|sidebar|footer): #18676

[scribu]

As I've said previously, I don't see the benefit of a $directory parameter, as implemented in this patch.

What would be interesting if it were a boolean parameter, which, when set to true, would give "{$slug}/{$name}.php", instead of "{$slug}-{$name}.php".

So, with $dir = false:

/theme/loop.php
/theme/loop-category.php
/theme/loop-default.php

and with $dir = true:

/theme/loop.php
/theme/loop/category.php
/theme/loop/default.php

This way, you actually increase the flexibility of get_template_part(), instead of merely adding another syntax for the same thing.

[WraithKenny]

for organization, scibu's $dir flag sounds awesome.

[bitacre]

I would like this

[sethmatics]

I was looking for this feature today, really hoping to take some of the themes that use include file and large file libraries and convert them to use get_template_part(). At first I thought I would use the hack of putting the slash into the first parameter, but after reading this, if we follow scribu's last comment it would definitely break my themes if that implementation is the answer and I used the subfolder hack that was originally posted to the codex. Do you guys have any idea which direction your going to take this?

[nacin]

Do you guys have any idea which direction your going to take this?

Regardless of what we do here, we will not "break" the use of a forwards slash in the first parameter. It will always continue to work, even if it stops becoming best practice.

[scribu]

What nacin said. This will always work the same:

get_template_part( 'subdir/loop', get_post_format() );

/theme/subdir/loop-standard.php
/theme/subdir/loop.php

And if we add the dir flag, you'd get a sub-sub directory:

get_template_part( 'subdir/loop', get_post_format(), true );

/theme/subdir/loop/standard.php
/theme/subdir/loop.php

[nacin]

#18676 was marked as a duplicate.

[SergeyBiryukov]

#30288 was marked as a duplicate.

[mattoperry]

I'm specifically interested in the part of this thread that concerns directory traversal ... though Nacin mentions that some themes use get_template_part to traverse directories, this strikes me as a clear misuse of this function, and indeed a potential vulnerability. Any interest in at least applying validate_file or the like before get_template_part loads a template? Otherwise we'll have to require it to be called before get_template_part anyway in the case of any not-100%-trusted arguments to that function .. which would kind of bite.

I'm pretty agnostic on the other issues in this thread, but preventing inclusion from outside the current theme seems important.