#22501 closed defect (bug) (fixed)
class-wp-upgrader.php is using the wrong theme directory
Reported by: |
|
Owned by: |
|
---|---|---|---|
Milestone: | 3.7 | Priority: | normal |
Severity: | normal | Version: | 2.9 |
Component: | Upgrade/Install | Keywords: | has-patch needs-testing |
Focuses: | Cc: |
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)
Change History (18)
#1
@
12 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
@
12 years ago
Even then should each theme upgrade go into the directory where the theme was installed, and the upgrade nag should go away.
#4
@
12 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
#7
in reply to:
↑ 6
@
12 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
@
12 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
@
12 years ago
- Cc bob.ellison@… added
+1
Simple fix, No reason to have hardcoded urls producing an error.
#10
@
12 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.
#12
@
12 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:
- Removing wp-content/themes from the
$wp_theme_directories
global - Removing, or altering, the default Theme root to a WP_CONTENT_DIR-relative directory through the existing filters.
#13
@
12 years ago
- Keywords has-patch needs-testing added; needs-patch removed
- Milestone changed from Future Release to 3.7
#14
@
12 years ago
- Owner set to dd32
- Resolution set to fixed
- Status changed from new to closed
In 25082:
#15
@
12 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.
Replace fixed path with get_theme_root()