WordPress.org

Make WordPress Core

Opened 2 years ago

Last modified 3 weeks ago

#40969 reviewing enhancement

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

Reported by: sphakka Owned by: johnbillion
Milestone: 5.3 Priority: normal
Severity: normal Version:
Component: Themes Keywords: has-patch dev-feedback
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 (6)

40969.diff (379 bytes) - added by tferry 15 months ago.
updates get_template_part to return the value of locate_template
40969.2.diff (665 bytes) - added by tferry 13 months ago.
40969.3.diff (2.0 KB) - added by tferry 8 months ago.
Updates multiple get_*() template part functions to return the value of locate_template
40969.4.diff (1.3 KB) - added by tferry 7 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 7 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 6 months ago.

Download all attachments as: .zip

Change History (38)

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

  • Component changed from General to Themes
  • Focuses template added

#3 @sphakka
2 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
15 months ago

updates get_template_part to return the value of locate_template

#4 @tferry
15 months 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.

Version 0, edited 15 months ago by tferry (next)

#5 @tferry
15 months ago

  • Keywords has-patch added

#6 @johnbillion
13 months 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
13 months ago

#7 @tferry
13 months 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
13 months ago

  • Resolution invalid deleted
  • Status changed from closed to reopened

#9 @tferry
13 months ago

  • Keywords needs-refresh removed

#10 @johnbillion
11 months ago

  • Milestone changed from 5.0 to 5.1

#11 @pento
8 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
8 months ago

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

#12 @tferry
8 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
8 months ago

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

#14 @pento
8 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
7 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
7 months ago

  • Keywords revert added

#17 @johnbillion
7 months ago

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

#18 @desrosj
7 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
7 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
7 months ago

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

#21 @johnbillion
7 months ago

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

@tferry
7 months ago

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

#22 @tferry
7 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
7 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
7 months ago

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

#24 @tferry
7 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
7 months ago

  • Keywords dev-feedback added

#26 @desrosj
7 months ago

  • Keywords close removed

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


6 months ago

#28 @garrett-eclipse
6 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
6 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
4 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
3 weeks 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
3 weeks ago

  • Keywords 2nd-opinion removed
  • Owner changed from pento to johnbillion
  • Status changed from reopened to reviewing
Note: See TracTickets for help on using tickets.