Make WordPress Core

Opened 7 years ago

Last modified 16 months ago

#42855 new enhancement

Add ability to filter header, sidebar, searchform, footer and template_part file paths

Reported by: atanasangelovdev's profile atanasangelovdev Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version:
Component: Themes Keywords: has-patch needs-refresh dev-feedback
Focuses: template Cc:

Description

I've seen several tickets that appear related but are either dead or include bigger changes compared to what I'm proposing (the most relevant being #21062).

My proposal (including patch):

  1. Introduce get_template_part_hierarchy( $slug, $name = null );.
  2. Update get_header(), get_sidebar(), get_search_form(), get_footer() and get_template_part() to use the new function.
  3. Add a template_part_{$slug}_template_hierarchy filter to the new function so all 5 general template functions' results are filterable.

What these changes achieve:

  1. Provide a way to filter the location of header.php, sidebar.php, searchform.php, footer.php and any file loaded by get_template_part()
  2. Reduce code duplication (all metioned functions had the same $slug + $name logic to create a $templates array)

Also, @MikeSchinkel has mentioned a few benefits and arguments for why such a filter is beneficial:
https://core.trac.wordpress.org/ticket/21062#comment:19

Attachments (1)

42855.patch (3.4 KB) - added by atanasangelovdev 7 years ago.

Download all attachments as: .zip

Change History (16)

#1 @atanasangelovdev
7 years ago

  • Keywords has-patch added

#2 @atanasangelovdev
7 years ago

Any response to this suggestion?

This ticket was mentioned in Slack in #core by atanas.angelov.dev. View the logs.


7 years ago

#4 @joyously
7 years ago

I agree with the comments in the referenced ticket, that filtering the template name seems really dangerous, and it is not worth the security risks.
Since locate_template() is not checking for directory traversal, but is checking for 3 specific folders, it is best that the theme and child theme are the only code that can influence what is loaded, and those will usually be literals and that is a good thing.

If there is a filter, not only could the template part name be changed, but the path to it could be changed, and that just doesn't seem right, because it would encourage relative paths to plugins. The flexibility of an installation is that you can define where your folders are, and allowing a filter on a part of a theme template breaks the integrity of the theme. The child theme can change it easily, so it's only plugins that would "benefit" from a filter, and they would have to specify a relative path to the plugin because only 3 theme folders are checked for the file.

It actually doesn't make sense to me to have get_header() able to get something other than the header that the theme defined. Same for footer, sidebar, etc.

#5 follow-up: @atanasangelovdev
7 years ago

Thank you for the feedback, @joyously, I really appreciate it.

A few rebuttals, If I may:

Since locate_template() is not checking for directory traversal, but is checking for 3 specific folders, it is best that the theme and child theme are the only code that can influence what is loaded ...
If there is a filter, not only could the template part name be changed, but the path to it could be changed, and that just doesn't seem right ...
... allowing a filter on a part of a theme template breaks the integrity of the theme.

Except the template_include filter exists which can do exactly what you argue should not be allowed. Also, "doesn't seem right" is not a valid argument.

The child theme can change it easily ...

This is not entirely correct. Yes, you can change the contents of a partial but if you wish to change the location of a partial you have to essentially copy-paste the parent partial/template and change how it loads the partial you wish to move.

... so it's only plugins that would "benefit" from a filter ...

I ended up suggesting this filter because we are building a starter theme, not a plugin, and wanted to reorganize the template/view files away from dotfiles, library code, classes, assets etc. but could not do so to a satisfactory level since core template partials are required to exist and are locked to be in the same directory as the style.css file (this is just one example).

... they would have to specify a relative path to the plugin because only 3 theme folders are checked for the file.

I agree on this point - I can see relative paths from themes to plugin being used and there should be a better solution for this.

It actually doesn't make sense to me to have get_header() able to get something other than the header that the theme defined.

It does not necessarily have to be something different. It can be the header, just in another place or with a fallback. Also the filter does not apply only to core partials but to any partial loaded using get_template_part().


I would really like to stress that the above are just example use cases. @MikeSchinkel has mentioned several other valid use cases that I believe are important (and which convinced @johnbillion to reopen the ticket) and are left without a response/rebuttal:
https://core.trac.wordpress.org/ticket/21062#comment:19

Last edited 7 years ago by atanasangelovdev (previous) (diff)

#6 in reply to: ↑ 5 ; follow-up: @joyously
7 years ago

The child theme can change it easily ...

Yes, you can change the contents of a partial but if you wish to change the location of a partial you have to essentially copy-paste the parent partial/template and change how it loads the partial you wish to move.

That is a good thing, not a problem. It is best (for debug, extending, and review) that each theme follows the same rules.

... so it's only plugins that would "benefit" from a filter ...

I ended up suggesting this filter because we are building a starter theme, not a plugin, and wanted to reorganize the template/view files away from dotfiles, library code, classes, assets etc. but could not do so to a satisfactory level since core template partials are required to exist and are locked to be in the same directory as the style.css file (this is just one example).

That doesn't sound like a valid reason. Each theme is standalone, so what does it matter if the header file is in the main folder? It can always be found, easily, that way. Consistency is a good thing.

It actually doesn't make sense to me to have get_header() able to get something other than the header that the theme defined.

It does not necessarily have to be something different. It can be the header, just in another place or with a fallback.

Of course it does not have to be different, but with a filter, it could be different. And that doesn't make much sense. The theme decides what is in its header template and also decides when to output it. If plugins need to modify the contents, the theme can allow for that, or not. Child themes can very easily change what's in the file. Nothing else needed.

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

Replying to joyously:

There are many reasons for what @atanasangelovdev is suggesting that your replies do not anticipate. For example:

  1. Support for alternate output based on Google AMP and similar technologies. A plugin could capture the header call, set ob_start(), run the theme's header, grab its output with ob_get_clean(), clean up the header to be valid AMP, then output it. A plugin could do the same with the footer and with all partials.
  1. Support for alternate output such as JSON. A plugin could to the same above but generate JSON output instead when a URL parameter ?json is added.
  1. Support for testing using similar techniques to above "unit test" a header, a footer, or a partial.
  1. Output comments to annotate a theme using similar techniques to above for a plugin to specify which header, sidebars and footer were called.

As the web becomes more complicated, the lack of this type of functionality is hamstringing WordPress from being able to be used in contexts that are beyond the simple "Outputting blog posts for people to view via a desktop browser." This aspect of WordPress is still in the aughts.

I really wish the people blocking these types of improvements had worked on some of these use-cases to appreciate how limiting these missing bits of control can be.

#8 in reply to: ↑ 7 ; follow-ups: @joyously
7 years ago

Replying to MikeSchinkel:
I can see your 4th point (perhaps), as a filter, but the others are not. They would be better done as action hooks, which already exist for these functions.

#9 in reply to: ↑ 8 ; follow-ups: @MikeSchinkel
7 years ago

Replying to joyously:

I can see your 4th point (perhaps), as a filter, but the others are not.

Why can you not see the need to make the output of a theme valid AMP? Why not for JSON? Why not for Unit Testing?

They would be better done as action hooks, which already exist for these functions.

"pre" actions exist, but "post" hook do not. So would you support adding post actions to these functions?

Version 1, edited 7 years ago by MikeSchinkel (previous) (next) (diff)

#10 in reply to: ↑ 8 @MikeSchinkel
7 years ago

BTW, @atanasangelovdev I want to thank you for contributing your first ticket.

I do know how agonizing it can feel to make a request here on trac and then have people tell your ideas don't make sense. I for one want to let you know that not everyone will respond that way.

And I also want you to know that at least one person (me) is glad to see you participate here. Don't let the negative replies scare you aware from participating in the future, please.

#11 in reply to: ↑ 9 @MikeSchinkel
7 years ago

Replying to joyously:

CORRECTION: get_template_part() does not have a general purpose "pre" action hook. It's pre hook requires a plugin to have advanced knowledge of the slug, for which an effectively infinite number of permutation could be used.

So we would also need a new general purpose "pre" action hook for get_template_part().

Last edited 7 years ago by MikeSchinkel (previous) (diff)

#12 in reply to: ↑ 9 ; follow-up: @joyously
7 years ago

Replying to MikeSchinkel:

Why can you not see the need to make the output of a theme valid AMP? Why not for JSON? Why not for Unit Testing?

I'm not saying that your use cases should not be done, I'm saying that they would not be accomplished by filtering the part name, as proposed.

They would be better done as action hooks, which already exist for these functions.

"pre" action hooks exist, but "post" action hooks do not.
So are you saying you would join us in advocating for post action hooks to be added to these functions?

Yes, each of the specific functions has a a "pre" action hook (like get_header and get_footer), but nothing after the template part is finished, so the current method would be to know the order they are called in so that the next one can do the finish work on the previous.
Or, add an action for all, and apply logic to the action names that start with get_template_part.
Or, any filtering of the HTML should be done on the client end.

#13 in reply to: ↑ 12 @MikeSchinkel
7 years ago

Replying to joyously:

I'm saying that they would not be accomplished by filtering the part name, as proposed.

I am sorry but I have to disagree with that. But I will instead discuss actions as they work for me as well, assuming they are provided with the name of the "post" template file that was loaded.

so the current method would be to know the order they are called in so that the next one can do the finish work on the previous.

I have done that before for sites when force. But from experience it is very fragile and often conflicts with other plugins. Certainly you are not advocating for this?

Or, add an action for all, and apply logic to the action names that start with get_template_part.

Using the all action is an anti-pattern because it requires processing for every single hook on a page load, which can measure in the thousands.

If the all action were sufficient, WordPress would not have added all the different hooks. Certainly you are not advocating that we write plugins using the overhead of the all action?

Besides all cannot see local variables such as the context often passed as parameters to hooks it is not even a substitute in many cases. Including these.

Or, any filtering of the HTML should be done on the client end.

Again, almost anything that needs a named action or filter could filter on the client end. That is handing the surgeon a sledgehammer when they ask for a scalpel, and it is also fraught with incompatibility with other plugins. Certainly you are not advocating for this?

Can you please explain the explicit scenarios that you fear such that you are making proposals that can result in plugins breaking other plugins rather than concur that these hooks are needed and have been needed for a very long time?

Last edited 7 years ago by MikeSchinkel (previous) (diff)

#14 @ocean90
5 years ago

#49496 was marked as a duplicate.

#15 @oglekler
16 months ago

  • Keywords needs-refresh dev-feedback added

I like to have the ability to work with template paths in the get_footer() and get_header() functions, as get_template_part() does, but I'm not sure about implementation. I just want to have a better template structure and have different parts of the same template (page) in the dedicated folder and not to store all of them in the theme main folder.

Note: See TracTickets for help on using tickets.