Allow migrate_drupal_ui source database to be set in settings.php

+++ b/core/modules/migrate_drupal_ui/src/Form/MigrateUpgradeFormBase.php
@@ -3,6 +3,7 @@
+use Drupal\Core\Database\Connection;

This should've been in the the CredentialForm class. PHP fatal otherwise :) I guess this was a last-second change prior to uploading 🙈

Works splendidly! Thanks :)

I feel the same pain when working on the migrate Functional tests but I am not sure why one would be debugging migrations from the core UI when it is so much easier with drush. I must be missing something.

I recall discussions when adding back incremental migrations where we deliberately made the choice to make the user re-enter credentials but I don't see that in #2687843: Add back incremental migrations through the UI. Anyone remember that?

+++ b/core/modules/migrate_drupal_ui/src/Form/CredentialForm.php
@@ -212,32 +216,44 @@ 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 set up the correct database info?</li><li>Have you set up the correct username and password?</li><li>Have you set up the correct database hostname?</li></ul>', ['%error' => $e->getMessage()]);

This string is deliberately copied from core/lib/Drupal/Core/Database/Driver/mysql/Install/Tasks.php and shouldn't be changed. At least that is what I think Gabor is saying in 2864849;#42

I feel the same pain when working on the migrate Functional tests but I am not sure why one would be debugging migrations from the core UI when it is so much easier with drush. I must be missing something.

Reasons:

• migrate_upgrade is not yet compatible with Drush 10.
• Wanting to test using the same UI that less technical end users use.

Hm. Not sure about hiding the fieldset entirely. That could be confusing, esp. if you're not the one who set this up. A better approach might be disabling all fields so they're greyed out and informing the person that these values are set from settings.php and to change them there. Or is not displaying this data back to the user done for security reasons..?

Tagging for UX team review to get some other opinions.

However, THE BIGGEST +1 OF ALL TIME for the general idea, because re-entering my credentials 40,000 times during my failed attempt to migrate webchick.net almost cost me a laptop. :P

I agree with webchick that hiding all the fields is confusing. So, this patch keeps them visible and adds a test.

Some cleanup and change the database key from 'migrate_drupal_source' to 'migrate' which is the fallback key.

In general, when tagging an issue for usability review, please make sure that the issue summary is up to date. Especially

• Proposed resolution
• User interface changes (screenshots)
• Remaining tasks

I am adding a usability review to the Remaining tasks, but someone who has worked on the issue should do the rest.

It does not really affect the usability review, but the issue summary still refers to migrate_drupal_source despite #11.

I guess the usability question is whether to completely hide the fieldset, disable it, or fill in default values? And that last option has security implications?

Updated IS.

New patch that renames the file keys in settings to refer to 'source_base_path' which is used on the UI form. The keys are now, 'migrate_source_base_path_public' and 'migrate_source_base_path_private'.

@benjifisher, thanks for the reminders in #12. I think everything is addressed now.

According to the screenshot in the issue summary, the current patch goes with the option of filling in default values, leaving off the password for security reasons. So the user will still have to fill that in.

I do not really like the idea of having to fill in the password if it is already set in settings.php.

I like the way the migrate_upgrade module does it. When I run drush migrate:upgrade, I either supply a database key, such as "drupal7" or "migrate", or else I supply credentials, formatted as a URI.

Personally, I would like equivalent functionality in the UI. Perhaps a Select list with keys from the $databases array, excluding "default". If one of those keys is "migrate", then default to that; otherwise default to the first thing on the list. The last entry would be something like "- custom -" or "- credentials -". If that is selected, then expose the existing form elements.

Alternatively, have two radio buttons: "database key" and "database credentials". The user has to select one of them, and then either a select list or the current form is presented.

I have a use case, although not a common one. Recently, I set up a copy of Drupal for testing migrations. I have "drupal6" and "drupal7" database keys, and I want the option of choosing one, depending on which source I am testing.

