setup-config.php step 2 broken

[sivel]

11881.5.diff tested ok with my installs.

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