Make WordPress Core

Opened 11 months ago

Closed 11 months ago

Last modified 9 months ago

#58866 closed defect (bug) (duplicate)

Avoid consts inside `locate_template` as it gives incorrect template path after switch_theme

Reported by: thekt12's profile thekt12 Owned by: thekt12's profile thekt12
Milestone: Priority: normal
Severity: normal Version:
Component: Themes Keywords: has-patch
Focuses: template Cc:

Description

To replicate

Add this following code in any active plugin -

<?php
function test_localtest_template() {
        print_r("<pre>");
        $current_theme = wp_get_theme()->stylesheet;
        print_r( "<br>Current theme :". $current_theme );
        print_r( "<br>path of current style.css asper locate_template : ". locate_template( 'style.css' ) );
        $themes = wp_get_themes();
        unset( $themes[$current_theme] );
        $switch_to_theme = array_pop($themes)->stylesheet; 
        print_r( "<br>Switching to :". $switch_to_theme );
        switch_theme( $switch_to_theme );
        print_r( "<br>Theme after switching ". wp_get_theme()->stylesheet );
        print_r( "<br>path of new style.css asper locate_template : ". locate_template( 'style.css' ) );
        print_r("</pre>");
        die();
}
add_action( 'init', 'test_localtest_template' );

For me the result were -

Current theme :twentytwentytwo
path of current style.css asper locate_template : /var/www/src/wp-content/themes/twentytwentytwo/style.css
Switching to :twentytwentythree
Theme after switching twentytwentythree
path of new style.css asper locate_template : /var/www/src/wp-content/themes/twentytwentytwo/style.css

The issue was fist discovered while writing testcases for https://core.trac.wordpress.org/ticket/58576

This was further discussed in detail on Making WordPress slack's #core-performance open channel- link to discussion (https://wordpress.slack.com/archives/C02KGN5K076/p1689770957730469)

Similar issue may exist for comments_template and is_child_theme.

Solution is to avoid use of STYLESHEETPATH and TEMPLATEPATH and instead use get_stylesheet_directory() and get_template_directory() at all places.

Meanwhile we must also consider deprecating both the constants as it can't be updated by filter once set giving us two different values (one via constant and one via function) for the same thing.

Change History (15)

#1 @audrasjb
11 months ago

  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to 6.4

The logic of your proposal sounds good.
Let's consider this for 6.4.

#2 @audrasjb
11 months ago

  • Component changed from General to Themes
  • Keywords good-first-bug added

#3 @huzaifaalmesbah
11 months ago

I concur with your perspective. Well stated!

This ticket was mentioned in PR #4880 on WordPress/wordpress-develop by @thekt12.


11 months ago
#4

  • Keywords has-patch added; needs-patch removed

Trac ticket: https://core.trac.wordpress.org/ticket/58866

This bug was found while working on performance ticket https://core.trac.wordpress.org/ticket/58576
This PR replaces usage of STYLESHEETPATH and TEMPLATEPATH with get_template_directory() and get_stylesheet_directory()

This ticket was mentioned in Slack in #core-committers by flixos90. View the logs.


11 months ago

#6 @joemcgill
11 months ago

  • Keywords reporter-feedback added
  • Owner set to thekt12

This looks likely to be a duplicate of #18298. @thekt12 can you confirm? If so, we can likely close this ticket and update the PR to point to the other ticket. Also, we need to be mindful of the performance effects of a change, given that is what caused the fix to the other ticket to be reverted.

#7 @mukesh27
11 months ago

@joemcgill Yesterday, I came across ticket #18298, where I collected the data that I intended to share here. However, you posted it before I could. Thank you for sharing it.

This ticket addresses the deprecation of TEMPLATEPATH and STYLESHEETPATH constants in WordPress and proposes using the respective functions instead.

As mentioned in https://core.trac.wordpress.org/ticket/18298#comment:31, if we switch to using functions instead of constants, we may experience performance regression.

