Make WordPress Core

Opened 3 months ago

Last modified 42 hours ago

#60025 accepted defect (bug)

Changeset 57156 breaks themes that relied on update_option( 'stylesheet', '...' )

Reported by: blindmikey's profile blindmikey Owned by: joemcgill's profile joemcgill
Milestone: 6.5 Priority: normal
Severity: normal Version: 6.4.2
Component: Themes Keywords: 2nd-opinion needs-testing
Focuses: Cc:

Description

The changes to get_stylesheet_directory() and presumably get_template_directory() will render themes that set a custom stylesheet or template directory via update_option() to render all pages as blank.

Reverting these two functions to their previous state resolves the issue in the meantime.

Change History (22)

#1 @joemcgill
3 months ago

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

Hi @blindmikey, thanks for the report. [57156] reverted both of those functions to their previous behavior prior to 6.4.0, due to issues reported in #59847, so reverting those functions would reintroduce those issues.

Would you mind providing additional reproduction steps so we can investigate? Thanks.

#2 @blindmikey
3 months ago

Issue came about because the theme was storing templates in a subdirectory within the theme, and then setting the wp_option "stylesheet" to "{theme_dir}/templates" where {theme_dir} is the directory name of the child theme. This worked fine in 6.4.0, and in releases prior, but stopped working in 6.4.2 - causing every page to render blank.

The fix for us moving forward is to use the 'stylesheet_directory' filter to set the stylesheet directory to "{theme_dir}/templates".

However this update will effect any theme that's updating the stylesheet directory via updating the "stylesheet" wp_option.

#3 @sfdam
3 months ago

I'm having the same/or similar issue as this. I'm using Roots' Sage base theme version 9.0.0-beta.2 https://github.com/roots/sage/releases/tag/9.0.0-beta.2

It works in 6.4.1 but has a blank frontend in 6.4.2. If I revert the function "get_template_directory" in wp-includes/theme.php it works again but if I switch off and then on my theme it says that my theme is broken. If I revert to version 6.3.2 it prompts me to upgrade the database and everything will work again (including switching the theme off and then on).

#4 @blindmikey
3 months ago

@sfdam In that version of Sage, line 75 of functions.php is setting a subdirectory for templates via update_option('template', ...), similar to what my theme was doing.

A solution for now might be also adding a filter for 'template_directory' that returns "{theme_dir}/templates" where {theme_dir} is the directory name of the active theme.

#5 @CodeGeek
2 months ago

We also have a theme that is based on Roots' Sage base theme version 9 and have also experienced the same issues as reported in this ticket.

What seems to have fixed the issue for us is adding the following filter to our functions.php:

<?php
add_filter('stylesheet_directory', function ($tdir, $temp, $root) {
    if (!str_contains($tdir, 'templates')) {
        $tdir .= "/templates";
    }
   return $tdir;
}, 10, 3);

Then we had to update the lib/setup.php mix() function to change $rootPath from get_stylesheet_directory() to get_template_directory():

