Problem/Motivation

If the user visits the site index for a Drupal site that has database credentials in settings.php but has not yet been installed completely, the user will receive a fatal error that does not in any way indicate that the user should visit install.php to complete the installation.

Proposed resolution

Detect an empty database during bootstrap, and redirect to install.php if installation is not already in progress.

Patch in #728702-17: Visiting index.php should redirect to install.php if settings.php already has database credentials but database is empty. is confirmed to resolve this issue for both D7 and D8.

Remaining tasks

None.

User interface changes

User will be redirected to install.php if there is a database configuration but the database is empty.

API changes

None.

Original report by @arpeggio

The setup are:
-database empty
-drupal updated from CVS
-set the C:\xampp\htdocs\sites\sites.php with $sites['localhost'] = 'kapitbisig.com';
-manually populated the the database credentials in C:\xampp\htdocs\sites\kapitbisig.com\settings.php

I got the following error message when I browsed http://localhost

Fatal error: Call to undefined function _system_rebuild_theme_data() in C:\xampp\htdocs\includes\theme.inc on line 586

Files: 
CommentFileSizeAuthor
#62 drupal-728702-62.patch1.82 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-728702-62.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#54 install-redirect-on-empty-database-728702-54.patch659 bytesreglogge
PASSED: [[SimpleTest]]: [MySQL] 33,751 pass(es).
[ View ]
#52 install-redirect-on-empty-database-728702-52.patch654 bytesreglogge
PASSED: [[SimpleTest]]: [MySQL] 33,762 pass(es).
[ View ]
#36 install-redirect-on-empty-database-728702-36.patch1.1 KBpillarsdotnet
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch install-redirect-on-empty-database-728702-36.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots.
[ View ]
#34 install-redirect-on-empty-database-728702-34.patch1.09 KBpillarsdotnet
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in index.php.
[ View ]
#29 install-redirect-on-empty-database-728702-29.patch2.28 KBpillarsdotnet
PASSED: [[SimpleTest]]: [MySQL] 33,054 pass(es).
[ View ]
#21 install_redirect_on_empty_database-728702-21.patch1.79 KBDavid_Rothstein
PASSED: [[SimpleTest]]: [MySQL] 33,599 pass(es).
[ View ]
#17 install_redirect_on_empty_database-728702-17.patch1.29 KBpillarsdotnet
PASSED: [[SimpleTest]]: [MySQL] 30,039 pass(es).
[ View ]
#10 728702-redirect-to-install-empty-db.patch979 bytesDamien Tournoud
PASSED: [[SimpleTest]]: [MySQL] 29,401 pass(es).
[ View ]
#9 728702-redirect-to-install-empty-db.patch980 bytesDamien Tournoud
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 728702-redirect-to-install-empty-db_0.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#7 728702-redirect-to-install-empty-db.patch948 bytesDamien Tournoud
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

Comments

BTW, I would like to suggest if the settings.php is already populated with database credential and the database is empty, automatically redirect the user to http://localhost/install.php to avoid seeing this error. Thanks.

I can definitely reproduce this. I had actually been meaning to file something about this for a while (some version of this bug actually dates back to Drupal 6, I think) but never bothered - so thanks :) It wouldn't shock me if there is already an existing issue for this, although I can't find one.

The current code in Drupal that redirects you to the installer when you visit the main site URL without having installed Drupal yet is this:

<?php
/**
* Redirect the user to the installation script.
*
* This will check if Drupal has not been installed yet (i.e., if no $databases
* array has been defined in the settings.php file) and we are not already
* installing. If both are true, the user is redirected to install.php.
*/
function _db_check_install_needed() {
  global
$databases;
  if (empty(
$databases) && !drupal_installation_attempted()) {
    include_once
DRUPAL_ROOT . '/includes/install.inc';
   
install_goto('install.php');
  }
}
?>

However, not only is this code not designed to catch the situation described in this issue, it also seems like we hit a fatal error before this function ever gets called...

I see there's already a function handling this issue but it seems not get called. For a mean time, I suggest to have a note in settings.php something like:

