Prevent loading wp-content/themes/functions.php when 'stylesheet' option is empty

[danielbachhuber]

--skip-themes is a WP-CLI feature to disable loading of a given theme or all themes.

It works by returning an empty string for 'template' and 'stylesheet':

​https://github.com/wp-cli/wp-cli/blob/a71167e1030462275a48c419854261efceba11e3/php/WP_CLI/Runner.php#L1751-L1755

This hack has worked pretty reliably for a long time. However, there's a specific edge case we discovered: --skip-themes will load wp-content/themes/functions.php if it exists.

See ​https://github.com/wp-cli/wp-cli/issues/5240 for some additional context.

What's the best way to prevent loading wp-content/themes/functions.php? Maybe wp_get_active_and_valid_themes() should return an empty array if 'template' and 'stylesheet' are empty?

[danielbachhuber]

I don't love the idea of offering a filter in the Bootstrap/Load process, but #34936 is a ticket where we did this in the past

[SergeyBiryukov]

Replying to danielbachhuber:

What's the best way to prevent loading wp-content/themes/functions.php? Maybe wp_get_active_and_valid_themes() should return an empty array if 'template' and 'stylesheet' are empty?

That makes sense to me at a glance.

However, it is worth noting that wp-content/themes/functions.php would only be loaded if it exists​. Unless I'm missing something, that file is not supposed to exist on a regular installation, so this seems like an edge case to me.

The linked issue includes this step:

Create a functions.php file with a syntax error or an undefined function like <?php genesis_register_sidebar(); and place under /wp-content/themes/

Is there a real-world use case where that file would exist?

[hellofromTonya]

What's the best way to prevent loading wp-content/themes/functions.php? Maybe wp_get_active_and_valid_themes() should return an empty array if 'template' and 'stylesheet' are empty?

I assume you are referring to when a filter sets get_template() and get_stylesheet() to return an empty string, but the main themes directory root (i.e. WP_CONTENT_DIR . '/themes') is defined for the constants TEMPLATEPATH and STYLESHEETPATH (i.e. from get_theme_root()).

If yes, then the `functions.php` file is loaded if it exists in the main themes directory root.

My question is the same as @SergeyBiryukov's:

• Why would a WP_CONTENT_DIR . '/themes/functions.php' exist?
• Is this a real-world use case for it to exist?
• If yes, how prevalent is this real-world use case?

[hellofromTonya]

In that line of thinking:

What if there is a real-world use case for WP_CONTENT_DIR . '/themes/functions.php' to exist? If there is, then returning empty strings from wp_get_active_and_valid_themes() would break loading that file on those sites.

What are the other impacts of real-world usages of invoking wp_get_active_and_valid_themes() when get_stylesheet() and/or get_template() return an empty string?

Is wp_get_active_and_valid_themes() intentionally designed to allow only pointing at the main themes directory (root)? Or is this an oversight? hmm

I'm curious of the potential BC breaks of such a change.

[danielbachhuber]

Replying to hellofromTonya:

In that line of thinking:

What if there is a real-world use case for WP_CONTENT_DIR . '/themes/functions.php' to exist? If there is, then returning empty strings from wp_get_active_and_valid_themes() would break loading that file on those sites.

Personally, I think it's fine to make this change. It's a bug that WordPress is doing this in the first place, not a documented feature.

The actual bug seems like it's that TEMPLATEPATH and STYLESHEETPATH are set to paths when template and stylesheet are empty. It seems like they should be defined to false. However, changing this particular behavior increases our surface area for unexpected side effects.

Is wp_get_active_and_valid_themes() intentionally designed to allow only pointing at the main themes directory (root)? Or is this an oversight? hmm

That sounds like a different bug report :D

Create a functions.php file with a syntax error or an undefined function like <?php genesis_register_sidebar(); and place under /wp-content/themes/

Is there a real-world use case where that file would exist?

The problem has manifested in the real world at least a couple of times now. It's certainly an edge case, though.

See https://core.trac.wordpress.org/ticket/57928

Returns an empty array from wp_get_active_and_valid_themes() when get_template() and get_stylesheet() return falsy values. The empty array prevents wp-content/themes/functions.php from being loaded erroneously.

Returns an empty array from wp_get_active_and_valid_themes() when ! wp_using_themes(). The empty array prevents wp-content/themes/functions.php from being loaded erroneously.

See https://core.trac.wordpress.org/ticket/57928

See https://core.trac.wordpress.org/ticket/57928

[danielbachhuber]

@SergeyBiryukov @hellofromTonya @flixos90 @spacedmonkey I put together a few options to consider:

1. ​Return an empty array from wp_get_active_and_valid_themes() when ! wp_using_themes().
   
   This feels like the most correct approach to me. However, it would be a backwards compat break for any site with add_filter( 'wp_using_themes', '__return_false' ); and a valid template value, because the theme's functions.php would no longer be loaded.
   
   
2. ​Return an empty array from wp_get_active_and_valid_themes() when get_template() and get_stylesheet() are falsy.
   
   This is more of a WP-CLI-specific fix and has less surface area. However, it would be a backwards compat break for any site that has template set to '' but is still filtering template_directory (unlikely).
   
   
3. ​Add a filter to wp_get_active_and_valid_themes().
   
   Maybe the least risky change or maybe the most risky change. I'm not sure we actually want to introduce a filter here, but we do have filters for other parts of wp-includes/load.php.

Thoughts?

[bpayton]

Replying to hellofromTonya:

My question is the same as @SergeyBiryukov's:

• Why would a WP_CONTENT_DIR . '/themes/functions.php' exist?

