WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 2 years ago

Last modified 2 years ago

#18331 closed enhancement (wontfix)

Make get_header(), get_sidebar() and get_footer() use get_template_part()

Reported by: Viper007Bond Owned by:
Milestone: Priority: normal
Severity: normal Version: 3.2.1
Component: Template Keywords: has-patch commit
Focuses: Cc:

Description (last modified by SergeyBiryukov)

We should consolidate code and make get_sidebar() and it's compatriots use get_template_part().

While we're at it, these functions should support a subfolder argument so that sidebars can be stored in a "sidebar" folder for example.

AaronCampbell came up with this elegantly simple code on IRC: http://pastebin.com/xYjASLYe

Attachments (3)

18331.diff (2.0 KB) - added by scribu 3 years ago.
18331.2.diff (2.0 KB) - added by scribu 3 years ago.
18331.3.patch (2.1 KB) - added by WraithKenny 2 years ago.

Download all attachments as: .zip

Change History (40)

comment:1 Viper007Bond3 years ago

  • Summary changed from get_sidebar(), get_header(), etc. should make use of get_template_part() to get_sidebar(), get_header(), etc. should be refreshed

comment:2 follow-up: SergeyBiryukov3 years ago

  • Description modified (diff)

Corrected the link.

comment:3 aaroncampbell3 years ago

Related: #15086
Once that one lands we can make all these functions wrappers of get_template_part() AND they can support files in sub-directories cleanly.

comment:4 in reply to: ↑ 2 Viper007Bond3 years ago

Reminder: We should pass the subfolder argument to the action.

comment:5 follow-up: scribu3 years ago

Neat, but what about theme-compat? I'm guessing it's too early to drop it.

comment:6 in reply to: ↑ 5 Viper007Bond3 years ago

Replying to scribu:

Neat, but what about theme-compat? I'm guessing it's too early to drop it.

True. Then something like this should do: http://pastebin.com/7KgqUXR5 Possibly without the sidebar- prefix on the subfolder path.

comment:7 scribu3 years ago

Or, we could make get_template_part() return the result of locate_template().

scribu3 years ago

comment:8 scribu3 years ago

  • Keywords has-patch added; needs-patch removed

18331.diff: initial patch; no subfolder parameter.

comment:9 ptahdunbar3 years ago

  • Cc trac@… added
  • Summary changed from get_sidebar(), get_header(), etc. should be refreshed to deprecate get_header(), get_sidebar() and get_footer() for get_template_part()

comment:10 nacin3 years ago

  • Summary changed from deprecate get_header(), get_sidebar() and get_footer() for get_template_part() to Make get_header(), get_sidebar() and get_footer() use get_template_part()

We won't be deprecating these, just change the internals.

comment:11 scribu3 years ago

  • Milestone changed from Awaiting Review to 3.3

comment:12 Otto423 years ago

+1 to this, however your patch contains an unused $subfolder argument.

comment:13 chipbennett3 years ago

+1 as well, and I'm all in favor of dropping theme_compat, the earlier the better. By now, Theme devs know that they are required to include header.php when calling get_header() (et al).

comment:14 chipbennett3 years ago

  • Cc chip@… added

scribu3 years ago

comment:15 scribu3 years ago

18331.2.diff: remove unused $subfolder arg.

comment:16 Viper007Bond3 years ago

