wp_not_installed() has an unbounded check for "install.php"

[nacin]

This is a good-first-bug.

Steps to reproduce: Don't install WordPress. (Easy way to 'uninstall': change the table prefix.) Go to wp-admin/plugin-install.php. You'll be redirected to /wp-admin/upgrade.php. Specifically, it'll be at the root, so if you have WP installed not at the root of your domain, it'll probably fail. Also, it'll fail because WP isn't installed.

The issue is wp_not_installed() does false === strpos( $_SERVER['PHP_SELF'], 'install.php' ) which would fail for wp-admin/plugin-install.php or theme-install.php.

To fix this check we probably need to be a bit smarter. (Don't forget that PHP_SELF isn't necessarily normalized and slashes could be in different directions.) However, since we already are checking WP_INSTALLING, we can possibly excise this PHP_SELF conditional entirely.

The solution here should be paired with a quick study of the history of this code (note: it pre-dates 3.0, even though wp_not_installed() is new in 3.0, and sat probably in wp-settings.php before then) to determine why this check is there to begin with, seeing that it appears to be redundant with WP_INSTALLING. If it predates WP_INSTALLING, and the constant check was added for other reasons, that would explain it. Please cross-reference any relevant tickets and changesets you find.

[joemcgill]

tl;dr - I think it's safe to kill the check for install.php.

A brief history lesson:

• In version 3, nacin creates wp_not_installed() from logic moved out of wp-settings.php (changeset [12732])
• The check for strstr($_SERVER[ 'PHP_SELF']. 'install.php') dates back to v1.5, at which point there were no other *-install.php files in core.
• Aside: Mark Jaquith commited changeset [4990] in 2.2 which replace strstr() with strpos() for performance reasons, otherwise this has been mostly unchanged forever. (related: #3920)
• The oldest relevant changeset available in Trac [2486] shows that the check for install.php and WP_INSTALLING were originally paired as an OR instead of an AND, which would explain the redundancy.
• Also, since define( 'WP_INSTALLING', true ); was added to install.php in 1.5.1, I'm guessing the redundancy really began to come into play at that point.

[joemcgill]

Replying to DrewAPicture: Total trac newb question (this is my second patch, ever): anything I need to do to carry this through since I've been assigned ownership? If so, I'm happy to do so, just not sure. Thanks.

[DrewAPicture]

Replying to joemcgill:

Replying to DrewAPicture: Total trac newb question (this is my second patch, ever): anything I need to do to carry this through since I've been assigned ownership? If so, I'm happy to do so, just not sure. Thanks.

Just keep on what you've been doing :) The ownership thing is just for the benefit of the good-first-bug list so that it shows as "claimed".

[joemcgill]

Replying to DrewAPicture:

Replying to joemcgill:

Replying to DrewAPicture: Total trac newb question (this is my second patch, ever): anything I need to do to carry this through since I've been assigned ownership? If so, I'm happy to do so, just not sure. Thanks.

Just keep on what you've been doing :) The ownership thing is just for the benefit of the good-first-bug list so that it shows as "claimed".

Thanks. Just didn't want to drop any balls unknowingly.

[ocean90]

In 30581:

Remove unbounded check for "install.php" in wp_not_installed().

The check is true for wp-admin/plugin-install.php and wp-admin/theme-install.php too. This will end in a redirect to /wp-admin/upgrade.php or a screen with a bunch of errors.
The WP_INSTALLING constant was added in WordPress 1.5.1 to wp-admin/install.php which makes the check for "install.php" redundant.

props joemcgill.
fixes #29694.