WordPress.org

Make WordPress Core

#22252 closed defect (bug) (fixed)

register_theme_directory() usage can break the current theme

Reported by: johnjamesjacoby Owned by:
Milestone: 3.5 Priority: normal
Severity: normal Version: 3.5
Component: Themes Keywords: has-patch
Focuses: Cc:

Description (last modified by johnjamesjacoby)

Problem:

It's possible for the active theme and it's roots to become out of sync when performing a specific set of actions, resulting in the site white-screening because of the active theme directory being incorrect.

Hard to explain, so I've included more information below.


Steps to reproduce:

  1. Activate BuddyPress.
  2. Activate bp-default theme.
  3. Deactivate BuddyPress.
  4. Visit Appearance > Themes. (Theme shows as 'Twenty Twelve')
  5. Visit site: Twenty Twelve appears
  6. Check DB: stylesheet and template are changed to 'twentytwelve'.
  7. Check DB: stylesheet_root and template_root still point to '/plugins/buddypress/bp-themes/'.
  8. Reactivate BuddyPress.
  9. Visit Appearance > Themes. (Theme shows as 'twentytwelve')
  10. twentytwelve reports back as broken.
  11. Visit site: white-screen of doom

Stack:

  • wp-admin/themes.php:line 107 calls validate_current_theme()
  • wp-includes/theme.php:719 calls switch_theme()
  • wp-includes/theme.php 682 counts $wp_theme_directories, and skips the roots.
  • This results in the _root options pointing to '/plugins/buddypress/bp-themes', and the current stylesheet and template being set as 'twentytwelve'.
  • If you re-activate BuddyPress, $wp_theme_directories will now be greater than 1, and the _root options will not get updated.

Possible fixes:

  • Remove count( $wp_theme_directories ) check from switch_theme(). This forces the theme root to be accurate every time the theme switches, regardless of how many directories are ever registered. In my opinion, this is the most sensible option. (Patch attached)
  • Improve search_theme_directories() to update 'stylesheet_root' and 'template_root' when 'theme_roots' transient doesn't match up. This works because we're already looping through the $cached_roots to make sure each theme is valid. The problem here is we're depending on the expiration of the transient, and doing additional logic each time that happens. (No patch attached)

The crux of the issue: counting $wp_theme_directories isn't a reliable way to guarantee the current theme and its _root's match. This bug doesn't occur if more than 1 additional theme directory is registered, because switch_theme() will always update the theme root in that circumstance.

By forcing switch_theme() to always save the roots, we're able to bypass the issue completely.

Conversely, the only reason to store the _root's is when register_theme_directory() is used more than once. If it's only used by core to register '/themes', it's never a problem. If it's always used by several plugins, it's never a problem. It's only a problem when switching between it being used more than once, and then only by core.

This results in two additional options (even when there are never multiple theme roots) which sucks, but it always fixes the bug.

If we fix it in search_theme_directories(), we're able to update the _root's when they need it, but we're performing that extra logic on each iteration when the transients expire. Also, because search_theme_directories() has some funky sub-directory checks, it's a messy bit to try and hack in there.

Attachments (3)

22252.patch (923 bytes) - added by johnjamesjacoby 18 months ago.
Remove $wp_theme_directories check from switch_theme()
22252.2.patch (1.6 KB) - added by johnjamesjacoby 18 months ago.
Compare against transient, loop through themes, check for mismatched roots, delete option if $root = '/theme' to prevent useless database entries
22252.diff (536 bytes) - added by nacin 17 months ago.

Download all attachments as: .zip

Change History (19)

johnjamesjacoby18 months ago

Remove $wp_theme_directories check from switch_theme()

comment:1 johnjamesjacoby18 months ago

  • Description modified (diff)

comment:2 johnjamesjacoby18 months ago

  • Description modified (diff)

comment:3 johnjamesjacoby18 months ago

  • Description modified (diff)

comment:4 johnjamesjacoby18 months ago

  • Description modified (diff)

comment:5 johnjamesjacoby18 months ago

  • Description modified (diff)

comment:6 johnjamesjacoby18 months ago

Apologies for the multiple edits. Kept thinking of more ways to clarify what's going on.

comment:7 scribu18 months ago

  • Milestone changed from Awaiting Review to 3.5

I've actually encountered this bug when playing around with BP recently. Not sure if it was the same sequence of events or not.

comment:8 johnjamesjacoby18 months ago

  • Description modified (diff)

johnjamesjacoby18 months ago

Compare against transient, loop through themes, check for mismatched roots, delete option if $root = '/theme' to prevent useless database entries

comment:9 johnjamesjacoby18 months ago

I've mitigated this problem on BuddyPress's deactivation hook in trunk, which I think is good practice anyways. This doesn't cover the situation where a user deletes the BuddyPress directory on their own (the patches above will cover that case in WordPress core.)

See: http://buddypress.trac.wordpress.org/changeset/6436/trunk/bp-core/bp-core-update.php

comment:10 follow-up: nacin18 months ago

Is this a regression as a result of WP_Theme in 3.4, or did this exist in 2.9?

comment:11 in reply to: ↑ 10 ; follow-up: johnjamesjacoby18 months ago

Replying to nacin:

Is this a regression as a result of WP_Theme in 3.4, or did this exist in 2.9?

This issue did not ever happen then, but I'm doubtful the version of BuddyPress that works with 2.9 will have this code in it. That was pre-merge, around the time when BuddyPress was still MU only.

Gut feeling is it's a regression; testing this on 2.9 will be a significant time investment.

comment:12 in reply to: ↑ 11 nacin17 months ago

Replying to johnjamesjacoby:

Gut feeling is it's a regression; testing this on 2.9 will be a significant time investment.

With "2.9," I was just referencing when multiple-directory support was introduced. I was really aiming for gauging whether this was a bug with 3.4, or existed prior to 3.4.

comment:13 nacin17 months ago

I don't see what 22252.2.patch does. get_raw_theme_root( $stylesheet_or_template, true ) directly consults the theme_roots transient just the same. What am I missing?

/themes isn't a useless database entry, as if more than one theme root is registered, we're going to consult the root options either way. If the option doesn't exist, then we end up with a call to the DB to fetch a nonexistent option. Much cheaper to store an autoloaded option. We avoid storing the root options when only one theme directory is registered, which is sufficient.

A unit test would help. I'm going to keep looking into this.

nacin17 months ago

comment:14 nacin17 months ago

validate_current_theme() calls switch_theme() to the default theme when it finds that the theme is invalid. This is why on step 4 (in the original report description), the theme is back to Twenty Twelve.

The problem is that by this point, only one theme root exists, so template_root and stylesheet_root are left untouched. Deleting them here would fix most of these issues.

The remaining issue is that validate_current_theme() should actually validate that any stored theme roots are accurate, to catch potential edge cases. (Such as when the same bp-default theme is in both roots, then one of the roots disappears.) I'd like to try doing that without causing non-existent option calls on single-site, though. Basically:

  • If there's one theme root, it should delete any _root options.
  • If there's more than one theme root, it should make sure any _root options are accurate (similar to your code in switch_theme() in 2.patch).

For 3.5, 22252.diff seems sufficient.

Version 0, edited 17 months ago by nacin (next)

comment:15 nacin17 months ago

In 22545:

In switch_theme(), make sure there are no residual template_root or stylesheet_root options remaining if there is only one theme root registered. see #22252.

comment:16 nacin17 months ago

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

#22414 for more.

Note: See TracTickets for help on using tickets.