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

[SergeyBiryukov]

has_template_part() seems like a good addition. However, I would also like to reconsider the original approach of adding a return value to get_template_part().

The concerns in comment:15 were about themes outputting the return value via echo get_template_part( ... ):

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

These can both be addressed by returning void (null) on success (same as now) and false on failure, which would not result in any unexpected output, but would still allow for debugging.

This is more or less consistent with some template functions returning void|string on success (void if $echo argument is true, which is the case by default, string otherwise):

• wp_list_authors()
• wp_list_bookmarks()
• wp_list_categories()
• wp_tag_cloud()
• comment_class()
• trackback_url()
• wp_list_comments()
• get_search_form()
• wp_loginout()
• wp_login_form()
• wp_register()
• wp_get_archives()
• get_calendar()

and some other functions returning void|false (void on success, false on failure):

• the_terms()
• wpdb::print_error()
• wpdb::bail()

and also some admin functions returning void|false (void on success, false on failure):

• parent_dropdown()
• update_nag()
• maintentance_nag()
• wp_plugin_update_row()
• wp_theme_update_row()

See 40969.7.diff​. Any feedback welcome :)

[tferry]

Hey @SergeyBiryukov, I'm in favor of your approach - looks like a good way to address the raised concerns while still allowing for error checking... and without the fuss of adding a whole new function.

I would assume that changing the return value of this function also means the related test [46328] would need to be updated to test for a false return value as well as null/void?

[davidbaumwald]

This ticket was marked early for some soak time and we're well beyond that now for making it in for 5.4. With 5.4 Beta 1 landing tomorrow, this is being moved to Future Release. If any maintainer or committer feels this can be included in 5.4 or wishes to assume ownership during a specific cycle, feel free to update the milestone accordingly.

[tferry]

Returning back to this ticket, I've realised I was mistaken in my previous comment - there are already tests on the previous patch (thanks @SergeyBiryukov, and apologies for missing them before).

I've added some further tests that apply to the other get_*() functions that have been updated. As mentioned on a previous patch, testing the other functions for failure is unnecessary due to the theme-compat files (see comment:22).

As such, I believe this is ready for testing, and possible inclusion in the next milestone, but any feedback is welcome.

[tferry]

Last patch simply updates the WP version number in the PHP function comment blocks

[birgire]

Replying to tferry:

Last patch simply updates the WP version number in the PHP function comment blocks

Hi, unable to test it for now, but I wonder if tests can be simplified with assertNull, assertFalse and expectOutputString.

[tferry]

Hey @birgire, thanks for the suggestions - I've updated the tests to be simpler with some of the phpunit functions you mentioned.

I tried expectOutputString() but it didn't handle the newlines in the test files particularly well, so I used expectOutputRegex() instead.

[williampatton]

The use of expectOutputRegex() seems ok to me here :)

[davidbaumwald]

With 5.5 Beta 1 approaching, the time for early tickets has now passed, and this ticket is being moved to Future Release. If any maintainer or committer feels this can be quickly resolved for 5.5 or wishes to assume ownership of this ticket during a specific cycle, feel free to update the milestone accordingly.

[SergeyBiryukov]

In 48209:

Themes: Add a return value to theme functions calling locate_template():

• get_header()
• get_footer()
• get_sidebar()
• get_template_part()

These functions now return false if the template file could not be found, to allow for easier debugging.

Props tferry, sphakka, johnbillion, pento, davidbinda, desrosj, birgire, garrett-eclipse, williampatton, davidbaumwald, SergeyBiryukov.
Fixes #40969.