[PP-1] Add detail to AlreadyInstalledException

Adding more details to exceptions is always a good idea!

1. +++ b/core/includes/install.core.inc
   @@ -1045,7 +1047,8 @@ function install_verify_completed_task() {
      if (isset($task)) {
        if ($task == 'done') {
   -      throw new AlreadyInstalledException(\Drupal::service('string_translation'));
   +      throw new AlreadyInstalledException(\Drupal::service('string_translation'),
   +        "I don't know why this is an exception.");
        }
   

   I think the point is that we should not verify tasks again, once we have completed the test. Maybe throw something like: 'Installation already finished'?

2. +++ b/core/lib/Drupal/Core/Installer/Exception/AlreadyInstalledException.php
   @@ -19,13 +19,18 @@ class AlreadyInstalledException extends InstallerException {
   +      $message .= $this->t("<li>$detail</li>\n");
   

   If we really want to translate those messages, we have to translate them directly in the beginning so that potx can scan them.

The patch looks good to me and it also helps with debugging random failures for Postgres/SQLite on DrupalCI.

Let's just use different exceptions here and change AlreadyInstalledException into something abstract.

Just re-uploading patch to enable PG-bot testing. Setting to NW again later!

Back to NW...

#4 seems like a reason for this to be major even.

Changed the AlreadyInstalledException to be a abstract class, added 4 implementations of that class and throw those instead.

Added a screenshot of what this looks like when Implemented.

+++ b/core/includes/install.core.inc
@@ -488,7 +491,8 @@ function install_begin_request($class_loader, &$install_state) {
+      throw new ActiveConfigurationExists($translator);

@@ -1069,7 +1073,8 @@ function install_verify_completed_task() {
+      throw new InstallationTasksCompleted($translator);

Don't we need the Exception suffix for these two as well?

@amateescu, you're right, fixed in the attached patch.

Ah, I should've caught this sooner:

+++ b/core/lib/Drupal/Core/Installer/Exception/AlreadyInstalledException.php
@@ -12,7 +12,13 @@
+  protected $specificMessage = '';

@@ -22,16 +28,20 @@ class AlreadyInstalledException extends InstallerException {
+      $message .= $this->t('<li>' . $this->specificMessage . '</li>');

Dynamic strings won't get picked up by the translatable strings extractor so we need to translate the message in the constructor of each exception class. See https://www.drupal.org/developing/api/8/localization ("Variables in user interface text").

1. +++ b/core/lib/Drupal/Core/Installer/Exception/AlreadyInstalledException.php
   @@ -15,24 +15,20 @@
   +   *  A specific message set from a class implementing this Exception.
   

   Missing a space at the beginning and maybe we should shorten the comment to just "A specific exception message.", mostly because subclasses are extending this class, they don't implement it.

2. +++ b/core/lib/Drupal/Core/Installer/Exception/AlreadyInstalledException.php
   @@ -15,24 +15,20 @@
   +      $message .= $this->t($specific_message);
   

   No need to pass $specific_message through $this->t() anymore, but we still need the wrapping <li>s :P

#14.1: Fixed.
#14.2: I had moved the wrapping <li>'s to the subclasses, but this makes more sense, fixed.

Thanks, this looks ready to me.

+++ b/core/lib/Drupal/Core/Installer/Exception/DatabaseAlreadyPopulatedException.php
@@ -0,0 +1,24 @@
+    parent::__construct($string_translation, $this->t('Database is already populated.'));

This will call $this->t() before the $string_translation is set thereby ignoring the passed in service and just reacing into \Drupal as a service locator.

The attached patch moves setting stringTranslation to the implementing exceptions.

#17: good point!

The more I look at this code the more I get weirded out by the use of translated string in exceptions. The installer is the only place this happens in core. And the exception statndards says not to do this. I think we should fix this here.

Adding interdiff

Forget to remove some, fixed.

@PieterJanPut we still need the translated message but we should NOT use exceptions to pass translated string around.

What we need to do is continue to introduce the new exceptions here. Add a new class to the installer classes called ConvertInstallerExceptionToMessage() which has the logic for converting an exception to a translated message. And then where the exception is caught use this new utility class to get the translated message for the particular exception.

[edit: corrected incorrect first sentence]

+++ b/core/lib/Drupal/Core/Installer/Exception/AlreadyInstalledException.php
@@ -12,26 +12,28 @@
+    $title = 'Drupal already installed.';
...
+    $message .= '<li>To start over, you must empty your existing database and copy <em>default.settings.php</em> over <em>settings.php</em>.</li>
+<li>To upgrade an existing installation, proceed to the <a href="' . $GLOBALS['base_path'] . 'update.php">update script</a>.</li>
+<li>View your <a href="' . $GLOBALS['base_url'] . '">existing site</a>.</li>
+</ul>';

Doesn't that break any kind of translatability?

@dawehner yes it does that's what I was trying to say in #25.

I think getting this patch in is worthwhile[1] and will be happy to update it (and make translation work properly).

@alexpott: You wrote 3 years ago (!): "What we need to do is continue to introduce the new exceptions here. Add a new class to the installer classes called ConvertInstallerExceptionToMessage() which has the logic for converting an exception to a translated message."

Looking at the current implementation of AlreadyInstalledException, do you still think ConvertInstallerExceptionToMessage is needed? Isn't it just to introduce the new more specific exceptions (following the current AlreadyInstalledException)?

[1] When helping users struggling with a failed install, a ,more specific exception is very helpful for us doing support. Just spent 10 minutes helping a user on Slack. A frustrating experience for both of us.

Attached is a reroll for 8.7 with the translation issue not addressed.

Patch was incomplete

...and fixing the patch level.

Resolved some coding standard issue & added an interdiff.

This would be a useful improvement.

The patch applies cleanly to 8.9.x-dev.

I could create the conditions to throw the various exceptions but adding translations seems not to work.

Status unchanged 'Needs Review'.

1) InstallationTasksCompletedException

After a fresh install revisit install.php.

Result:
Drupal already installed.

Installation tasks have already been completed.
To start over, you must empty your existing database and copy default.settings.php over settings.php.
To upgrade an existing installation, proceed to the update script.
View your existing site.

2) DatabaseAlreadyPopulatedException

Comment out $databases array in settings.php and revisit install.php.

Result:
Drupal already installed.

Database is already populated.
To start over, you must empty your existing database and copy default.settings.php over settings.php.
To upgrade an existing installation, proceed to the update script.
View your existing site.

3) ActiveConfigurationExistsException

Drop table sessions (do not drop config table) and revisit install.php.

Result:
Drupal already installed.

Active configuration already exists.
To start over, you must empty your existing database and copy default.settings.php over settings.php.
To upgrade an existing installation, proceed to the update script.
View your existing site.

Note Drupal Core doesn't create an active config directory anymore and The sync directory is defined in $settings and not $config_directories.

4) SettingsAlreadyExistsException

After a fresh install, change database name to a non-existant database in settings.php and revisit install.php.

Result:
Drupal already installed.

Configured settings.php already exists.
To start over, you must empty your existing database and copy default.settings.php over settings.php.
To upgrade an existing installation, proceed to the update script.
View your existing site.

5) Translation

Enabled module Interface translation, added a language (nl) and added a language path prefix for English.
Cleared cache.

But the new strings never show up at admin/config/regional/translate to add translations.

Is this code or configuration (or both) related? Needs further review.

Patch no longer applies to 8.9.2 or newer.

Incomplete re-roll. Something is funky here. The hunk that was to be replaced with throwing SettingsAlreadyExistsException doesn't exist in 9.1.x branch anymore. Nor 9.0.x or 8.9.x. Looks like it was removed in #3120731: Incorrect "Drupal already installed" if any database settings are wrong or unsatisfactory.

So I believe all of core/lib/Drupal/Core/Installer/Exception/SettingsAlreadyExistsException.php is dead code, since we're not throwing it anywhere.

But I don't want to leave it out of the patch (yet), since I haven't taken the time to fully understand this issue and all the gory details. So leaving at NW for sorting this out and cleaning it up.

Interdiff isn't working, so here's a raw diff of #39 vs. this.

Also note that I fixed the @throws annotation on install_verify_database_ready() and the comment where it's called.

There's some other stuff I'd fix in here, but I want to start with as close to a "pure" reroll as possible, before making other changes.

Also, tagging for 'Needs tests', since there's no test coverage of the new exceptions in here, and we certainly want that. ;)

Thanks,
-Derek

p.s. I named this "9_1_x" but I just checked and it applies to 8.9.x and 9.0.x, too... sorry for any potential confusion. Although I doubt this would be backported, so it probably doesn't matter.

Here ares some of the other things we should change before this is NR:

1. +++ b/core/includes/install.core.inc
   @@ -525,7 +527,8 @@ function install_begin_request($class_loader, &$install_state) {
   -      throw new AlreadyInstalledException($container->get('string_translation'));
   +      $translator = $container->get('string_translation');
   +      throw new ActiveConfigurationExistsException($translator);
   
   @@ -1127,7 +1130,8 @@ function install_verify_completed_task() {
   -      throw new AlreadyInstalledException(\Drupal::service('string_translation'));
   +      $translator = \Drupal::service('string_translation');
   +      throw new InstallationTasksCompletedException($translator);
   
   @@ -1177,7 +1181,8 @@ function install_verify_database_ready() {
   -    throw new AlreadyInstalledException(\Drupal::service('string_translation'));
   +    $translator = \Drupal::service('string_translation');
   +    throw new DatabaseAlreadyPopulatedException($translator);
   

   Don't understand this pattern with the new $translator local variables. Why not:

   throw new WhateverItShouldBeException($container->get('string_translation'));
   

   basically as before?

2. +++ b/core/lib/Drupal/Core/Installer/Exception/ActiveConfigurationExistsException.php
   @@ -0,0 +1,20 @@
   + * Exception thrown if Drupal is installed already.
   
   +++ b/core/lib/Drupal/Core/Installer/Exception/DatabaseAlreadyPopulatedException.php
   @@ -0,0 +1,20 @@
   + * Exception thrown if Drupal is installed already.
   
   +++ b/core/lib/Drupal/Core/Installer/Exception/InstallationTasksCompletedException.php
   @@ -0,0 +1,20 @@
   + * Exception thrown if Drupal is installed already.
   
   +++ b/core/lib/Drupal/Core/Installer/Exception/SettingsAlreadyExistsException.php
   @@ -0,0 +1,20 @@
   + * Exception thrown if Drupal is installed already.
   

   All of these comments should be more specific for the separate exception types.

3. +++ b/core/lib/Drupal/Core/Installer/Exception/AlreadyInstalledException.php
   @@ -7,26 +7,28 @@
   +   * @var string $specific_message
   

   s/@var/@param

4. +++ b/core/lib/Drupal/Core/Installer/Exception/AlreadyInstalledException.php
   @@ -7,26 +7,28 @@
   +   *   A specific message exception message.
   

   This message needs message help (message). 😉

   Maybe:

   (Optional) A specific message to include when the exception is thrown.

   ?

   Guess it should also mention that this custom message is prepended to the main text that's always printed...

5. +++ b/core/lib/Drupal/Core/Installer/Exception/AlreadyInstalledException.php
   @@ -7,26 +7,28 @@
   -    $this->stringTranslation = $string_translation;
   

   Don't we still need this line?

   Although, why are we passing the TranslationInterface at all? Doesn't look like it's used. Maybe the better fix is to remove it everywhere?

6. +++ b/core/lib/Drupal/Core/Installer/Exception/SettingsAlreadyExistsException.php
   @@ -0,0 +1,20 @@
   +class SettingsAlreadyExistsException extends AlreadyInstalledException {
   

   Per #44, I believe this is all dead code now. TBD.

7. +++ b/core/lib/Drupal/Core/Installer/Exception/SettingsAlreadyExistsException.php
   @@ -0,0 +1,20 @@
    
   

   Patch ends before any tests are added. ;) #NeedsTests.

Thanks,
-Derek

Re: #45.5b: Looks like we should have a follow-up to remove (deprecate?) StringTranslationTrait from InstallerException since we don't want to translate exception titles or messages anymore. Quick search doesn't turn up anything, but maybe someone else knows if it already exists.

I think we have sniffer now to prevent translation of exceptions starting from 8.9 #2055851: Remove translation of exception messages

Right. So is this the issue to deprecate StringTranslationTrait from InstallerException, or should we do that separately (probably first), and then this patch will be smaller and simpler?

++ file blocker for the issue and postpone that one

#3161140: Remove the StringTranslationTrait from InstallerException

Assuming that lands, something like this.