One other point. Maybe I am remembering wrong, but I recently set up a migration from D7 in the UI. Instead of full paths to the files, I entered the equivalent of /opt/sites/d8 and the relative path to the public/private files directory was extracted from the source database. Has this changed since 8.9.0-beta2? I think I would like it better if I could use this form to give the full path, since then I would not have to create those silly subdirectories when preparing my files for migration.

Thanks for the review.

I do not really like the idea of having to fill in the password if it is already set in settings.php.

The password does not need to be entered. It is just not displayed.

The idea of a select list or radio buttons is interesting and my only preference is for the one that is easiest for the least experienced user.

Regarding the file paths, I just entered any path for testing, there is no guarantee that it is a valid one for a migration. If a change is desired to the file paths and how they are used it will need to be done in a separate issue.

We discussed this issue at the #3133883: Drupal Usability Meeting 2020-05-12.

First, in response to #16. Thanks for clarifying the password. I agree that the file paths are out of scope for this issue, but if we make any more screenshots then we should use something more typical in order to avoid confusion. I suggest something like /opt/site/d7.

The consensus at the Usability meeting was to use a select list, assuggested in #15. More specifically,

1. If the only key in the $databases array is "default", then present the current form elements, with no default values. No change from current behavior.
2. Otherwise, present a select list showing the keys other than "default" and an option to enter credentials.
3. If one of the keys is "migrate", then default to that.
4. If the "other" option is selected, then present the current form.

We did not discuss the labels and such, and we did not discuss what the initial state should be when there are keys other than "default" but no "migrate".

In #15, I suggested defaulting to the first item on the list. That will be convenient if there are exactly two keys, say "default" and "drupal7". But maybe it is safer to default to the empty choice, something like "- Select -".

I suggest keeping the "other" option short, something like "- Other -" rather than "Enter credentials". Technically, either one is a possible value for an array key, but I would like to pretend that no one will ever be that silly. I would add some help text (description): something like

Choose one of the keys from the $databases array or else select "Other" and enter database credentials.

Maybe we should work harder to come up with a UI that does not need help text, but IMO this is not too bad: most people will ignore the text unless they need it.

Here is an attempt at implementing #17.

Improvements to CredentialFormTest and in CredentialForm::validateForm only use the manually entered values if the source_connection value is NULL or 'Other'.

Make sure that $error_key is defined.

Closed #2947341: Make uid/password re-entry optional as a duplicate of this.

This looks good, and looks like the needs of the usability review were met. I'm tagging 'Needs product manager review' because 'Needs Webchick Review' is just not a tag that would get noticed. :-p

The patch does not apply, so I am adding the tags "Needs reroll" and "Novice".

I added Reroll to the Remaining tasks, but that section of the issue summary probably needs further updates.

Rerolled the patch in #22. Added reroll_diff_3096101_22-27.txt as well. Please review.

Thank you.

Back to RTBC to get it in front of product managers.

+++ b/core/modules/migrate_drupal_ui/tests/src/Functional/d7/CredentialFormTest.php
@@ -0,0 +1,211 @@
+  /**
+   * {@inheritdoc}
+   */
+  public static $modules = [
+    'migrate_drupal',
+    'migrate',
+    'migrate_drupal_ui',
+  ];

This needs to be protected.

Thanks @mikelutz for the suggestion. I have updated the patch.

Ignore the provious patch.
#31 comment's patch is the correct one. Apologies for the confusion.

Ignore the #32 comment's patch.

Re-uploading the patch from #31 so it is the last in the issue and it is less confusing for committers/reviewers. Resetting to RTBC, hoping to get in front of @webchick

1. +++ b/core/modules/migrate_drupal_ui/src/Form/CredentialForm.php
   @@ -112,11 +121,33 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   +      $list['Other'] = '- Other -';
   ...
   +        '#description' => 'Choose one of the keys from the $databases array or else select "Other" and enter database credentials.',
   

   These need to be translatable strings.

