WordPress.org

Make WordPress Core

Opened 5 years ago

Last modified 13 months ago

#13265 reopened enhancement

Filter results of get_page_templates()

Reported by: nathanrice Owned by: nathanrice
Milestone: Future Release Priority: normal
Severity: normal Version: 3.0
Component: Themes Keywords: has-patch
Focuses: template Cc:

Description

It doesn't make sense to not have a filter on the returned value of this function. I can think of several instances where this would be useful.

I also inserted a "pre_" filter at the beginning of the function so plugins/themes can short-circuit the function as well.

I'm hoping this is small enough to get in for 3.0.

Attachments (9)

theme.php.diff (653 bytes) - added by nathanrice 5 years ago.
13265.patch (834 bytes) - added by wpsmith 3 years ago.
Filter moved to WP_Theme
core-page-templates.zip (1.7 KB) - added by wpsmith 3 years ago.
Sample plugin per Nacin's request
core-page-templates.2.zip (1.7 KB) - added by wpsmith 3 years ago.
Sample plugin per Nacin's request
plugin.php (2.0 KB) - added by wpsmith 3 years ago.
test.php (540 bytes) - added by wpsmith 3 years ago.
template file for plugin.php
core-remove-page-templates.php (1.2 KB) - added by downstairsdev 18 months ago.
A plugin test case for the page_templates filter. Removes a page template that is used in the TwentyFourteen theme.
core-add-page-templates.zip (3.1 KB) - added by downstairsdev 18 months ago.
Uses the proposed page_templates filter to inject a new template.
13265.diff (4.2 KB) - added by nacin 17 months ago.
Add post context to the filter.

Download all attachments as: .zip

Change History (67)

comment:1 follow-up: @nacin5 years ago

  • Milestone changed from 3.0 to Future Release

There's a typo in the patch. I don't like the idea of adding filters after the feature freeze and especially once we're deep in beta because we generally don't have the time to develop the idea out to the point where we are satisfied with implementation, as we need to then account for filters in the future. Maintaining back compat is difficult, and this just makes it more so.

In this case, two tickets jump out at me for why we should not be rushing this: #12253 and #10959. Just because we return page templates doesn't mean they won't then work.

comment:2 follow-up: @nacin5 years ago

  • Cc nacin westi ryan removed

CC should be used so people can subscribe themselves, not so others can subscribe them (unless requested). We look at all of the tickets, we don't need the extra emails.

@nathanrice5 years ago

comment:3 in reply to: ↑ 1 @nathanrice5 years ago

Replying to nacin:

There's a typo in the patch.

Indeed there was. Fixed and re-uploaded.

I don't like the idea of adding filters after the feature freeze and especially once we're deep in beta

Fair enough. I knew it was a long-shot for 3.0. Just hoped it was a small enough change to make it in. You guys know more about the larger implications than I do, so I defer to your judgment on that one.

we need to then account for filters in the future. Maintaining back compat is difficult, and this just makes it more so.

Is it just a matter of naming conventions? I've got no problem renaming the filter to something more specific like "get_page_templates" to match the function name, if that helps.

In this case, two tickets jump out at me for why we should not be rushing this: #12253 and #10959. Just because we return page templates doesn't mean they won't then work.

I don't see the relevance in #12253, but I do see some issue with #10959. But given the fact that a plugin/theme could filter, and thus control, the entire array (keys and values), assuming they do it correctly, the new page template would show up in the dropdown with the correct file location as the value (folder/file.php or folder/folder/file.php).

comment:4 in reply to: ↑ 2 @nathanrice5 years ago

Replying to nacin:

CC should be used so people can subscribe themselves, not so others can subscribe them (unless requested). We look at all of the tickets, we don't need the extra emails.

Sorry, probably bad timing. I just recall westi mentioning that this was the best way to get him to take a look at a patch:
http://lists.automattic.com/pipermail/wp-hackers/2009-February/024952.html (his last sentence).

If that does still apply, I can understand why that might not necessarily be true at the moment, with 3.0 lurking.

comment:5 @nacin5 years ago

  • Cc westi added

Sorry, probably bad timing. I just recall westi mentioning that this was the best way to get him to take a look at a patch: http://lists.automattic.com/pipermail/wp-hackers/2009-February/024952.html (his last sentence).

Fair enough. I'll re-CC westi especially since he was heavily involved in #10959, which I think could pose problems here if we ever revisit #11216. (I meant that ticket, 12253 is actually [12253], the commit in #10959.)

comment:6 follow-up: @westi5 years ago

