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

CommentFileSizeAuthor
#163 728702-163.patch22.16 KBdaffie
#152 interdiff-728702.txt8.71 KBCrell
#152 728702-install-redirect.patch22.25 KBCrell
#146 interdiff.txt3.34 KBamateescu
#146 728702-146.patch22.4 KBamateescu
#145 interdiff-728702.txt2.71 KBCrell
#145 728702-install-redirect.patch21.38 KBCrell
#142 728702-142-UncaughtExceptionTest.patch693 bytesdaffie
#140 interdiff-728702-135-140.txt7.71 KBdaffie
#140 728702-140.patch20.53 KBdaffie
#137 interdiff-728702-135-137.txt7.71 KBdaffie
#137 728702-137.patch19.84 KBdaffie
#135 interdiff-728702.txt988 bytesCrell
#135 728702-install-redirect.patch15 KBCrell
#133 interdiff-728702.txt8.15 KBCrell
#133 728702-install-redirect.patch14.04 KBCrell
#130 728702-install-redirect.patch8.26 KBCrell
#130 interdiff-728702.txt2.47 KBCrell
#121 interdiff-728702.txt3.29 KBCrell
#121 728702-install-redirect.patch8.72 KBCrell
#118 interdiff-728702-100-118.txt4 KBdaffie
#118 interdiff-728702-115-118.txt663 bytesdaffie
#118 728702-118.patch8.99 KBdaffie
#116 interdiff-728702-100-115.txt4.02 KBdaffie
#115 728702-115.patch9 KBdaffie
#115 interdiff-728702-100-115.txt3.16 KBdaffie
#110 interdiff.txt11.68 KBDavid_Rothstein
#110 728702-install-redirect-110.patch8.56 KBDavid_Rothstein
#107 interdiff-728702.txt3.55 KBCrell
#107 728702-install-redirect.patch6.24 KBCrell
#104 interdiff-728702.txt2.23 KBCrell
#104 728702-install-redirect.patch2.69 KBCrell
#97 728702-install-redirect.patch1.85 KBCrell
#92 drupal-redirect_to_install-728702-92.patch3.18 KBpjcdawkins
#85 drupal-redirect_to_install-728702-85.patch3.18 KBpjcdawkins
#78 drupal-redirect_to_install-728702-78.patch3.19 KBpjcdawkins
#77 drupal-redirect_to_install-728702-77.patch4.49 KBpjcdawkins
#62 drupal-728702-62.patch1.82 KBtim.plunkett
#54 install-redirect-on-empty-database-728702-54.patch659 bytesreglogge
#52 install-redirect-on-empty-database-728702-52.patch654 bytesreglogge
#36 install-redirect-on-empty-database-728702-36.patch1.1 KBpillarsdotnet
#34 install-redirect-on-empty-database-728702-34.patch1.09 KBpillarsdotnet
#29 install-redirect-on-empty-database-728702-29.patch2.28 KBpillarsdotnet
#21 install_redirect_on_empty_database-728702-21.patch1.79 KBDavid_Rothstein
#17 install_redirect_on_empty_database-728702-17.patch1.29 KBpillarsdotnet
#10 728702-redirect-to-install-empty-db.patch979 bytesDamien Tournoud
#9 728702-redirect-to-install-empty-db.patch980 bytesDamien Tournoud
#7 728702-redirect-to-install-empty-db.patch948 bytesDamien Tournoud
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

arpeggio’s picture

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.

David_Rothstein’s picture

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:

