Incorrect "Drupal already installed" if any database settings are wrong or unsatisfactory

This is also occurring on MySQL 5.6 setups. Basically if you have a preconfigured settings file from a webhost (Acquia, Pantheon, etc) that is set to MySQL 5.6, instead of getting an error about the wrong version, you get this cryptic error.

Looking at lines ~518 in install.core.inc, the database_verified is coming up as False, and I think an error probably was thrown somewhere else, but its not visible yet so you just get the AlreadyInstalledException on Line ~527.

There might be some concern/disagreement about why this is marked critical, and framework maintainers please feel free to disagree.
Mainly, I think it falls under 'Render a site unusable and have no workaround.' Why?
* The user may not know what version of Database they are running or they don't know its the wrong one. While the latter can be addressed by reading documentation, the first one might be impossible to find out.
* This error is cryptic, so unless you're well versed in Drupal, you will never know its because you're running an out of date version of a database. This error is caused by other issues as well.
* The work around would require scouring support queues where people talk about 'drupal already is installed', leading to a very poor initial user experience to Drupal.
* While probably rare, users could inadvertently wipe their database to get the error to disappear. Those upgrading would result in data loss.

Because we're talking about the very first touchpoint in Drupal and users being misled from the start, I think this qualifies as a Critical bug.

I agree with the critical priority here.

Chances are this affects D8 as well; we just haven't noticed because most hosts meet the D8 DB requirements by default.

Depending on the specific changes in the patch, we might backport this as far as 8.8.x if it's fixed before the final 8.8 bugfix release May the 4th ✨⚔️, or 8.9.x if it's fixed after.

See also #2840218: Installer sometimes gives "Drupal already installed" message even if install failed.

Thanks @cilefen.

1. Since this issue can happen for a lot of reasons, the simplest fix might be to improve the message so that it doesn't lie about the installation status and points the user at a resource to debug it. Something like:

   Drupal already installed, or installation is incomplete

   • Same bullets as before
   • The Drupal installation may have failed due to unmet requirements or other issues. Review the handbook page on installation errors.

   Not totally great, but it would at least give the user a clue.
    

2. The more complicated but also more complete solution would be to actually surface the mismatch or error in the installation. Not sure if that is possible.

We might want to mark this as a duplicate of #2840218: Installer sometimes gives "Drupal already installed" message even if install failed and merge the priorities and discussion here onto that issue. Anyone up to update the IS of the other issue for that?

First shot at fixing this. It addresses both minimum versions not being met as well as throwing errors whenever the db system throws an error.

Previously, errors would just return FALSE. Since I didn't want to copy/paste the method, it returns TRUE/FALSE/Errors. Certainly open to adjustments if people don't like that approach.

+++ b/core/includes/install.core.inc
@@ -2176,10 +2193,10 @@ function install_check_requirements($install_state) {
           'description' => t('@drupal requires read permissions to %file at all times. The <a href=":handbook_url">webhosting issues</a> documentation section offers help on this and other topics.', [
-              '@drupal' => drupal_install_profile_distribution_name(),
-              '%file' => $file,
-              ':handbook_url' => 'https://www.drupal.org/server-permissions',
-            ]),
+            '@drupal' => drupal_install_profile_distribution_name(),
+            '%file' => $file,
+            ':handbook_url' => 'https://www.drupal.org/server-permissions',
+          ]),

This looks like an out of scope change?

Bah, yes I thought I caught all of those editor changes it tried to impose. Removed those lines from the patch

I like the fix in the patch. Great work.