This definitely needs exploration before popping a set of filters in as we are quite particular in what does work.

It would be interesting to see how you intend to use the filter from a plugin too :-)

comment:7 in reply to: ↑ 6 @nathanrice5 years ago

  • Cc nathanrice added

Replying to westi:

It would be interesting to see how you intend to use the filter from a plugin too :-)

Plugins, maybe not so much :-) Though, it is theoretically possible, assuming I had access to the page templates array. Would be fun :-)

comment:8 @scribu5 years ago

I think #14310 covers this.

comment:9 @scribu5 years ago

Nevermind, I confused it with get_page_template().

comment:10 @scribu4 years ago

  • Keywords has-patch added

comment:11 @nathanrice4 years ago

  • Keywords page templates filter removed

comment:12 @pross4 years ago

  • Cc pross@… added

comment:13 @arpowers4 years ago

I'd also like to submit a request that something is done with this.. Adding static files for every page template is not ideal.

A filter here would allow page templates to be added dynamically, and would allow page template names to be changed on the fly.

comment:14 follow-ups: @nacin4 years ago

This won't be enough to make this work. You'd also have to filter get_page_template() (via the {$type}_template filter in get_query_template(), with $type being 'page'), because the validate_file() check in get_page_template() will fail.

Selecting a special template doesn't help much when the template loader can't do anything with it.

In order to back a patch like this, it would be great seeing a plugin uploaded here that then implements something using it. That way we know we're doing it correctly. If that happens soon this is something that can make it into trunk after some review.

comment:15 @arpowers4 years ago

Ya, basically having this functionality is the only thing preventing database driven template handing.

In PageLines' framework, templates are more dynamic. Basically, it allows users to configure their own templates using the 'drag and drop' interface.

Ideally we'd be able to allow them to create and remove templates at will, then set those templates up.
Unfortunately, because of this issue; we'd have to resort to generating static files, which leads to other problems.

We will look in to doing the patch/plugin for you if needed.

comment:16 @mikeschinkel4 years ago

  • Cc mikeschinkel@… added

comment:17 in reply to: ↑ 14 @johnjamesjacoby4 years ago

Replying to nacin:

This won't be enough to make this work. You'd also have to filter get_page_template() (via the {$type}_template filter in get_query_template(), with $type being 'page'), because the validate_file() check in get_page_template() will fail.

Selecting a special template doesn't help much when the template loader can't do anything with it.

bbPress 2.0/2.1 both have functions to do everything except filter get_page_templates(). I stopped short of that, because it simply was not possible to do.

It's neigh impossible to accurately guess the mark-up needed to match a header/body/footer in a cookie-cutter page template; twentyeleven and twentyten are different enough where the same 1 page template will not work with both themes. As it stands, I'm not convinced that putting a filter on get_page_template() actually will solve the problems outlined here; maybe for parent/child themes, or for creating some kind of "theme compatibility pack" for each specific theme where functionality can be almost guaranteed.

In order to back a patch like this, it would be great seeing a plugin uploaded here that then implements something using it. That way we know we're doing it correctly. If that happens soon this is something that can make it into trunk after some review.

See: bbp-template-functions.php for the functions needed to bypass WordPress limitations.

See: bbp-template-loader.php to see how bbPress mirrors WordPress's template loader.

See: bbp-theme-compatibility.php to see how bbPress shoehorns itself into existing themes.

Even with WordPress core filters, the functions bbPress uses still need to exist; instead they would filter existing WordPress core functions that currently lack filters. The filters you mention above would make it easier for plugins to ship working templates or template parts with their new functionality without needing to write custom template loaders.

If anyone is looking to incorporate a patch for WordPress core and is unsure where to start, I'm happy to provide some direction and support.

comment:18 @anointed3 years ago

  • Cc anointed added

comment:19 @anmari3 years ago

  • Cc anmari@… added

comment:20 @wpsmith3 years ago

  • Cc travis@… added

This would be a good addition, especially for plugins who want to add a page template for certain themes.

comment:21 @ciobi3 years ago

  • Cc alex.ciobica@… added
  • Resolution set to fixed
  • Status changed from new to closed

comment:22 @ciobi3 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

sorry about that :)

comment:23 @jontro3 years ago

I think this would be a very nice addition as well. We are using wordpress as a pure cms and being able to override the template list would help us greatly.

comment:24 in reply to: ↑ 14 @F J Kaiser3 years ago

Replying to nacin:

In order to back a patch like this, it would be great seeing a plugin uploaded here that then implements something using it. That way we know we're doing it correctly. If that happens soon this is something that can make it into trunk after some review.

