Make WordPress Core

Opened 13 months ago

Closed 12 months ago

Last modified 7 months ago

#59847 closed defect (bug) (fixed)

Since WordPress 6.4, the functions.php of a theme moved to a different location using register_theme_directory is no longer called

Reported by: partyfrikadelle's profile partyfrikadelle Owned by: joemcgill's profile joemcgill
Milestone: 6.4.2 Priority: normal
Severity: critical Version: 6.4
Component: Bootstrap/Load Keywords: has-patch fixed-major dev-reviewed commit has-testing-info has-unit-tests
Focuses: template, performance Cc:

Description

We operate a WordPress Multisite instance with a quite specific setup. Part of our setup is that we do not store the themes we developed in the normal wp-content/themes folder, but in a special folder of a plugin. We register this folder with register_theme_directory so that the themes are recognized by WordPress.

Unfortunately, this no longer works since the update to 6.4. I have noticed that the functions get_stylesheet_directory() and get_template_directory() produce different results depending on when they are called in the process.

If I call both functions in the index.php of the theme, the correct paths to the moved themes are outputted. However, if I call the two functions in the function wp_get_active_and_valid_themes() in /wp-includes/load.php, incorrect paths to the standard theme directory are generated, which do not exist. As a result, the functions.php of the parent theme and child theme are not found and the files are not included.

I am still trying to figure out exactly what is causing this and would provide a patch if I should find a solution before others.

Best regards!

Attachments (2)

mre-test-for-register-theme-directory.zip (2.5 KB) - added by partyfrikadelle 13 months ago.
MRE for register_theme_directory that works.
mre-test-for-register-theme-directory.2.zip (2.6 KB) - added by partyfrikadelle 13 months ago.
New Version of MRE Test fpor Register Theme Directory that fails

Download all attachments as: .zip

Change History (57)

#1 @coreyw
13 months ago

All of our sites have or will be broken by this update as well. This has to do with the new memoization of the get_stylesheet_directory() and get_template_directory() functions. They only memoize if there are no filters applied on template, theme_root, or template_directory, but the first call to these functions runs even before must-use plugins, thus they always cache the first result which is the default theme directory and then causes register_theme_directory to be useless. Huge oversight to release this.

#2 @joemcgill
13 months ago

  • Focuses performance added
  • Owner set to joemcgill
  • Status changed from new to reviewing

Hi all, thanks for reporting the issue. I suspect one of the changes introduced in #18298 is the cause for this issue. Likely [56635]. Could you provide a reduce test case that demonstrates the issue?

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


13 months ago

#4 @coreyw
13 months ago

  • Focuses performance removed

@flixos90 introduced in [56635].

@joemcgill reduced test case is simply to use register_theme_directory('/test/directory/or/whatever') within a must-use plugin, place a theme in that specified directory, and then attempt to activate that theme.

Last edited 13 months ago by coreyw (previous) (diff)

#5 @coreyw
13 months ago

  • Focuses performance added

@partyfrikadelle
13 months ago

MRE for register_theme_directory that works.

#6 @partyfrikadelle
13 months ago

Unfortunately, it's not that simple. I tried to create an MRE and wrote a simple plugin for that, which registers a theme folder in the plugin directory as an additional theme folder. There, I stored a very simple theme with a function in the functions.php, which is called from the index.php.

In this very simple function, it works. But @coreyw's hint still seems to be correct. Probably in my setup, the get_stylesheet_directory() is called somewhere in advance, before my plugin can register the respective folders.

I continue to work on figuring out exactly what the issue is...

@partyfrikadelle
13 months ago

New Version of MRE Test fpor Register Theme Directory that fails

#7 @partyfrikadelle
13 months ago

By adding calls to get_stylesheet_directory() and get_template_directory() before the register_Theme_directory i was able to reproduce the error.

Just install the second Version of the Plugin and activate it. Then activate the new "Simple Test Theme". Under WordPress 6.3 it works fine and fails with WordPress 6.4.

