WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 3 months ago

Last modified 8 weeks ago

#40969 closed enhancement (fixed)

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

Reported by: sphakka Owned by: williampatton
Milestone: 5.5 Priority: normal
Severity: normal Version:
Component: Themes Keywords: has-patch dev-feedback early has-unit-tests needs-testing has-dev-note
Focuses: template Cc:

Description

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?

Attachments (10)

40969.diff (379 bytes) - added by tferry 2 years ago.
updates get_template_part to return the value of locate_template
40969.2.diff (665 bytes) - added by tferry 2 years ago.
40969.3.diff (2.0 KB) - added by tferry 20 months ago.
Updates multiple get_*() template part functions to return the value of locate_template
40969.4.diff (1.3 KB) - added by tferry 19 months ago.
Adds a new has_template_part() function, which determines whether a template part file exists
40969.5.diff (1.4 KB) - added by tferry 19 months ago.
Adds a new has_template_part() function, which determines whether a template part file exists (updated docblock)
40969.6.diff (1.4 KB) - added by garrett-eclipse 19 months ago.
40969.7.diff (2.9 KB) - added by SergeyBiryukov 8 months ago.
40969.8.diff (5.1 KB) - added by tferry 4 months ago.
40969.9.diff (5.1 KB) - added by tferry 4 months ago.
40969.10.diff (5.1 KB) - added by tferry 4 months ago.

Download all attachments as: .zip

Change History (65)

#1 @johnbillion
3 years ago

  • Keywords 2nd-opinion added

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.

#2 @johnbillion
3 years ago

  • Component changed from General to Themes
  • Focuses template added

#3 @sphakka
3 years ago

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
2 years ago

updates get_template_part to return the value of locate_template

#4 @tferry
2 years ago

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.

Last edited 2 years ago by tferry (previous) (diff)

#5 @tferry
2 years ago

  • Keywords has-patch added

#6 @johnbillion
2 years ago

  • Keywords needs-refresh added; 2nd-opinion removed
  • Milestone changed from Awaiting Review to 5.0
  • Owner set to johnbillion
  • Status changed from new to reviewing

@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
2 years ago

#7 @tferry
2 years ago

  • Resolution set to invalid
  • Status changed from reviewing to closed

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

#8 @tferry
2 years ago

  • Resolution invalid deleted
  • Status changed from closed to reopened

#9 @tferry
2 years ago

  • Keywords needs-refresh removed

#10 @johnbillion
2 years ago

  • Milestone changed from 5.0 to 5.1

#11 @pento
21 months ago

  • Keywords needs-refresh added
  • Milestone changed from 5.1 to Future Release

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
20 months ago

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

#12 @tferry
20 months ago

  • Keywords needs-refresh removed

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.

#13 @pento
20 months ago

  • Milestone changed from Future Release to 5.1
  • Owner changed from johnbillion to pento
  • Status changed from reopened to accepted

#14 @pento
20 months ago

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

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.

#15 @pento
20 months ago

  • Keywords has-patch removed
  • Priority changed from normal to high
  • Resolution fixed deleted
  • Severity changed from normal to major
  • Status changed from closed to reopened

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.

#16 @johnbillion
20 months ago

  • Keywords revert added

#17 @johnbillion
20 months ago

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

#18 @desrosj
20 months ago

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.

#19 @desrosj
20 months ago

  • Keywords close added; revert removed
  • Milestone changed from 5.1 to 5.2

[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.

#20 @desrosj
20 months ago

  • Priority changed from high to normal
  • Severity changed from major to normal

#21 @johnbillion
20 months ago

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

@tferry
19 months ago

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

#22 @tferry
19 months ago

  • Keywords has-patch 2nd-opinion added

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.

#23 @birgire
19 months ago

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
19 months ago

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

#24 @tferry
19 months ago

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

#25 @tferry
19 months ago

  • Keywords dev-feedback added

#26 @desrosj
19 months ago

  • Keywords close removed

This ticket was mentioned in Slack in #core by tferry. View the logs.


19 months ago

#28 @garrett-eclipse
19 months ago

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

#29 @desrosj
18 months ago

  • Milestone changed from 5.2 to 5.3

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

#30 follow-up: @birgire
16 months ago

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.

#31 in reply to: ↑ 30 @williampatton
13 months ago

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.

#32 @johnbillion
13 months ago

  • Keywords 2nd-opinion removed
  • Owner changed from pento to johnbillion
  • Status changed from reopened to reviewing

#33 @davidbaumwald
12 months ago

  • Keywords commit needs-unit-tests needs-dev-note added

@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.

This ticket was mentioned in Slack in #core by williampatton. View the logs.


12 months ago

#35 @davidbaumwald
12 months ago

  • Keywords early added; commit removed
  • Milestone changed from 5.3 to 5.4

@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.

#36 @johnbillion
12 months ago

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

This ticket was mentioned in Slack in #core by david.baumwald. View the logs.


8 months ago

#38 @SergeyBiryukov
8 months ago

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 :)

This ticket was mentioned in Slack in #core by david.baumwald. View the logs.


8 months ago

#40 @tferry
8 months ago

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?

#41 @johnbillion
8 months ago

  • Owner johnbillion deleted

#42 @davidbaumwald
8 months ago

  • Keywords needs-refresh added
  • Milestone changed from 5.4 to Future Release

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.

This ticket was mentioned in Slack in #core by tferry. View the logs.


4 months ago

#44 @SergeyBiryukov
4 months ago

  • Milestone changed from Future Release to 5.5

@tferry
4 months ago

#45 @tferry
4 months ago

  • Keywords has-unit-tests needs-testing added; needs-unit-tests needs-refresh removed

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
4 months ago

#46 follow-up: @tferry
4 months ago

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

#47 in reply to: ↑ 46 @birgire
4 months ago

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
4 months ago

#48 @tferry
4 months ago

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.

#49 @williampatton
4 months ago

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

This ticket was mentioned in Slack in #core by david.baumwald. View the logs.


4 months ago

#51 @williampatton
4 months ago

  • Owner set to williampatton

#52 @davidbaumwald
3 months ago

  • Milestone changed from 5.5 to Future Release

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.

#53 @SergeyBiryukov
3 months ago

  • Milestone changed from Future Release to 5.5

#54 @SergeyBiryukov
3 months ago

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

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.

Note: See TracTickets for help on using tickets.