Normally, the relevance of a plugin is meassured by its downloads. BUT: Every update equals a download. Conclusion: Deliver tons of minor updates & make a plugin become relevant. (As I've seen with my own, every update: downloads += 70%).

If you want to know how relevant such an issue is, then you'll have to search the support channels: Tons of Qs about it.

Last edited 3 years ago by F J Kaiser (previous) (diff)

comment:25 follow-up: @nacin3 years ago

I didn't say anything about relevance. I wanted to see a sample plugin that implemented the patch.

comment:26 in reply to: ↑ 25 ; follow-up: @F J Kaiser3 years ago

Replying to nacin:

I didn't say anything about relevance. I wanted to see a sample plugin that implemented the patch.

Just that I get this right: You want to know what the possible use case might be?

comment:27 in reply to: ↑ 26 ; follow-up: @nacin3 years ago

Replying to F J Kaiser:

Just that I get this right: You want to know what the possible use case might be?

Not even that. I want to see a plugin that successfully implements the hooks in this patch and shows the selected template on the frontend.

comment:28 in reply to: ↑ 27 @F J Kaiser3 years ago

Replying to nacin:

Not even that. I want to see a plugin that successfully implements the hooks in this patch and shows the selected template on the frontend.

I know that the current patch isn't working. And I also don't know if it worked for 2.9(? - 12 month ago). The only thing I want is to not let this ticket die, as people seem to need it.

The only real problem is that you can't add templates to the meta box drop down, but as I can't find a ticket for that, I'm adding to this one. If there's a ticket for the drop-down: We don't need the current ticket.

comment:29 @helenyhou3 years ago

#22566 was marked as a duplicate.

comment:30 @dway3 years ago

It seems this feature request is not getting attention anymore, but it's a really good one. What about its integration to core ?

comment:31 @pbaylies3 years ago

  • Cc pbaylies added

comment:32 @mordauk3 years ago

  • Cc pippin@… added

comment:33 @ethitter3 years ago

  • Cc erick@… added

@wpsmith3 years ago

Filter moved to WP_Theme

@wpsmith3 years ago

Sample plugin per Nacin's request

@wpsmith3 years ago

Sample plugin per Nacin's request

@wpsmith3 years ago

@wpsmith3 years ago

template file for plugin.php

comment:34 @mercime3 years ago

  • Cc mercijavier@… added

comment:35 @SergeyBiryukov3 years ago

@wpsmith: uploading attachments doesn't trigger any email notifications, so whenever you've uploaded something, please leave a comment.

comment:36 @andyadams2 years ago

  • Cc andyadamscp@… added

comment:37 @stephenharris2 years ago

  • Cc stephenharris added

I don't think the filter should be inside the WP_Theme & parent themes.

Adding the filter in the class method means a call to get_page_templates() will trigger that filter twice (potentially): once for the parent templates - and once for the child & parent templates.

It would make sense to just filter the parent & child templates in the function get_page_templates() instead.

comment:38 @toscho2 years ago

  • Cc info@… added

comment:40 @SergeyBiryukov2 years ago

#24333 was marked as a duplicate.

comment:41 @c3mdigital2 years ago

#21891 was marked as a duplicate.

comment:42 follow-up: @GaryJ18 months ago

  • Focuses template added

One small use case that doesn't appear to have been mentioned yet - having a page templates filter would allow a child theme (or plugin) to hide a template included with the parent theme, that the developer doesn't want made available to clients in the Page Templates dropdown list.

comment:43 in reply to: ↑ 42 @downstairsdev18 months ago

Replying to GaryJ:

That's the use case I have with a project. I'd like to make certain page templates in the theme available only if portfolio post types are active on the site. I'm working around the lack of filter with javascript for the moment: http://wptheming.com/?p=14753

Below I attached a plugin that uses the page_templates filter to remove a template from TwentyFourteen (page-templates/contributors.php): https://core.trac.wordpress.org/attachment/ticket/13265/core-remove-page-templates.php

If that specific template has been selected for the page the #page-template select box will default to "Default Template"- but the actual template loaded on the page does not update until the post is re-saved.

I also tested @wpsmith's plugin. I was able to select the "test" page template successfully from the admin- however, the template did not appear to load "test.php" template on the front end. Perhaps a little more work is needed? I'll see if I can find a solution.

Last edited 18 months ago by downstairsdev (previous) (diff)

@downstairsdev18 months ago

A plugin test case for the page_templates filter. Removes a page template that is used in the TwentyFourteen theme.

comment:44 @MikeSchinkel18 months ago

I've got the same use-case on my current project too. We have the ability to "skin" a section of the site, and in that section only templates of the selected skin should be valid.

comment:45 @nathanrice18 months ago

having a page templates filter would allow a child theme (or plugin) to hide a template included with the parent theme, that the developer doesn't want made available to clients in the Page Templates dropdown list.

I hadn't thought of that. Would be VERY useful.

Do we need an updated patch? What's the next step to getting this ticket reconsidered?

@downstairsdev18 months ago

Uses the proposed page_templates filter to inject a new template.

comment:46 @downstairsdev18 months ago

I modified the code by @wpsmith that adds a template via a plugin. It loads the correct template on the front-end now. See: https://core.trac.wordpress.org/attachment/ticket/13265/core-add-page-templates.zip

The latest patch by @wpsmith still applies fine, just offset a couple lines:
https://core.trac.wordpress.org/attachment/ticket/13265/13265.patch

comment:47 @nacin17 months ago

In 27297:

Add a filter to remove or rename page templates for a theme. This does not yet handle adding page templates. see #13265.

comment:48 follow-up: @nacin17 months ago

  • Milestone changed from Future Release to 3.9

[27297] handles two common use cases:

  • Removing a page template you don't want to appear.
  • Renaming a page template (this runs post-translation).

It deliberately prevents new templates, for the moment. I didn't want this ticket to keep getting stuck on that without giving theme authors tools that are otherwise straightforward. (One use case highlighted would be a child theme removing a parent theme's template, for example.)

