Opened 2 years ago
Last modified 2 months ago
#40969 reviewing enhancement
RFE: get_template_part() to return something or warn when nothing found
Reported by: |
|
Owned by: |
|
---|---|---|---|
Milestone: | 5.4 | Priority: | normal |
Severity: | normal | Version: | |
Component: | Themes | Keywords: | has-patch dev-feedback needs-unit-tests needs-dev-note early |
Focuses: | template | Cc: | |
PR Number: |
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)
Change History (42)
#3
@
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.
#4
@
18 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.
#6
@
16 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()
.
#7
@
16 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.
#11
@
11 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.
@
11 months ago
Updates multiple get_*() template part functions to return the value of locate_template
#12
@
11 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
@
11 months ago
- Milestone changed from Future Release to 5.1
- Owner changed from johnbillion to pento
- Status changed from reopened to accepted
#15
@
10 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.
#17
@
10 months ago
There's several instances in the theme directory too: https://wpdirectory.net/search/01D2YXN15FBXVEYPCRH8CM7K49
#19
@
10 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.
#21
@
10 months ago
Another option might be a has_template_part()
function that can be used for conditional logic.
@
10 months ago
Adds a new has_template_part()
function, which determines whether a template part file exists
#22
@
10 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
@
10 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.
@
10 months ago
Adds a new has_template_part() function, which determines whether a template part file exists (updated docblock)
#24
@
10 months ago
Hey @birgire, thanks for the review and recommendations! I've updated the docblock with the missing parts and attached the new diff.
This ticket was mentioned in Slack in #core by tferry. View the logs.
9 months ago
#28
@
9 months ago
Uploaded 40969.6.diff as a minor spelling fix to switch the two instances of 'specialised' to the en_US 'specialized'.
#29
@
9 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:
↓ 31
@
6 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
@
3 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
@
3 months ago
- Keywords 2nd-opinion removed
- Owner changed from pento to johnbillion
- Status changed from reopened to reviewing
#33
@
2 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.
Thanks for the ticket, @sphakka.
You're right that
get_template_part()
doesn't return a value, but if it returned the value oflocate_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.