2. +++ b/core/modules/migrate_drupal_ui/src/Form/CredentialForm.php
   @@ -112,11 +121,33 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   +          ':input[name=source_connection]' => ['value' => 'Other'],
   
   @@ -124,6 +155,11 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   +          ':input[name=source_connection]' => ['value' => 'Other'],
   
   @@ -140,6 +176,24 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   +          ':input[name=source_connection]' => ['value' => 'Other'],
   ...
   +            ':input[name=source_connection]' => ['value' => 'Other'],
   ...
   +            ':input[name=source_connection]' => ['value' => 'Other'],
   

   I'd set the value for - Other - to an empty string then it'll never conflict with something someone has in the database keys already. As unlikely as it is 'Other' is a possible database key.

3. +++ b/core/modules/migrate_drupal_ui/src/Form/CredentialForm.php
   @@ -212,26 +268,33 @@ public function validateForm(array &$form, FormStateInterface $form_state) {
   +    if ($source_connection == 'Other' || is_null($source_connection)) {
   

   This can then be empty($source_connection)

4. +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/d7/CredentialFormTest.php
   @@ -0,0 +1,211 @@
   +    if (($source_connection == 'Other') || (is_null($source_connection))) {
   ...
   +    if (($source_connection == 'Other') || (is_null($source_connection))) {
   

   $source_connection never is 'Other' here as far as I can see.

35.1 Fixed
35.2 and .3. Doing this changes the way for form works and making it different than what was suggested by the UX review, #17. At least as I understand it.
35.4 Added test for other. Also moved the test out of the d7 subdirectory because the test is not dependent on the source version.

@quietone, I don't believe @alexpott meant to say to change the display text of the '- Other -' option, only to change the internal representation of the list value from 'other' to '', in case someone has defined a database already called 'other', which wouldn't seem odd to me.

@mikelutz thanks for putting my thoughts into clearer language #38 is exactly what I was trying to say.

When the internal representation of the list value is an empty string, '', the option '- Other -' is not displayed and the form for the db credentials is displayed. That is different that the feedback from the UX review, #17. Not sure what to do here.

How about this. Let's change - Other - to - User defined - since other is a strange would to have on its own. And if we don't make the field required then is behaves as we'd like it to.

Ah, I see. That works for me.

I tested the patch and noticed that when the 'migrate' key is found it is not set as the default. This should fix that.

+++ b/core/modules/migrate_drupal_ui/src/Form/CredentialForm.php
@@ -107,11 +116,31 @@ public function buildForm(array $form, FormStateInterface $form_state) {
+    if ($list) {
+      $form['source_connection'] = [
+        '#type' => 'select',
+        '#title' => $this->t('Source connection'),
+        '#options' => $list,
+        '#default_value' => array_key_exists('migrate', $list) ? 'migrate' : NULL,
+        '#empty_option' => $this->t('- User defined -'),
+        '#description' => $this->t('Choose one of the keys from the $databases array or else select "User defined" and enter database credentials.'),
+      ];
+    }

We should always have the form element - makes life much easier for form alters. We should use #access to determine whether to display it to the user.

ie. #access => !empty($list)

44.1 OK, Fixed. thanks for the clear instructions.

+++ b/core/modules/migrate_drupal_ui/src/Form/CredentialForm.php
@@ -107,11 +116,32 @@ public function buildForm(array $form, FormStateInterface $form_state) {
+    if ($list) {

Obviously not clear enough :(

This if can now be removed. It's much better to use #access than ifs in forms so the elements are always there - it makes altering much more predictable.

NP, that is not uncommon for me.
Fixed.

Feedback has been addressed, back to RTBC, thanks @alexpott and @quietone for sticking with this.

Unfortunately this needs a reroll.

The patch in #47 has a new file CredentialFormTest.php but because of #3143720: Create a separate CredentialFormTest there is an existing file of that name. Therefor the test was renamed to SettingsTest.php and deprecated functions changed and comments changed.

Thanks, @quiteone

"Needs product manager review" has still not happened. Also I think we should add a section to default.settings.php about the new settings and migration. Plus we need a change record.

Added a change record and removed the tag. Added a new section to default.settings.php and removed the 'needs documentation' tag which I hope is what that tag meant in #52.

1. +++ b/core/assets/scaffold/files/default.settings.php
   @@ -762,6 +762,25 @@
   + * - d6_source_path_path - The document root of the source Drupal6 site.
   + * - source_base_path - A local file directory containing your site (e.g.
   + *   /var/www/docroot), or your site address (for example http://example.com)
   

   What is exactly the difference between "the document root of the source site" and "a local file directory containing your site"? It is a bit confusing to me.

2. Could we add some example code what the setting could be. Next code copied from default.settings.php:

    * Sample Database configuration format for a driver in a contributed module:
    * @code
    *   $databases['default']['default'] = [
    *     'driver' => 'my_driver',
    *     'namespace' => 'Drupal\my_module\Driver\Database\my_driver',
    *     'autoload' => 'modules/my_module/src/Driver/Database/my_driver/',
    *     'database' => 'databasename',
    *     'username' => 'sqlusername',
    *     'password' => 'sqlpassword',
    *     'host' => 'localhost',
    *     'prefix' => '',
    *   ];
    * @endcode
   

Could we make the text in default.settings.php such that it is easily understandable for somebody who has not so much experience with migrations.

@daffie, good points.

This is an attempt to address the above comment.

Fix formatting in default.settings.php.

This issue is not specific to Drupal 7 sources, removing tag.

I think the patch needs a reroll.

Re-rolled

@anmolgoyal74, the reroll looks good. Thanks for that!

Practical notes:

• The patch in #60 applies to the current 9.2.x. The patch in #57 applies to 9.1.5.
• Both patches change a file outside of core/, so they cannot be applied with the composer patches plugin. I want to use the patch today, so I saved a copy locally and removed the hunk that updates sites/default/default.settings.php.

I am setting back to NW for this: from #13,

New patch that renames the file keys in settings to refer to 'source_base_path' which is used on the UI form. The keys are now, 'migrate_source_base_path_public' and 'migrate_source_base_path_private'.

That was updated later to 'migrate_file_public_path' and 'migrate_file_private_path'. The code now has these keys, but the comments and example lines in settings.php (two copies of that file!) still have the old names. We need to fix that!

I have been testing this patch, and it seems to work well. I will give a full review once the settings.php files are updated.

I've updated the array keys so they start with 'migrate_' and updated the comments and tests accordingly.

+++ b/core/modules/migrate_drupal_ui/tests/src/Functional/SettingsTest.php
@@ -0,0 +1,214 @@
+    $settings['settings']['source_base_private_path'] = (object) [

wrong key

The new test was passing locally with this error and I wondered why. The test was setting the values of the base paths fields when submitting the form, it should not do that. It should let the Credential form get it from the settings.php.

So, while I was working on the previous patch, I noticed that the key for the source database connection was not being read from settings.php. That meant that if it was set to 'foo' the form was not automatically set to that value. I also happened to test with a Drupal 6 source and the version always went to the default, '7'. So, I added a new parameter in settings.php to allow the version to be set as well.

And the test needed some serious work as well. It was completing form fields when it should not have been.

@quietone:

Thanks for all the updates!

When testing, I did notice that I had to select the connection. That is so much easier than filling in the fields that I did not mind at all!

I have not looked yet at the test.

1. The Credential form is at /upgrade/credentials.

    +++ b/core/assets/scaffold/files/default.settings.php
    @@ -768,6 +768,54 @@
    ...
    +/**
    + * The default settings for migration sources.
    + *
    + * These settings are used as the default settings on the Credential form at
    + * /upgrade.
    + *
    + * - migrate_source_version - The version of the source database. Defaults to
    + *   '7'.

   We might also be explicit that the allowed options are 6 and 7, but I think it is OK as is because of the examples that follow.

   Remember to change both copies of this file.

2. Again, remember to update both copies of the file:

    + * - migrate_d6_file_public_path - The location of the source Drupal 6 files.
    + *   This can be a local file directory containing the source Drupal 6 site
    + *   (e.q. /var/www/docroot), or the Drupal 6 site address (e.q.
    + *    http://example.com).

   It should be "e.g.", not "e.q.". I see a total of 5 places in each file (so 10 in the patch). I was taught to add a comma: "e.g.,", just as if it were spelled out "for example,".

3. Same snippet: there is an extra space on the last quoted line.

4. The second sentence should start with "This can be":

     * - migrate_file_private_path - The location of the source Drupal 7 private
     *   files. The can be a local file directory containing the source Drupal 7
     *   site (e.q. /var/www/docroot), or empty to use the same value as Public
     *   files directory.

5. Do we really need separate keys migrate_d6_file_public_path and migrate_file_public_path? Why not combine them, and update the comments?

6. Why supply a default value for the connection but not the version? Come to think of it, do we need these values at all? If the setting is not there, then Settings::get(...) will return NULL. Or you can add a default value (FALSE or '', for example) as an optional second parameter.

    +$settings['migrate_source_connection'] = 'migrate';
    +$settings['migrate_source_version'] = '';
    +$settings['migrate_d6_file_public_path'] = '';
    +$settings['migrate_file_public_path'] = '';
    +$settings['migrate_file_private_path'] = '';

7. Can we move the logic outside the form array and just have #default_value' => $migrate_source_version?

    @@ -99,19 +101,44 @@ public function buildForm(array $form, FormStateInterface $form_state) {
    ...
    +    $migrate_source_version = Settings::get('migrate_source_version');
         $form['version'] = [
           '#type' => 'radios',
    -      '#default_value' => 7,
    +      '#default_value' => in_array($migrate_source_version, ['6', '7']) ? $migrate_source_version : '7',

8. Same snippet: we do not plan ever to support different values, so I am not worried about making these lines easy to maintain. Instead of in_array(), I would use

    $migrate_source_version = Settings::get('migrate_source_version') == '6' ? '6' : '7';

9. I would like to simplify this a little:

    +    $all_connections = Database::getAllConnectionInfo();
    +    $available_connections = array_diff_key($all_connections, ['default' => []]);
    +    $keys = array_keys($available_connections);
    +    $list = array_combine($keys, $keys);
    +    $default_value = array_key_exists('migrate', $list) ? 'migrate' : NULL;
    +    $migrate_source_connection = Settings::get('migrate_source_connection');
    +    if ($migrate_source_connection) {
    +      $default_value = array_key_exists($migrate_source_connection, $list) ? $migrate_source_connection : NULL;
    +    }
    +    $form['source_connection'] = [
    ...

   Something like this (untested):

    $available_connections = array_diff(array_keys(Database::getAllConnectionInfo()), ['default']);
    $migrate_source_connection = Settings::get('migrate_source_connection');
    $preferred_connections = $migrate_source_connection
      ? [$migrate_source_connection, 'migrate']
      : ['migrate'];
    $default_options = array_intersect($preferred_connections, $available_connections);
    $form['source_connection'] = [
      '#type' => 'select',
      '#title' => $this->t('Source connection'),
      '#options' => array_combine($available_connections, $available_connections),
      '#default_value' => array_pop($default_options),

   If you do not like that, I would still like to rename $default_value to $default_connection. And if you prefer to keep array_combine() outside the form array, then use $options instead of $list.

10. It is a little funny to declare both $element['#required'] and $element['#states']['required'], but I guess we need that so that, with JS disabled, the field is required.

     @@ -119,6 +146,11 @@ public function buildForm(array $form, FormStateInterface $form_state) {
            '#title' => $this->t('Database type'),
            '#required' => TRUE,
            '#default_value' => $default_driver,
     +      '#states' => [
     +        'required' => [
     +          ':input[name=source_connection]' => ['value' => ''],
     +        ],

    On second thought, it is more complicated than that. If JS is disabled, then this form element will be required, no matter what. Maybe the right thing to do is remove '#required' => TRUE and make sure that the validation works. Someone who has JS disabled AND leaves out the setting will be a little unhappy, but anyone filling in the new connection option, with or without JS, will win.

11. I am confused here. Let me add the full text of the comment at the top:

     @@ -136,6 +168,24 @@ public function buildForm(array $form, FormStateInterface $form_state) {
            // This is a multi-step form and is not rebuilt during submission so
            // #limit_validation_errors is not used. The database and username fields
            // for mysql and pgsql must not be required.
            $form['database']['settings'][$key]['database']['#required'] = FALSE;
            $form['database']['settings'][$key]['username']['#required'] = FALSE;
     +      $form['database']['settings'][$key]['database']['#states'] = [
     +        'required' => [
     +          ':input[name=source_connection]' => ['value' => ''],
     +        ],
     +      ];
     +      if ($key != 'sqlite') {
     +        $form['database']['settings'][$key]['username']['#states'] = [
     +          'required' => [
     +            ':input[name=source_connection]' => ['value' => ''],
     +          ],
     +        ];
     +        $form['database']['settings'][$key]['password']['#states'] = [
     +          'required' => [
     +            ':input[name=source_connection]' => ['value' => ''],
     +          ],
     +        ];
     +      }

    From the comment and the two lines of code that follow it, it seems that "Database name" and "Database username" are not required, but the patch makes them so if no connection is selected. But I tested without the patch, and the two fields act like they are required.

12. Forgive a little nit:

     @@ -183,12 +234,12 @@ public function buildForm(array $form, FormStateInterface $form_state) {
     ...
          $form['source']['source_private_file_path'] = [
            '#type' => 'textfield',
            '#title' => $this->t('Document root for private files'),
     -      '#default_value' => '',
            '#description' => $this->t('To import private files from your current Drupal site, enter a local file directory containing your site (e.g. /var/www/docroot). Leave blank to use the same value as Public files directory.'),
            '#states' => [
              'visible' => [
     @@ -196,6 +247,7 @@ public function buildForm(array $form, FormStateInterface $form_state) {
              ],
            ],
            '#element_validate' => ['::validatePaths'],
     +      '#default_value' => Settings::get('migrate_file_private_path'),

    Why not leave the #default_value key near the top of the form array? And then maybe make the other changes to be consistent with this one instead of making this consistent with the others.

13. I think the comment should go inside the if clause:

     @@ -208,26 +260,33 @@ public function validateForm(array &$form, FormStateInterface $form_state) {
          // Retrieve the database driver from the form, use reflection to get the
          // namespace, and then construct a valid database array the same as in
          // settings.php.
     -    $driver = $form_state->getValue('driver');
     ...
     +    $source_connection = $form_state->getValue('source_connection');
     +    if (empty($source_connection)) {
     +      $driver = $form_state->getValue('driver');
     ...
     +    else {
     +      $info = Database::getConnectionInfo($source_connection);
     +      $database = reset($info);
     +    }

14. Same snippet: my personal preference is to have short if clauses and long else clauses: if ($source_connection) { ... } else { ... }. But it is just a preference, so you get to choose.

#67
1, 2, 3, 4: Fixed
6. Removed setting a default for the source connection. Folks should set this and 'migrate' is the default in code now.
7/8. Fixed
9. I'm on the fence about these changes but I did them and then SettingsTest failed. array_intersect returns the correct values but not in a predictable order so when array_pop used to get the value it can get the wrong value. Therefore there is only cleanup here.
13/14. Fixed

TODO
5, 10, 11, 12

#67. 5 and 12 Fixed.

TODO
10, 11

#67. 10 and 11

Looking back through the history the '#states' were added to the form in #18. I haven't looked further - it is too close to bedtime.

10. When '#required' => TRUE' is removed the little red asterisk does not appear. Shouldn't that be kept?
11. Yes, this is an interesting form. This a href="https://www.drupal.org/project/drupal/issues/2678510#comment-13723743">comment from another issue about this form may be useful here.

One bad thing, I played with the form and found that for Sqlite I could only submit the form when JS is disabled. I hope I am wrong ....

I will not have time to look at this today, but I have a couple of quick comments:

array_intersect returns the correct values but not in a predictable order

I did a couple of quick tests and decided that the order would be the same as in the first parameter, but I guess I should have looked for documentation to that effect.

In fact, I did some more testing and consistently got the expected order. The PHP docs do not address this, but the answer to this StackOverflow question claims that the order will be the same as in the first argument.

When '#required' => TRUE' is removed the little red asterisk does not appear. Shouldn't that be kept?

Is that the case even when JS is enabled, so that the field is required because of the '#states' setting?

If the red asterisk only goes away when JS is disabled, then I think that is acceptable, as long as we check it in the validation stage.

Ah, your searching about array_intersect was better than mine. I've added that to this patch.

Also added a new data set, where there is a migrate database defined in settings.php but migrate_source_connection is not set. To do that another parameter was added to the source provider, the expected_source_connection.

I am getting the following error in the console.

Steps to reproduce:
1) Updated settings.php to set the fields in the credential form.
2) Now select " - User Defined - " under Source connection.
3) Database type selected is "MySQL, Percona Server, or equivalent".
4) Fill the rest of the form.
5) Click on "Review Upgrade".
The Form is not submitting.

@anmolgoyal74, thanks for testing!

I installed the patch and found that whenever 'user defined' is selected the form does not submit. This is now outside my field of knowledge.

Can anyone jump in and assist with getting this form working?

What happens to the form is that we have a set of fields for each database driver. And because of this state, each field for each driver is required when the "- User defined -" option is selected:

$form['database']['settings'][$key]['database']['#states'] = [
    'required' => [
        ':input[name=source_connection]' => ['value' => ''],
    ],
];

For example, if we select "PostgreSQL" Database type, the fields for "MySQL, Percona Server, or equivalent" remains required. Altough these fields are hidden, the validation fails anyway. See: https://stackoverflow.com/a/28340579/6314031

The fix is either to use the same fields for all database types (it seems this is how it's done in install_settings_form) or make required fields only attached to the selected database type.

I'm attaching a patch with 2nd approach.

This is a really nice feature that looks really close to being ready to commit. I tested the patch in #76 manually and it seemed to work great for me.

That said, I think the automated tests could be adjusted a little bit. For one, I think it would be really nice to have a test that fails the way manual testing in #73 fails. That way we won't regress in the future. I'm attaching a patch which is identical to #72 other than the tests. I add a new test case that I expect to fail and make some slight changes to a few other test cases, which I think were accidents. I also changed the order of a few things to try to make the test a little clearer.

In order to fail like in #73 I had to change the test to be a FunctionalJavascript test.

This does not have the fix from #76 yet because I want the new test case to fail.

Here's the fix from #76 added.

Random fail in Drupal\Tests\field_ui\FunctionalJavascript\EntityDisplayTest::testExtraFields

There's already an issue for it: #3203712: [random test failure] EntityDisplayTest::testExtraFields()

retesting...

@Matroskeen, thanks for fixing the form and explaining the problem.

I updated the IS and the change record to use the latest array keys.

I then did a little testing and was getting confusing errors on the files paths. For example, say the version is 7, the source path was still being tested as if it applied to Drupal 6. Therefor, I updated the validatePaths method to only check the elements for the relevant version. That seems to work. This screen shot may help to explain. The first error message is for d6 and the second one is d7.

I also changed the sourceProvider in the new tests (thanks danflanagan8) just to add an array key for the last value.

Queuing a test against 9.4 to see if this needs a re-roll

FWIW, this works great for me on 9.3.0.

Moving to 10.0.x

Marking for Needs Review. looks like test failures are unrelated.

This has been reviewed a few times, and requested changes have been made. The code looks good to me, and I think this is a useful feature.

queued a test on D9.5.x

This change is looking really good - nice work on all the javascript testing and use of #states.

1. +++ b/core/assets/scaffold/files/default.settings.php
   @@ -752,6 +752,49 @@
   +$settings['migrate_source_connection'] = '';
   +$settings['migrate_source_version'] = '';
   +$settings['migrate_file_public_path'] = '';
   +$settings['migrate_file_private_path'] = '';
   
   +++ b/sites/default/default.settings.php
   @@ -752,6 +752,49 @@
   +$settings['migrate_source_connection'] = '';
   +$settings['migrate_source_version'] = '';
   +$settings['migrate_file_public_path'] = '';
   +$settings['migrate_file_private_path'] = '';
   

   Normally the default is provided as part of the Settings::get() call.

   I think these settings should be commented out. There's no need for 99% of sites to have them set to empty strings. That's what we do for all the other optional settings.

2. +++ b/core/modules/migrate_drupal_ui/src/Form/CredentialForm.php
   @@ -99,19 +101,42 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   +    $migrate_source_version = Settings::get('migrate_source_version') == '6' ? '6' : '7';
   ...
   +    $migrate_source_connection = Settings::get('migrate_source_connection');
   

   Use the NULL default here makes sense.

3. +++ b/core/modules/migrate_drupal_ui/src/Form/CredentialForm.php
   @@ -164,6 +215,7 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   +      '#default_value' => Settings::get('migrate_file_public_path'),
   
   @@ -176,6 +228,7 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   +      '#default_value' => Settings::get('migrate_file_public_path'),
   
   @@ -188,7 +241,7 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   +      '#default_value' => Settings::get('migrate_file_private_path'),
   

   Could set the default here to an empty string.

@alexpott, thanks for the review.

This patch should address #92.

92.1 Fixed
92.2 no change needed
92.3. Fixed

Thanks @quietone!

The latest fail was unrelated:

1) Drupal\Tests\ckeditor\FunctionalJavascript\CKEditorIntegrationTest::testOffCanvasStyles
Behat\Mink\Exception\ElementNotFoundException: Element matching css ".cke_top" not found.

Resetting status to RTBC.

Another unrelated fail, different from the one in #96..
Drupal\Tests\layout_builder\FunctionalJavascript\InlineBlockPrivateFilesTest::testPrivateFiles

I meant to restore the RTBC.

Failure in an unrelated test, retesting.
1) Drupal\Tests\ComposerIntegrationTest::testExpectedScaffoldFiles with data set #6 ('.htaccess', 'assets/scaffold/files/htaccess')

Tests are passing so I am restoring the RTBC.

Saving issue credit.

Removing the "Needs product manager review". I think the general idea got a +1 from @webchick in #7 and the tag has been on the issue for years without a review. Given that the changes are largely to settings.php and UI has been tested by @benjifisher of the usability working group I think we're good here.

Discussed with @longwave and @catch - we agreed to put this in 9.5.x as this only affects new migrations and is quite beneficial.

Committed and pushed 9fbab426bf to 10.1.x and 639705575b to 10.0.x and 9cc941593f to 9.5.x. Thanks!

#82 the patch worked great. I failed to update at first. Turns out I had to truncate migrate_map_d7_file, migrate_message_d7_file and migrate_message_upgrade_d7_file. The $settings for public and private downloads are working perfectly.