We have encountered automation failures with core-like WordPress.com sites where users have created this file. It was not clear whether they were backing up a file or doing something else. Attempting to load the file caused fatals when using WP-CLI with the --skip-themes option, and we almost always want to use --skip-themes to avoid errors due to user-provided software while performing system administration tasks.

[danielbachhuber]

I think ​https://github.com/WordPress/wordpress-develop/pull/4409 is the approach I'd like to land, unless there are any objections.

Seems like a hacky test failure...

[costdev]

Regarding available usage:

• Here's ​a GitHub Code Search result for wp-content/themes/functions.php that might provide some insight into its usage.
• Here's ​7 plugin results at WP Directory that hook wp_using_themes.
  • Sensei LMS, Print My Blog, and Leyka explicitly return false.
  • Print My Blog filters wp_using_themes inside a template_redirect action callback. Immediately after hooking template_redirect, it filters template_directory. This may not be pertinent, but as an earlier comment mentioned the template_directory filter when used alongside an empty template value, I thought I'd mention this one so that it could be considered/ruled out as a risk factor.

---

From the comments on this ticket, there's no doubt that this has caused real-world issues for several folks when using --skip-themes. It's an edge case, therefore not widespread, but still a valid issue. At the moment, we have three approaches for a Core patch: two with known backward compatibility breaks, and one that might be the least/most risky option. As yet, I don't have a strong opinion on which approach is preferred/most viable.

However, given that wp_get_active_and_valid_themes() itself expects that filtering wp_using_themes with false should skip loading themes ​Ref and then returns an empty array, ​PR 4409 looks to be consistent with Core.

@danielbachhuber Regarding ​PR 4409, you previously said:

However, it would be a backwards compat break for any site with add_filter( 'wp_using_themes', '__return_false' ); and a valid template value, because the theme's functions.php would no longer be loaded.

I'm wondering if this really constitutes a BC break by changing behaviour that some people want, or whether it's actually just fixing undocumented, buggy behaviour that no one wants. At the moment, I suspect it's the latter.

Can you think of a scenario/use case for someone expecting wp-content/themes/functions.php or wp-content/themes/theme-slug/functions.php to be loaded here, despite passing __return_false to wp_using_themes? Aside from the fact it currently does load it, I can't quite think of an example where someone would say "Don't use themes, but go ahead and load this file in the themes directory".

Some unit tests for this would be useful to understand what this change means.

Closing in favor of ​https://github.com/WordPress/wordpress-develop/pull/4409

Closing in favor of ​https://github.com/WordPress/wordpress-develop/pull/4409

Oh nooo! This was my favorite PR of 2023 🙃

Oh nooo! This was my favorite PR of 2023

@szepeviktor How were you intending to use it?

I'm not a WordPress user, just forced to live in the ecosystem.

Some unit tests for this would be useful to understand what this change means.

Added a test case in 35091f8d096e3bceb58554a4ff291c4fdd4b4dd3

wp_get_active_and_valid_themes() is only used here:

​https://github.com/WordPress/wordpress-develop/blob/35091f8d096e3bceb58554a4ff291c4fdd4b4dd3/src/wp-settings.php#L587-L593

Interestingly, the random test failures in my first commit were caused by WP_USE_THEMES being undefined. Apparently that hasn't ever bit anyone before.

WP_USE_THEMES is defined in src/index.php and originates from https://core.trac.wordpress.org/changeset/2303. The test suite doesn't load src/index.php, so the constant wasn't ever defined.

[danielbachhuber]

Replying to costdev:

Regarding available usage:

Thanks for the great research, @costdev. Your work makes me feel a lot more confident with ​https://github.com/WordPress/wordpress-develop/pull/4409

I'm wondering if this really constitutes a BC break by changing behaviour that some people want, or whether it's actually just fixing undocumented, buggy behaviour that no one wants. At the moment, I suspect it's the latter.

Can you think of a scenario/use case for someone expecting wp-content/themes/functions.php or wp-content/themes/theme-slug/functions.php to be loaded here, despite passing __return_false to wp_using_themes? Aside from the fact it currently does load it, I can't quite think of an example where someone would say "Don't use themes, but go ahead and load this file in the themes directory".

I agree with the "fixing undocumented, buggy behavior" assessment. More so, I wanted to identify the behavior change so it was documented. I think it's perfectly fine to fix.

[danielbachhuber]

In 55890:

Load: Avoid loading a theme's functions.php when ! wp_using_themes().

Updates wp_get_active_and_valid_themes() to return early when wp_using_themes() returns false. This prevents a theme's functions.php from being loaded erroneously when the site isn't using themes.

Also adds define( 'WP_USE_THEMES', true ); to the test suite bootstrap. Some tests randomly break without it because they were dependent on the previous buggy behavior.

Props bpayton, costdev, danielbachhuber, hellofromtonya, sergeybiryukov, spacedmonkey.
Fixes #57928.

Shipped with https://core.trac.wordpress.org/changeset/55890

[danielbachhuber]

In 55891:

Bootstrap/Load: Revert [55890].

As it turns out, WP-CLI *also* doesn't define( 'WP_USE_THEMES', true );, which means an active theme's functions.php isn't loaded by default and causes a backwards compatibility break.

See #57928.

[jorbin]

Thinking about this a bit, I wonder if the bailing early should be if TEMPLATEPATH and STYLESHEETPATH are equal to get_theme_root().

[audrasjb]

This ticket was committed then reverted during milestone 6.3.
As WP 6.3 beta 1 is planned for today, let's move the ticket to our next milestone, 6.4.

[danielbachhuber]

Given how minimal this issue is, I don't think it's worth changing core behavior at this time. Instead, I'm going to fix this with a more descriptive error message on our wp_die() handler: ​https://github.com/wp-cli/wp-cli/pull/5817