WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 5 years ago

#11742 closed defect (bug) (fixed)

MU isn't fully compatible with custom content dir

Reported by: Denis-de-Bernardy Owned by:
Milestone: 3.0 Priority: normal
Severity: major Version: 3.0
Component: Upload Keywords: has-patch
Focuses: multisite Cc:

Description

In wp-content/blogs.php, line 3:

http://core.trac.wordpress.org/browser/trunk/wp-content/blogs.php?rev=12603#L3

require_once( dirname( dirname( __FILE__) ) . '/wp-load.php' );

if the site is set up as so, this won't work:

/index.php
/wp-core
/wp-content

the file should be re-engineered accordingly. One option could be to place it in the wp-includes folder, and make WP automatically create a blogs.php file that goes:

<?php
require '/path/to/wp-includes/blogs.php'
?>

Attachments (5)

11742.diff (6.1 KB) - added by nacin 5 years ago.
11742.2.diff (6.0 KB) - added by nacin 5 years ago.
Sets error_reporting to 0, removing requirement for a new SHORTLOAD constant and error suppression operators.
11742.3.diff (5.9 KB) - added by nacin 5 years ago.
Only sets error_reporting(0) after WP is loaded. Also: wp-load.php is included "_once" in the current MU blogs.php so we won't have an issue there (and the constant redefinition would be suppressed).
11742.4.diff (7.7 KB) - added by nacin 5 years ago.
deprecate blogs.php
12742.5.diff (1.3 KB) - added by nacin 5 years ago.
Return error_reporting(0) to ms-files.php, reverting [13038]. Remove deprecated warnings as it breaks media file handling, and no translation support exists, reverting part of [12977].

Download all attachments as: .zip

Change History (65)

comment:1 @nacin5 years ago

  • Keywords multisite added

comment:2 @wpmuguru5 years ago

(In [12924]) internalize uploaded media rewrite rule, see #11742

comment:3 @wpmuguru5 years ago

To test that you will need to remove the /files rewrite rule.

Can someone test and confirm that this works on sub sites if the WP install already has a custom content directory before the network/multisite is enabled?

comment:4 follow-up: @nacin5 years ago

There's a typo in the patch -- $base_len vs $media_len.

As suggested in IRC, I think we should be more drastic with blogs.php and really hack it up, while maintaining compatibility. We should probably make this new MEDIA_FILE the default behavior (and modify the default rewrite rules), and then start to think of blogs.php as an optional drop-in.

comment:5 @wpmuguru5 years ago

(In [12926]) pass #2 change uploaded media rewrite rule, props nacin, see #11742

comment:6 in reply to: ↑ 4 @wpmuguru5 years ago

Replying to nacin:

There's a typo in the patch -- $base_len vs $media_len.

As suggested in IRC, I think we should be more drastic with blogs.php and really hack it up, while maintaining compatibility. We should probably make this new MEDIA_FILE the default behavior (and modify the default rewrite rules), and then start to think of blogs.php as an optional drop-in.

Thanks :)

To test this you will need to update to [12926] before enabling network/multisite.

comment:7 @nacin5 years ago

[12926] looks good, thanks :)

Thinking we should then do the following:

  • Rename wp-includes/blogs.php to wp-includes/ms-files.php.
  • Copy all wp-content/blogs.php functionality to ms-files.php, and remove blogs.php from trunk.
  • At the very start of ms-files.php, check if wp-content/blogs.php exists, if it does, include it and die(). Otherwise, let ms-files.php run its course.

This has the benefit of:

  • moving core functionality out of wp-content and into a file that can be updated easily.
  • being backwards compatible with anyone running a customized version of blogs.php.
  • being forwards compatible by allowing anyone to drop-in their own blogs.php if they want their own file handling.

Patch to this effect coming in a bit.

comment:8 @nacin5 years ago

Patch attached. Two things not included to prevent useless red and green: wp-includes/blogs.php will need to be renamed to ms-files.php, and wp-content/blogs.php will need to be removed.

In order to be completely backwards compatible, I needed to create a new SHORTLOAD constant that is identical to SHORTINIT. This prevents an existing blogs.php drop-in that defines SHORTINIT from generating an unsuppressed E_NOTICE for redefining a constant.

If they don't want to use wp-includes/ms-files.php at all (say if they've reconfigured much of their drop-in and they don't want wp-load.php to be included) then they will need to do a custom rewrite rule.

@nacin5 years ago

