Make WordPress Core

Opened 11 months ago

Closed 10 months ago

Last modified 9 months ago

#58576 closed enhancement (fixed)

locate_template does not check to see site has parent theme.

Reported by: spacedmonkey's profile spacedmonkey Owned by: flixos90's profile flixos90
Milestone: 6.4 Priority: normal
Severity: normal Version:
Component: Themes Keywords: good-first-bug has-patch commit
Focuses: performance Cc:

Description

In locate_template, function checks to exist if a file exists in parent and child themes. But if the current theme is not a child theme, this check is pointless and checks if the same file exists twice per request.

So

if ( file_exists( STYLESHEETPATH . '/' . $template_name ) ) {
        $located = STYLESHEETPATH . '/' . $template_name;
        break;
} elseif ( file_exists( TEMPLATEPATH . '/' . $template_name ) ) {
        $located = TEMPLATEPATH . '/' . $template_name;
        break;
}

Should become

if ( file_exists( STYLESHEETPATH . '/' . $template_name ) ) {
        $located = STYLESHEETPATH . '/' . $template_name;
        break;
} elseif ( STYLESHEETPATH !== TEMPLATEPATH && file_exists( TEMPLATEPATH . '/' . $template_name ) ) {
        $located = TEMPLATEPATH . '/' . $template_name;
        break;
}

Attachments (2)

58576.diff (731 bytes) - added by nihar007 11 months ago.
Patch added
58576-2.diff (716 bytes) - added by nihar007 10 months ago.
Another patch added

Download all attachments as: .zip

Change History (14)

@nihar007
11 months ago

Patch added

#1 @dhrumilk
11 months ago

  • Keywords has-patch added

#2 @spacedmonkey
11 months ago

  • Milestone changed from Awaiting Review to 6.4

#3 @thekt12
10 months ago

Patch is tested and it works as expected. However, due to use of STYLESHEETPATH and TEMPLATEPATH in locate_template we are unable write testcase around here.
STYLESHEETPATH and TEMPLATEPATH are set to default theme path well before setup() function in a test file.

We generally use switch_theme() inside a testcase to switch to a parent or child test themes.
Unfortunately, this doesn't effect the constants and so locate_template gives out the initial default value irrespective of active theme.

#4 @thekt12
10 months ago

I have added a new ticket https://core.trac.wordpress.org/ticket/58866#ticket for the bug found during writing testcases here.

#5 follow-up: @flixos90
10 months ago

@nihar007 The patch looks good, one recommendation though: Instead of manually checking the condition, I think we should use the is_child_theme() function instead which already contains that logic.

An additional small benefit from using that will be that if we implement #58866 we would not need to update this logic here, but only the used function internally.

#6 in reply to: ↑ 5 @nihar007
10 months ago

Replying to flixos90:

@nihar007 The patch looks good, one recommendation though: Instead of manually checking the condition, I think we should use the is_child_theme() function instead which already contains that logic.

An additional small benefit from using that will be that if we implement #58866 we would not need to update this logic here, but only the used function internally.

Hi @flixos90,
Thanks for your feedback. I am working on it

@nihar007
10 months ago

Another patch added

#7 @mukesh27
10 months ago

  • Keywords commit added

Thank you, @spacedmonkey, for creating this ticket and also, thanks to @nihar007 for providing the patch.

@thekt12, you are absolutely right. We cannot test the functionality with a constant in the unit test. Great catch on issue #58866.

The changes made in the patch look good and appear to be ready for deployment.

Adding the commit keyword to request further consideration. Since Jonny is currently on vacation, @flixos90, could you please handle the commit process for this ticket?

#8 @flixos90
10 months ago

  • Owner set to flixos90
  • Status changed from new to reviewing

#9 @flixos90
10 months ago

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

In 56357:

Themes: Avoid unnecessary check whether parent template file exists when not using a child theme.

Prior to this change, the locate_template() function would unconditionally check whether the relevant template file exists in the parent theme, which for sites not using a child theme is an exact duplicate of the previous check. This is wasteful and has a small impact on performance since file_exists() checks have a cost.

Props nihar007, thekt12, spacedmonkey, mukesh27.
Fixes #58576.

#10 @spacedmonkey
9 months ago

#58384 was marked as a duplicate.

This ticket was mentioned in Slack in #core-performance by joemcgill. View the logs.


9 months ago

This ticket was mentioned in Slack in #core-performance by spacedmonkey. View the logs.


9 months ago

Note: See TracTickets for help on using tickets.