Eliminating TEMPLATEPATH and STYLESHEETPATH was designed to make things more testable, I get that. But that doesn't apply to wp-settings.php, for example. This also exposes the ability to tinker with locate_template() by filtering the theme directory itself, which seems pretty scary. And, the changes in locate_template() mean these functions are called in a loop, rather than outside of it, especially problematic because it could be called up to four times per iteration even though the values might be the same (in the case of no child theme).

#8 @audrasjb
11 months ago

Thanks @joemcgill and @mukesh27 for retrieving the history of this issue.

By the way, would it be possible to avoid performance issues by storing the result of things like get_stylesheet_directory() in variables for the whole file? So the function calls wouldn't be repeated each time when used in a loop.

#9 @thekt12
11 months ago

@joemcgill Yes, I can confirm this is a duplicate of #18298.
@mukesh27 I agree that switching to function will cause performance regression.

w.r.t Real site, unless the site is build to run heavy business logic after switching theme in background, this would hardly cause any major issue as most of the issues dies off once the page is reloaded; It would be just one entry for each theme switch.

However, if we stick to the constants the way it is right now we will lose the ability to write test cases for some functions consuming it. i.e locate_template or is_child_theme

I personally feel both functions and constants are not the best approach, better would be to use global variables for both stylesheet and template.

This way we can call wp_templating_constants() functions again after switch_theme and update the variables. And also we address the looping issue and most of the performance regression caused by use of functions.

Last edited 11 months ago by thekt12 (previous) (diff)

#10 @mukesh27
11 months ago

  • Keywords good-first-bug reporter-feedback removed

Thanks @thekt12. Could we close this ticket and discuss the next step in the #18298?

Do you have a POC for using global variables?

#11 @joemcgill
11 months ago

  • Milestone 6.4 deleted
  • Resolution set to duplicate
  • Status changed from assigned to closed

Duplicate of #18298.

Let's go ahead and close this as a duplicate and pick up conversation in the other ticket.

@flixos90 commented on PR #4880:


11 months ago
#12

@kt-12 I've updated the PR description to reference the ticket https://core.trac.wordpress.org/ticket/18298, given we discovered that the new one is a duplicate.

@thekt12 commented on PR #4880:


9 months ago
#13

@mukeshpanchal27 @felixarntz
Conducted benchmarking of CONST(trunk), FUNCTION(this pr) and GLOBAL(PR #5156).

cmd used to beanchmark npm run research -- benchmark-server-timing -u http://localhost:8889 -n 500

| CONST (trunk) | GLOBAL | FUNCTION |   | BEST | WORST

-- | -- | -- | -- | -- | -- | --
Twenty Twenty-Three |   |   |   |   |   |  
Response Time (median) | 147.69 | 151.99 | 149.52 |   | CONST | GLOBAL
wp-before-template (median) | 55.85 | 57.47 | 60.34 |   | CONST | FUNCTION
wp-template (median) | 88.9 | 91.72 | 86.15 |   | FUNCTION | GLOBAL
wp-total (median) | 145.05 | 149.38 | 146.99 |   | CONST | GLOBAL

|   |   |   |   |   |  

Twenty Twenty-One |   |   |   |   |   |  
Response Time (median) | 143.74 | 142.72 | 143.19 |   | GLOBAL | CONST
wp-before-template (median) | 39.3 | 39.51 | 39.52 |   | CONST | FUNCTION
wp-template (median) | 101.46 | 100.5 | 100.82 |   | GLOBAL | CONST
wp-total (median) | 140.95 | 140.02 | 140.39 |   | GLOBAL | CONST

For block theme constants had a better performance over other methods. wp-template (median) performance was better for function. global was the worst performing candidate. If we need performance then we can keep the constants intact, however if we need flexibility then the next best option asper data is function. Possible reason for global preforming low, might be due to the lookup time in finding variable in hash table ( I couldn't find anything relevant to support his claim though)

For classic theme, the values were near about similar for all 3 scenario with performance of global slightly better than others.

@mukesh27 commented on PR #4880:


9 months ago
#14

Thanks @kt-12 for sharing the benchmarking report.

@flixos90 commented on PR #4880:


9 months ago
#15

Closing in favor of #5160.

Note: See TracTickets for help on using tickets.