@nacin5 years ago

Sets error_reporting to 0, removing requirement for a new SHORTLOAD constant and error suppression operators.

@nacin5 years ago

Only sets error_reporting(0) after WP is loaded. Also: wp-load.php is included "_once" in the current MU blogs.php so we won't have an issue there (and the constant redefinition would be suppressed).

comment:9 @wpmuguru5 years ago

(In [12936]) improvements to custom content dir support, props nacin, see #11742

comment:10 @ocean905 years ago

@nacin

Good job. I like the new ms-files.php.
But I have a suggestion.

if ( !is_file( $file ) ) { 
    status_header( 404 ); 
    die( '404 &#8212; File not found.' ); 
} 

I think we should do this more customizable. For example it should load the 404 page template of the theme.
It's good for themes, which have a function in the 404 template, which send an email to the admin, so that he can check what went wrong.

comment:11 @nacin5 years ago

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

Because we load wp-settings.php only to a certain point, we never get near initializing the theme, text domain, or any plugins for that matter.

I could favor the idea of a file-404.php drop-in, that if exists ( when !is_file() ) we can include, instead of displaying the rudimentary die() you see here.

I'm thinking other improvements to ms-files.php can be handled in other tickets. There weren't any changes that actually went into it here (other than whitespace), it was just moved out of wp-content. I saw a few other things I thought we could address as well but I'll create new tickets for those once I get around to running some test.

Resolving as fixed.

comment:12 @ryan5 years ago

Should wp-content/blogs.php be removed? If someone doesn't update their rewrite rules, they could be running with an old copy of blogs.php that could have security issues or just lack up-to-date functionality.

comment:13 @ryan5 years ago

Maybe leave wp-content/blogs.php. Have it load ms-files.php with the assumption that wp-content is in the usual place. Add deprecated warnings to blogs.php suggesting updating the rewrite rules.

comment:14 follow-up: @nacin5 years ago

I was under the impression that perhaps current MU sites were using hacked versions of their blogs.php files, for special handling, performance/optimization considerations, or to use a custom content directory.

If we don't want to support drop-in capability (which would indeed restrict us updating the file), then yes we should leave it, nearly empty it out, and and add a deprecated warning. If a site wants their own handling then they can just as easily modify the htaccess rules to rewrite uploads to their own script.

comment:15 @ryan5 years ago

blogs.php has always been weird. It is in wp-content implying customizability yet is svn controlled and will be stomped during upgrades. Given the amount of stuff in there, it really isn't something that should be left laying around unmaintained. wp.com hacks the hell out of it to support its complicated architecture. Other sites probably do too. Since those sites are preserving their changes and ignoring what comes down from the WP distribution, I think we can safely replace the default blogs.php with one that loads ms-files.php. Those who have hacked theirs can continue what they are doing, ignoring upstream updates to blogs.php. Those using stock blogs.php will get maintained code coming from ms-files.php.

@nacin5 years ago

deprecate blogs.php

comment:16 in reply to: ↑ 14 @wpmuguru5 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Replying to nacin:

If we don't want to support drop-in capability (which would indeed restrict us updating the file), then yes we should leave it, nearly empty it out, and and add a deprecated warning. If a site wants their own handling then they can just as easily modify the htaccess rules to rewrite uploads to their own script.

We can't add anything like a deprecated warning because blogs.php is loaded directly. But I'll put a comment at the top.

comment:17 @nacin5 years ago

  • Keywords has-patch added

11742.4.diff adds a deprecated warning and should be good to go. It first pulls in wp-load on SHORTINIT so it can generate a warning, then pulls in ms-files.php.