I imagine the get_stylesheet_directory/get_template_directory-Calls are done by prioritzed Plugins in my setup.

#8 follow-up: @coreyw
13 months ago

@partyfrikadelle good work. You're right, WP core does not call get_stylesheet_directory() that early by default. In my case, we are including Advanced Custom Fields as a "must-use" plugin, and it calls that function first, before our custom theme directory is registered. A "simple" solution is to make our plugin load first (must-use are loaded in alphabetical order far as I know). Since there is a workaround I'm not sure if there needs to be any fix applied, but it should be documented that calls to register_theme_directory() should be prioritized.

Edit: in my case I also need to add add_filter('theme_root', fn (string $value): string => $value); while registering the custom directory for it to work.

Last edited 13 months ago by coreyw (previous) (diff)

#9 @partyfrikadelle
13 months ago

@coreyw That makes sense, we also use ACF. Although it would be great if our current setup could just continue to work, I understand that redirecting the themes folders is probably so rare that it doesn't make sense to sacrifice caching for it.

However, it should definitely be mentioned in the register_theme_directory documentation.

#10 follow-up: @joemcgill
13 months ago

Thanks for the investigation @partyfrikadelle. I've been struggling to reproduce this locally with a default install of WP, but seeing something call get_stylesheet_directory() early makes sense. I wonder if it would be useful for core to reset the globals for $wp_stylesheet_path and $wp_template_path when register_theme_directory() is called, just to be safe?

#11 in reply to: ↑ 8 @partyfrikadelle
13 months ago

Replying to coreyw:

Edit: in my case I also need to add add_filter('theme_root', fn (string $value): string => $value); while registering the custom directory for it to work.

Interesting. Got it working on my Development-Site for our Multisite Project too, but also had to add a theme_root-filter. Even if i add the register_theme_directory-Call as the first Call in the mu-plugins i have to add the filter.

I have no idea at the moment what could call get_stylesheet_directory or get_template_directory before that.

I'll give it another look tomorrow. Its getting late in germany.

#12 in reply to: ↑ 10 @partyfrikadelle
13 months ago

Replying to joemcgill:

Thanks for the investigation @partyfrikadelle. I've been struggling to reproduce this locally with a default install of WP,

The Plugin i uploaded should® work on a clean wordpress install. I used it on a fresh Docker-Wordpress-Image and just installed wp-downgrade for changing the wp-releases.

but seeing something call get_stylesheet_directory() early makes sense. I wonder if it would be useful for core to reset the globals for $wp_stylesheet_path and $wp_template_path when register_theme_directory() is called, just to be safe?

I'll test that tomorrow!

#13 follow-up: @coreyw
13 months ago

@joemcgill It doesn't appear that resetting the globals would help. It seems that calls to get_stylesheet_directory() return an invalid path if called before a certain point (when the theme is setup perhaps).

So, when ACF calls it during its setup (plugins_loaded or similar), get_stylesheet_directory() returns the default theme path (wp-content/themes/whatever), but any call to that function during after_setup_theme returns the correct path (custom-directory/whatever).