/**
 * 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...

arpeggio’s picture

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!

David_Rothstein’s picture

Title: Error occurred in installation if the settings.php populated manually » Installing 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.

Tor Arne Thune’s picture

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.

Damien Tournoud’s picture

Status: Active » Needs review
FileSize
948 bytes

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.

Damien Tournoud’s picture

Status: Needs work » Needs review
FileSize
980 bytes

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

Damien Tournoud’s picture

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

BarisW’s picture

Status: Needs review » Closed (duplicate)

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

Damien Tournoud’s picture

Status: Closed (duplicate) » Needs review

This one is definitely older.

Tor Arne Thune’s picture

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.

David_Rothstein’s picture

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.

Tor Arne Thune’s picture

Version: 7.x-dev » 8.x-dev
Issue tags: +Needs backport to D7
Tor Arne Thune’s picture

pillarsdotnet’s picture

Re-roll including suggestion from #14.

pillarsdotnet’s picture

pillarsdotnet’s picture

catch’s picture

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

David_Rothstein’s picture

Rerolled with that change.

pillarsdotnet’s picture

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.

pillarsdotnet’s picture

Status: Needs work » Needs review
Issue tags: +Needs backport to D7
pillarsdotnet’s picture

Status: Needs review » Reviewed & tested by the community
xjm’s picture

tstoeckler’s picture

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

klausi’s picture

Status: Reviewed & tested by the community » Needs work

does not apply anymore

pillarsdotnet’s picture

Status: Needs work » Needs review
FileSize
2.28 KB

Re-rolled.

pillarsdotnet’s picture

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

Better title.

tstoeckler’s picture

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

tstoeckler’s picture

Status: Needs review » Reviewed & tested by the community

Huh?, maybe this time...

catch’s picture

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.

pillarsdotnet’s picture

Status: Needs review » Needs work

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

pillarsdotnet’s picture

Corrected typo by adding the missing closing parenthesis.

pillarsdotnet’s picture

Status: Needs work » Needs review
BarisW’s picture

Works great! Good work ;)

reglogge’s picture

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?

pillarsdotnet’s picture

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

tstoeckler’s picture

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.

catch’s picture

xjm’s picture

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.

xjm’s picture

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

reglogge’s picture

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

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

pillarsdotnet’s picture

Status: Needs review » Needs work
+    include_once DRUPAL_ROOT . '/includes/install.inc';

Should be:

+    include_once DRUPAL_ROOT . '/core/includes/install.inc';
reglogge’s picture

Status: Needs work » Needs review
FileSize
659 bytes

of course.

xjm’s picture

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?

pillarsdotnet’s picture

Do we need test coverage for this?

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

xjm’s picture

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

pillarsdotnet’s picture

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

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");
  }
 
pillarsdotnet’s picture

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');
endyope’s picture

Status: Needs work » Needs review
pillarsdotnet’s picture

Status: Needs review » Needs work

Observation in #59 is still valid.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
1.82 KB

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.

xjm’s picture

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

tstoeckler’s picture

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.

xjm’s picture

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.

ijf8090’s picture

Assigned: Unassigned » ijf8090
tstoeckler’s picture

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

himerus’s picture

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.

sun’s picture

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: Move settings.php to /settings directory, fold sites.php into settings.php, I'm relatively confident that this convenience redirect feature cannot be supported anymore. To install Drupal (8), you need to visit install.php.

pillarsdotnet’s picture

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

pillarsdotnet’s picture

Issue summary: View changes

Updated issue summary.

mgifford’s picture

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?

sun’s picture

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.

milos.kroulik’s picture

I find sun's comment from #73 very reasonable.

jhedstrom’s picture

Version: 8.0.x-dev » 7.x-dev
Status: Needs work » Active

Bumping back to Drupal 7 since this is pretty well handled in 8 now. However, given #73, it might just be a 'wont-fix'?

pjcdawkins’s picture

Version: 7.x-dev » 8.0.x-dev

jhedstrom: is it well handled in D8? I can still reproduce this - after manually configuring everything in settings.php/settings.local.php, but before installing, I'm not redirected.

pjcdawkins’s picture

Status: Active » Needs review
FileSize
4.49 KB

I'm not convinced by #70 or #73. We can easily check whether something is a database error, as opposed to a core database table not existing. And regarding 'convenience', well, that's Drupal's raison d'être.

I see there is no longer a 'system' table.

I expect this could be a lot more efficient, and I'm not sure about how to write tests for this, but here's a PoC.

A couple of disclaimers:

  • The existing check for whether any database connection info exists assumes that Drupal is going to have a database. That seems a bit old-fashioned, but I have not changed that assumption.
  • I noticed that install_verify_database_ready() already checks for the existence of the url_alias table (it checks the last table defined in system_schema()), to see whether Drupal is already installed. I'm checking for the 'sessions' table, as that seems even more essential than URL aliases, and I didn't want to include the system.install file.
pjcdawkins’s picture

A simpler patch - the try/catch was a red herring, detection is actually about whether table(s) exists, database exceptions are not relevant.

cweagans’s picture

Status: Needs review » Needs work

From #73:

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.

We probably need to address this if this patch is going to move forward. I think it's convenient, but we need to make sure that we're not redirecting when the database is down or otherwise unreachable, and this behavior should be something that can be disabled in settings.php as well.

cweagans’s picture

Note: patch in https://www.drupal.org/node/2581457#comment-10421415 also creates the drupal_is_installed() function, but slightly expanded and hardened using the contents of another existing function in core.

pjcdawkins’s picture

Re. #79: that is no longer relevant, as I'm not triggering the redirect based on a PDOException. A database exception will just bubble up uncaught; no redirect will take place.

There is another possible problem, in that this introduces a database call during an earlier phase of bootstrap than normal. I'm not sure whether that's a genuine problem though.

Re. #80 OK, but the system_schema() function is only available if you include the system.install file, which I thought was not ideal (I mentioned that in #77).

cweagans’s picture

re PDO exception - gotcha. Okay. That's probably fine then. I still think it ought to be something that can be disabled though. It's very rare that I'd actually want that behavior in anything other than a development environment. In fact, I'd go so far to say that it should be off by default and turned on in the example.settings.local.php, but that's just me.

Re extra db query - I don't think this is a problem. Part of the work that I'm going to end up doing tomorrow is to convert installation detection to a service, which means that it's swappable, so you can set up a NoOpInstallationCheck that always returns true to save the db query.

#81 - I think it's the most solid way to verify based on a DB table whether or not Drupal is installed. Plus, this file is already included at any point in the installer that you'd want to check if Drupal is already installed or not. It'll always be accurate, and we won't have to update it if tables are removed or renamed. I've still got some more work to do over in the other issue, but I'm going to have to tackle it tomorrow, as it's quite late right now.

pjcdawkins’s picture

"It's very rare that I'd actually want that behavior in anything other than a development environment"

Yes I agree with that, if it can be enabled in settings.php. My use case is to provide a D8 demo/example, without users complaining that they see an exception when hitting /.

Users who haven't got a settings.php yet won't encounter this problem, so disabled by default is good.

"Re extra db query - I don't think this is a problem."

Yes, well, it's certainly not a problem if you make it disabled by default.

pjcdawkins’s picture

Status: Needs work » Needs review

Reroll of #78 FWIW - I haven't been following closely.

cweagans, have you made any progress on the other strategy?

pjcdawkins’s picture

mgifford’s picture

Does this need the <h3>Why this should be an RC target</h3> info & RC phase evaluation table? Also there's the "rc target triage" tag.

I'd like this in for the 8.0 release for sure.

cweagans’s picture

@pjcdawkins, FYI, I uploaded a new patch on the other issue. I don't think we should hardcode a new database query into every page load without giving users a way to turn it off. The other patch essentially converts it to a service and uses it in the appropriate places.

Just note the other issue as a dependency of this one, and then this patch is basically only this hunk:

diff --git a/core/lib/Drupal/Core/DrupalKernel.php b/core/lib/Drupal/Core/DrupalKernel.php
index 0c16cb3..3bb87bb 100644
--- a/core/lib/Drupal/Core/DrupalKernel.php
+++ b/core/lib/Drupal/Core/DrupalKernel.php
@@ -13,7 +13,6 @@
 use Drupal\Component\Utility\UrlHelper;
 use Drupal\Core\Config\BootstrapConfigStorageFactory;
 use Drupal\Core\Config\NullStorage;
-use Drupal\Core\Database\Database;
 use Drupal\Core\DependencyInjection\ContainerBuilder;
 use Drupal\Core\DependencyInjection\ServiceModifierInterface;
 use Drupal\Core\DependencyInjection\ServiceProviderInterface;
@@ -627,9 +626,9 @@ public function handle(Request $request, $type = self::MASTER_REQUEST, $catch =
       $this->initializeSettings($request);
 
       // Redirect the user to the installation script 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 (!Database::getConnectionInfo() && !drupal_installation_attempted() && PHP_SAPI !== 'cli') {
+      // installed yet, and we are not already installing, and we are not on the
+      // command line.
+      if (!drupal_installation_attempted() && PHP_SAPI !== 'cli' && !drupal_is_installed()) {
         $response = new RedirectResponse($request->getBasePath() . '/core/install.php');
       }
       else {
Rafael Cansinos’s picture

I am not sure if this help, but I had a similar problem, all the time redirect to install.php, with data base with some problems, but not empty, and I resolve it changing permission of settings.php to: chmod 444 settings.php. And it is important to have default.settings.php in default folder.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Status: Needs review » Needs work

The last submitted patch, 85: drupal-redirect_to_install-728702-85.patch, failed testing.

jonathanshaw’s picture

Issue tags: +Needs reroll

Needs reroll for 8.1

pjcdawkins’s picture

Assigned: ijf8090 » Unassigned
Status: Needs work » Needs review
FileSize
3.18 KB

Re-rolled

dimaro’s picture

Issue tags: -Needs reroll

The latest patch now apply.

Crell’s picture

I can confirm this behaves as expected when we run it on Platform.sh. The redirect happens without incident.

alexpott’s picture

Status: Reviewed & tested by the community » Needs review

The current patch will add an extra query during bootstrap for all sites. I'm not sure that that is acceptable. Is it possible to do the query and redirect in the exception handler? Therefore we'd only have the extra query if stuff was already going wrong.

hussainweb’s picture

We could also look at #2581457-5: Pre-configured settings.php with uninstalled Drupal causes the installer to refuse to move forward which makes it possible to override the database query.

Crell’s picture

Hm. Fair point, Alex. Here's a completely new approach that catches the exception in DrupalKernel, instead.

We could quibble over what other "not installed" detections we want to include, but I think these are fine. I also tried trapping access denied, but that results in a redirect to the installer, which tells you the site is already installed. :-)

Patch is against 8.1.x, but should apply to 8.2.x as well, probably.

hussainweb’s picture

When I tested this by clearing out the configured database, it did not throw an exception in \Drupal\Core\DrupalKernel at all. Instead, this is the exception thrown in Symfony's HttpKernel.

Symfony\Component\HttpKernel\Exception\NotFoundHttpException: in Drupal\Core\PathProcessor\PathProcessorFront->processInbound() (line 43 of core/lib/Drupal/Core/PathProcessor/PathProcessorFront.php).

This is returned as a regular response to DrupalKernel(with 404 code) which just prints it. The exception handler is not entered at all. I found this when I was trying the same approach as @Crell in #97.

I'll try the patch anyway later but I thought I'd share my finding here.

hussainweb’s picture

Status: Needs review » Needs work

I just tried the patch in #97 and as expected, it didn't work. DrupalKernel's handleException is never called for this.

To test, I applied the patch and ran drush sql-drop -y for good measure. I then tried to load the website and got the same output as shown in #98 (i.e., without the patch).

Symfony\Component\HttpKernel\Exception\NotFoundHttpException: in Drupal\Core\PathProcessor\PathProcessorFront->processInbound() (line 43 of core/lib/Drupal/Core/PathProcessor/PathProcessorFront.php).

This is the reason I suggested looking into #2581457-5: Pre-configured settings.php with uninstalled Drupal causes the installer to refuse to move forward in #96.

David_Rothstein’s picture

Odd, I tried #97 (on 8.2.x) and it redirected fine for me.

Running the check in an exception handler is definitely an improvement - that was actually the direction the issue was headed previously (a mere 4 years ago...) but for some reason it was abandoned.

  1. +    // Code 1049 is "missing database".
    +    if ($e->getCode() == 1049) {
    +      return TRUE;
    +    }
    

    Clever. And hopefully safe? (i.e. no chance of a false-positive where this error code appears on a live site in the case of a "database down" situation?)

  2. +    // This weird set of conditions is how we detect that the error was the
    +    // sessions table was missing, which indicates an uninstalled Drupal.
    +    if ($e->getCode() == 0
    +        && strpos($e->getMessage(), 'SQLSTATE[42S02]') !== FALSE
    +        && strpos($e->getMessage(), 'sessions') !== FALSE) {
    +      return TRUE;
    +    }
    

    This is fragile because it relies on a particular query being the first one that Drupal runs. Why not do something like the earlier patches and just run a query to check this directly instead, e.g. Database::getConnection()->schema()->tableExists('sessions')?

  3. This may also need the drupal_installation_attempted() check somewhere that was in the earlier patches (to prevent the installer from ever redirecting back to itself if an exception is thrown while Drupal is being installed).
  4. +      return new RedirectResponse($request->getBasePath() . '/core/install.php');
    

    Earlier patches used https://api.drupal.org/api/drupal/core!includes!install.inc/function/ins... here, which has some stuff in it to make sure the redirect isn't treated as permanent. That might be important.

hussainweb’s picture

Definitely weird. Can you tell me how you tested this? I might be missing something.

I switched to 8.2.x and tried and it and it still didn't work. I made sure to run composer install and drush sql-drop both times.

hussainweb’s picture

To make sure, I cleaned everything and tested this again and it worked this time. I got the expected exception without the patch and then redirect with the patch. I am not sure what changed but it was the same sequence of commands I tried earlier. Anyway, I am going to leave it to needs work for comments in #100.

David_Rothstein’s picture

I actually hadn't run composer install the last time I did a git update, so I tried it again with that (just to make sure) and I do get the same results - patch works on both 8.1.x and 8.2.x. Glad you are getting the same thing now :)

Crell’s picture

Status: Needs work » Needs review
FileSize
2.69 KB
2.23 KB

Attached patch addresses #100.3 and 4. Also, if we're going to send a 302 redirect it makes sense to do so for the other install-check as well.

PDO error codes are, unfortunately, grossly inconsistent. They're a combination of SQL-standard error codes and database-specific error codes. After some additional testing, here's a revised version. However, I'm not sure this is fully final yet.

Here's what my testing found, after installing normally and then breaking it in various ways:

1) If logged in and the database is missing, redirect to installer. OK.
2) If anonymous and database is missing, redirect to installer. OK.
3) If logged in and a table is missing, the first fail is for the session table, which happens at the bootstrap level, so this code catches it. OK.
4) If anonymous and a table is missing, the first fail is from PathProcessorFront, which gets caught by the exception listeners in place inside HttpKernel, and results in a 404 Response. NO error handling code in DrupalKernel runs at all.

I am unclear if this is sufficient; I suspect not, but I'm not certain of the best way to handle it. (But it's probably why the previous approach was more stable, since it made an active check rather than a passive check and thus didn't have this problem.)

Perhaps we need to duplicate this functionality in a kernel exception listener, which can depend on the database service and therefore do a session table check? That would add an SQL query to every exception path, although those are presumably more rare than kernel boot. Except on 404s/403s, which would then get an extra query. And on other database errors which may or may not then break as a result...

David_Rothstein’s picture

The fixes for #100.3 and 4 look good.

If anonymous and a table is missing, the first fail is from PathProcessorFront, which gets caught by the exception listeners in place inside HttpKernel, and results in a 404 Response.

Oh, that completely explains what @hussainweb saw above then.

Perhaps we need to duplicate this functionality in a kernel exception listener, which can depend on the database service and therefore do a session table check? That would add an SQL query to every exception path, although those are presumably more rare than kernel boot. Except on 404s/403s, which would then get an extra query.

It's worth asking why that actually needs to be every exception rather than just database exceptions. PathProcessorFront::processInbound throws a NotFoundHttpException(), but you'd think it should fail earlier than that with a database exception (since it's trying to load config from the database but the config table doesn't even exist).

The main answer seems to be that DatabaseStorage::readMultiple catches exceptions silently. That doesn't seem right but trying to fix that here would probably lead down a rabbit hole, so I guess the conclusion is indeed that Drupal 8 will need to run the extra query on 404 pages (at the very least). It's not ideal but it doesn't seem that bad to me either, and still way better than the earlier patch which ran it on every request.

David_Rothstein’s picture

Crell’s picture

Well... nominally we can't assume that Drupal is running on SQL at all, can we? 99% of installs are, but we technically support running on all-non-SQL. At least in theory. :-) So if we do a blanket "on exception, if there's no sessions table then redirect to installer" then anyone on MongoDB would get redirected to the installer for any missing url, which seems undesireable... But to do more than that, we'd need to either do an affirmative check (query on every request) or get a more specific exception that we could check against, because yes the logic in processInbound() and readMultiple() right now is totally stupid.

But... eh, let's try this and see what happens. I'm going to be out of town for the next few days so if someone else wants to pick this up and run with it, feel free. (Or mark it RTBC and get it committed, that's good too.)

Status: Needs review » Needs work

The last submitted patch, 107: 728702-install-redirect.patch, failed testing.

Crell’s picture

Well, that didn't work... I am guessing from the test log that the problem is we're redirecting to install way too often, which is what I was afraid of.

David_Rothstein’s picture

I went ahead and played with this a bit and came up with the attached version:

  1. I combined the two separate checks into a single helper function to consolidate the code, which now does the tableExists() whenever it can.
  2. An advantage of that is that we no longer need to redirect whenever 'SQLSTATE[42S02]' appears in the error message - i.e. we don't need to assume that any early-bootstrap query on a nonexistent table means Drupal isn't installed yet. It is better not to make that assumption.
  3. I fixed a couple typos, the biggest being tableExists('session') => tableExists('sessions') which is an important one :)

There seem to be at least eight different scenarios for how the redirect can happen (!), which include all possible combinations of:

  • User with or without a session
  • Trying to visit the homepage vs. some other page
  • Database exists or doesn't exist

All eight seem to work with this new patch based on manual testing (on 8.1.x), and I also tested that exceptions on an already-installed site (e.g. 404 pages, database connection failures, etc.) don't trigger the redirect.

If we can write automated tests for at least some of those scenarios that would be nice... In the meantime let's see if it passes existing tests.

Regarding sites that don't store their sessions in an SQL database (or that don't use SQL at all) it seems to me like that's actually covered? I wrote this in the patch:

+    // Check for the presence of a database table provided in the hook_schema()
+    // implementation of a required core module. That should always be present
+    // if Drupal is already installed.
+    if (isset($connection)) {
+      try {
+        return !$connection->schema()->tableExists('sessions');

If sessions aren't in an SQL database, the sessions table may be empty/unused, but based on how it gets created during install it seems to me like tableExists('sessions') always should return TRUE on an installed site, unless someone went in and manually deleted it by hand. But if I'm wrong we can add back the comment about that limitation.

Status: Needs review » Needs work

The last submitted patch, 110: 728702-install-redirect-110.patch, failed testing.

alexpott’s picture

@David_Rothstein #110 looks really nice and complete... one thought:

+++ b/core/lib/Drupal/Core/Installer/CheckInstalledTrait.php
@@ -0,0 +1,80 @@
+    // Check for the presence of a database table provided in the hook_schema()
+    // implementation of a required core module. That should always be present
+    // if Drupal is already installed.
+    if (isset($connection)) {
+      try {
+        return !$connection->schema()->tableExists('sessions');
+      }
+      catch (\Exception $e) {
+        // If the query failed, use the exception it failed with for the rest
+        // of the checks in this function; it will be more reliable since
+        // unlike the original exception, we know exactly where it came from.
+        $exception_to_check = $e;
+      }
+    }
+
+    // If we still have an exception at this point, we need to be careful since
+    // we should not redirect if the exception represents an error on an
+    // already-installed site (for example, if the database server went down).
+    // However a database error with code 1049 means "missing database", and it
+    // should be safe to redirect in that case.
+    return ($exception_to_check instanceof \PDOException || $exception_to_check instanceof DatabaseException) && $exception_to_check->getCode() == 1049;

I think rather than doing this here we could delegate at this point to the database driver / connection class. The default implementation would be as we see here - so mysql, psql, sqlite all do this (i.e. check sessions table and this particular exception code) but something like Mongo or whatever else can do it's own thing.

Crell’s picture

Nice consolidation, David! Just a few minor points:

  1. +++ b/core/lib/Drupal/Core/EventSubscriber/ExceptionDetectNeedsInstallSubscriber.php
    @@ -24,65 +22,44 @@ class ExceptionDetectNeedsInstallSubscriber extends HttpExceptionSubscriberBase
    +  public function __construct(Connection $connection) {
    +    $this->connection = $connection;
       }
    

    A side effect of this is that even on a non-SQL-based site, we will still need to initialize an empty SQL connection on every exception. That likely makes this subscriber another class that a non-SQL-based site needs to rip out entirely. That should be documented somehow, at least in the class's docblock.

  2. +++ b/core/lib/Drupal/Core/Installer/CheckInstalledTrait.php
    @@ -0,0 +1,80 @@
    +    $exception_to_check = $exception;
    

    Why?

It would be nice if we could somehow see what happens on an entirely non-SQL installation, since we do technically enable that in D8. That said, I'm unclear what automated tests for this would look like that aren't too micro-level to really test that the integrated behavior is correct.

Crell’s picture

Status: Needs work » Needs review

Silly bot...

daffie’s picture

FileSize
4.02 KB

Sorry, wrong interdiff.

Status: Needs review » Needs work

The last submitted patch, 115: 728702-115.patch, failed testing.

Crell’s picture

Status: Needs review » Reviewed & tested by the community

No idea why it hiccuped on Postgres before, but it seems to be working now and the code looks OK. Thanks, daffie!

tstoeckler’s picture

Status: Reviewed & tested by the community » Needs work

In Connection::databaseEmpty():

+      // since we should not redirect if the exception represents an error on an

This should not mention "redirecting" as that is not within the function's scope.

+    // Default to FALSE to minimize the chance of overwriting an installed
+    // database.
+    return FALSE;

AFAIK this can never be reached, so we can just drop it I think.

In CheckInstalledTrait::shouldRedirectToInstaller():

+    // Redirect if the database is empty.
+    if ($connection) {
+      return $connection->databaseEmpty();
+    }
+  }

I guess this is where the return FALSE should go (maybe with a different comment, though), as this function - per its docs - should return a boolean.

Crell’s picture

Hm, you're right. In that case, however, let's move the databaseEmpty() check out of the connection object (since it's doing a very Drupal-specific check, and we want to keep the DB system not Drupal-dependent) and put it into the trait, where the comment is valid.

daffie’s picture

Nitpick.

+++ b/core/lib/Drupal/Core/Installer/CheckInstalledTrait.php
@@ -54,7 +54,37 @@ protected function shouldRedirectToInstaller(\Exception $exception, Connection $
+   * @return bool

Should this then become: @return bool|null

tstoeckler’s picture

I think #121 looks much better, thanks!

I don't quite get #122, in #121 the function does always return a boolean AFAICT.

daffie’s picture

Status: Needs review » Reviewed & tested by the community

My patch from comment #118 was RTBC for user Crell.
The changes made in the patch from comment #121 are for me RTBC.

I have one minor documentation change that can be done on commit and that is to the documentation of the return value CheckInstalledTrait::databaseEmpty(): @return bool|null. The return value can be null.

tstoeckler’s picture

Can you explain in which case NULL will be returned? I don't see it...

daffie’s picture

@tstoeckler: If an exception is thrown wail fetching the session table existence and you get an error that is not 1049, the proces will run to the end of the method and return null. Please correct me if I am wrong.

tstoeckler’s picture

If an exception is thrown while fetching the sessions table, then we reach the catch() clause. That contains the return statement as the only thing that will run - and it will always run. No if statement or anything similar. If the error code is not 1049 - for example - the boolean in the return statement will yield FALSE so that will be returned, not NULL.

daffie’s picture

@tstoeckler: I was wrong and you are right. Thank you for the explanation.

David_Rothstein’s picture

Status: Reviewed & tested by the community » Needs work

#121 looks similar to #110 but is missing code that was there to deal with some of the scenarios; so for example, if the database doesn't exist, #110 (and also Crell's earlier patch in #104) works, but #121 doesn't.

It may still be possible to simplify #110... but I assume the issue is that if the database doesn't even exist, Drupal is incapable of establishing a connection and therefore no $connection object gets created? (I haven't actually debugged it.) That is why the code in #110 dealt with that scenario by falling back on examining the passed-in exception object instead.

Also:

  • #112: It looks to me like most of this isn't database specific, except maybe the $e->getCode() == 1049 check? In any case, if we need to deal with an empty connection object, we probably can't (easily) have the code live in the database driver anyway.
  • #113.1: Seems like a good idea to document that, yes. (On the other hand, are we actually assuming this is an SQL-based database anywhere? What happens if you call $connection->schema()->tableExists() on a non-SQL database.... I would assume it still has to work? This is related to the above point as well.)
  • #113.2: I think I used the multiple variable names to make sure the passed-in $exception object didn't get modified, but it may not have been necessary in the end. (I guess you can't actually modify an object in PHP by overwriting it inside a function, only by doing direct changes to it inside the function... The "objects are passed-by-reference but not exactly" feature of PHP is very confusing :)
Crell’s picture

The issue was the check of the exception itself to see if it's a missing-DB only happened inside databaseEmpty(), which is only called if there is a $connection, which there isn't if the database is missing. Go team! :-)

So let's break that up and do the missing-DB check first, at which point we don't need to bother with the separate method anyway. This works in my manual testing.

David: A non-SQL backend should not be using a Database\Connection object. In Drupal 7 such hackery was necessary, along with some black magic and sacrificing of goats. In Drupal 8, use of Database\Connection for anything that isn't an SQL driver is completely wrong and entirely unsupported. (Well, that was true in Drupal 7, too, but there wasn't an alternative.) Swapping out services at a higher level is the correct way to handle that in Drupal 8.

Which means this code SHOULD work for all cases where Drupal has an SQL backend, and the sessions table exists (even if it's not being used). If Drupal is operating in a completely non-SQL environment, I think it will treat all exceptions as install issues, as it will either say there's no DB credentials at all or they are invalid/missing DB. I... am not sure how to address that.

dawehner’s picture

+++ b/core/lib/Drupal/Core/Installer/CheckInstalledTrait.php
@@ -0,0 +1,79 @@
+    if (($exception instanceof \PDOException || $exception instanceof DatabaseException) && $exception->getCode() == 1049) {

Is this check really super reliable? Isn't the 1049 coming from mysql itself and is not really a thing in pgsql/sqlite?

David_Rothstein’s picture

  1. So let's break that up and do the missing-DB check first

    That does seem to work, but it makes the assumption that this function will only ever get a NotFoundHttpException passed to it when the database exists. The previous code did not assume this so was a little more robust. (In practice, though, it appears to always be the case.)

  2. +    // Default to FALSE to minimize the chance of overwriting an installed
    +    // database.
    +    return FALSE;
    

    That code comment doesn't really make sense, since all this code does is redirect to the installer. (Hopefully there is no scenario in which the installer lets you overwrite an already-installed database... but either way, the above code does nothing to minimize the chance of it happening.)

    Could replace with something like "... to minimize the chance of redirecting to the installer on an already-installed site."

  3. Is this check really super reliable? Isn't the 1049 coming from mysql itself and is not really a thing in pgsql/sqlite?

    It could be, although this patch would still be pretty useful even if it only worked on MySQL. I guess the big question would be whether 1049 means a different kind of error on some other database system (since then you could have a false redirect there). A quick web search for "1049" for PostgreSQL and SQLite did not turn up anything one way or the other.

Crell’s picture

You know what, this is silly. We *already have* a DatabaseNotFoundException, why aren't we using it?

Let's use it. I also added an access denied exception while I was at it, although I realized we shouldn't use that in the redirect check since the installer won't run in that case anyway. I left the new exception in because, really, we ought to have done that in the first place 8 years ago anyway.

#132.1: If this method is called for a not-found exception, it will still run through the same checks; if it's a not-found exception that's not due to a missing or empty database, all the checks will fail and we'll return false (most likely on the tableExists check inside the try block), and the rest of the exception handling process will happen normally. This case is only to handle the, frankly, bug in PathProcessorFront, which we don't want to dig into here as it's a separate rabbit hole.

Status: Needs review » Needs work

The last submitted patch, 133: 728702-install-redirect.patch, failed testing.

Crell’s picture

Status: Needs work » Needs review
FileSize
15 KB
988 bytes

It amuses me that would be the issue. It amuses me even more that there was a spelling error that's been there since who knows when. :-)

David_Rothstein’s picture

Needs some manual testing, but at first glance those changes make a whole lot of sense.

+    // If the database wasn't found, assume the user hasn't entered it properly
+    // and redirect to the installer.
+    if ($exception instanceof DatabaseNotFoundException) {
+      return TRUE;
+    }
+
+    // Never redirect if we're already in the installer.
+    if (drupal_installation_attempted()) {
+      return FALSE;
+    }

I think the order of the above two checks should be reversed, no?

+  protected function shouldRedirectToInstaller(\Exception $exception, Connection $connection = NULL) {

I wonder if it's possible/worthwhile to write tests specifically for this function (like pseudo-unit tests).... There are so many combinations of scenarios that can occur here now, and although actual functional tests would be complicated (and probably impossible for some of the scenarios) testing this function directly would be easier.

Status: Needs review » Needs work

The last submitted patch, 137: 728702-137.patch, failed testing.

daffie’s picture

I created the new method Drupal\Core\Installer\CheckInstalledTrait::isCli() so that it can be mocked in PHPUnit testing.

Status: Needs review » Needs work

The last submitted patch, 140: 728702-140.patch, failed testing.

daffie’s picture

Status: Needs work » Needs review
neclimdul’s picture

+++ b/core/lib/Drupal/Core/Database/DatabaseAccessDeniedException.php
@@ -5,4 +5,4 @@
  * Exception thrown if specified database is not found.

Clearly your failures are coming from this comment saying its thrown when the database is not found but its thrown when access is denied.

Wish I could be of more help but those are weird.

Crell’s picture

I talked with amateescu in IRC, and he pointed out that the Postgres PDO error codes are horribly unreliable. See: #2371709: Move the on-demand-table creation into the database API, where he and chx addressed the problem with a utility method to parse out the SQLState code from the error message, Because that's the best we can do. Because BAH!

So here's an updated version that gets the error code that way, using the code from that issue, and corrects the code being used based on amateescu's tests.

I have no idea idea what's up with #142, honestly. That's just weird.

Oh yeah, and I also fixed the super-critical issue that neclimdul pointed out in #144. :-P

Let's see how the bot likes this one.

amateescu’s picture

Investigated this for a while and found out that the problem is that for Postgres the error code (7) and the sql state code (08006) are the same for both exceptions, it's just the exception message that differs:

SQLSTATE[08006] [7] FATAL:  database "<database>" does not exist
SQLSTATE[08006] [7] FATAL:  password authentication failed for user "<bad_username>"

So the only reasonable way to proceed is... wait for it... more string parsing! :)

daffie’s picture

I performed the following manual checks:

  1. On a installed site I removed the $databases array from settings.php. I was redirected to the installer.
  2. After putting back the $databases array to settings.php, I got a warning from the installer that there was already an installed site with a redirect link to the installed site.
  3. Changed the database name in the $databases array in the file settings.php. I was redirected to the installer.
  4. Changed the username in the $databases array in the file settings.php. I got the following error: "The website encountered an unexpected error. Please try again later.".
  5. Changed the password in the $databases array in the file settings.php. I got the following error: "The website encountered an unexpected error. Please try again later.".

The last two responses do not apply for SQLite, because that database does not have that kind of access checking.

If there needs to be checked more for "Needs manual testing" then let me know. I removed the manual testing tag because my main addition to the current patch is the testing class. If someone does not agree with this then please put back the tag.

jonathanshaw’s picture

Shouldn't this be against 8.2.x now?

Crell’s picture

The fails against 5.6 are HEAD fails and do not block this issue, per Alexpott: #2762549: Drupal\field\Tests\Update\FieldUpdateTest, Drupal\views\Tests\Update\EntityViewsDataUpdateTest and Drupal\comment\Tests\CommentFieldsTest fail on 8.1.x.

So the patch is now working. Someone please RTBC so we can get it committed! :-)

daffie’s picture

@Crell: This issue is for me RTBC. With the exception of the added test. Which I wrote. If you think my test is RTBC, then for me is the whole issue RTBC.

alexpott’s picture

+++ b/core/lib/Drupal/Core/Installer/CheckInstalledTrait.php
@@ -0,0 +1,91 @@
+/**
+ * Provides methods for checking if Drupal is already installed.
+ */
+trait CheckInstalledTrait {
...
+  /**
+   * Determines if an exception handler should redirect to the installer.
+   *
+   * @param \Exception $exception
+   *   The exception to check.
+   * @param \Drupal\Core\Database\Connection|null $connection
+   *   (optional) The default database connection. If not provided, a less
+   *   comprehensive check will be performed. This can be the case if the
+   *   exception occurs early enough that a database connection object isn't
+   *   available from the container yet.
+   *
+   * @return bool
+   *   TRUE if the exception handler should redirect to the installer because
+   *   Drupal is not installed yet, or FALSE otherwise.
+   */
+  protected function shouldRedirectToInstaller(\Exception $exception, Connection $connection = NULL) {

The name and description of the trait and it's only useful method don't really tie. It doesn't provide methods to check if Drupal is installed. It provides a method to check if you should redirect to the installer. So about InstallerRedirectTrait?

Crell’s picture

Renamed per #151. Also, while I was at it I noticed that the test for the trait was using string literal classes. We should be using ::class constants now for PHP 5.5, which allows the class name to be parsed by the engine or by an IDE, which in turn makes this sort of refactoring easier. :-) (I nearly didn't catch that the name of the trait was in a string in that file and therefore wasn't picked up by PHPStorm's Refactor command.)

No functional change, just naming things.

daffie’s picture

@Crell: You made some changes to the test that I wrote. Thank you, because I learned something new.

I think the patch looks great now and for me it is now RTBC. Only I have written some of the patch so I cannot set the status to RTBC.

daffie’s picture

Status: Needs review » Reviewed & tested by the community

Changed my mind. @crell reviewed my the part that I wrote (the InstallerRedirectTraitTest) with the changes that he made to it, in comment #152. The rest of the patch is RTBC for me. I did the manual testing in comment #147.

If somebody does not agree with this, then please change the status back to "Needs review".

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 152: 728702-install-redirect.patch, failed testing.

daffie’s picture

Back to RTBC.

daffie’s picture

Status: Needs work » Reviewed & tested by the community

Back to RTBC.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs change record
+++ b/core/lib/Drupal/Core/Database/Connection.php
@@ -1419,6 +1419,26 @@ public function quote($string, $parameter_type = \PDO::PARAM_STR) {
+  protected static function getSQLState(\Exception $e) {

Needs to be mentioned in a CR for alternate DB engines since they might not have an SQLState and might need to do something different.

+++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Connection.php
@@ -113,7 +122,21 @@ public static function open(array &$connection_options = array()) {
+    try {
+      $pdo = new \PDO($dsn, $connection_options['username'], $connection_options['password'], $connection_options['pdo']);
+    }
+    catch (\PDOException $e) {
+      if (static::getSQLState($e) == static::CONNECTION_FAILURE) {
+        if (strpos($e->getMessage(), 'password authentication failed for user') !== FALSE) {
+          throw new DatabaseAccessDeniedException($e->getMessage(), $e->getCode(), $e);
+        }
+        elseif (strpos($e->getMessage(), 'database') !== FALSE && strpos($e->getMessage(), 'does not exist') !== FALSE) {
+          throw new DatabaseNotFoundException($e->getMessage(), $e->getCode(), $e);
+        }
+      }
+      throw $e;
+    }

Alternate DB Drivers will need to implement something like this - no? - This likes more for the change record to mention.

daffie’s picture

Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs change record

Added a change record.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

alexpott’s picture

Version: 8.2.x-dev » 8.3.x-dev
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

Given the changes this patch makes and the need for CR I think we can only release this in a minor version so bumping the version. Also the patch needs a reroll.

Crell’s picture

Alex: A CR was added 2 weeks ago, and the patch RTBCed initially 2 months ago and sat for a month without comment. Without this patch, Drupal 8.2 will continue to be uninstallable on Platform.sh. (We currently apply an older version of the patch from higher in the thread in our composer.json file.) Another 7 months of needing a now non-small patch to install Drupal on a read-only file system does not strike me as a good situation for anyone involved, but I don't know what exactly we could have done earlier or can do now.

Edit: Correction, it's installable, but it's poor UX since you have to manually go to /core/install.php. Still frustrating given how long this patch has been sitting idle. :-(

daffie’s picture

Rerolled the patch.

cilefen’s picture

Only after looking twice did I realize the CR actually does refer to this issue.

Crell’s picture

Status: Needs review » Reviewed & tested by the community

We just successfully tested and deployed #163 on Platform.sh with 8.1.10. Thanks, daffie!

Edit: Spoke too soon; it's #152 that still applies and works on 8.1.10. I'm just all over jumping the gun today. :-(

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 29ce04e and pushed to 8.3.x. Thanks!

@Crell back Drupal has behaved this way for 7 years. Yes it is great that this will be fixed in the future but it seems odd that Platform.sh is really a behaviour that has never been there.

diff --git a/core/lib/Drupal/Core/Database/Driver/pgsql/Install/Tasks.php b/core/lib/Drupal/Core/Database/Driver/pgsql/Install/Tasks.php
old mode 100755
new mode 100644
diff --git a/core/lib/Drupal/Core/DrupalKernel.php b/core/lib/Drupal/Core/DrupalKernel.php
old mode 100755
new mode 100644

Please don't change files perms in patches... fixed back to 644 on commit.

diff --git a/core/lib/Drupal/Core/Database/Driver/pgsql/Install/Tasks.php b/core/lib/Drupal/Core/Database/Driver/pgsql/Install/Tasks.php
index 7c85eaf..77e0a47 100755
--- a/core/lib/Drupal/Core/Database/Driver/pgsql/Install/Tasks.php
+++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Install/Tasks.php
@@ -4,7 +4,6 @@
 
 use Drupal\Core\Database\Database;
 use Drupal\Core\Database\Install\Tasks as InstallTasks;
-use Drupal\Core\Database\Driver\pgsql\Connection;
 use Drupal\Core\Database\DatabaseNotFoundException;
 
 /**
diff --git a/core/lib/Drupal/Core/Installer/InstallerRedirectTrait.php b/core/lib/Drupal/Core/Installer/InstallerRedirectTrait.php
index b060b1e..067802d 100644
--- a/core/lib/Drupal/Core/Installer/InstallerRedirectTrait.php
+++ b/core/lib/Drupal/Core/Installer/InstallerRedirectTrait.php
@@ -4,7 +4,6 @@
 
 use Drupal\Core\Database\Connection;
 use Drupal\Core\Database\Database;
-use Drupal\Core\Database\DatabaseAccessDeniedException;
 use Drupal\Core\Database\DatabaseException;
 use Drupal\Core\Database\DatabaseNotFoundException;
 use Symfony\Component\HttpKernel\Exception\NotFoundHttpException;
diff --git a/core/tests/Drupal/Tests/Core/Installer/InstallerRedirectTraitTest.php b/core/tests/Drupal/Tests/Core/Installer/InstallerRedirectTraitTest.php
index d88a738..8ca55d2 100644
--- a/core/tests/Drupal/Tests/Core/Installer/InstallerRedirectTraitTest.php
+++ b/core/tests/Drupal/Tests/Core/Installer/InstallerRedirectTraitTest.php
@@ -92,7 +92,7 @@ function testShouldRedirectToInstaller($expected, $exception, $connection, $conn
       // Mock the database connection info.
       $db = $this->getMockForAbstractClass(Database::class);
       $property_ref = new \ReflectionProperty($db, 'databaseInfo');
-      $property_ref->setAccessible(true);
+      $property_ref->setAccessible(TRUE);
       $property_ref->setValue($db, ['default' => $connection_info]);
 
       if ($connection) {

Fixed the following coding standards violations on commit.

  • alexpott committed 29ce04e on 8.3.x
    Issue #728702 by Crell, daffie, pillarsdotnet, pjcdawkins, Damien...
Mile23’s picture

Interestingly, Drupal\Tests\Core\Installer\InstallerRedirectTraitTest causes a fatal when run from PHPUnit, but not run-tests.sh, for 8.3.x.

$ ./vendor/bin/phpunit -c core/ --filter InstallerRedirectTraitTest

Fatal error: Cannot redeclare drupal_valid_test_ua() (previously declared in /Users/paulmitchum/projects/drupal8/core/tests/Drupal/Tests/Core/DrupalKernel/DrupalKernelTest.php:144) in /Users/paulmitchum/projects/drupal8/core/includes/bootstrap.inc on line 657

Basically the test is testing code that has a hard dependency on a bootstrapped kernel, so it should be a KTB test instead of a unit test.

Follow-up issue: #2805757: InstallerRedirectTraitTest causes fatal when run from PHPUnit

alexpott’s picture

It is possible that this change has introduced a new random fail. https://www.drupal.org/pift-ci-job/477829

alexpott’s picture

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

heddn’s picture

If we plan to backport this to D7, can this also get backported to 8.2?

cilefen’s picture

It applies cleanly.

heddn’s picture

Status: Closed (fixed) » Reviewed & tested by the community

Moving to RTBC to see if we can get this into 8.2.x. Is that the right status?

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 163: 728702-163.patch, failed testing.

heddn’s picture

Version: 8.3.x-dev » 8.2.x-dev
Status: Needs work » Needs review
heddn’s picture

Status: Needs review » Reviewed & tested by the community

Tests pass on 8.2. Moving to RTBC.

alexpott’s picture

Version: 8.2.x-dev » 8.3.x-dev
Status: Reviewed & tested by the community » Fixed

@heddn in #161 I wrote that this change was making additions only suitable for a new minor version. These changes are:

  1. +++ b/core/core.services.yml
    @@ -1197,6 +1197,11 @@ services:
    +  exception.needs_installer:
    

    New service

  2. +++ b/core/lib/Drupal/Core/Database/Connection.php
    @@ -1445,6 +1445,26 @@ public function quote($string, $parameter_type = \PDO::PARAM_STR) {
    +  protected static function getSQLState(\Exception $e) {
    

    New method on a class that is subclassed for alternative DB drivers.

  3. +++ b/core/lib/Drupal/Core/Database/DatabaseAccessDeniedException.php
    @@ -0,0 +1,8 @@
    +class DatabaseAccessDeniedException extends \RuntimeException implements DatabaseException {}
    

    New exception type.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

esteinborn’s picture

FYI, Neither patch #152 nor #163 applies cleanly to 8.7.1.

cilefen’s picture

It should not. We merged this years ago.

esteinborn’s picture

I guess I'm confused as to what this fixed then.

On a fresh install with a settings.php file with filled in DB credentials that point to an empty database, I get an error on index.php, no redirect to /core/install.php. then when I visit /core/install.php it tells me that Drupal is already installed (It is not, it's a completely empty database).

esteinborn’s picture

Please ignore my last post. I had and issue with MySQL 8.

joseph.olstad’s picture