WordPress.org

Make WordPress Core

Opened 10 years ago

Closed 10 years ago

#11881 closed defect (bug) (fixed)

setup-config.php step 2 broken

Reported by: sivel Owned by:
Milestone: 3.0 Priority: normal
Severity: major Version: 3.0
Component: Upgrade/Install Keywords: has-patch
Focuses: multisite Cc:

Description (last modified by ryan)

setup-config.php step 2 is broken now that wp-db.php calls is_multisite(). In addition it also causes a notice.

Notice: Use of undefined constant WP_DEBUG - assumed 'WP_DEBUG' in /var/www/trunk/wp-includes/wp-db.php on line 370
Fatal error: Call to undefined function is_multisite() in /var/www/trunk/wp-includes/wp-db.php on line 373

Because setup-config.php uses wp-db.php without first including wp-settings.php which is where WP_DEBUG and is_multisite are defined it causes a notice and a fatal error.

Attachments (12)

11881.diff (17.4 KB) - added by nacin 10 years ago.
Shortens wp-settings by about 200 lines, creates new WPINC/init.php. Could probably be abstracted much further than this.
11881.2.diff (48.2 KB) - added by nacin 10 years ago.
11881.3.diff (49.5 KB) - added by nacin 10 years ago.
Somewhat improved patch.
11881.fixes-setup-config-only.diff (2.3 KB) - added by nacin 10 years ago.
As a stopgap -- moves is_multisite() to new wp-includes/load.php and includes load.php from setup-config.php.
11881.4.diff (48.9 KB) - added by nacin 10 years ago.
11881.5.diff (49.0 KB) - added by nacin 10 years ago.
Fixes ordering of some MU/MS defines/includes
11881.6.diff (34.1 KB) - added by nacin 10 years ago.
inline docs for wp-settings.php and load.php
11881.7.diff (34.7 KB) - added by nacin 10 years ago.
Restores inclusion of files to global scope. (Inline docs included in this patch.)
11881.8.diff (41.0 KB) - added by nacin 10 years ago.
Inline docs + return includes to global scope + return MS includes to global scope (last part untested)
11881.9.diff (1.1 KB) - added by nacin 10 years ago.
Typo fix
11881.10.diff (25.1 KB) - added by nacin 10 years ago.
plastic surgery for multisite load files
default-constants.diff (25.4 KB) - added by nacin 10 years ago.
Different approach to default constants…

Download all attachments as: .zip

Change History (60)

#1 @sivel
10 years ago

My proposal is to create a new file that we can include early on in wp-settings.php and other files when needed. We could then move code out of wp-settings.php to make it a bit smaller.

@nacin
10 years ago

Shortens wp-settings by about 200 lines, creates new WPINC/init.php. Could probably be abstracted much further than this.

#2 @nacin
10 years ago

First pass on the idea that came out of IRC discussion. Probably breaks a few things -- PHP_SELF for example will need to be set as a global.

#3 @nacin
10 years ago

westi suggested a new file could inherit some defines as well. Wondering if we should create wp-includes/default-constants.php and move large chunks of those checks from wp-settings.php there. could probably also centralize SCRIPT_DEBUG and various other constants that are currently scattered across core.

If we did that, we would have to delay some defines until after more of WP is loaded. We could wrap functions around those and call the function later in wp-settings.php.

#4 @ryan
10 years ago

Looks good so far. I'll wait for westi to weigh in before commit though.

#5 @wpmuguru
10 years ago

That patch worked in my test installs.

#6 @westi
10 years ago

I think it is worth being more brutal on wp-settings.php and trying to move more into file(s) in wp-includes.

I am also wondering about stuff like:

if ( version_compare( $required_php_version, phpversion(), '>' ) ) 
    die( sprintf( /*WP_I18N_OLD_PHP*/'Your server is running PHP version %s$1 but WordPress requires at least %1$2.'/*/WP_I18N_OLD_PHP*/, phpversion(), $required_php_version ) ); 

and

if ( file_exists(ABSPATH . '.maintenance') && !defined('WP_INSTALLING') ) 
    wp_maintenance();

I think these would be better as functions we call like require_wp_db() is.