If the $databases = array(); is manually or already populated, the user should use http://example.com/install.php instead of the base URL http://example.com to have fresh Drupal installation.

So we can save the users from hair pulling :) I myself did it until I figured out the workaround is to use the http://example.com/install.php whew!

Title:Error occurred in installation if the settings.php populated manuallyInstalling Drupal by visiting index.php (rather than install.php) fails if settings.php already has database credentials

#906828: on fresh setup no redirect to installer script but PDOException table xy not found was duplicate.

Adding a slightly more descriptive title here.

Well, this saved me some hair-pulling, as I am currently setting up a settings.php file for a site, instead of having to input all the data during the installation. I will be sure to direct my browser directly to the install.php file.

Status:Active» Needs review
StatusFileSize
new948 bytes
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

This is an important usability issue.

I suggest we check if the table exists on a cache miss in variable_initialize(). A cache miss there is sufficiently unlikely for this not to affect performance of any site.

Status:Needs review» Needs work

The last submitted patch, 728702-redirect-to-install-empty-db.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new980 bytes
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 728702-redirect-to-install-empty-db_0.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Right, we also need to check if we are not already in maintenance mode :)

StatusFileSize
new979 bytes
PASSED: [[SimpleTest]]: [MySQL] 29,401 pass(es).
[ View ]

And the name of the table is 'variable' not 'variables'.

Status:Needs review» Closed (duplicate)

Duplicate? See my issue of a few months ago: #890820: Redirect to install.php when database is empty

Status:Closed (duplicate)» Needs review

This one is definitely older.

Tested the patch and it worked like a charm.

Without patch: Visited the base url with database settings already defined in settings.php, and with a clean database, causes error.

With patch: Successfully visited the base url with database settings already defined in settings.php, and with a clean database, lets me install like normal. After installation I can visit my new site through the base url like normal.

This patch would save me from having to remember to explicitly specify http://.../install.php when I want to install my sites with a predefined settings.php.

The overall approach seems reasonable to me.

+    if (!db_table_exists('variable') && !defined('MAINTENANCE_MODE')) {

Instead of checking MAINTENANCE_MODE itself here (and specifically all values of it), I think it would be better to call drupal_installation_attempted() instead? That's what's done in similar code in _drupal_bootstrap_database() already, and it would be easier to understand also.

Version:7.x-dev» 8.x-dev
Issue tags:+needs backport to D7

StatusFileSize
new1.29 KB
PASSED: [[SimpleTest]]: [MySQL] 30,039 pass(es).
[ View ]

Re-roll including suggestion from #14.

Why not put the lock_acquire() in a try/catch and do the db_table_exists() there?

StatusFileSize
new1.79 KB
PASSED: [[SimpleTest]]: [MySQL] 33,599 pass(es).
[ View ]

Rerolled with that change.

Status:Needs review» Reviewed & tested by the community

Looks good to me, and solves the problem for both d8 and d7.

Status:Reviewed & tested by the community» Needs work
Issue tags:-needs backport to D7

The last submitted patch, install_redirect_on_empty_database-728702-21.patch, failed testing.

Status:Needs work» Needs review
Issue tags:+needs backport to D7

Status:Needs review» Reviewed & tested by the community

I didn't see this was RTBC already, so I tested it.
While I can't judge the code, it works beautifully!
RTBC++

Status:Reviewed & tested by the community» Needs work

does not apply anymore

Status:Needs work» Needs review
StatusFileSize
new2.28 KB
PASSED: [[SimpleTest]]: [MySQL] 33,054 pass(es).
[ View ]

Re-rolled.

Title:Installing Drupal by visiting index.php (rather than install.php) fails if settings.php already has database credentialsVisiting index.php should redirect to install.php if settings.php already has database credentials but database is empty.

Better title.

Back to RTBC.
I checked that the patch is equivalent with #21.

Status:Needs review» Reviewed & tested by the community

Huh?, maybe this time...

Status:Reviewed & tested by the community» Needs review

Would it be a really bad idea to put this try/catch directly in index.php around the drupal_bootstrap? That way we don't need to move it around every time the first database query in bootstrap moves around.

StatusFileSize
new1.09 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in index.php.
[ View ]

Like this?

Status:Needs review» Needs work

The last submitted patch, install-redirect-on-empty-database-728702-34.patch, failed testing.

StatusFileSize
new1.1 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch install-redirect-on-empty-database-728702-36.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots.
[ View ]

Corrected typo by adding the missing closing parenthesis.

Status:Needs work» Needs review

Works great! Good work ;)

