WordPress.org

Make WordPress Core

Opened 4 years ago

Last modified 4 years ago

#11881 closed defect (bug)

setup-config.php step 2 broken — at Version 26

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.

Change History (32)

comment:1 sivel4 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.

nacin4 years ago

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

comment:2 nacin4 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.

comment:3 nacin4 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.

comment:4 ryan4 years ago

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

comment:5 wpmuguru4 years ago

That patch worked in my test installs.

comment:6 westi4 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.

nacin4 years ago

comment:7 nacin4 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.

comment:8 nacin4 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.

nacin4 years ago

Somewhat improved patch.

nacin4 years ago

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

comment:9 nacin4 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.

comment:10 wpmuguru4 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.

comment:11 nacin4 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?

comment:12 wpmuguru4 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.

comment:13 TobiasBg4 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.

comment:14 nacin4 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].

comment:15 follow-up: TobiasBg4 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.

comment:16 in reply to: ↑ 15 nacin4 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.

comment:17 ryan4 years ago

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

comment:18 follow-up: nacin4 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.

comment:19 nacin4 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.

comment:20 in reply to: ↑ 18 wpmuguru4 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.

comment:21 ryan4 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.

nacin4 years ago

comment:22 nacin4 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.

nacin4 years ago

Fixes ordering of some MU/MS defines/includes

comment:23 follow-up: nacin4 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.

comment:24 in reply to: ↑ 23 wpmuguru4 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.

comment:25 westi4 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.

comment:26 ryan4 years ago

  • Description modified (diff)
Note: See TracTickets for help on using tickets.