By all means, the discussion for new templates can continue. As downstairsdev can attest, I do follow this ticket, as last night at the WordPress Austin meetup I knew he had posted a patch. :-) I don't really love what happens in plugin.php, does anyone else have a preference?

p.s. I didn't mean to omit props on [27297]; that was a group effort. I'll make sure people end up on the credits page if they don't otherwise.

comment:49 in reply to: ↑ 48 @MikeSchinkel17 months ago

Replying to nacin:

[27297] handles two common use cases:

+1 for partial progress.

It deliberately prevents new templates, for the moment.
By all means, the discussion for new templates can continue.

Would you be able to summarize the concerns you see for adding templates and/or potential scenarios where adding might be a problem? I ask so that the 19 people watching this ticket might know what concerns need to be addressed before the ability to add templates could become a reality.

@nacin17 months ago

Add post context to the filter.

comment:50 follow-up: @nacin17 months ago

13265.diff adds post context to the filter. This would be necessary so as not to rely on the global post for things like deprecating a template and only allowing it to be used for posts that already use it.

Would you be able to summarize the concerns you see for adding templates and/or potential scenarios where adding might be a problem?

At the moment, page template values are relative paths to physical template files. I'll be honest, the proposed plugins are cleaner than I was expecting, but the use of the template_include hook has me wanting something a bit better.

I suppose this could work:

add_filter( 'page_templates', function( $templates ) {
    $template['my_plugin_template_one'] = 'My Plugin Template';
});

add_filter( 'page_template', function( $template ) {
    if ( is_page_template( 'my_plugin_template_one' ) ) {
        return dirname( __FILE__ ) . '/templates/one.php';
    }
    return $template;
});

Something feels wrong about it. Page templates are fairly rigid, are only available for pages, and are intrinsic to the theme. For nearly all use cases, a plugin can't just call get_header() and get_footer() and pray the markup will work. It makes sense that a child theme or a parent theme needs to register them, at least in their current form. That's more of an indictment of themes, yes, but my point is templates in plugins just don't work well. So should we encourage what is a certain level of "slop"?

comment:51 in reply to: ↑ 50 @MikeSchinkel17 months ago

Replying to nacin:

Something feels wrong about it. Page templates are fairly rigid, are only available for pages, and are intrinsic to the theme. For nearly all use cases, a plugin can't just call get_header() and get_footer() and pray the markup will work. It makes sense that a child theme or a parent theme needs to register them, at least in their current form. That's more of an indictment of themes, yes, but my point is templates in plugins just don't work well.

Ok, thanks for taking the effort to clarify and I do indeed see the concern. OTOH let me give you a use-case where adding page templates could be beneficial and yet would not violate your concerns?

Consider a soft drink company with many brands that has a large WordPress website for a company-wide campaign where they allow a representative from each of their brands to publish pages on this shared website. Of course each brand has it's own design standards and each brand hires their own agency to build out their brand's WordPress page templates. The IT group for this company has set up the server so that each brand can update their brand's folder but only their folder (after an exhaustive code review, of course.) The brand folders are located like so:

  • /wp-content/themes/soft-drinks-r-us/brands/{brand}/

