Improve migrate UI database credential form

No actual change here to review?

One basic thing we can do to give some context is add a sentence or two of page-level help text. If there is or will be a handbook page with more detailed instructions we could link to it from there. Something like:

"Provide the information to access the database you want to upgrade. Files can be imported into the upgraded site as well. See the [a]Upgrade documentation for more detailed instructions[/a]"

This information is already there in context of the respective forms so this may not be super great addition if there's not also a docs page to link to :)

As for the error, that looks like the standard wording that mysql itself returns, which may be hard to modify?

The only thing I can think of for improving the error message is doing some regex split on the last closing square bracket, so that we could put the description ("Unknown database...") above or before the error number ("SQLSTATE[HY000] [1049]").

Reviewed in the weekly migrate maintainers meeting. Tagging more appropriately. It would be super helpful if someone with a UX eye could provide some guidance on what this should look like.

+++ b/core/modules/migrate_drupal_ui/src/Form/CredentialForm.php
@@ -217,10 +222,11 @@ public function validateForm(array &$form, FormStateInterface $form_state) {
+      $msg = $this->t('Failed to connect to your database server. The server reports the following message: %error.<ul><li>Is the database server running?</li><li>Does the database exist, and have you entered the correct database name?</li><li>Have you entered the correct username and password?</li><li>Have you entered the correct database hostname?</li></ul>', ['%error' => $e->getMessage()]);
...
+        '#items' => [$msg],

Usually an item_list accepts an array of values that become ul/li items. I don't see that happening here.

https://api.drupal.org/api/drupal/core%21themes%21stable%21templates%21d... seems to have a title attribute. Perhaps we could utilize it? Also, I wonder if we should soft block this on #2562845: Validate file path on Credential form? Or have that one soft block on this. But I feel better about soft blocking this one.

$error_message = [
        '#title' => $this->t('Resolve all issues below to continue the upgrade.'),
        '#theme' => 'item_list',
        '#items' => $msg,
      ];

We are adding title there, Please see , https://www.drupal.org/node/1842756

+++ b/core/modules/migrate_drupal_ui/src/Form/CredentialForm.php
@@ -222,11 +222,24 @@
+        '#markup' => $this->t('Failed to connect to your database server. The server reports the following message: %error', ['%error' => $e->getMessage()]),
...
+            '#markup' => $this->t('Does the database exist, and have you entered the correct database name?'),

But this is still using #markup instead of just a simple list item array.

#2562845: Validate file path on Credential form is now in so no longer postponed.

I'll work on the re-roll.

Re-rolled.

Now working on comments in #17.

Replaced #markup with '#theme' => 'item_list'.

Minor test changes to match error wording changes.

Do we still need UX review on this? And can we get a new screenshot?

It is unfortunate that the string in Tasks.php is only slightly different. But it includes words that are specific to an install to so we cannot take advantage of the same translation.

As far as UX review, anything to bring wording into consistency is better UX. Especially as the install process has already gone through UX review. Getting rid of the tag.

And for the same reason, marking RTBC. This is a wording only change to the forms. No functionality has adjusted. Onward and upward.

Back to RTBC again.

Sorry to ping-pong on this, but the inline markup in the translatable string is not very good and also leads to possible errors if the translators happen to mistype a (closing) tag that eats up the whole page.

Yeah if we already have this string, then we can use it. I was hoping we can avoid inlining HTML somehow because this is a big potential for breaking the page with translations :/ If we have no reasonable way in this system, then we can inline the HTML. The above patches attempted to use a rendering process to do that but that was changed to inline HTML without comment as to why was that not adequate. So it is not clear if that was a dead-end that did not work or just did not look good? Or was it for the string reuse?

Looks great except for a couple of nitpicks. Re-tagging with the Usability tag because yes, this issue does have to do with usability.

1. +++ b/core/modules/migrate_drupal_ui/src/Form/CredentialForm.php
   @@ -245,7 +245,8 @@ public function validateForm(array &$form, FormStateInterface $form_state) {
   -        $this->errors[$database['driver'] . '][database'] = $e->getMessage();
   

   Let's do $error_key = $database['driver'] . '[database'; and re-use that variable.

2. +++ b/core/modules/migrate_drupal_ui/src/Form/CredentialForm.php
   @@ -245,7 +245,8 @@ public function validateForm(array &$form, FormStateInterface $form_state) {
   +        $msg = $this->t('Failed to connect to your database server. The server reports the following message: %error.<ul><li>Is the database server running?</li><li>Does the database exist, and have you entered the correct database name?</li><li>Have you entered the correct username and password?</li><li>Have you entered the correct database hostname?</li></ul>', ['%error' => $e->getMessage()]);
   

   What if we put this string in a public static method somewhere in the database system, so we don't have to copy and paste it here?

3. +++ b/core/modules/migrate_drupal_ui/src/Form/CredentialForm.php
   @@ -265,13 +266,14 @@ public function validateForm(array &$form, FormStateInterface $form_state) {
   +          $msg = $msg . ' ' . $this->t('The server reports the following message: %error.', ['%error' => $e->getMessage()]);
   

   Should be $msg .= ' ' . $this->t(...)

Can we do this in a followup? I'd rather not introduce something that will slow these UI issues down.

Agreed.

RTBC once green on all backends. :)

+++ b/core/modules/migrate_drupal_ui/src/Form/CredentialForm.php
@@ -226,11 +226,12 @@ public function validateForm(array &$form, FormStateInterface $form_state) {
       try {

@@ -245,7 +246,8 @@ public function validateForm(array &$form, FormStateInterface $form_state) {
+        $msg = $this->t('Failed to connect to your database server. The server reports the following message: %error.<ul><li>Is the database server running?</li><li>Does the database exist, and have you entered the correct database name?</li><li>Have you entered the correct username and password?</li><li>Have you entered the correct database hostname?</li></ul>', ['%error' => $e->getMessage()]);
+        $this->errors[$error_key] = $msg;

do we need this local variable?

Sounds good

New title to make it clear this isn't changing the installer

No longer applies :(

error: patch failed: core/modules/migrate_drupal_ui/src/Form/CredentialForm.php:226
error: core/modules/migrate_drupal_ui/src/Form/CredentialForm.php: patch does not apply
make: *** [patch87] Error 1

RTBC on green.

Fixed on commit

diff --git a/core/modules/migrate_drupal_ui/src/Form/CredentialForm.php b/core/modules/migrate_drupal_ui/src/Form/CredentialForm.php
index 57549b9e10..74b542525e 100644
--- a/core/modules/migrate_drupal_ui/src/Form/CredentialForm.php
+++ b/core/modules/migrate_drupal_ui/src/Form/CredentialForm.php
@@ -3,7 +3,6 @@
 namespace Drupal\migrate_drupal_ui\Form;

 use Drupal\Component\Utility\UrlHelper;
-use Drupal\Core\Database\DatabaseException;
 use Drupal\Core\Form\FormStateInterface;
 use Drupal\Core\TempStore\PrivateTempStoreFactory;
 use Drupal\migrate\Exception\RequirementsException;

Committed 61fa673 and pushed to 8.7.x. Thanks!