Installer checks wrong config storage to verify installation state

This might behave better.

One problem with this is essentially the concern that I had mentioned in #2161591-85: Change default active config from file storage to DB storage already: The new config storage attempts to automatically create the storage, no?

It's not clear to me why this error is triggered via Drush, but not in the regular interactive installer, nor in tests (which are using the non-interactive installer)...?

How can this error be reproduced?

@sun - I was getting this intermittently in the ui too. Not sure how to test though.

the listAll() method does NOT create the storage (by design). Here's the code:

  public function listAll($prefix = '') {
    try {
      return $this->connection->query('SELECT name FROM {' . $this->connection->escapeTable($this->table) . '} WHERE name LIKE :name', array(
        ':name' => $this->connection->escapeLike($prefix) . '%',
      ), $this->options)->fetchCol();
    }
    catch (\Exception $e) {
      return array();
    }
  }

I'm consistently when trying to reinstall as well. I just commented out the throw call in the OP and Drupal installed fine.

@pwolanin: Ah, sorry, I wrongly remembered that part.

It is still not clear to me why I'm not able to reproduce this issue locally, and why our existing interactive installer tests are not encountering it either.

So either something is wrong with our existing tests, or we're missing test coverage for the scenario here. — What is the scenario here?

Some of you referred to "reinstall", but how exactly? With or without existing settings.php? Does settings.php contain a db connection? $config_directories? Is the database empty?

Separate from that, the non-interactive installer can only reach this code path, if the config storage exists ahead of time already. Since #2161591 that no longer means the active config directory, but the {config} database table.

This means that not only $install_state['config_verified'] must be TRUE, but $install_state['database_verified'] must also be TRUE. Both must be TRUE already when the installer is initially invoked, because install_begin_request() is only entered once in the non-interactive installer.

It's not clear why $install_state['config_verified'] is TRUE to begin with?

Right by reinstall I mean I have a settings.php with $databases and $config_directories (that exist). Previously that prompted the database settings form, which I just had to re-submit and Drupal would install. Now I get the "Drupal already installed." I'll try to re-produce again later.

@moshe weitzman clarified in IRC:

i get this with reinstall via drush site-install. more specifically:

existing settings.php with DB connnection (DB is empty) and with config folders configured and existing but empty

That scenario is not covered by tests yet. We have an InstallerExistingSettingsTest, but it is a misnomer, since the preconfigured settings.php only contains database settings (but nothing else).

Attached patch renames the existing test and adds a new one to test a fully populated settings.php. It fails for me locally, and is expected to fail.

Incorporating #2.

Thanks for adding the test.

Oops.

This test looks basically good, but I think it loses test coverage for one path to installation? why don't we add this new one instead of replacing the existing one?

Sorry for the confusion, I have renames=copies enabled. Both tests are retained; the diff file headers clarify:

similarity index 89%
copy from core/modules/system/lib/Drupal/system/Tests/Installer/InstallerExistingSettingsTest.php
copy to core/modules/system/lib/Drupal/system/Tests/Installer/InstallerExistingDatabaseSettingsTest.php
...
--- a/core/modules/system/lib/Drupal/system/Tests/Installer/InstallerExistingSettingsTest.php
+++ b/core/modules/system/lib/Drupal/system/Tests/Installer/InstallerExistingSettingsTest.php

Here'a re-roll.

Yup.

Committed/pushed to 8.x, thanks!