Should I make a new, separate ticket for subfolder support then? (I know it'd be reliant on #15086 going in.)

comment:17 scribu3 years ago

If this patch gets in, the subfolder discussion could be held entirely in #15086.

comment:18 scribu3 years ago

  • Keywords commit added

comment:19 nacin3 years ago

Should we consider something like this in get_template_part(), to make these functions completely wrappers, and get_template_part('header') identical to get_header()?

if ( 'header' == $slug || 'sidebar' == $slug || 'footer' == $slug )
    do_action( "get_$slug", $name );

comment:20 nacin3 years ago

In [18665]:

Use get_template_part() in get_header(), get_sidebar(), get_footer(). props scribu, see #18331.

comment:21 follow-up: scribu3 years ago

if ( 'header' == $slug || 'sidebar' == $slug || 'footer' == $slug )
    do_action( "get_$slug", $name );

I don't see any benefit in that.

comment:22 mattrude3 years ago

  • Cc matt@… added

comment:23 in reply to: ↑ 21 westi3 years ago

Replying to scribu:

if ( 'header' == $slug || 'sidebar' == $slug || 'footer' == $slug )
    do_action( "get_$slug", $name );

I don't see any benefit in that.

I wouldn't put these checks in every call to get_template_part.

If we want to slim the functions down more we could create a function to fire the action and hook it onto the action in get_template_part that is fired in these three cases.

comment:24 follow-up: scribu3 years ago

  • Resolution set to fixed
  • Status changed from new to closed

Or we could just leave them as is.

Adding a helper function to save one line of code isn't good for readability or performance.

I think we're done here.

comment:25 nacin3 years ago

I think we're done here.

Agreed.

comment:26 Viper007Bond3 years ago

Made a subfolder ticket: #18676

comment:27 in reply to: ↑ 24 westi3 years ago

Replying to scribu:

Or we could just leave them as is.

Adding a helper function to save one line of code isn't good for readability or performance.

Agreed - I was just pointing how I would implement it if we did not that I advocate doing it.

I think we're done here.

Agreed

comment:28 follow-ups: ryan2 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

This breaks themes that call get_header() twice as it does a double include of header.php. This seems to be a not uncommon situation.

comment:29 ryan2 years ago

In [19315]:

Revert [18665]. Causes fatal double include in themes that call get_header() twice. see #18331

comment:30 WraithKenny2 years ago

Can we add $require_once to get_template_part to fix?

function get_template_part( $slug, $name = null, $require_once = false ) {
	do_action( "get_template_part_{$slug}", $slug, $name );
	
	$templates = array();
	if ( isset($name) )
		$templates[] = "{$slug}-{$name}.php";

        $templates[] = "{$slug}.php";

        return locate_template($templates, true, $require_once);
}

WraithKenny2 years ago

comment:31 nacin2 years ago

  • Milestone 3.3 deleted
  • Resolution set to wontfix
  • Status changed from reopened to closed

We could, but get_template_part() is a higher level function, and there's been talk about adding more useful arguments like $folder.

This was designed as a simple internal abstraction, but it's not a perfect match. Closing.

comment:32 in reply to: ↑ 28 westi2 years ago

Replying to ryan:

This breaks themes that call get_header() twice as it does a double include of header.php. This seems to be a not uncommon situation.

Great catch!

comment:33 in reply to: ↑ 28 ; follow-up: chipbennett2 years ago

Replying to ryan:

This breaks themes that call get_header() twice as it does a double include of header.php. This seems to be a not uncommon situation.

Ryan, can you provide an example? I'm extremely curious why a Theme would need/want to call get_header() twice.

comment:34 in reply to: ↑ 33 ; follow-up: Viper007Bond2 years ago

Replying to chipbennett:

Ryan, can you provide an example? I'm extremely curious why a Theme would need/want to call get_header() twice.

It shouldn't be, but it's being done anyway. ;)

comment:35 in reply to: ↑ 34 chipbennett2 years ago

Replying to Viper007Bond:

Replying to chipbennett:

Ryan, can you provide an example? I'm extremely curious why a Theme would need/want to call get_header() twice.

It shouldn't be, but it's being done anyway. ;)

So, not that I disagree with the wontfix ticket resolution (I have no real stake in the matter), but I do wonder why we would decide that resolution based on what is essentially a _doing_it_wrong() action?

comment:36 nacin2 years ago

Including a file twice can result in a fatal error pretty easily, so warning them wouldn't help much. You basically have to rely on require_once(), which get_template_part() deliberately avoids using.
?
Any abstraction will require either another parameter or some conditional check for header/footer/sidebar, neither of which is particularly neat and enough for me to come out and say wontfix.

I'm annoyed I didn't catch this first time through as I had been down this path before..

comment:37 WraithKenny2 years ago

Chip, the use-case would be when a themer uses get_header() in a non-traditional way such as using it for an html5 header element include (which can be used more then once) rather then the complete top part of the html document. Now this isn't really normal for WordPress themes, but isn't incorrect per se as there was never a requirement to do it the normal way (only suggested via example in default themes). Hope that answers you question.

Note: See TracTickets for help on using tickets.