WordPress.org

Make WordPress Core

Opened 8 years ago

Closed 8 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 8 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 8 years ago.
11881.3.diff (49.5 KB) - added by nacin 8 years ago.
Somewhat improved patch.
11881.fixes-setup-config-only.diff (2.3 KB) - added by nacin 8 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 8 years ago.
11881.5.diff (49.0 KB) - added by nacin 8 years ago.
Fixes ordering of some MU/MS defines/includes
11881.6.diff (34.1 KB) - added by nacin 8 years ago.
inline docs for wp-settings.php and load.php
11881.7.diff (34.7 KB) - added by nacin 8 years ago.
Restores inclusion of files to global scope. (Inline docs included in this patch.)
11881.8.diff (41.0 KB) - added by nacin 8 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 8 years ago.
Typo fix
11881.10.diff (25.1 KB) - added by nacin 8 years ago.
plastic surgery for multisite load files
default-constants.diff (25.4 KB) - added by nacin 8 years ago.
Different approach to default constants…

Download all attachments as: .zip

Change History (60)

#1 @sivel
8 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
8 years ago

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

#2 @nacin
8 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
8 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
8 years ago

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

#5 @wpmuguru
8 years ago

That patch worked in my test installs.

#6 @westi
8 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
8 years ago

#7 @nacin
8 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
8 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
8 years ago

Somewhat improved patch.

@nacin
8 years ago

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

#9 @nacin
8 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
8 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
8 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
8 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
8 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
8 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
8 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
8 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
8 years ago

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

#18 follow-up: @nacin
8 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
8 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
8 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
8 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
8 years ago

#22 @nacin
8 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
8 years ago

Fixes ordering of some MU/MS defines/includes

#23 follow-up: @nacin
8 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
8 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
8 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
8 years ago

  • Description modified (diff)

#27 @ryan
8 years ago

  • Description modified (diff)

#28 @automattor
8 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
8 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
8 years ago

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

#30 @nacin
8 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
8 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
8 years ago

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

#32 @nacin
8 years ago

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

@nacin
8 years ago

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

#33 @nacin
8 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
8 years ago

  • Cc m@… added

#35 @demetris
8 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
8 years ago

11881.8.diff needs refershment.

#37 @ryan
8 years ago

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

@nacin
8 years ago

Typo fix

#38 @nacin
8 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
8 years ago

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

#40 @nacin
8 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
8 years ago

plastic surgery for multisite load files

#41 @wpmuguru
8 years ago

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

#42 @nacin
8 years ago

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

#43 @wpmuguru
8 years ago

That last patch fails when I try to apply it.

@nacin
8 years ago

Different approach to default constants...

#44 @nacin
8 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
8 years ago

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

#46 @westi
8 years ago

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

#47 @automattor
8 years ago

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

#48 @nacin
8 years ago

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