I can confirm that the patch works. I emptied the database of an already installed Drupal and got redirected to install.php when hitting the site.

However, I found this behavior jarring. I got no notification that Drupal had trouble accessing the variables table in the database, in fact no notification that anything was amiss at all.

Wouldn't at least a message like "Drupal has problems accessing the database xyz, which is registered in settings.php. By selecting an installation profile, you can reinstall Drupal now. Yadda yadda yadda..." be in order?

Under what conditions could this behavior be triggered? Wouldn't anonymous users accessing the site with a missing variables table (for whatever reasons) be presented with the installer? Do we want that?

@reglogge

Wouldn't at least a message ... be in order?

Sorry, but Drupal currently does not support passing messages through a redirect without database support. I suppose it would be possible using $_GET or $_SESSION or via Javascript, but that would be a separate feature request, not part of this issue.

Under what conditions could this behavior be triggered? Wouldn't anonymous users accessing the site with a missing variables table (for whatever reasons) be presented with the installer?

Yes, and if you have a multisite install that lacks a sites/default/settings.php file, then anonymous users accessing the site via an unconfigured domain name will also be presented with the installer.

Do we want that?

Well, that's part of what this issue is trying to figure out. Feel free to offer your opinion.

Under what conditions could this behavior be triggered? Wouldn't anonymous users accessing the site with a missing variables table (for whatever reasons) be presented with the installer?

Well a common scenario for production sites is that anonymous users do not have permissions to execute install.php, in which case they get a 403.

Status:Needs review» Needs work
Issue tags:+Novice

Note that the Drupal 8.x patch will need to be rerolled, because the core directory structure for Drupal 8 has now changed. (For more information, see #22336: Move all core Drupal files under a /core folder to improve usability and upgrades). When the patch has been rerolled, please set the issue back to "Needs Review."

Tagging as novice for the task of rerolling the Drupal 8.x patch.

If you need help rerolling this patch, you can come to core office hours or ask in #drupal-gitsupport on IRC.

Status:Needs work» Needs review
Issue tags:-Novice

Righht, so, as reglogge points out, this may be the one patch in all of core that does not need a reroll. :)

StatusFileSize
new654 bytes
PASSED: [[SimpleTest]]: [MySQL] 33,762 pass(es).
[ View ]

Oh well. The patch doesn't apply anymore after all :-(

Rerolled and attached. This is a straight reroll from #36.

Status:Needs review» Needs work

+    include_once DRUPAL_ROOT . '/includes/install.inc';

Should be:

+    include_once DRUPAL_ROOT . '/core/includes/install.inc';

Status:Needs work» Needs review
StatusFileSize
new659 bytes
PASSED: [[SimpleTest]]: [MySQL] 33,751 pass(es).
[ View ]

of course.

Pruned an army of auto-generated testbot comments from yesterday.

I'm concerned that #52 didn't fail. Do we need test coverage for this?

Do we need test coverage for this?

Ask rfay, but I'm not sure that this can be tested via Simpletest.

Maybe just unit tests, if nothing else? We do have some bootstrap test coverage, though none for the installer that I could find.

Well, let's see. You'd have to delete (at least) the {variable} table, then load a page and check to see if it redirects to core/install.php.

So... something like:

