WordPress.org

Make WordPress Core

Opened 4 years ago

Last modified 3 months ago

#15086 accepted enhancement

get_template_part() should let you specify a directory

Reported by: aaroncampbell Owned by: westi
Milestone: Future Release 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 4 years ago.
15086.002.diff (9.2 KB) - added by aaroncampbell 4 years ago.
15086.003.diff (4.2 KB) - added by aaroncampbell 4 years ago.
15086.004.diff (3.9 KB) - added by aaroncampbell 3 years ago.
15086.005.diff (3.9 KB) - added by aaroncampbell 3 years ago.

Download all attachments as: .zip

Change History (65)

comment:1 aaroncampbell4 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

comment:2 scribu4 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'.

comment:3 aaroncampbell4 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.

comment:4 nacin4 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.

comment:5 aaroncampbell4 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.

comment:6 follow-up: westi4 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!

comment:7 in reply to: ↑ 6 ; follow-up: Viper007Bond4 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).

comment:8 in reply to: ↑ 7 westi4 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.

comment:9 follow-up: nacin4 years ago

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

comment:10 in reply to: ↑ 9 westi4 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 ;-)

comment:11 follow-ups: aaroncampbell4 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.

comment:12 in reply to: ↑ 11 westi4 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.

comment:13 in reply to: ↑ 11 Viper007Bond4 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). ;)

aaroncampbell4 years ago

comment:14 aaroncampbell4 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.

comment:15 Viper007Bond4 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.

comment:16 aaroncampbell4 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?

aaroncampbell4 years ago

comment:17 aaroncampbell4 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()`

comment:18 nacin4 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?

comment:19 aaroncampbell4 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.

comment:20 nacin4 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.

aaroncampbell4 years ago

comment:21 aaroncampbell4 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.

comment:22 rmccue4 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.

comment:23 nacin3 years ago

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

comment:24 sorich873 years ago

  • Cc sorich87@… added

comment:25 aaroncampbell3 years ago

  • Cc aaroncampbell added

comment:26 aaroncampbell3 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).

aaroncampbell3 years ago

comment:27 aaroncampbell3 years ago

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

comment:28 scribu3 years ago

Similar: #11216

comment:29 scribu3 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.

comment:30 aaroncampbell3 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

comment:31 gruvii3 years ago

  • Cc gruvii added

comment:32 follow-up: aaroncampbell3 years ago

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

comment:33 scribu3 years ago

  • Keywords 3.2-early removed

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

comment:34 aaroncampbell3 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.

aaroncampbell3 years ago

comment:35 in reply to: ↑ 32 westi3 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

comment:36 aaroncampbell3 years ago

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

comment:37 scribu3 years ago

Marked #18567 as dup.

comment:38 Viper007Bond3 years ago

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

comment:39 scribu2 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 2 years ago by scribu (previous) (diff)

comment:40 WraithKenny2 years ago

  • Cc Ken@… added

comment:41 WraithKenny2 years ago

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

comment:42 kwight2 years ago

  • Cc kwight@… added

comment:43 eddiemoya2 years ago

  • Cc eddie.moya+wptrac@… added

comment:44 DrewAPicture2 years ago

  • Cc xoodrew@… added

comment:45 MZAWeb2 years ago

  • Cc MZAWeb added

comment:46 Daniel Koskinen2 years ago

  • Cc wordpress@… added

comment:47 scribu2 years ago

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

comment:48 bitacre2 years ago

I would like this

comment:49 navjotjsingh2 years ago

  • Cc navjotjsingh@… added

comment:50 sethmatics22 months 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?

comment:51 nacin22 months 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.

comment:52 scribu22 months 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

comment:53 husobj22 months ago

  • Cc ben@… added

comment:54 goto1015 months ago

  • Cc dromsey@… added

comment:55 kovshenin15 months ago

  • Cc kovshenin added

comment:56 retlehs15 months ago

  • Cc retlehs added

comment:57 emzo7 months ago

  • Cc wordpress@… added

comment:58 dgwyer7 months ago

  • Cc d.v.gwyer@… added

comment:59 greenshady7 months ago

  • Cc justin@… added

comment:60 nacin3 months ago

#18676 was marked as a duplicate.

Note: See TracTickets for help on using tickets.