WordPress.org

Make WordPress Core

Opened 7 years ago

Closed 6 years ago

Last modified 6 years ago

#22501 closed defect (bug) (fixed)

class-wp-upgrader.php is using the wrong theme directory

Reported by: toscho Owned by: dd32
Milestone: 3.7 Priority: normal
Severity: normal Version: 2.9
Component: Upgrade/Install Keywords: has-patch needs-testing
Focuses: Cc:
PR Number:

Description

There are multiple places in class-wp-upgrader.php with a hard coded path to WP_CONTENT_DIR . '/themes'. Upgrades will fail if the theme directory is actually in another place.

If there is still a wp-content/themes directory, the theme will be installed into that, but the old theme is used and the upgrade nag doesn’t go away.

If there is no such directory the upgrader dies with an error.

In both cases the user is stuck with an outdated theme and no ability to the new one.

Test plugin to reproduce this bug:

<?php
/* Plugin Name: Local Theme Roots */

add_filter( 'theme_root_uri', 't5_switch_theme_root' );
add_filter( 'theme_root',     't5_switch_theme_root' );

/**
 * Create a custom theme directory.
 *
 * @wp-hook theme_root
 * @wp-hook theme_root_uri
 * @author  Thomas Scholz, http://toscho.de
 * @param   string $in URL or path
 * @return  string
 */
function t5_switch_theme_root( $in )
{
	if ( 'theme_root_uri' === current_filter() )
		return 'http://trunk-themes.wp';

	// If we made it so far we are in the 'theme_root' filter.
	$new_root = 'F:\wp.trunk.themes';
	register_theme_directory( $new_root );
	return $new_root;
}

The solution is to use get_theme_root() instead of a fixed path.

The attached patch does exactly that. Tested on latest Trunk.

Attachments (2)

class-wp-upgrader.get_theme_root.patch (2.5 KB) - added by toscho 7 years ago.
Replace fixed path with get_theme_root()
22501.diff (6.0 KB) - added by dd32 6 years ago.

Download all attachments as: .zip

Change History (18)

@toscho
7 years ago

Replace fixed path with get_theme_root()

#1 @nacin
7 years ago

  • Version changed from trunk to 2.9

I'm not sure how much this is a "bug". Yes, the upgrader expects that wp-content/themes exists, but so does core. We register '/themes' as the first theme root before any regular plugins are loaded.

#2 @toscho
7 years ago

Even then should each theme upgrade go into the directory where the theme was installed, and the upgrade nag should go away.

#3 @F J Kaiser
7 years ago

  • Cc 24-7@… added

+1

#4 @TJNowell
7 years ago

  • Cc tom@… added

Anything that generates a PHP error is a bug.

Even if we ignore that bug, I'd say that toschos patch should be added anyway on different grounds, of good practice and API consistency. get_theme_root 's function shouldn't be duplicate in code elsewhere in core

#5 @SergeyBiryukov
7 years ago

  • Milestone changed from Awaiting Review to 3.6

#6 follow-up: @ryan
6 years ago

  • Milestone changed from 3.6 to Future Release

#7 in reply to: ↑ 6 @F J Kaiser
6 years ago

Replying to ryan: Please do us all a favor and explain why this (extremely quick) fix has been pushed back. Thank you.

#8 @chrisvanpatten
6 years ago

Just want to jump in and say that this is critical for me. I keep themes outside of WP_CONTENT_DIR, and have had to manually re-activate them after every deploy. I really hope this can get put in core soon.

#9 @wycks
6 years ago

  • Cc bob.ellison@… added

+1

Simple fix, No reason to have hardcoded urls producing an error.

#10 @dd32
6 years ago

  • Keywords needs-patch added; has-patch removed

Let me preface this with: This needs to be fixed, don't get me wrong.

But since this isn't a regression from a previous release (Infact, it's been an issue since the upgrader was first added), so is low-priority right now while the 3.6 release is wrapped up - The focus of the entire dev team is on getting 3.6 finalised, not on fixing old bugs which have had no activity at all during the 3.6 cycle, and doesn't cause an issue for the 99% of the WordPress users out there.

FWIW, The patch here seemingly fixes the initial problem, and lets you use the filter. But what it doesn't do is actually support custom Theme directories, for that, the theme stylesheet being updated needs to be passed to get_theme_root().

While we could apply this patch, it's not going to fix it for people who actually use multiple theme directories, and may potentially break things for them if they're using the filter in a different way than you, testing patches and use-cases can take much more time than you might expect, and doing that to a core piece of code such as the updater, late in a release cycle is rather dangerous.

Come 3.7 dev time, bump the ticket, and get some eyes on it and the above issue fixed, get some proper testing with multiple theme directories involved, and get it in early enough in the dev cycle to allow real world testing, and you'll be gold.

As a work-around for now, I'd suggest you look into using a Symlink(or if you're using windows which also supports symlinks, a NTFS Junction point might also be suitable), to move the directory elsewhere, one of the reasons both of those functionalities were added to filesystems were to work around cases where paths are hard coded.

#11 @lkraav
6 years ago

  • Cc leho@… added

@dd32
6 years ago

#12 @dd32
6 years ago

Attachment 22501.diff​ added

Seems to test well for me with a custom wp-content dir, and custom theme folders.

Two cases that come to mind that I haven't tested:

  1. Removing wp-content/themes from the $wp_theme_directories global
  2. Removing, or altering, the default Theme root to a WP_CONTENT_DIR-relative directory through the existing filters.

#13 @dd32
6 years ago

  • Keywords has-patch needs-testing added; needs-patch removed
  • Milestone changed from Future Release to 3.7

#14 @dd32
6 years ago

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

In 25082:

Theme Installer/Updater: Handle custom Theme directories when updating themes, and installing new themes. Fixes #22501

#15 @dd32
6 years ago

Two cases that come to mind that I haven't tested:

Tested as best as I could, found a few interesting combinations of filters and paths that broke theme handling entirely (really _doing_it_wrong() style code), but couldn't fault this.

Re-open if you've got a case this completely breaks.

#16 @dd32
6 years ago

In 25180:

Theme Upgrader: Be super-careful and check the contents of the $wp_theme_directories variable before merging it, if someone has changed it directly, or worse, unset it, this could've resulted in the $protected_directories being empty. See [25082] See #22501

Note: See TracTickets for help on using tickets.