Make WordPress Core

Opened 13 years ago

Closed 13 years ago

Last modified 13 years ago

#18331 closed enhancement (wontfix)

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

Reported by: viper007bond's profile 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 13 years ago.
18331.2.diff (2.0 KB) - added by scribu 13 years ago.
18331.3.patch (2.1 KB) - added by WraithKenny 13 years ago.

Download all attachments as: .zip

Change History (40)

#1 @Viper007Bond
13 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

#2 follow-up: @SergeyBiryukov
13 years ago

  • Description modified (diff)

Corrected the link.

#3 @aaroncampbell
13 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.

#4 in reply to: ↑ 2 @Viper007Bond
13 years ago

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

#5 follow-up: @scribu
13 years ago

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

#6 in reply to: ↑ 5 @Viper007Bond
13 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.

#7 @scribu
13 years ago

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

@scribu
13 years ago

#8 @scribu
13 years ago

  • Keywords has-patch added; needs-patch removed

18331.diff: initial patch; no subfolder parameter.

#9 @ptahdunbar
13 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()

#10 @nacin
13 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.

#11 @scribu
13 years ago

  • Milestone changed from Awaiting Review to 3.3

#12 @Otto42
13 years ago

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

#13 @chipbennett
13 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).

#14 @chipbennett
13 years ago

  • Cc chip@… added

@scribu
13 years ago

#15 @scribu
13 years ago

18331.2.diff: remove unused $subfolder arg.

#16 @Viper007Bond
13 years ago

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

#17 @scribu
13 years ago

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

#18 @scribu
13 years ago

  • Keywords commit added

#19 @nacin
13 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 );

#20 @nacin
13 years ago

In [18665]:

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

#21 follow-up: @scribu
13 years ago

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

I don't see any benefit in that.

#22 @mattrude
13 years ago

  • Cc matt@… added

#23 in reply to: ↑ 21 @westi
13 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.

#24 follow-up: @scribu
13 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.

#25 @nacin
13 years ago

I think we're done here.

Agreed.

#26 @Viper007Bond
13 years ago

Made a subfolder ticket: #18676

#27 in reply to: ↑ 24 @westi
13 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

#28 follow-ups: @ryan
13 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.

#29 @ryan
13 years ago

In [19315]:

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

#30 @WraithKenny
13 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);
}

#31 @nacin
13 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.

#32 in reply to: ↑ 28 @westi
13 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!

#33 in reply to: ↑ 28 ; follow-up: @chipbennett
13 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.

#34 in reply to: ↑ 33 ; follow-up: @Viper007Bond
13 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. ;)

#35 in reply to: ↑ 34 @chipbennett
13 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?

#36 @nacin
13 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..

#37 @WraithKenny
13 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.