installer.test
<?php
class InstallerRedirectTestCase extends DrupalWebTestCase {
  public static function
getInfo() {
    return array(
     
'name' => 'Installer redirect test',
     
'description' => 'Tests whether removing the {variable} table results in an installer redirect.',
     
'group' => 'Installer',
    );
  }
 
/**
   * Tests whether removing the {variable} table results in an installer redirect.
   */
 
function testInstallerRedirect() {
   
// Rename the variable table out of the way.
   
$prefix = $GLOBALS['drupal_test_info']['test_run_id'];
   
$variable_table = $prefix . 'variable';
   
$variable_temp = $variable_table . 'TEMP';
   
mysql_query("RENAME TABLE $varable_table TO $variable_temp");
   
// Load the main page, which should redirect to the installer.
   
$this->drupalGet('');
   
// I have no idea how to test this, so for now just print it.
   
$this->verbose($this->drupalGetContent());
   
// Rename the variable table back.
   
mysql_query("RENAME TABLE $varable_temp TO $variable_table");
  }

?>

Status:Needs review» Needs work

By the way, the patch in #54 is still wrong.

+    install_goto('install.php');

should be:

+    install_goto('core/install.php');

Status:Needs work» Needs review

Status:Needs review» Needs work

Observation in #59 is still valid.

Status:Needs work» Needs review
StatusFileSize
new1.82 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-728702-62.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

drupalGet fails if the variable table is missing, so I switched it to system. It wasn't clear from the thread exactly WHY the variable table was chosen, so I hope this is still fine. Otherwise, I'm not sure how this would be tested.

Yeah, the installer is totally not very testable, which I did not know seven months ago.

Edit: #630446: Allow SimpleTest to test the non-interactive installer

This looks really good, but since it has seen quite a few iterations and is a pretty low-level patch, I'm not going to mark RTBC myself.

Status:Needs review» Needs work
Issue tags:+Needs manual testing

Thanks @tim.plunkett and @tstoeckler.

+++ b/core/modules/system/tests/installer.test
@@ -50,3 +50,30 @@ class InstallerLanguageTestCase extends WebTestBase {
+class InstallerRedirectTestCase extends WebTestBase {

This class needs a docblock.

I'm ishy on the whole rename-the-system-table thing, but it seems like the only solution for testing this.

It would also be good to do a round of manual testing in D8, and again in D7 if we backport.

Assigned:Unassigned» ijf8090

Not removing the tag, but I tested this a couple times before. (We were checking for {variable} not {system} then, but that shouldn't matter.)

Status:Needs work» Needs review
Issue tags:-Needs manual testing, -needs backport to D7

#62: drupal-728702-62.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:+Needs manual testing, +needs backport to D7

The last submitted patch, drupal-728702-62.patch, failed testing.

I'd recommend to close this issue.

The redirect functionality in question does not work at all in D8 anymore, and after studying the code for quite some time as part of #1757536: Require settings.php to exist in the root folder and to explicitly opt-in for multi-site functionality, I'm relatively confident that this convenience redirect feature cannot be supported anymore. To install Drupal (8), you need to visit install.php.

I guess it's too late for a D7-only patch?

Issue summary:View changes

Updated issue summary.

Issue summary:View changes

This is what is in Core now. Seems pretty close to what @tim.plunkett suggested in 2012.

try {
  drupal_handle_request();
}
catch (Exception $e) {
  $message = 'If you have just changed code (for example deployed a new module or moved an existing one) read <a href="http://drupal.org/documentation/rebuild">http://drupal.org/documentation/rebuild</a>';
  if (\Drupal\Component\Utility\Settings::get('rebuild_access', FALSE)) {
    $rebuild_path = $GLOBALS['base_url'] . '/rebuild.php';
    $message .= " or run the <a href="$rebuild_path">rebuild script</a>";
  }
  print $message;
  throw $e;
}

Can we close this issue?

I'd be very hesitant to add even more of these checks...

In particular the proposed one here would be executed on all production sites in case the condition happens to be triggered for any kind of reason.

In essence, any kind of database error — including when the database server is temporarily down — will trigger the code path, greeting all visitors of your site with the Drupal installer.

Therefore, I'm actually strongly opposed to this proposal and would recommend to close this issue.


In fact, even the already existing check for (empty) database settings in the regular bootstrap is problematic from my perspective.

I think we're trying to do way too much "convenience" operations in the critical path of every single bootstrap.

Not to mention that it allows anyone to install another instance of Drupal on your server, in case the right conditions for a multi-site directory happen to be met (for whichever reason).

If you're installing Drupal, then you should know that you're install Drupal. Go to install.php.