setup-config.php step 2 broken

[sivel]

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.

[sivel]

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]

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

[nacin]

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.

[nacin]

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.

[ryan]

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

[wpmuguru]

That patch worked in my test installs.

[westi]

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]

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.

[nacin]

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]

Somewhat improved patch.

[nacin]

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

[nacin]

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.

[wpmuguru]

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.

[nacin]

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?

[wpmuguru]

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.

[TobiasBg]

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.

[nacin]

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].

[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.

[nacin]

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.

[ryan]

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

[nacin]

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.

[nacin]

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.

[wpmuguru]

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.

[ryan]

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]

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

Should be good to go.

[nacin]

Fixes ordering of some MU/MS defines/includes

[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.

[wpmuguru]

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.

[westi]

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.

[automattor]

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

[nacin]

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]

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

[nacin]

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?

[nacin]

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]

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

[nacin]

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

[nacin]

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

[nacin]

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).

[demetris]

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

[demetris]

11881.8.diff needs refershment.

[ryan]

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

[nacin]

Typo fix

[nacin]

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.

[dd32]

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

[nacin]

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]

plastic surgery for multisite load files

[wpmuguru]

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

[nacin]

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

[wpmuguru]

That last patch fails when I try to apply it.

[nacin]

Different approach to default constants...

[nacin]

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.

[westi]

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

[westi]

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

[automattor]

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