The idea would be to make the top-level code flow down to the wp() call at the bottom much more readable.

@nacin
10 years ago

#7 @nacin
10 years ago

Ok, second patch strips down wp-settings.php much further and creates wp-includes/load.php (originally init.php, but now this matches ms-load.php) and wp-includes/default-constants.php.

Definitely needs some review.

If we like this, I can go through and inline docs, which we'll need a decent amount of.

#8 @nacin
10 years ago

In the meantime we should probably create load.php and move is_multisite() there, then include that file in setup-config.php, that way setup-config.php isn't broken on trunk for much longer.

@nacin
10 years ago

Somewhat improved patch.

@nacin
10 years ago

As a stopgap -- moves is_multisite() to new wp-includes/load.php and includes load.php from setup-config.php.

#9 @nacin
10 years ago

  • Keywords has-patch added

New patches for review: 11881.3.diff is brutal on wp-settings.php, as requested.

Since this will require thorough code review (particularly since so much is leaving global scope), I have also uploaded 11881.fixes-setup-config-only.diff, which is meant as a stopgap to fix this ticket.

#10 @wpmuguru
10 years ago

The 11881.3.diff worked ok in my test installs. I didn't try doing an install.

11881.fixes-setup-config-only.diff white screened my test installs. There may be some path of logic where a function is being defined twice.

#11 @nacin
10 years ago

Didn't test 11881.fixes-setup-config-only.diff, so I figured I might have screwed something up, but it works fine here. You may not have applied the whole patch, perhaps is_multisite() is still in wp-settings.php?

If that's not it, maybe change error_reporting(0) to E_ALL?

#12 @wpmuguru
10 years ago

I was getting duplicate functions because with the second patch the functions are still in wp-settings. I tested the .3 patch first and reverting it left the load.php file there.

I removed load.php and applied the patch and it looks ok in my test installs.

#13 @TobiasBg
10 years ago

This in the new wp_default_constants():

/** 
 * Should be exactly the same as the default value of SECRET_KEY in wp-config-sample.php 
 * @since 2.5.0 
 */ 
$wp_default_secret_key = 'put your unique phrase here';

doesn't really make sense, or should the user also have to change this in the new file? I'd rather set it to SECRET_KEY by default, if that's the required value anyways.

#14 @nacin
10 years ago

That's the default value of AUTH_KEY, SECURE_AUTH_KEY, et al., in wp-config-sample.php, not what the user changes it to. See how it is used in wp_salt() in pluggable.php.

Also, changing it to default to SECRET_KEY would require not only logic changes in wp_salt() but it would break any pluggable implementations of wp_salt() as well.

If anyone has a PHP 4 environment, what especially needs testing is I think you no longer assign the return value of a new object by reference in non global scope, because the global keyword would create a reference to the global variable. [Relevant docs, http://www.php.net/manual/en/language.variables.scope.php#language.variables.scope.references Relevant PHP docs].

#15 follow-up: @TobiasBg
10 years ago

Ok, thanks for the clarification. Wasn't really aware of its usage, just stumbled upon it while flying over the patch and the corresponding comment to the variable.

#16 in reply to: ↑ 15 @nacin
10 years ago

Replying to TobiasBg:

Ok, thanks for the clarification. Wasn't really aware of its usage, just stumbled upon it while flying over the patch and the corresponding comment to the variable.

No problem - I had to look it up myself when you commented. :)


Once we're good to go with this patch, I imagine the next step will be inline docs, followed by moving whatever constants we can across WP into default-constants.php.

We can easily add new contexts to wp_default_constants() and only define them if necessary, for example such as the five or six constants in script-loader.php.

#17 @ryan
10 years ago

Looking good. I'll try some PHP 4 and 5 testing tomorrow.

#18 follow-up: @nacin
10 years ago

I'm wondering that if instead of calling wp_default_constants() with context strings I came up with, we instead call it with an array of constant names to define.

Potentially, setup-config.php could then be set up with wp_default_constants( array('ABSPATH', 'WPINC', 'WP_CONTENT_DIR', 'WP_DEBUG') );.