1. +++ b/core/includes/install.core.inc
   @@ -388,7 +389,16 @@ function install_begin_request($class_loader, &$install_state) {
   +  $install_state['database_verified'] = FALSE;
   +  if ($database_status === TRUE) {
   +    $install_state['database_verified'] = $database_status;
   +  }
   

   Can we rewrite this to: $install_state['database_verified'] = ($database_status === TRUE) ? TRUE : FALSE;.

2. +++ b/core/lib/Drupal/Core/Installer/Exception/DatabaseRequirementsException.php
   @@ -0,0 +1,32 @@
   +    foreach ($database_error as $key => $error) {
   +      $message .= $this->t('<h3>Error %number</h3>', ['%number' => ($key+1)]);
   +      $message .= $database_error[$key];
   +    }
   

   In this way we will get the correct message from the exception, only how will the message be outlined/presented on the page that the user will see.

3. Is there any way we can write a test for this issue, other then manual testing the fix.

Thanks for the review! Not sure if you had any suggestion in comment #2? But yes the goal was to display the error.

I actually took a fresh look at comment #1 and there is a much simpler way... lol:

  $install_state['database_verified'] = $database_status === TRUE;

I didn't notice that $install_state['database_verified'] = FALSE already was set at the top.

As for #3, not sure how you test against other versions of MySQL? It seems like the installer in general could use an overhaul, but not going to try to do it here right before RC.

+++ b/core/lib/Drupal/Core/Installer/Exception/DatabaseRequirementsException.php
@@ -0,0 +1,32 @@
+    $title = $this->t('Unable to install on database');
+    $message = '';
+    // It is possible more than one error is found, loop through all of them.
+    foreach ($database_error as $key => $error) {
+      $message .= $this->t('<h3>Error %number</h3>', ['%number' => ($key+1)]);
+      $message .= $database_error[$key];

this kind of concatenation of translatable strings looks wrong
Instead it could use FormattableMarkup

+++ b/core/includes/install.core.inc
@@ -1156,15 +1167,18 @@ function install_verify_completed_task() {
- * @return bool
- *   TRUE if there are no database errors.
+ * @return bool|array
+ *   TRUE if there are no database errors. Otherwise an array of errors.

Is there a way to prevent BC break here?

The only way we can prevent a BC break is to create a new function. The current function is only used once by core and that is in the function install_begin_request(). We I do a search: http://grep.xnddx.ru/search?text=install_verify_database_settings&filename=, I find on other usage of the function, including in Drush I do not have problems with this very minor API change.

Nitpick:

+++ b/core/includes/install.core.inc
@@ -1156,15 +1167,18 @@ function install_verify_completed_task() {
+ * @return bool|array

More correct would be: @return true|array

Regarding the BC break, I'm not sure it actually does break BC... like Daffie said, nothing is (or really should) be using it. IMHO it should be a private method, but yay old time drupal code we're working with here.

I took in the other suggestions, and posted a patch.

Patch looks good, almost there.

1. +++ b/core/lib/Drupal/Core/Installer/Exception/DatabaseRequirementsException.php
   @@ -0,0 +1,33 @@
   +   * Constructs a new "already installed" exception.
   

   Can we change this to: 'Constructs a new "database requirements" exception.'

2. +++ b/core/includes/install.core.inc
   @@ -1156,15 +1167,18 @@ function install_verify_completed_task() {
   - * @return bool
   - *   TRUE if there are no database errors.
   + * @return true|array
   + *   TRUE if there are no database errors. Otherwise an array of errors.
     */
    function install_verify_database_settings($site_path) {
   

   @andypost is right that this change is a BC break. It is not a big one and it the function is only called by install_begin_request(). Lets fix this by making install_verify_database_settings() @internal.

3. +++ b/core/lib/Drupal/Core/Installer/Exception/DatabaseRequirementsException.php
   @@ -0,0 +1,33 @@
   +    $title = $this->t('Unable to install on database');
   

   Can we change the title to, something like: "Database requirements errors"

@japerry: Could you be so kind to add an interdiff.txt the next time. It makes reviewing you patch a lot easier.

I have manually tested the patch and it all works as it should.

Thanks @daffie, @andypost, and @japerry.

We provide BC and deprecations even for internal functionality; see: https://www.drupal.org/core/deprecation#internal

We need a test here also. Thanks!

@xjm: How do you want to test this? For the other Installer exceptions (InstallerException, AlreadyInstalledException and NoProfilesException) are also no tests in core.

Talked to @catch on Slack. He said that we should add testing in a followup. Created #3135043: Add tests for the different installer exceptions.

Also talked with @catch on Slack about changing the function install_verify_database_settings(). His suggestion was to create a new function called _install_verify_database_settings() (starts with an underscore) and mark that as @internal. Deprecated the old function.

Had a look at existing test coverage with @xjm. It looks like InstallerExistingDatabaseSettingsTest is enough of a basis to build one off here to me so we could probably do the test coverage here rather than a follow-up - we've got more installer test coverage than I remembered this morning.

Agreed. If it turns out not to be feasible, we can discuss then, but we should at least give it a shot. We used to break the installer alarmingly regularly before we added test coverage for it. The only tricky part might be mocking the database for the error condition, but we do test other installer error conditions. (grep for extends InstallerTestBase.)Thanks!

Some other thoughts:

1. I asked up in #9 to mark this as a duplicate of #2840218: Installer sometimes gives "Drupal already installed" message even if install failed and try to solve the whole problem but it looks like my feedback there was skipped. We might be okay handling just the error for this one condition and then doing a general solution as a followup, but since we already are introducing internal API changes, we don't want to introduce more of those. The other issue leads me to suspect the entire error handling flow of the installer is buggy and needs work -- not just this one condition.

2. +++ b/core/includes/install.core.inc
   @@ -388,7 +389,13 @@ function install_begin_request($class_loader, &$install_state) {
   +  // Check to see if the database is setup. If so, return TRUE. If the database
   

   Nit: "set up" should be two words. (When it's one word, it's a noun; two is a verb.)

3. +++ b/core/includes/install.core.inc
   @@ -526,6 +533,10 @@ function install_begin_request($class_loader, &$install_state) {
   +      throw new DatabaseRequirementsException($container->get('string_translation'), $database_status);
   
   +++ b/core/lib/Drupal/Core/Installer/Exception/DatabaseRequirementsException.php
   @@ -0,0 +1,33 @@
   +  public function __construct(TranslationInterface $string_translation, $database_error) {
   +    $this->stringTranslation = $string_translation;
   

   Why are we injecting the translation service rather than just using StringTranslationTrait? Is there some reason the installer shouldn't use the trait?

4. +++ b/core/lib/Drupal/Core/Installer/Exception/DatabaseRequirementsException.php
   @@ -0,0 +1,33 @@
   + * Exception thrown if Drupal database requirements aren't met.
   

   Technically under our standard this should start with a verb. "Defines an exception thrown..." would work.

5. +++ b/core/lib/Drupal/Core/Installer/Exception/DatabaseRequirementsException.php
   @@ -0,0 +1,33 @@
   +      $error = new FormattableMarkup("<h3>Error #:error_number</h3><div>@error</div>", [':error_number' => ($key+1), '@error' => $database_error[$key]]);
   

   Why are we not translating this (the word "Error" here and the numbers), if we're going to the lengths to have the translation service available?

   Is the error numbering like this something we do elsewhere?

We should be sure to test both the interactive and non-interactive installer with this, and do a UX review of the error. Let's get a screenshot for that.

For this to be testable we need to have the method Drupal\Core\Database\Connection->clientVersion() return a set value like: "10.2.31-MariaDB-1:10.2.31+maria~bionic-log". When somebody has any idea how to do this in a functional installer test, then please explain it.

Adding screen shot of the new error message:

1. I think there is another way to achieve this. Without altering BC. In install_check_requirements() we can call install_database_errors if Database::getConnectionInfo() is an array. That way we can put the errors in the proper place. Also have a look at what \Drupal\Core\Installer\Form\SiteSettingsForm::getDatabaseErrors() with respect to formatting the output here.

   Furthermore - if we do this maybe at some point we can stop running the database verification tasks on every install request. Atm on a standard install on mysql we create the drupal_install_test table 13 times on my computer! Once for each request to the installer once db settings have been entered. But that's stuff for a follow-up.

2. +++ b/core/lib/Drupal/Core/Installer/Exception/DatabaseRequirementsException.php
   @@ -0,0 +1,33 @@
   +   * Constructs a new "already installed" exception.
   

   It's not an already installed exception.

While reviewing this issue i noticed and filed #3135310: Remove completely unused 'database_ready' install state logic - noted here because a 'database_ready' setting looks enticing but ends up just being confusing.

So I went down the suggestions Alex was suggesting. The main issue is that we need to get around the 'Drupal is already installed' error that occurs right at the beginning of the install process, this is well before verifying requirements.

Luckily the task install_verify_database_ready(); actually does what we need it to, So by replacing the line and making it so it can catch invalid database settings (because the database might not be setup yet), this works pretty well.

This is a pretty drastic re-write to the old patch, so I'm not including an interdiff. Its also vastly more simple.

Note, in the requirements error loop, it could potentially throw multiple DB errors with the same title "Unable to install database." I think this is fine since the errors themselves have no titles, and I haven't been able to throw multiple DB errors at once. (IE: bad username/password error isn't also going to throw the version error because it cannot login yet to see that its the wrong version).

While I don't know how we're going to test this, I think it should be easier to review.

Here's a fix for the test and a test-only patch that proves with incorrect DB settings we end up on the requirements page and not at the already installed exception.

Small move of stuff to not do unnecessary work.

This is looking pretty good. Tested locally with mysql 5.6 and i'm getting the error as expected. Then tried to install with the MySQL56 driver and that worked as well.

When I go back to install again, I get prompted with 'Site is already installed' which is also what was expected.

Thanks for the tests. This fix also seems like a sounder direction to me, since it addresses making the "Drupal already installed" actually most likely true by checking for data in the database.

1. +++ b/core/includes/install.core.inc
   @@ -2221,6 +2223,24 @@ function install_check_requirements($install_state) {
   +    // Lastly, check to see if Database Requirements are met
   

   Other nit: Missing period. Edit: "Database Requirements" is also not a proper noun and therefore probably does not need to be capitalized.

2. +++ b/core/tests/Drupal/FunctionalTests/Installer/InstallerExistingDatabaseSettingsTest.php
   @@ -61,6 +61,27 @@ protected function setUpSettings() {
   +    $this->assertSession()->pageTextContains('UNABLE TO INSTALL DATABASE');
   

   Nit, but I THINK THE ALL CAPS IS STYLING AND NOT THE ACTUAL TEXT. (pageTextContains() is case-insensitive AFAIK).

Cleaned up text from @xjm's comments and copied the actual text it should show.

+++ b/core/includes/install.core.inc
@@ -2221,6 +2223,24 @@ function install_check_requirements($install_state) {
+    // If the database requirements were not met, throw an error.

We're not throwing an error here. I think it's important to mention that this check is for when settings.php already contains database settings prior to visiting the Drupal installer.

Addressing #37.

1. +++ b/core/includes/install.core.inc
   @@ -520,18 +520,12 @@ function install_begin_request($class_loader, &$install_state) {
   +    install_verify_database_ready();
   

   Could we add a comment with that a AlreadyInstalledException can be thrown.

2. +++ b/core/includes/install.core.inc
   @@ -2221,6 +2223,25 @@ function install_check_requirements($install_state) {
   +      if (count($errors)) {
   

   This if-statement is not necessary. install_database_errors() always returns an array (when it is an empty array then there are no errors).

3. The added test does not test the given bug from the IS.

Adding screen shot of the new error message when following the instructions from the IS:

Patch attached fixes points 1 and 2. Nice catches. Also improved the docs to install_verify_database_ready() since this exception is quite important.

@daffie the test added does actually test the cause of the bug. If you run the test is fails on

the dreaded "Drupal already installed"

and now it does not. It correctly has a requirements problem. We don't have an incorrect database with an old version to connect to. This test is probably the best we can do. And it tests the code we've changed as shown by the test-only patch.

All my points are addressed.
The added test requirements problems only not the one from the IS.
I have manual tested that the patch fixes the probem given in the IS. The result is the same as my screenshot from comment #39. Removing the tag "Needs manual testing".
All code changes look good to me.
For me it is RTBC.

+++ b/core/includes/install.core.inc
@@ -2221,6 +2229,22 @@ function install_check_requirements($install_state) {
+      foreach (install_database_errors($database, $settings_file) as $key => $error) {
+        $requirements["database_error_$key"] = [
+          'title' => t("Unable to install database."),
+          'description' => $error,

Only a wording nitpick but is 'Unable to install database' the right title here? We're not really installing the database, we're installing Drupal with the database.

Should it be something like 'Unable to install with current database settings'?

@catch good point. I think we should use the same language as we do on \Drupal\Core\Installer\Form\SiteSettingsForm::getDatabaseErrors() as essentially we're validating the same thing.

Here's a screenshot:

+++ b/core/includes/install.core.inc
@@ -2221,6 +2229,34 @@ function install_check_requirements($install_state) {
+      if (count($errors)) {
+        $error_message = [
+          '#type' => 'inline_template',
+          '#template' => '{% trans %}Resolve all issues below to continue the installation. For help configuring your database server, see the <a href="https://www.drupal.org/docs/8/install">installation handbook</a>, or contact your hosting provider.{% endtrans%}{{ errors }}',
+          '#context' => [
+            'errors' => [
+              '#theme' => 'item_list',
+              '#items' => $errors,
+            ],
+          ],
+        ];
+        $requirements["database_install_errors"] = [
+          'title' => t("Database settings"),
+          'description' => \Drupal::service('renderer')->renderPlain($error_message),
+          'severity' => REQUIREMENT_ERROR,
+        ];
+      }

This approach also fixes a problem with multiple errors in #40 where you'd get multiple requirements fails all with the same title.

Oh good! Yah I was trying to figure out how to make that inline template work, but didn't quite know where to fit it in. This format looks much better. I'll let one of you all RTBC it, but the manual testing for both outdated mysql and wrong user/passwords worked for me.
+1

I think we can create automated testing for having the database settings in settings.php connection to a database with a deprecated version. Added a patch that should do that, only I am getting a very weird and unrelated error:

Error
The website encountered an unexpected error. Please try again later.
Drupal\Component\Plugin\Exception\PluginNotFoundException: The "search_extra_type_search" plugin does not exist. Valid plugin IDs for Drupal\search\SearchPluginManager are: node_search, user_search in Drupal\Core\Plugin\DefaultPluginManager->doGetDefinition() (line 53 of core/lib/Drupal/Component/Plugin/Discovery/DiscoveryTrait.php).

Drupal\Core\Plugin\DefaultPluginManager->doGetDefinition(Array, 'search_extra_type_search', 1) (Line: 25)
Drupal\Core\Plugin\DefaultPluginManager->getDefinition('search_extra_type_search') (Line: 16)
Drupal\Core\Plugin\Factory\ContainerFactory->createInstance('search_extra_type_search', Array) (Line: 83)
Drupal\Component\Plugin\PluginManagerBase->createInstance('search_extra_type_search', Array) (Line: 62)
Drupal\Core\Plugin\DefaultSingleLazyPluginCollection->initializePlugin('search_extra_type_search') (Line: 51)
Drupal\search\Plugin\SearchPluginCollection->initializePlugin('search_extra_type_search') (Line: 80)
Drupal\Component\Plugin\LazyPluginCollection->get('search_extra_type_search') (Line: 44)
Drupal\search\Plugin\SearchPluginCollection->get('search_extra_type_search') (Line: 83)
Drupal\Core\Plugin\DefaultSingleLazyPluginCollection->setConfiguration(Array) (Line: 99)
Drupal\Core\Plugin\DefaultSingleLazyPluginCollection->addInstanceId('search_extra_type_search', Array) (Line: 55)
Drupal\Core\Plugin\DefaultSingleLazyPluginCollection->__construct(Object, 'search_extra_type_search', Array) (Line: 33)
Drupal\search\Plugin\SearchPluginCollection->__construct(Object, 'search_extra_type_search', Array, 'dummy_search_type') (Line: 129)
Drupal\search\Entity\SearchPage->getPluginCollection() (Line: 118)
Drupal\search\Entity\SearchPage->getPlugin() (Line: 318)
Drupal\node\NodeViewsData->getViewsData() (Line: 178)
views_views_data()
call_user_func_array('views_views_data', Array) (Line: 392)
Drupal\Core\Extension\ModuleHandler->invoke('views', 'views_data') (Line: 237)
Drupal\views\ViewsData->getData() (Line: 154)
Drupal\views\ViewsData->get('block_content') (Line: 91)
Drupal\views\Plugin\Derivative\ViewsEntityRow->getDerivativeDefinitions(Array) (Line: 101)
Drupal\Component\Plugin\Discovery\DerivativeDiscoveryDecorator->getDerivatives(Array) (Line: 87)
Drupal\Component\Plugin\Discovery\DerivativeDiscoveryDecorator->getDefinitions() (Line: 284)
Drupal\Core\Plugin\DefaultPluginManager->findDefinitions() (Line: 175)
Drupal\Core\Plugin\DefaultPluginManager->getDefinitions() (Line: 146)
views_theme(Array, 'module', 'views', 'core/modules/views') (Line: 454)
Drupal\Core\Theme\Registry->processExtension(Array, 'views', 'module', 'views', 'core/modules/views') (Line: 341)
Drupal\Core\Theme\Registry->build() (Line: 240)
Drupal\Core\Theme\Registry->get() (Line: 88)
Drupal\Core\Utility\ThemeRegistry->initializeRegistry() (Line: 69)
Drupal\Core\Utility\ThemeRegistry->__construct('theme_registry:runtime:seven', Object, Object, Array, 1) (Line: 260)
Drupal\Core\Theme\Registry->getRuntime() (Line: 142)
Drupal\Core\Theme\ThemeManager->render('html', Array) (Line: 431)
Drupal\Core\Render\Renderer->doRender(Array, 1) (Line: 200)
Drupal\Core\Render\Renderer->render(Array, 1) (Line: 144)
Drupal\Core\Render\Renderer->Drupal\Core\Render\{closure}() (Line: 573)
Drupal\Core\Render\Renderer->executeInRenderContext(Object, Object) (Line: 145)
Drupal\Core\Render\Renderer->renderRoot(Array) (Line: 66)
Drupal\Core\Render\BareHtmlPageRenderer->renderBarePage(Array, Object, 'install_page', Array) (Line: 76)
Drupal\Core\ProxyClass\Render\BareHtmlPageRenderer->renderBarePage(Array, Object, 'install_page', Array) (Line: 1059)
install_display_output(Array, Array) (Line: 159)
install_drupal(Object) (Line: 44)

The test works (does not result in the above error) for my locally when I remove from the class Drupal\driver_test\Driver\Database\DrivertestMysqlDeprecatedVersion\Connection the following methods: isMariaDb(), version(), clientVersion() and getServerVersion(). The test Drupal\FunctionalTests\Installer\InstallerExistingDatabaseSettingsTest will end with an "Drupal already installed" exception. Not with the desired "requirement problem".

Applied the patch #43 with a lower version of MariaDB than 10.3, Install page is opening and showing the appropriate error message in the Verify requirement page.

Manual Test works.

Varify requirements for MariaDB working fine with the patch #43.

for what is worth the test added by #43 (and built upon in #46) fails random locally for me. Need to try and work out why.

Ok worked this out. The test can't re-use the existing test because that gets the install_task state set to done which breaks the idea of testing a fresh install. Using a new test allows us to test this predicatably.

Ah the test relies on database drivers as modules. Let's makes this 9.0.x for now to make testing easier. We can commit a version with no test to 8.9.x / 8.8.x if we want.

We have the possibility to commit this to 8.9, because #3120096: Support contrib database driver directories in a fixed location in a module has also been committed to 8.9. For 8.8 is this patch not possible.
The automatic testing is now fixed and it looks good.
All code changes are in order.
For me it is RTBC.

@daffie good point about 8.9.x. Let's target that version.

Rerolled since #3135629: Minimum MySQL version requirement is not confirmed when upgrading existing sites from Drupal 8 to Drupal 9 landed and that has the same db driver.

Back to RTBC.

NW for #2, also some nits/questions.

1. +++ b/core/includes/install.core.inc
   @@ -520,18 +520,14 @@ function install_begin_request($class_loader, &$install_state) {
   +  if (!$install_state['database_verified']) {
   +    // Do not install over an existing installation. The call to
   +    // install_verify_database_ready() will throw an AlreadyInstalledException
   +    // if this is the case.
   +    install_verify_database_ready();
      }
   

   This is a good change, +1

2. +++ b/core/includes/install.core.inc
   @@ -2221,6 +2229,34 @@ function install_check_requirements($install_state) {
   +    if ($database = Database::getConnectionInfo()) {
   +      $request = Request::createFromGlobals();
   +      $site_path = empty($install_state['site_path']) ? DrupalKernel::findSitePath($request, FALSE) : $install_state['site_path'];
   +      $database = $database['default'];
   +      $settings_file = './' . $site_path . '/settings.php';
   +
   +      $errors = install_database_errors($database, $settings_file);
   +      if (count($errors)) {
   

   Why not use install_verify_database_settings() here?
   If there is a good reason not to, let's document that here.

3. +++ b/core/includes/install.core.inc
   @@ -2221,6 +2229,34 @@ function install_check_requirements($install_state) {
   +          '#type' => 'inline_template',
   +          '#template' => '{% trans %}Resolve all issues below to continue the installation. For help configuring your database server, see the <a href="https://www.drupal.org/docs/8/install">installation handbook</a>, or contact your hosting provider.{% endtrans%}{{ errors }}',
   

   This would be the first usage of {% trans %} in PHP code. Other usages of inline_template in requirements move the string to a variable wrapped in t(), and the #template is something like
   {{ description }}{{ errors }}

4. +++ b/core/includes/install.core.inc
   @@ -2221,6 +2229,34 @@ function install_check_requirements($install_state) {
   +          'title' => t("Database settings"),
   

   Nit: only use double quotes when needed

I see install_verify_database_settings() mentioned a bunch in previous comments, but none of it clear.
EDIT: Oh, because it returns a bool and not the actual errors.
Which would have been fixed by #23. Why wasn't that pursued?

Why wasn't that pursued?

Because that would result in a BC break. A small one, but still a BC break.

How would introducing a new function be a BC break?

Thanks for the review @tim.plunkett

Re #60
2. As per #61 because that does not return the errors. Re the BC break - changing this to return errors would mean that we'd now return a truthy value for something that used to FALSE.
3. This is not the first usage in core. It's a direct copy from \Drupal\Core\Installer\Form\SiteSettingsForm::getDatabaseErrors() and it is used \Drupal\node\Controller\NodeController::revisionOverview() too.
4. Fixed.

Note this test only works on 9.0.x and up - we broke 8.9.x with #3135629: Minimum MySQL version requirement is not confirmed when upgrading existing sites from Drupal 8 to Drupal 9 because the db driver relies on #3124354: Remove unnecessary classes from the database drivers - we could choose to add the delete class or 8.9.x and it should work. I'll test in the other issue.

+++ b/core/includes/install.core.inc
@@ -2221,6 +2229,34 @@ function install_check_requirements($install_state) {
+          '#template' => '{% trans %}Resolve all issues below to continue the installation. For help configuring your database server, see the <a href="https://www.drupal.org/docs/8/install">installation handbook</a>, or contact your hosting provider.{% endtrans%}{{ errors }}',

Does there needs to be a space between "endtrans" and the character "%"?

#64

2) My question was about adding a new function, not changing the existing one. Idk how that's not clear. But whatever, it's installer, we can keep spreading special magic around.

3) Bad grep on my part, fair enough.

#65
A space should be added, but it's functional without. akin to writing if( instead of if (.

Fixing the space. I meant to file a follow-up to do this but I think given the space issue and the fact this is the installer and SiteSettingsForm is marked @internal adding a public static method should be okay. We could add a whole template file to do this too but I think this is okay. It;s the installer and there's no case to theme this differently.

Changes in #67 look good. Thanks.

The patch will be different for 8.9.x, but we should still also backport it. Thanks!

Can we at least file a follow-up for #23? Or do it here? If we add the @internal underscored version, we'd be saving the inline code duplication added in the patch (even the deprecated function could call the new one).

I think we do that in a follow-up. I'd like to refactor install_verify_database_settings(), install_verify_database_ready(), install_database_errors() into an Install helper class. Adding new methods piecemeal is our we're ending up with a disjointed API where we want the same method to do different things. We'll probably be able to make SiteSettingsForm use this too for added consistency and less code duplication.

I've created the follow-up: #3136855: Refactor install_* database helpers into an install helper class

Well it's not really adding a new method as such, it's adding a replacement method and deprecating the old one, but that follow-up seems fine.

1. +++ b/core/includes/install.core.inc
   @@ -520,18 +521,14 @@ function install_begin_request($class_loader, &$install_state) {
   -    // Do not install over a configured settings.php.
   -    if (Database::getConnectionInfo()) {
   -      throw new AlreadyInstalledException($container->get('string_translation'));
   -    }
   ...
   +    // Do not install over an existing installation. The call to
   +    // install_verify_database_ready() will throw an AlreadyInstalledException
   +    // if this is the case.
   +    install_verify_database_ready();
   

   So here install_begin_request() is now relying on install_verify_database_ready() to throw the exception, rather than throwing it directly. That's OK because install_verify_database_ready() did already throw the exception anyway.

   This is the first direct call to install_verify_database_ready(). The other is indirectly as an install task (which are hook-like). It's the task between install_settings_form and install_base_system. install_run_tasks() is what kicks all that off, in a try on line 117. Whereas this is being called from install_begin_request(), which is done just before that in a try on line 114.

   Do we need to verify the database is ready twice like that?

2. +++ b/core/includes/install.core.inc
   @@ -1173,16 +1170,28 @@ function install_verify_database_settings($site_path) {
   +    catch (\Exception $e) {
   

   Do we really want to catch all \Exception? Can we make this more targeted in terms of what we catch?

3. +++ b/core/includes/install.core.inc
   @@ -2221,6 +2230,25 @@ function install_check_requirements($install_state) {
   +      $errors = install_database_errors($database, $settings_file);
   

   This is the second call to install_database_errors() in the installer. The first one is in install_verify_database_settings(), with its boolean return values, called from install_begin_request() much earlier.

   I guess it would be in scope for #3136855: Refactor install_* database helpers into an install helper class to address that?

   I understand the need to call the errors directly because of the return value public API, but it does mean we're checking for database errors twice. What's different about the much earlier check?

4. +++ b/core/includes/install.core.inc
   @@ -2221,6 +2230,25 @@ function install_check_requirements($install_state) {
   +        $requirements["database_install_errors"] = [
   

   Why double quotes on the array key? It's not wrong exactly but it's not standard.

5. +++ b/core/includes/install.core.inc
   @@ -2221,6 +2230,25 @@ function install_check_requirements($install_state) {
   +          'description' => \Drupal::service('renderer')->renderPlain($error_message),
   

   😬 at the early/manual rendering here. I guess this is necessary because it's the installer, or automatic rendering is not happening somehow? But we're not renderPlain()ing anywhere else in the installer.

   The only other place install.core.inc uses the renderer service is in install_display_output() (which does the actual rendering generally) and there it's doing renderBarePage().

   Can we document why we're rendering early here? And an inline comment at least.

6. +++ b/core/lib/Drupal/Core/Installer/Form/SiteSettingsForm.php
   @@ -194,16 +194,7 @@ protected function getDatabaseErrors(array $database, $settings_file) {
   -        '#template' => '{% trans %}Resolve all issues below to continue the installation. For help configuring your database server, see the <a href="https://www.drupal.org/docs/8/install">installation handbook</a>, or contact your hosting provider.{% endtrans%}{{ errors }}',
   

   The earlier mention about {% trans %} in an inline template in PHP code is out of scope, since this is just being factored into a helper method.

7. +++ b/core/lib/Drupal/Core/Installer/Form/SiteSettingsForm.php
   @@ -214,6 +205,28 @@ protected function getDatabaseErrors(array $database, $settings_file) {
   +   * @param \Drupal\Core\StringTranslation\TranslatableMarkup[] $errors
   

   I love the specific typing here.

8. +++ b/core/lib/Drupal/Core/Installer/Form/SiteSettingsForm.php
   @@ -214,6 +205,28 @@ protected function getDatabaseErrors(array $database, $settings_file) {
   +   * @return array
   

   This is actually true for once since the render array is only one level deep, and includes children that are string and array both. We could say mixed[].

9. +++ b/core/lib/Drupal/Core/Installer/Form/SiteSettingsForm.php
   @@ -214,6 +205,28 @@ protected function getDatabaseErrors(array $database, $settings_file) {
   +  public static function getDatabaseErrorsTemplate(array $errors) {
   

   It needs to be public because it's called from install.core.inc.

10. +++ b/core/tests/Drupal/FunctionalTests/Installer/InstallerExistingBrokenDatabaseSettingsTest.php
    @@ -0,0 +1,73 @@
    + * Tests the installer with an existing settings file with database connection
    + * info.
    

    Very-nit: this is supposed to be one line. Maybe...

    Tests installation with existing database connection settings.

NW for several of the points above; others are questions that may or may not be in scope but I would like to understand before committing. Edit: 6, 7, and 9 are non-actionable observations.

Thanks for the review.

1. Probably not. But (a) there's no harm (b) the task list is sometimes manipulated by install profiles to inject their own tasks. It's not that simple to alter the list without ending relying on the structure core has so whilst I don't think this list is API I think it is worth handling the removal of install_verify_database_ready() in a follow-up. I think #3136855: Refactor install_* database helpers into an install helper class is a good fit for that task because removing it from the list might help shape the API.

2. Some of the closest analogous situations we have in core catch \Exception. It's tricky because it can either be one of our exceptions or something from \PDO. Here's an example... \Drupal\Core\Cache\DatabaseBackend::setMultiple() - how it'll fall back to trying create the table if a db query results in an exception.

3. Yes making the number of calls to install_database_errors() part of the follow-up is a good idea. If you install standard via the browser then we do the db tests on every request. See #27.

4. Fixed

5. We already do this in the installer in \Drupal\Core\Installer\Form\SiteSettingsForm::getDatabaseErrors() however testing reveals that it is not needed here. So I've removed the call to renderPlain(). Nice catch.

10. Fixed

The 8.9.x patch is exactly the same apart from the assertion message $this->assertSession()->pageTextContains('The database server version 5.5.2 is less than the minimum required version'); as 8.9 and 9.0 have different requirements.

I've add #74.1 and #74.3 to #3135043: Add tests for the different installer exceptions

Point #74.8 is still open.

Whoops missed that. Not sure mixed[] is that much better than array. But also not sure it matters.

All points of @xjm are addressed.
Back to RTBC.

I debated whether this issue would need a CR for the changed behavior, or mention in the release notes, but mostly we're just making the installer report more meaningful errors. So it doesn't need a CR and it will just be in the list of fixed criticals for the release.

Committed/pushed to 9.1.x, cherry-picked to 9.0.x, and committed the 8.9.x patch. Don't think we need to backport this to 8.8.x.

Hi all, not sure if anybody is still subscribed to this issue. But it appears that this commit led to #3163925: "Undefined index: value" when running tests with database errors. I'd be happy to help along but I am confused on whether or not the use of a renderer is allowed. Perhaps one of you could have a look and provide some guidance?

I am scratching my head as to how fix this Drupal Already Installed problem, as I can't find any direct instructions on how to solve this.

I have downloaded MAMP and then used composer create-project drupal/recommended-project drupal in my command line.

I then did Drupal installation through localhost/drupal/web
and that is when it came up with the Drupal 9.2.3 Drupal Already installed message at the end of the installation.

I have my problem outlined in more detail on my post here https://www.drupal.org/forum/support/installing-drupal/2021-09-02/drupal...

I need some help to solve this problem and learn why it is happening.