RFE: get_template_part() to return something or warn when nothing found

[sphakka]

AFAIK get_template_part() doesn't return anything. This is a nightmare when args are misspelled -- think of a leading/trailing space. Given that internally locate_template() does return what it finds, could we please either expose this return value or warn when nothing is found?

[johnbillion]

Thanks for the ticket, @sphakka.

You're right that get_template_part() doesn't return a value, but if it returned the value of locate_template() I'm not sure that this would tell you anything that you don't already know. If there's a typo in your file name then an empty string will be returned. If there's no typo but the file doesn't exist, it'll still return an empty string.

[sphakka]

The problem is that get_template_part( 'non-existing-file' ) fails silently. Of course page rendering would break, so you'd see that something is wrong, but that's not trivial without a message clue. The rationale is that something like

if ( ! get_template_part( 'I_misspelled_this_one' )
  error_log( 'I_misspelled_this_one: template file not found' );

would help spotting the mistake. It doesn't really need to report what it found (returning bool would be fine), but since locate_template() is the last statement in the code, then just having

return locate_template($templates, true, false);

would settle the matter with minimum effort ;-)

Actually, I now realize that this is an instance of a more general problem with many (all?) get_*() functions calling locate_template() at end. So, along the principle that silent failure is bad practice (unless I'm missing some obscure reason for not having the possibility of checking programmatically what goes on), a review of all those functions would be advised. That would also make them more consistent with other get_whatever() functions that already return something.

[tferry]

updates get_template_part to return the value of locate_template

[tferry]

I agree with @sphakka - I've also run into this issue and it would be useful for get_template_part not to fail silently, so we have the option to check the return value.

I've attached a patch to pass on the return value from locate_template, as suggested.

[johnbillion]

@tferry I agree with this function passing back the result of locate_template(). The docblock needs updating to reflect the new @return value; it should probably be identical to that of locate_template().

[tferry]

Thanks for the spot @johnbillion, docs are super important. Updated with the @return value added to the docblock.

[pento]

Rather than only update get_template_part(), it'd be better to update all of the functions that call locate_template() at the same time.

This will also need an addition to the docblock for each function, along the lines of @since x.y.z Added the return value.

[tferry]

Updates multiple get_*() template part functions to return the value of locate_template

[tferry]

Thanks for the suggestion and guidance @pento

New diff uploaded to add the return values to all get_*() functions within wp-includes/general-template.php (including relevant @since and @return docblock tags)

I've left the @since version tag as literally x.y.z as I don't know when this'll be included in core, but happy to update as and when.

[pento]

In 44678:

Themes: Return the value of locate_template() in functions that call it.

The get_header(), get_footer(), get_sidebar(), and get_template_part() functions all call locate_template() as their final action, which returns the name of the template being loaded. Returning this value helps handle problems with templates being loaded.

Props tferry.
Fixes #40969.

[pento]

I'm re-opening this ticket, as a problem as come up.

There are ​a lot of examples of themes printing the return value of get_template_part(). The function name implies that it returns something, so this isn't really a surprising usage.

This causes two problems:

• Unexpected content being sent to the browser.
• A path disclosure security issue.

Unless anyone has good ideas for a workaround, we're going to need to revert it.

Props @david.binda for discovering this.

[johnbillion]

There's several instances in the theme directory too: ​https://wpdirectory.net/search/01D2YXN15FBXVEYPCRH8CM7K49

[desrosj]

In 44725:

Themes: Revert returning the value of locate_template() in functions that call it.

Because the names of the get_header(), get_footer(), get_sidebar(), and get_template_part() functions indicate that a value is returned, some plugins and themes already have echo get_template_part() in their codebase. Adding a return value to these functions using the approach in [44678] will cause the two unintended side effects of unexpected content being sent to the browser, and accidental path disclosure.

Reverts [44678].

Props davidbinda.
See #40969.

[desrosj]

[44678] has been reverted in trunk. Moving this to 5.2 for further exploration and also marking it close. Unless a better approach can be thought of, this will probably be too risky to introduce.

[johnbillion]

Another option might be a has_template_part() function that can be used for conditional logic.

[tferry]

Adds a new has_template_part() function, which determines whether a template part file exists

[tferry]

Thanks @johnbillion, I like your suggestion of a new function - it solves the original problem while avoiding the issues of changing the return value of the existing function, as discovered. I have attached a diff with this new function for review.

I considered creating equivalent has_*() functions for get_header(), get_sidebar(), and get_footer() respectively, but the use of the theme-compat files in the locate_template() function renders these new functions redundant - they would all return true, regardless of theme files.

[birgire]

Thanks for the patch @tferry

Few suggestions regarding the inline documentation in 40969.4.diff​:

• Add the missing @return part.
• Add the null type.
• Mention the default name value.
• Mention that the name is optional.

So instead of this part:

* @param string $slug The slug name for the generic template.
* @param string $name The name of the specialised template.

it could be:

* @param string      $slug The slug name for the generic template.
* @param string|null $name Optional. The name of the specialised template. Default null.
*
* @return bool Whether the template part exists.

[tferry]

Adds a new has_template_part() function, which determines whether a template part file exists (updated docblock)

[tferry]

Hey @birgire, thanks for the review and recommendations! I've updated the docblock with the missing parts and attached the new diff.

[garrett-eclipse]

Uploaded 40969.6.diff as a minor spelling fix to switch the two instances of 'specialised' to the en_US 'specialized'.

[desrosj]

The 5.2 beta deadline is just about here. Going to punt this one.

[birgire]

It came to mind to bail early within has_template_part() with:

if ( ! is_string( $slug ) || '' === $slug ) {
	return false;
}

to avoid '.php' template cases from these examples:

has_template_part( '' )
has_template_part( false )
has_template_part( null )

but when testing, it seems '.php' will work just fine as a template file, corresponding to locate_template( array( '.php' ) ) and get_template_part( '' ) :-)

Though I haven't seen '.php' used in the real world as a template file, I'm inclined to not suggesting bailing early and keep 40969.6.diff​ as is.

[williampatton]

40969.6.diff​ looks good to me. I'm with @birgire that some of those are edge-casey enough that we do not need any additional early bail in this function to account for them.

[davidbaumwald]

@johnbillion Do you think this ready for 5.3 Beta 1 tomorrow? Would unit tests be a sensible addition to the patch?

I also added needs-dev-note. It may not need a full dev note, but it at least deserves a small call out.

[davidbaumwald]

@williampatton mentioned that there should be some additional testing before this is merged in. With version 5.3 Beta 1 releasing shortly, this is being moved to early 5.4 for more visibility.

[johnbillion]

In 46328:

Themes: Add a test to ensure get_template_part() does not return a value.

This function must not be modified to return anything due to existing themes which output the return value, for example via echo get_template_part( ... ).

See #40969