As in this case, some constants might need to be set up at different spots. Just trying to figure out the most forward thinking way of handling this. I was looking through other areas of the code from which we can pull constants into default-constants.php. This would make calls of wp_default_constants() more clear and easier to read, I'd think.

#19 @nacin
10 years ago

Closed #11909 as a duplicate.

Now I'm wondering if in the 2.9 branch we should add define('WP_DEBUG', false); to setup-config.php (or if( defined('WP_DEBUG') ... to wp-db.php).

Right now, $this->show_errors() will always run since an undefined constant evaluates to true.

#20 in reply to: ↑ 18 @wpmuguru
10 years ago

Replying to nacin:

As in this case, some constants might need to be set up at different spots. Just trying to figure out the most forward thinking way of handling this. I was looking through other areas of the code from which we can pull constants into default-constants.php. This would make calls of wp_default_constants() more clear and easier to read, I'd think.

Most of the multisite specific constants can be defined in wp-content/sunrise.php, so don't move those before ms-load.php in wp-settings.

#21 @ryan
10 years ago

With php4 and this patch I get this on a front page load:

Undefined property: is_paged in /Applications/MAMP/htdocs/trunk/wp-includes/query.php on line 416

Without the patch, no problem.

@nacin
10 years ago

#22 @nacin
10 years ago

attachment:11881.4.diff moves the variables that contain a reference to an object back into global scope.

Should be good to go.

@nacin
10 years ago

Fixes ordering of some MU/MS defines/includes

#23 follow-up: @nacin
10 years ago

I have questions about two variables losing global scope and whether we need to globalize them:

  • $mu_plugins, an array of PHP files from the must use plugins folder. We unset the corresponding array holding plain WP plugins to include. It is used nowhere in core and I highly doubt it needs to remain a global.
  • $prefix, as in $prefix = $wpdb->set_prefix($table_prefix); -- which is then checked for is_wp_error().

I think the answer to both is no (just being cautious), in which case 11881.5.diff is good on my end.

#24 in reply to: ↑ 23 @wpmuguru
10 years ago

  • Description modified (diff)

Replying to nacin:

I have questions about two variables losing global scope and whether we need to globalize them:

  • $mu_plugins, an array of PHP files from the must use plugins folder. We unset the corresponding array holding plain WP plugins to include. It is used nowhere in core and I highly doubt it needs to remain a global.
  • $prefix, as in $prefix = $wpdb->set_prefix($table_prefix); -- which is then checked for is_wp_error().

I think the answer to both is no (just being cautious), in which case 11881.5.diff is good on my end.

#25 follow-up: @westi
10 years ago

  • Cc westi added

Looking good to me.

I think I would rename a few of the new functions but I can do that after the patch lands.

I would probably split the default constants function into one function for each time it is called as well as the switch is unnecessary.

I'll look at both of these after it lands in the review and improve phase.

#26 @ryan
10 years ago

  • Description modified (diff)

#27 @ryan
10 years ago

  • Description modified (diff)

#28 @automattor
10 years ago

(In [12732]) Cleanup wp-settings. Move functions needed at startup into load.php. Props nacin. see #11881

#29 in reply to: ↑ 25 @nacin
10 years ago

Replying to westi:

I would probably split the default constants function into one function for each time it is called as well as the switch is unnecessary.

They were separate functions in patch 2 but it felt messy. Feel free to break it back up again.

Next up, inline docs for and bringing in constants from across WP.

@nacin
10 years ago

inline docs for wp-settings.php and load.php

#30 @nacin
10 years ago

Initial inline docs and some basic coding cleanup for wp-settings.php and wp-includes/load.php.

The docs are rather bland and we may wish to cross-reference further or come to a better standard. Because we're dealing with a series of function calls in wp-settings.php, not really sure how else to use phpdoc there.

I've skipped default-constants.php for now (both inline docs, and pulling in constants from elsewhere in WP) as I'd like to hear westi's thoughts first.

Should we update the summary and description of the ticket to better reflect what's going on?

#31 @nacin
10 years ago