So this could be the fault of ACF calling the function too early (I've opened a ticket with them to investigate), but there is no mention of this being an issue on the docs page. Or, WordPress should be handling this better and not returning an invalid path.

Last edited 13 months ago by coreyw (previous) (diff)

#14 in reply to: ↑ 13 @joemcgill
13 months ago

Replying to coreyw:

@joemcgill It doesn't appear that resetting the globals would help. It seems that calls to get_stylesheet_directory() return an invalid path if called before a certain point (when the theme is setup perhaps).

Interesting. Perhaps the answer would be to make sure we don't memoize these paths until the plugins_loaded or setup_theme action has fired, to ensure that early calls to those functions don't result in stale values?

#15 @coreyw
13 months ago

@joemcgill further digging here. get_raw_theme_root seems to be part of the issue.

if ( ! is_array( $wp_theme_directories ) || count( $wp_theme_directories ) <= 1 ) {
        return '/themes';
}

If I comment out that return '/themes'; line, everything works. Before after_setup_theme, $wp_theme_directories only contains one entry, the custom one we registered. After, it contains the default directory and our custom one. I wonder why that conditional is there in the first place.

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


13 months ago

#17 @kdowns
13 months ago

I am noting this same issue but slightly different context.

I have a plugin that hooks stylesheet_directory filter to pass in a custom value. The hook fires on plugins_loaded and generally works with a couple of exceptions.

The issue is noted any time $wp_stylesheet_path is defined before my filter can be applied which results in $stylesheet_dir = apply_filters( 'stylesheet_directory', $stylesheet_dir, $stylesheet, $theme_root ); becoming unreachable.

This can occur if $wp_stylesheet_path ends up being called and defined by translations or a different plugin based on plugin load priority.

Test case example 1, I have a WordPress site running a French translation. The translations load prior to plugins_loaded which results in $wp_stylesheet_path being defined before my filter can be applied. This results in the inability to apply my filter.

Test case example 2, my plugin is named B-example.php and contains hooks into the stylesheet_directory filter. I also have plugin A-example.php active which through some form or another results in get_stylesheet_directory() being called which in turn will define $wp_stylesheet_path. Because plugin A is loaded first due to naming order,$wp_stylesheet_path is defined before plugin B can fire it's hook to add it's filter.

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


13 months ago
#18

  • Keywords has-patch added

get_template_directory and get_template_directory memoize the values for the active theme. However, if a plugin calls these functions before a custom theme directory is registered, a stale value could end up being stored. This ensures that calls to these functions prior to plugins being loaded aren't memoized.

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

#19 follow-ups: @joemcgill
13 months ago

  • Keywords has-patch removed

I doubt that conditional is part of the problem. That early return was added 12 years ago in [20015] and the conditional hasn't changed in 6 years [41174]. It's probably coincidental that commenting out that line fixes the issue.

I've put together a proof of concept PR that seems to fix the original issue, by making sure that those functions only memoize the paths when called after plugins have loaded.

@kdowns I'm curious if this approach would address your issue as well?

#20 @joemcgill
13 months ago

  • Keywords has-patch added
Last edited 13 months ago by joemcgill (previous) (diff)

#21 @coreyw
13 months ago

I'm guessing the return '/themes'; line was there to short-circuit the function and avoid the database lookup because it assumes that the default theme directory must be the only directory in $wp_theme_directories array. The "problem" though, is that the default theme directory is not registered in wp-settings.php until after muplugins_loaded has run.

So if you register a custom directory in a must-use plugin, (and if other must-use plugins reference that through use of get_stylesheet_directory() or similar), then things will be off.

I feel like a must-use plugin is the most logical place to register a custom directory though. One workaround right now is to always register at least two custom directories, but that seems a little silly to register a dummy directory (plus it has to actually exist).

I always had registered two custom directories (one for our custom directory, and one for the default themes when installing WordPress in a subfolder) so I never noticed the issue. But we are no longer registering that WordPress wordpress/wp-content/themes directory so it is now an issue for us.

This must-use plugin works:

<?php
register_theme_directory('/custom/path');
register_theme_directory(ABSPATH.'wp-content/themes');

while this does not:

<?php
register_theme_directory('/custom/path');

#22 @kdowns
13 months ago

@joemcgill That does seem to track as a possible solution.

A local bypass I've been able to validate is to call the global variable in my plugin and run a comparison against what I'm expecting. If the values do not match, I'm simply redefining the global. I don't like this solution but it's working as a temporary option that's within my control on the plugin side of things. I'd rather wp hooks just work as expected, lol.

Last edited 13 months ago by kdowns (previous) (diff)

#23 @rebasaurus
13 months ago

#59855 was marked as a duplicate.

#24 @rebasaurus
13 months ago

I'm running into this issue as well. I've tested @joemcgill's PR @ https://github.com/WordPress/wordpress-develop/pull/5642/files and unfortunately, that does not fix the issue at hand. Cross-posting some Slack discussion: https://wordpress.slack.com/archives/C02RQBWTW/p1699487216191699

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


13 months ago
#25

An alternative approach to #5642 is to remove the memoization completely, which reverts these functions to their 6.3 behavior.

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

#26 follow-up: @joemcgill
13 months ago

Thanks @rebasaurus. I'm curious why the previous approach didn't fix your issue. However, the best path forward may be to remove the memoization entirely, which I think was added as a performance optimization and not specifically needed for #18298. I've added a new PR that reverts these functions to their previous behavior.

I'd like to get feedback from @flixos90 or @thekt12 about the potential side effects of removing the memoization, since they worked on the original ticket.

#27 @rebasaurus
13 months ago

@joemcgill Confirmed that removing the memoization fixes it.

#28 in reply to: ↑ 26 @meta4
13 months ago

Replying to joemcgill:

Hey @joemcgill,

Just a quick note that your original PR https://github.com/WordPress/wordpress-develop/pull/5642/files fixed the issue for me.

I was attempting to filter 'stylesheet' in a plugin, and having an early call by ACF to get_stylesheet_directory() defining $wp_stylesheet_path and ignoring my filter.


#29 in reply to: ↑ 19 @partyfrikadelle
13 months ago

Replying to joemcgill:

I've put together a proof of concept PR that seems to fix the original issue, by making sure that those functions only memoize the paths when called after plugins have loaded.

@kdowns I'm curious if this approach would address your issue as well?

Both Patches work perfectly on my setup.

#30 @jorbin
13 months ago

  • Keywords needs-unit-tests added

I think it would be good to introduce some unit tests with any fix here to help prevent a future similar regression

#31 in reply to: ↑ 19 @kdowns
13 months ago

Replying to joemcgill:

I doubt that conditional is part of the problem. That early return was added 12 years ago in [20015] and the conditional hasn't changed in 6 years [41174]. It's probably coincidental that commenting out that line fixes the issue.

I've put together a proof of concept PR that seems to fix the original issue, by making sure that those functions only memoize the paths when called after plugins have loaded.

@kdowns I'm curious if this approach would address your issue as well?

I was able to validate this fix works for me as well.

#32 @joemcgill
13 months ago

  • Keywords needs-unit-tests removed

I've updated the latest PR to clean up some places where the newly introduced globals are now unnecessary. @jorbin I'll work on some unit tests that demonstrate the problem, but it's a bit difficult to reproduce in a test.

In the mean time, for anyone who is struggling with this issue, I've put together a quick plugin that you can install that should help.

https://gist.github.com/joemcgill/dd569c287013ad545f41495f93d7a27e

#33 @joemcgill
13 months ago

  • Keywords needs-unit-tests added

#34 follow-up: @kdowns
13 months ago

@joemcgill Had a thought about the memoization so wanted to drop it in here as a possible consideration going forwards. Previously, get_stylesheet_directory() always returned a string value regardless of the stack and how early it was called.

With memoization and the proposed PR, there are cases when early calls to get_stylesheet_directory() will return as null. Are there potential concerns if the value is null on early calls? This doesn't directly affect anything I'm attempting to do on the plugin end as my hook will assign it as expected, but it might be something to consider from a higher WP level.

Last edited 13 months ago by kdowns (previous) (diff)

#35 @rebasaurus
13 months ago

#59849 was marked as a duplicate.

@joemcgill commented on PR #5642:


13 months ago
#36

Closing in favor of #5644.

@joemcgill commented on PR #5644:


13 months ago
#37

@felixarntz I can do some benchmarks, but to be clear, this just restores these functions to their previous behavior, so it should have no impact when compared with 6.3 and previous versions.

@flixos90 commented on PR #5644:


13 months ago
#38

@joemcgill Yes but part of why the memorizing was introduced was because these functions are now being called more often (rather than checking a constant). So there's a chance this change could make performance worse than pre-WP6.4. I would assume the impact is negligible but we should verify.

@joemcgill commented on PR #5644:


13 months ago
#39

@felixarntz the performance test summary for this PR shows very little change between this and trunk. However, given that those results can fluctuate, I ran local benchmarks of this branch vs trunk and got the following server timing numbers:

metric | trunk | PR | diff | diff%
-- | -- | -- | -- | --
wp-total (p10) | 311.71 | 313.89 | 2.18 | 0.70%
wp-total (p25) | 313.7 | 315.34 | 1.64 | 0.52%
wp-total (p50) | 317 | 317.86 | 0.86 | 0.27%
wp-total (p75) | 319.7 | 320.75 | 1.05 | 0.33%
wp-total (p90) | 326.99 | 324.57 | -2.42 | -0.74%

Seems like a safe change to me.

#40 @SergeyBiryukov
13 months ago

  • Milestone changed from Awaiting Review to 6.4.2

#41 in reply to: ↑ 34 @joemcgill
13 months ago

Replying to kdowns:

With memoization and the proposed PR, there are cases when early calls to get_stylesheet_directory() will return as null.

Good catch, @kdowns! I made a mistake in the initial PR and should have been returning $template_dir rather than returning $wp_template_path when did_action( 'plugins_loaded' ) returns false. Either way, I think that PR #5644 is probably the better approach here.

#42 @flixos90
13 months ago

I agree that https://github.com/WordPress/wordpress-develop/pull/5644 probably makes most sense. Once the test issues have been addressed, it should be good for commit.

@joemcgill commented on PR #5644:


13 months ago
#43

Added some unit tests to this PR that demonstrate the problems in trunk but now pass with this PR. The second set are tricky to show in unit tests because you have to switch to set the application state to where a theme is active from an alternate theme root directory that is not yet registered when the functions are called the first time, so there's a bit of filter mocking going on in the tests. Would be happy for feedback on ways of improving those tests, if anyone has ideas.

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


13 months ago

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


13 months ago

#46 @joemcgill
13 months ago

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

In 57129:

Themes: Remove memoization from stylesheet and theme directories.

This fixes bugs introduced in [56635] whereby the template or stylesheet path could be memoized incorrectly if get_template_directory() or get_stylesheet_directory() were called before the theme has been fully initialized.

Props partyfrikadelle, coreyw, kdowns, rebasaurus, meta4, flixos90, mukesh27, joemcgill.
Fixes #59847.

#47 @joemcgill
13 months ago

  • Keywords fixed-major added; needs-unit-tests removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening to backport to the 6.4 branch.

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


13 months ago

#50 @jorbin
12 months ago

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

In 57156:

Themes: Remove memoization from stylesheet and theme directories.

This fixes bugs introduced in [56635] whereby the template or stylesheet path could be memoized incorrectly if get_template_directory() or get_stylesheet_directory() were called before the theme has been fully initialized.

Reviewed by Jorbin.
Merges [57129] to 6.4 branch.

Props partyfrikadelle, coreyw, kdowns, rebasaurus, meta4, flixos90, mukesh27, joemcgill, icaleb.
Fixes #59847.

#51 @hellofromTonya
12 months ago

  • Keywords dev-reviewed commit has-testing-info has-unit-tests added

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


12 months ago

#53 @jorbin
12 months ago

#59877 was marked as a duplicate.

#54 @hellofromTonya
7 months ago

#59949 was marked as a duplicate.

#55 @hellofromTonya
7 months ago

#59834 was marked as a duplicate.

Note: See TracTickets for help on using tickets.