Prevent installation over an already installed database

Not verified the actual functionality, but here is a code review:

1. +++ b/core/includes/install.core.inc
   @@ -332,6 +332,15 @@ function install_begin_request(&$install_state) {
      $install_state['database_verified'] = install_verify_database_settings();
      $install_state['settings_verified'] = $install_state['config_verified'] && $install_state['database_verified'];
    
   ...
   +
   +  $install_state['settings_verified'] = $install_state['config_verified'] && $install_state['database_verified'];
   +
   

   This line is doubled?

2. +++ b/core/includes/install.core.inc
   @@ -365,6 +374,10 @@ function install_begin_request(&$install_state) {
   +
   +
   +
   +
   

   nit - whitespace

3. +++ b/core/includes/install.core.inc
   @@ -679,6 +692,11 @@ function install_tasks($install_state) {
   +      'run' => $install_state['database_ready'] ? INSTALL_TASK_SKIP : INSTALL_TASK_RUN_IF_NOT_COMPLETED,
   +      //'function' => install_database_ready(),
   +
   

   nit - commented out code

4. +++ b/core/includes/install.core.inc
   @@ -967,6 +985,7 @@ function install_base_system(&$install_state) {
      \Drupal::moduleHandler()->install(array('user'), FALSE);
   +  \Drupal::service('router.builder')->rebuild();
    
   

   Is this really necessary?

   Is this due to the moduleHandler setting the needsRebuild flag is not enough?

5. +++ b/core/includes/install.core.inc
   @@ -1007,6 +1026,7 @@ function install_verify_completed_task() {
    function install_verify_database_settings() {
   +
      if ($database = Database::getConnectionInfo()) {
   

   nit - whitespace change

+++ b/core/includes/install.core.inc
@@ -967,6 +985,7 @@ function install_base_system(&$install_state) {
   // Enable the user module so that sessions can be recorded during the
   // upcoming bootstrap step.
   \Drupal::moduleHandler()->install(array('user'), FALSE);
+  \Drupal::service('router.builder')->rebuild();

This should not be required anymore.

The issue summary is not very clear nor the issue title.

The purpose of this issue is to *prevent* installation over an existing installation, correct?

Reroll. No conflicts just merging.

Fixes from #9 except (4)

Rewrote the title/summary to clarify that we want to prevent installation into a populated database, rather than add support for it (see #11/12).

I verified that this is an issue and that #14 fixes the problem but needs a reroll.

Attached is an updated patch with the following changes:

1. +++ b/core/includes/install.core.inc
   @@ -172,6 +172,8 @@ function install_state_defaults() {
   +    // TRUE if database is empty & ready to install
   

   Comment should end in a full stop. I also changed the function name to install_verify_database_ready() to be more inline with the other verification steps (though I'm not set on any name in particular).

2. +++ b/core/includes/install.core.inc
   @@ -281,7 +283,7 @@ function install_begin_request(&$install_state) {
   -  if ($install_state['interactive'] && strpos($request->server->get('HTTP_USER_AGENT'), 'simpletest') !== FALSE && !drupal_valid_test_ua()) {
   

   This seems to have been an accidental change.

3. +++ b/core/includes/install.core.inc
   @@ -321,9 +323,7 @@ function install_begin_request(&$install_state) {
   +
   

   The extra line isn't needed.

4. +++ b/core/includes/install.core.inc
   @@ -980,6 +990,7 @@ function install_base_system(&$install_state) {
   +  \Drupal::service('router.builder')->rebuild();
   

   This has since been removed and is no longer necessary.

5. +++ b/core/includes/install.core.inc
   @@ -1032,6 +1043,22 @@ function install_verify_database_settings() {
    /**
   + * Check if database is ready (assuming no drupal previously installed)
   + * @param $install_state
   + */
   

   This could be worded a bit better and should end in a full stop.

   I don't believe that the $install_state argument is necessary.

it should be possible to test this in a pretty easy way? Pick one of the installer tests that we have, then go to core/install.php and assert that you get an already installed message.

Added a test to verify that Drupal can't be reinstalled immediately after installation or when settings.php is removed and the user attempts to reinstalled while an existing installation is available.

+++ b/core/includes/install.core.inc
@@ -1031,6 +1040,21 @@ function install_verify_database_settings() {
+ * Verified that the database is ready (no existing Drupal installation).

Extreme nitpick, should probably be 'verify' ?

I'd be fine with RTBC though and see what the committers think and maybe change this during commit ?

Actually let's see what Alex thinks re #20 :)

+++ b/core/includes/install.core.inc
@@ -1031,6 +1040,21 @@ function install_verify_database_settings() {
+  end($system_schema);
+  $table = key($system_schema);

I don't love end($foo); key($foo) as a pattern, but alternatives with array_slice()/array_keys() are worse.

Fixed Verified/Verify on commit.

Looks great otherwise, so committed/pushed to 8.0.x, thanks!