Looks like some things will need to return to global scope (ref: #11924, dev chats, etc.):

wp_load_mu_plugins()
wp_load_plugins()

What I am thinking is turn these into helper functions that return arrays of files we can then include in wp-settings.php.

We would also need to modify:
wp_find_locale()

And probably move entirely back into wp-settings.php:
wp_load_theme_functions()

@nacin
10 years ago

Restores inclusion of files to global scope. (Inline docs included in this patch.)

#32 @nacin
10 years ago

New patch (includes inline docs) restores inclusion of files to global scope.

@nacin
10 years ago

Inline docs + return includes to global scope + return MS includes to global scope (last part untested)

#33 @nacin
10 years ago

Okay, I also noticed that during the merge, we moved some multisite includes out of global scope in wp-settings.php and into functions in ms-load.php.

11881.8.diff also adds them back into global scope (untested on an MS site).

#34 @mattrude
10 years ago

  • Cc m@… added

#35 @demetris
10 years ago

  • Cc dkikizas@… added

11881.8.diff brings things back to normal for me on a single-site installation. (re themes, plugins and local/global scope.)

#36 @demetris
10 years ago

11881.8.diff needs refershment.

#37 @ryan
10 years ago

(In [12762]) phpdoc for load.php, return some includes to global scope. Props nacin. see #11881

@nacin
10 years ago

Typo fix

#38 @nacin
10 years ago

In ms-load.php, ms_network_plugins():

$network_plugins = WP_PLUGIN_DIR . '/' . $plugin_file;

should be

$network_plugins[] = WP_PLUGIN_DIR . '/' . $plugin_file;

Patch attached.

#39 @dd32
10 years ago

(In [12772]) Typo fix in Network Plugins. Props nacin. See #11881

#40 @nacin
10 years ago

  • Keywords multisite added

11881.10.diff rearranges the multisite initialization files.

More or less, ms-settings.php and ms-load.php have swapped large amounts of code. ms-load.php now mirrors load.php, in that it only contains functions required for/during initialization. ms-settings.php is now included in wp-settings.php (instead of ms-load.php) and includes ms-load.php and ms-default-constants.php.

ms-settings.php contains no functions and, as you can imagine, now mirrors wp-settings.php.

ms-default-constants.php also ate up ms_network_cookies() (I suppose pun intended).

This is ready for review and commit. Ideally we could then spin off some code in ms-settings.php into functions in ms-load.php, the same way we split up wp-settings.php in this ticket.

I know westi indicated he wanted to refactor wp_default_constants() at some point; the same would apply to ms_default_constants() as well.

@nacin
10 years ago

plastic surgery for multisite load files

#41 @wpmuguru
10 years ago

(In [12885]) rearrage multisite initialization, remove deprecated $wpmuBaseTablePrefix, props nacin, see #11881

#42 @nacin
10 years ago

ms-default-constants.php also needs to be added.

#43 @wpmuguru
10 years ago

That last patch fails when I try to apply it.

@nacin
10 years ago

Different approach to default constants...

#44 @nacin
10 years ago

I've been trying to come up with a different approach to default constants for some time now. I find the current implementation to be a little tough to follow, and I was never a fan of the $context strings. It's better than what we had but really it should be obvious exactly what is being defined, especially when using it outside of wp-settings.php (like in ms-files.php, script-loader.php...).

So, I came up with this:

wp_default_constants( 'WP_CONTENT_URL', 'WP_PLUGIN_DIR', 'WP_PLUGIN_URL', 'PLUGINDIR', 'WPMU_PLUGIN_DIR', 'WPMU_PLUGIN_URL', 'MUPLUGINDIR' );

The code itself in wp_default_constants() is now leaner -- checking whether the constant is already defined is handled at the top of the function, not repeatedly throughout -- but it introduces slightly more drag by looping through those constants in a switch wrapped in a foreach.

I'm not sold on it, but I wanted to propose it and see what was thought about it.

#45 @westi
10 years ago

(In [13062]) Improve the implementation of the default constant defining functions. See #11881.

#46 @westi
10 years ago

(In [13063]) Improve the implementation of the default constant defining functions. See #11881.

#47 @automattor
10 years ago

(In [13065]) Improve the implementation of the default constant defining functions for multisite. See #11881.

#48 @nacin
10 years ago

  • Resolution set to fixed
  • Status changed from new to closed
Note: See TracTickets for help on using tickets.