<?php
function mix($path)
{
    static $manifest;

    //$rootPath = get_stylesheet_directory();
    $rootPath = get_template_directory();
    $publicFolder = "/dist";
...

As best as we can figure, what appears to be happening is that there is a function is_child_theme() in wp-includes/theme.php which has changed. It had been dependent upon two global constants (STYLESHEETPATH and TEMPLATEPATH), but now it is using the functions get_template_directory() and get_stylesheet_directory().

In the past the two global constants were being set by calling the same functions, but likely before the theme was loaded or initialized. In WP 6.3 and earlier, the is_child_theme() function call returned false. Now that function call is returning true.

So WP is considering the Roots Sage theme to be a child theme now.

With a child theme, the stylesheet directory should be the folder containing the child theme (which in our case is the templates folder) whereas the template directory should be the parent theme (in our case is our theme folder).

So the code above adjusts the stylesheet_directory filter so it returns the templates folder while the mix function uses the parent directory for the rootPath, which is where it looks for all of the other theme files.

This fixed all of our issues except that now that the templates folder is considered a child theme, our theme fails the validate_current_theme check in wp-includes/theme.php(line 878), because the "child theme" doesn't have the expected style.css file. This check is run whenever an admin user visits the appearance->themes dashboard in the backend of wordpress. The failure automatically triggers the theme to be switched to a default (twentytwentyfour). To prevent this check, we have also added the following into our functions.php:

<?php
add_filter('validate_current_theme', function($val) { return false;});

I don't think that the Roots theme should be considered a child theme. But the way the WP code is now working, it is. If the WP authors choose to roll that check back, I don't think that these suggested code changes will cause anything to break, but there could be other factors at play here due to the extensiveness of the recent WP changes.

Last edited 2 months ago by CodeGeek (previous) (diff)

#6 @joemcgill
2 months ago

  • Milestone changed from Awaiting Review to 6.4.3
  • Status changed from reviewing to accepted

After spending some time trying to diagnose what's happening here, I think this bug was introduced by [56635], which deprecated the TEMPLATEPATH and STYLESHEET path constants in favor of get_template_directory() and get_stylesheet_directory(), respectively.

The Sage theme, and probably others, were relying a specific race condition of get_template_directory(), where it initially gets called by wp_templating_constants() in wp-settings.php before the active theme's functions.php file is loaded, and thus, before filters the theme has added are applied. This resulted in the TEMPLATEPATH environment variable always being set to the value of get_option( 'template' ) which was then used by locate_template() and a few other functions. After this environment value is set, the theme then filters the value of get_template_directory() back to the original location so that templates are loaded from a different path than what get_template_directory() will return.

A reduced test case to reproduce this behavior is to do something like this in your functions.php file:

<?php
// Set the template path option to a different location.
update_option( 'template', __DIR__ . '/templates' );

// Filter `get_template()` back to the theme root.
add_filter('template', function () {
    return __DIR__;
});

The reason this issue was only visible starting in WP 6.4.2, is that the original change shipped in 6.4.0 cached the value of get_template_directory() to a static global variable the first time the function was called, similar to how the value was first set as the environment variable by wp_templating_constants(). However, this led to bugs like #59847. Once that cached value was reverted in [57156], the change in behavior to locate_template() became observable.

We could consider fully reverting the change from [56635], but it I think doing so would be choosing to support unintended behavior here, since get_stylesheet() and get_template() (and their respective DB options) are meant to store the directory name of the current theme and parent theme (if applicable), not the path to where the theme stores it's template files (despite the fact that the naming convention used is easy to misunderstand).

I think the proposed workaround here—filtering the template_directory and stylesheet_directory values is a better solution if you need the store templates in a different directory than the theme root, but we also may need to consider revising locate_template() to prefer the unfiltered template and stylesheet option values, rather than the filtered value to maintain backwards compatibility, or introduce a new filter to locate_template() that would more easily allow themes to load their templates from a different location without resorting to these types of workarounds.

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


2 months ago

#8 @joemcgill
2 months ago

#59861 was marked as a duplicate.

#9 @joemcgill
2 months ago

Thinking more about this, the most straightforward way of supporting themes that want to move their templates to a subdirectory of the theme without using the workaround of setting the stylesheet or template option and then filtering the return value of the functions that retrieve that option would be to do something like #41362 and make the path filterable prior to looking for the template rather than trying to juggle the raw option and the filtered option when resolving template paths.

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


2 months ago

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


6 weeks ago

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


5 weeks ago

#13 @jorbin
5 weeks ago

  • Keywords close added

Thank you @joemcgill for all the research into this. This is sounding more and more like a wontfix situation since the previous behavior was both undocumented and unintentional. I do like the idea of exploring something like #41362 to provide this behavior in a documented way.

For that reason, I'm suggesting we close this, but I'm very open to hearing alternatives if someone can identify a small targeted fix for this so I'm going to leave this in the 6.4.3 milestone for the time being.

#14 @joemcgill
5 weeks ago

@jorbin I'm wondering if the approach I suggested in this related issue might also address this bug? Let's not close until that's been investigated.

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


4 weeks ago

#16 @joemcgill
4 weeks ago

  • Keywords 2nd-opinion added; close removed

After testing the latest iteration on this PR for #60290. I believe the same approach will fix this issue as well.

To test, here's an improved snippet that you can add to a theme's index.php file from what I provided earlier

<?php
$theme_base = wp_basename( __DIR__ );
$template_path = "$theme_base/templates";

// Set the template path option to a different location.
update_option( 'template', $template_path );

// Filter `get_template()` back to the theme root.
add_filter('template', function () use ( $theme_base ) {
    return $theme_base;
});

With this, you can add a new directory named "templates" to your theme root and add a template file, like home.php (for the homepage) and see that in the 6.3 branch when visiting the homepage, that the template from the subdirectory would be loaded, but in 6.4 it will not. However, with this PR applied, the previous behavior is restored.

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


4 weeks ago

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


4 weeks ago

#19 @joemcgill
4 weeks ago

  • Milestone changed from 6.4.3 to 6.5
  • Severity changed from critical to normal

Bumping this to the 6.5 milestone to await more testing/feedback on PR 5926

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


4 weeks ago

#21 @iaxosikevemed1966
10 days ago

It seems like changeset 57156 has caused issues for themes that relied on the update_option('stylesheet', '...') function. This function is commonly used to set a custom stylesheet or template directory.

The changes made to get_stylesheet_directory() and get_template_directory() likely affect how these functions retrieve the directory paths, leading to themes rendering all pages as blank.

To address this issue temporarily, reverting the changes made to these two functions back to their previous state could resolve the problem. This would involve undoing the modifications made in changeset 57156 to get_stylesheet_directory() and get_template_directory().

Once the functions are reverted, themes that rely on update_option('stylesheet', '...') should function as expected until a more permanent solution can be implemented or until the affected themes are updated to be compatible with the changes introduced in changeset 57156.

#22 @joemcgill
42 hours ago

  • Keywords needs-testing added; needs-patch removed

I believe [57685] should have also fixed this issue. Could someone confirm?

Note: See TracTickets for help on using tickets.