Thus either the theme or ideally a must-use plugin would need to be able to add the pages templates for the brand associated with the current logged-in user. In the case of a plugin it would just be providing the logic about where to find the page templates in the theme but even if done in the theme's functions files it would still need to be able to add files using the new 'page_templates' filter.

One way to support this use-case yet still disallow plugins adding their own templates would be to simply disallow any new absolute paths but do allow any new relative paths.

However...

So should we encourage what is a certain level of "slop"?

While the usage you describe would definitely be slop I could envision valid use-cases for having a plugin add a page template of it's own such as when the plugin needs to control the formatting. Image a promotional coupon plugin that allowed the user to publish a coupon with a URL path of /christmas-2014-promo/ or similar. Maybe the plugin developer wants the coupon to be formatted to look like an award certificate; in that case the plugin would rightly need to be in complete control of the formatting, and such a plugin would be a lightweight way for a motivated DIYer to add specialty coupons to their website.

Another use-case could be for a poorly-funded government agency that has a [mandate to provide an API for their data. A plugin could provide one of their non-technical staffers a page template that would format the values they typed into meta fields to be output as a JSON file, maybe at an URL path of /api/crime-stats/2013/.

Of course I'm stretching these use-cases a bit to provide examples that people might want to achieve, but limiting them means they definitely will not be any happy accidents for use-cases that require adding page templates. And it's really hard to know what good things people might dream up.

Conversely I'd ask if the potential downside of allowing it is likely to result in significant negative behavior. First off it's something people will have to learn how to do, it's not a case of them doing by accident. And most people building plugins want people to use their plugin but if they create a bad UX then fewer people will use it so it would seem to have a built-in disincentive to use badly. And disallowing it removes the potential for positive serendipity so why not allow and just document in the hook that it's a bad idea to use it to add page templates that expect their layout to work in an existing theme?

But even so, please at least allow adding page templates with relative paths, i.e. within deeper subdirectories.

Last edited 17 months ago by MikeSchinkel (previous) (diff)

comment:52 follow-up: @downstairsdev17 months ago

Thanks for the commit @nacin. This solves my use case of removing page templates: https://github.com/devinsays/portfolio-press/commit/3e5e50f5e25ee5303f26bd15b78eed146bef6ebc

@MikeSchinkel I didn't quite understand your first use case until I tested it. Page templates are available if they are in theme/folder/page-template.php, but not if they are another directory level deep, e.g. theme/folder/folder/page-template.php. I didn't realize that. I had assumed WordPress would find a template anywhere in the theme directory structure.

For a plugin to add a page template I would expect to:

1) Be completely self-sufficient (i.e. not rely on theme templates or styling)
2) Require the use of page data in some way (title, content, meta fields)

I think @MikeSchinkel's examples meet the criteria.

I could see a potential issue if someone wanted to create a basic theme and then offer additional features through a paid add-on plugin. They might be tempted to include the additional templates in the plugin even though it would be much preferable to have them in the theme folder.

However, I don't feel too strongly either way on this topic.

comment:53 in reply to: ↑ 52 @MikeSchinkel17 months ago

Replying to downstairsdev:

@MikeSchinkel I didn't quite understand your first use case until I tested it. Page templates are available if they are in theme/folder/page-template.php, but not if they are another directory level deep, e.g. theme/folder/folder/page-template.php. I didn't realize that. I had assumed WordPress would find a template anywhere in the theme directory structure.

Yep, get_page_templates() uses WP_Theme::get_files() to one level deep, not more, probably for performance reasons.

comment:54 @nacin17 months ago

In 27470:

Rename the new page_templates filter to theme_page_templates, and pass it a post object for proper context.

see #13265.

comment:55 @nacin17 months ago

In 27471:

I am having a bad day. see #13265.

comment:56 @nacin17 months ago

  • Milestone changed from 3.9 to Future Release

I opened #27334 to track the removing and renaming of page templates using the theme_page_templates filter. It is now closed as fixed on 3.9, and I'm going to move this ticket back out to Future Release for further discussion on adding page templates.

comment:57 @SergeyBiryukov16 months ago

#27967 was marked as a duplicate.

comment:58 @varun2113 months ago

Here's a use case for being able to add new templates:

An example would be child themes (like Prose from StudioPress) which may want to allow user's to create custom templates and keep them outside the theme's directory (like under /uploads/prose/mytemplate.php) so that the template isn't deleted when the theme is upgraded.

Note: See TracTickets for help on using tickets.