ms-files.php now has a simple check to see if SHORTINIT is defined first. We could also simply rely on require_once() functionality but sidestepping that by including the require under !defined('SHORTINIT') (which we'd have to include anyway) would be quicker.

comment:18 @wpmuguru5 years ago

(In [12977]) restore shell wp-content/blogs.php & add deprecated warnings, props nacin, see #11742

comment:19 @ryan5 years ago

PHP Fatal error:  Call to undefined function __() in /.../wp-includes/functions.php on line 3093

Probably because of SHORT_INIT.

comment:20 @nacin5 years ago

Yup, same reason why on commit Ron removed the message's translation. It should have occurred to me that we'll need __() again in deprecated_file. L10n happens far later in the process.

I guess three options:

  • Manually trigger a deprecated error if WP_DEBUG, from blogs.php.
  • Set up deprecated_file() to run without translations when __() doesn't exist.
  • Define __() in blogs.php, have it return the first argument.

Also, we load pomo before we stop at SHORTINIT. I don't think there's a reason why, and should probably move pomo down / the constant up.

comment:21 @wpmuguru5 years ago

blogs.php/ms-files.php deliver media vs html content. That's the reason l10n wasn't loaded. Adding translation support to media handling for the sole purpose of debugging seems excessive.

comment:22 @wpmuguru5 years ago

(In [13038]) remove error_reporting from ms-files.php, see #11742

comment:23 @nacin5 years ago

I didn't suggest we would set up translation support, only that we keep the deprecated file warning.

@nacin5 years ago

Return error_reporting(0) to ms-files.php, reverting [13038]. Remove deprecated warnings as it breaks media file handling, and no translation support exists, reverting part of [12977].

comment:24 @automattor5 years ago

(In [13040]) revert remove error_reporting from ms-files.php, remove _deprecated_file call from blogs.php, see #11742

comment:25 @wpmuguru5 years ago

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

Going to close this one as I think we got it. If someone has an issue, they can reopen.

comment:26 @nacin5 years ago

Due to rewrite rules, MU remains not fully compatible with a custom content directory out of the box, but that can be handled in other tickets. This looks good, though.

comment:27 follow-ups: @nacin5 years ago

In 3.0, wp-content/blogs.php will be a new file for millions of non-Multisite WordPress installations. That seems confusing and unnecessary. If we can avoid it, I think we should.

I think we should move blogs.php into wp-admin/includes or wp-includes, and during an upgrade, copy it over an existing blogs.php only if it exists.

comment:28 in reply to: ↑ 27 @Denis-de-Bernardy5 years ago

Replying to nacin:

In 3.0, wp-content/blogs.php will be a new file for millions of non-Multisite WordPress installations. That seems confusing and unnecessary. If we can avoid it, I think we should.

I think we should move blogs.php into wp-admin/includes or wp-includes, and during an upgrade, copy it over an existing blogs.php only if it exists.

agreed...

comment:29 @nacin5 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening for now.

comment:30 in reply to: ↑ 27 @wpmuguru5 years ago

Replying to nacin:

In 3.0, wp-content/blogs.php will be a new file for millions of non-Multisite WordPress installations. That seems confusing and unnecessary. If we can avoid it, I think we should.

I think we should move blogs.php into wp-admin/includes or wp-includes, and during an upgrade, copy it over an existing blogs.php only if it exists.

We are including wp-content/blogs.php for the upgraded MU installs. That file will not be used by upgraded WP installs or new 3.0 installs. After 3.0, we can set a rule to exclude the file, remove it or whatever.

Trying to move it around, etc. will create problems for anyone who had implemented a custom content directory in MU.

comment:31 follow-up: @nacin5 years ago

If there is going to be a special automatic MU > 3.0 upgrade path, then including it there makes sense. I just don't see the point in adding a legacy wp-content/blogs.php file to standard installs if we can avoid it.

Perhaps the solution would be to include it in the package, but remove it (or don't copy it over) when we're not MS, when blogs.php doesn't exist? update-core.php can easily take care of that.

comment:32 in reply to: ↑ 31 @wpmuguru5 years ago

Replying to nacin:

If there is going to be a special automatic MU > 3.0 upgrade path, then including it there makes sense. I just don't see the point in adding a legacy wp-content/blogs.php file to standard installs if we can avoid it.

I'm not sure that it's going to be special but there is going to be an automatic upgrade path from MU -> 3.0. We removed the file and rboren asked me to put it back.

comment:33 @automattor5 years ago

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

(In [13514]) Remove wp-content/blogs.php and add deprecated warning for MU admins. fixes #11742

comment:34 follow-up: @Denis-de-Bernardy5 years ago

Re Ryan's question ("And how about those who want to use a custom blogs.php. Do they see the message forever?"), we could add an option to dismiss the message.

comment:35 in reply to: ↑ 34 @nacin5 years ago

Replying to Denis-de-Bernardy:

Re Ryan's question ("And how about those who want to use a custom blogs.php. Do they see the message forever?"), we could add an option to dismiss the message.

I had replied with one option -- remove the hook, which someone maintaining a custom blogs.php file and managing an MU install can easily do. Ryan suggested another IIRC, and that is to call the file something else. So I see no reason to offer a "dismiss."

comment:36 follow-up: @ryan5 years ago

We have to be very sensitive to even the appearance of MU compat changes due to the highly politicized FUD being spread by some. We may need to roll the latest change back or take a different approach that doesn't require a notice.

comment:37 follow-up: @wpmuguru5 years ago

My understanding of the merge project has been that we were working on a single codebase that both existing WordPress and WordPress MU site owners would be able to and would want to upgrade to.

In my opinion, the last changeset on this ticket will be a significant barrier to WordPress MU site owners upgrading to WordPress 3.0.

comment:38 in reply to: ↑ 36 @nacin5 years ago

Replying to ryan:

We have to be very sensitive to even the appearance of MU compat changes due to the highly politicized FUD being spread by some. We may need to roll the latest change back or take a different approach that doesn't require a notice.

What about an option to dismiss the message, much like the generated password nag?

Sigh. We're just trying to keep their file handlers updated and also not clutter wp-content for single-install users.

comment:39 in reply to: ↑ 37 @Denis-de-Bernardy5 years ago

Replying to wpmuguru:

In my opinion, the last changeset on this ticket will be a significant barrier to WordPress MU site owners upgrading to WordPress 3.0.

Wow! Pardon for the ironic comment, but I sincerely feel that *they* are being privileged. You're meaning that, while those of us who have been using WP for ages have gone through all sorts of barriers to upgrade, broken plugins, broken themes, API changes, sufficiently massaged new features, outright new bugs, and more... a minority of WP MU users backed by vocal fear mongers will manage to make us prevent 20M WP sites from getting a useless file? Sounds like...:

http://hornbillunleashed.files.wordpress.com/2009/10/bullshit.jpg

comment:40 follow-ups: @westi5 years ago

  • Cc westi added
  • Resolution fixed deleted
  • Status changed from closed to reopened

Is there not a middle ground here which can give us the best of both worlds?

Make the wp-content/blogs.php solely issue redirects to the preferred url.
Ensure that ms-files.php is suitably over loadable to allow people to implement the kind of different stuff they are doing in blogs.php at the moment.

Can someone summarise how blogs.php is currently intended to be used so that we can better understand the upgrade implications?

comment:41 @ryan5 years ago

Comment 15 provides some background on blogs.php:

http://core.trac.wordpress.org/ticket/11742#comment:15

comment:42 in reply to: ↑ 40 @nacin5 years ago

Replying to westi:

Make the wp-content/blogs.php solely issue redirects to the preferred url.
Ensure that ms-files.php is suitably over loadable to allow people to implement the kind of different stuff they are doing in blogs.php at the moment.

That's what we had as of [13040]. It's pluggable in that they can simply change their rewrite rules to point to whichever file they'd like. Otherwise, they'd be getting updates via ms-files.php. We were all in agreement up to this point.

Can someone summarise how blogs.php is currently intended to be used so that we can better understand the upgrade implications?

For some reason, it was placed in wp-content from the get-go, presumably because it served wp-content uploads. Some MU sites do modify it, so it's kind of a drop-in of sorts, but most probably don't. Those that do simply ignore the upstream update.

The problem is, we still had wp-content/blogs.php as a deprecated fluff file, in a folder that would confuse many WP single-install users, and rightfully so. So I wanted to come up with a way to remove it from wp-content all together and point MU->MS admins to use ms-files.php.

Of course, if they don't want to use ms-files.php, they can remove the action that generates the nag and they'd be all set. ryan pointed out FUD that could go into this and obviously we can't keep it like this.

I see no reason to keep blogs.php for single installs, and there are various ways we can avoid that. My initial ideas were removing on install when not multisite, or moving into wp-content when we are multisite. Ultimately, [13514] was what I came up with.

As a compromise, I can't imagine a link to dismiss the nag not being acceptable to combat the FUD.

We're simply telling them that blogs.php is no longer getting upstream updates, and if they want to get upstream updates, then they should simply change their htaccess file to use ms-files.php instead. Allowing them to dismiss that notice -- instead of forcing them to rename blogs.php or kill a hook -- is a better idea than [13514]. They can still keep a blogs.php file and not bother to change their rewrite rules to something else.

That seems like the best of both worlds.

comment:43 @ryan5 years ago

Something else to keep in mind: Some sites upgrade via svn up. blogs.php will disappear but the rewrite rules will still reference it. The administrator would still have to run upgrade.php, so maybe we can stick some wpfs capable bits in there that will update the rewrite rules.

comment:44 follow-up: @ryan5 years ago

Hmmm, current MU svn users are using the wpmu repo, not the wp one. Maybe the svn scenario is a non-issue since you can't do a direct svn up to 3.0 from the mu repo.

comment:45 follow-up: @ryan5 years ago

BTW, there is still a reference to blogs.php in the IIS rules.

comment:46 in reply to: ↑ 44 @Denis-de-Bernardy5 years ago

Replying to ryan:

Hmmm, current MU svn users are using the wpmu repo, not the wp one. Maybe the svn scenario is a non-issue since you can't do a direct svn up to 3.0 from the mu repo.

Ouch... they're going to be in for a rough time to upgrade those sites. especially if they changed files...

comment:47 in reply to: ↑ 45 @nacin5 years ago

Replying to ryan:

BTW, there is still a reference to blogs.php in the IIS rules.

Hmm, didn't realize we had IIS rules for multisite, considering that the Apache rules are currently segregated.

comment:48 follow-up: @ryan5 years ago

I'll experiment with svn switch, but I doubt it will play nicely with a foreign repository.

comment:49 in reply to: ↑ 48 @Denis-de-Bernardy5 years ago

Replying to ryan:

I'll experiment with svn switch, but I doubt it will play nicely with a foreign repository.

it doesn't work at all, even. what I had to do when I changed my own, was this:

  1. mass-delete the .svn folders
  2. svn export to overwrite all of the site's files
  3. svn co in a tmp folder
  4. copy tmp/* to the site

step 3 is important because of the svn:externals. doing a simple svn co --force fails because of akismet.

comment:50 in reply to: ↑ 40 @wpmuguru5 years ago

Replying to westi:

Is there not a middle ground here which can give us the best of both worlds?

Make the wp-content/blogs.php solely issue redirects to the preferred url.
Ensure that ms-files.php is suitably over loadable to allow people to implement the kind of different stuff they are doing in blogs.php at the moment.

Can someone summarise how blogs.php is currently intended to be used so that we can better understand the upgrade implications?

The blogs.php that was removed was a shell file that loaded ms-files.php.

My issue/concern is not with removing blogs.php from trunk. As long as we can come up with a way to allow the MU installs to upgrade without deleting the blogs.php from the MU installs. Blogs.php from MU 2.9.X and ms-files.php from trunk are functionally identical, so the admin message saying that it's been deprecated isn't necessary in 3.0.

Prior to the patch being committed I suggested using something like a svn:ignore. Is it possible to build into the zip process a switch on that particular file so that it does get distributed to MU installs but not to WP installs?

comment:51 @ryan5 years ago

add_action( 'admin_notices', 'ms_deprecated_blogs_file' );

That's done in both ms-default-filters.php and wp-admin/includes/ms.php.

comment:52 @ocean905 years ago

Can we improve the notice a little bit, explain how I should update the .htaccess in detail? See #13002

comment:53 @pbearne5 years ago

can't this be scripted this is file remove and edit to .htaccess / web.config

comment:54 @nacin5 years ago

(In [14440]) Remove re-adding of filter. see #11742

comment:55 @nacin5 years ago

For 3.0, I think we should add a notice to network.php if you're running a custom content dir that says that MU has incomplete support for a custom content dir, and leave it at that.

comment:56 @nacin5 years ago

Closed #13002 as a duplicate.

comment:57 @wpmuguru5 years ago

(In [14609]) block ms-files.php when not multisite, see #11742

comment:58 follow-up: @ryan5 years ago

Good enough for 3.0?

comment:59 in reply to: ↑ 58 @Denis-de-Bernardy5 years ago

Replying to ryan:

Good enough for 3.0?

Depends on how much you want to get done in it. my test environment is so:

  • localhost
  • wp in ~denis/wp
  • plugins/themes in ~denis/wk
  • multisite with subfolders
  • wp-content cookie set for /~denis/wk
  • most all defines hard-coded in wp-config

the admin auth cookie define is completely broken. I had to kill that one to make things work. when not doing so, I would never get logged out of ~denis/wp/test/wp-admin, when logged in as test user. the cookie would "stick", so to speak.

other than that, I've only done some proxy tests, to verify plugins work etc, and it's functional so far. if you've anything specific in need to testing, just let me know.

comment:60 @nacin5 years ago

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

(In [14825]) Show warning in network.php if using a custom wp-content directory. fixes #11742.

Note: See TracTickets for help on using tickets.