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

[thekt12]

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.

[audrasjb]

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

[huzaifaalmesbah]

I concur with your perspective. Well stated!

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

[joemcgill]

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.

[mukesh27]

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

[audrasjb]

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.

[thekt12]

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

[mukesh27]

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

Do you have a POC for using global variables?

[joemcgill]

Duplicate of #18298.

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

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

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

Thanks @kt-12 for sharing the benchmarking report.

Closing in favor of #5160.