Make WordPress Core

Opened 13 years ago

Last modified 5 years ago

#15086 accepted enhancement

get_template_part() should let you specify a directory

Reported by: aaroncampbell's profile aaroncampbell Owned by: westi's profile westi
Milestone: Priority: normal
Severity: normal Version: 3.0
Component: Themes Keywords: has-patch westi-likes needs-unit-tests 2nd-opinion
Focuses: Cc:

Description

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)?

Attachments (5)

15086.001.diff (1.4 KB) - added by aaroncampbell 13 years ago.
15086.002.diff (9.2 KB) - added by aaroncampbell 13 years ago.
15086.003.diff (4.2 KB) - added by aaroncampbell 13 years ago.
15086.004.diff (3.9 KB) - added by aaroncampbell 13 years ago.
15086.005.diff (3.9 KB) - added by aaroncampbell 13 years ago.

Download all attachments as: .zip

Change History (68)

#1 @aaroncampbell
13 years ago

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

#2 @scribu
13 years ago

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'.

#3 @aaroncampbell
13 years ago

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.

#4 @nacin
13 years ago

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.

#5 @aaroncampbell
13 years ago

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.

#6 follow-up: @westi
13 years ago

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!

#7 in reply to: ↑ 6 ; follow-up: @Viper007Bond
13 years ago

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).

#8 in reply to: ↑ 7 @westi
13 years ago

  • Keywords needs-patch added; dev-feedback removed
  • Milestone changed from Awaiting Review to 3.1
  • Type changed from enhancement to defect (bug)

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.

#9 follow-up: @nacin
13 years ago

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

#10 in reply to: ↑ 9 @westi
13 years ago

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 ;-)

#11 follow-ups: @aaroncampbell
13 years ago

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.

#12 in reply to: ↑ 11 @westi
13 years ago

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.

#13 in reply to: ↑ 11 @Viper007Bond
13 years ago

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). ;)

#14 @aaroncampbell
13 years ago

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.

#15 @Viper007Bond
13 years ago

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

#16 @aaroncampbell
13 years ago

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?

#17 @aaroncampbell
13 years ago

  • Keywords dev-feedback has-patch added; needs-patch removed

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

#18 @nacin
13 years ago

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?

#19 @aaroncampbell
13 years ago

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.

#20 @nacin
13 years ago

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.

#21 @aaroncampbell
13 years ago

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.

#22 @rmccue
13 years ago

  • Cc me@… added

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.

#23 @nacin
13 years ago

  • Keywords 3.2-early added
  • Milestone changed from 3.1 to Future Release
  • Type changed from defect (bug) to enhancement

#24 @sorich87
13 years ago

  • Cc sorich87@… added

#25 @aaroncampbell
13 years ago

  • Cc aaroncampbell added

#26 @aaroncampbell
13 years ago

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).

#27 @aaroncampbell
13 years ago

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

#28 @scribu
13 years ago

Similar: #11216

#29 @scribu
13 years ago

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.

#30 @aaroncampbell
13 years ago

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

#31 @gruvii
13 years ago

  • Cc gruvii added

#32 follow-up: @aaroncampbell
13 years ago

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

#33 @scribu
13 years ago

  • Keywords 3.2-early removed

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

#34 @aaroncampbell
13 years ago

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.

#35 in reply to: ↑ 32 @westi
13 years ago

  • Keywords 3.3-early westi-likes needs-unit-tests added; dev-feedback removed
  • Owner set to westi
  • Status changed from new to accepted

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

#36 @aaroncampbell
13 years ago

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

#37 @scribu
13 years ago

Marked #18567 as dup.

#38 @Viper007Bond
13 years ago

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

#39 @scribu
12 years ago

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.

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

#40 @WraithKenny
12 years ago

  • Cc Ken@… added

#41 @WraithKenny
12 years ago

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

#42 @kwight
12 years ago

  • Cc kwight@… added

#43 @eddiemoya
12 years ago

  • Cc eddie.moya+wptrac@… added

#44 @DrewAPicture
12 years ago

  • Cc xoodrew@… added

#45 @MZAWeb
12 years ago

  • Cc MZAWeb added

#46 @Daniel Koskinen
12 years ago

  • Cc wordpress@… added

#47 @scribu
12 years ago

  • Keywords 2nd-opinion added; 3.3-early removed

#48 @bitacre
12 years ago

I would like this

#49 @navjotjsingh
12 years ago

  • Cc navjotjsingh@… added

#50 @sethmatics
12 years ago

  • Cc sethmatics added

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?

#51 @nacin
12 years ago

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.

#52 @scribu
12 years ago

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

#53 @husobj
12 years ago

  • Cc ben@… added

#54 @goto10
11 years ago

  • Cc dromsey@… added

#55 @kovshenin
11 years ago

  • Cc kovshenin added

#56 @retlehs
11 years ago

  • Cc retlehs added

#57 @emzo
11 years ago

  • Cc wordpress@… added

#58 @dgwyer
11 years ago

  • Cc d.v.gwyer@… added

#59 @greenshady
10 years ago

  • Cc justin@… added

#60 @nacin
10 years ago

#18676 was marked as a duplicate.

#61 @SergeyBiryukov
9 years ago

#30288 was marked as a duplicate.

This ticket was mentioned in Slack in #themereview by philip. View the logs.


9 years ago

#63 @mattoperry
8 years ago

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.

Note: See TracTickets for help on using tickets.