Add a getCredentials helper method to migrate_drupal_ui functional tests

This is a great idea! It takes all that code that confused me when I was reviewing #2925899: MigrateUpgradeImportBatch does not use source_private_file_path & source_base_path correctly, making it impossible to have public & private files in separate locations and puts it in one place, where we can add some better documentation.

1. See (3) below before making any changes to the @param comments.

    +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/MigrateUpgradeTestBase.php
    @@ -260,4 +260,41 @@ protected function assertMigrationResults(array $expected_counts, $version) {
    +  /**
    +   * Creates an array of credentials to post.
    +   *
    +   * @param array $connection_options
    +   *   Source database connection options.
    +   * @param string $version
    +   *   The legacy Drupal version.
    +   *
    +   * @return string[]
    +   *   The flattened $edit array suitable for BrowserTestBase::drupalPostForm().
    +   */
    +  protected function getCredentials(array $connection_options, $version) {

   This is for posting to the credential form, right? I think it should say so in the one-line description: "Creates an array of credentials to post to the credential form". Also add an @see for that form. Then someone who has never looked at this code before, like me, will know where to look for the expected keys.

   Can you say something more about the Source database connection options? From the code, I can see that it has keys 'driver' and 'prefix', but that is all I know.

   The legacy Drupal version can only be 6 or 7, right? That seems obvious today, but wait until Drupal 10 is released. ;)

2.   +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/MultilingualReviewPageTestBase.php
     @@ -103,27 +103,10 @@ public function prepare() {
          $driver = $connection_options['driver'];
          $connection_options['prefix'] = $connection_options['prefix']['default'];
   
     -    // Use the driver connection form to get the correct options out of the
     -    // database settings. This supports all of the databases we test against.
     -    $drivers = drupal_get_database_types();
     -    $form = $drivers[$driver]->getFormOptions($connection_options);
     -    $connection_options = array_intersect_key($connection_options, $form + $form['advanced_options']);
     +    // Get valid credentials.
     +    $connection_options = $this->sourceDatabase->getConnectionOptions();
          $version = $this->getLegacyDrupalVersion($this->sourceDatabase);

   It looks as though we can delete the first few lines of this function: everything before the comment, "Get valid credentials".

3. It seems that every call to this function starts with

    $connection_options = $this->sourceDatabase->getConnectionOptions();
    $version = $this->getLegacyDrupalVersion($this->sourceDatabase);

   Why not do that inside the function instead of passing these as parameters?

4. I am a little worried about how this will be affected by #2925899: MigrateUpgradeImportBatch does not use source_private_file_path & source_base_path correctly, making it impossible to have public & private files in separate locations:

    $edit = [
      $driver => $connection_options,
      'source_private_file_path' => $this->getSourceBasePath(),
      'version' => $version,
    ];
    if ($version == 6) {
      $edit['d6_source_base_path'] = $this->getSourceBasePath();
    }
    else {
      $edit['source_base_path'] = $this->getSourceBasePath();
    }

   Will it be as simple as replacing the line that defines 'source_private_file_path' with the new getSourcePrivateBasePath() (which defaults to NULL)? Even if it is that simple, we will have to update one of these issues when we commit the other, so I will add it as a related issue.

5. Related to the previous point:

    return $this->translatePostValues($edit);

   This makes it impractical for the calling function to modify the returned result. If we just return $edit, then it is a lot easier. The only cost is that the calling function has to call $this->translatePostValues(). It is an instance of the single-responsibility principle: what is the one thing that this function is responsible for doing?

The patch in #7 looks a lot better. Thanks! In particular, removing the parameters to getCredentials() means we do not need extensive @param comments. The API docs for drupal_get_database_types() are a few links away from getFormOptions(), which makes the structure pretty clear. I think we could add some information to $sourceDatabase, but that is out of scope for this issue.

I have just a couple more requests:

1. Follow-up to #4.5:

    +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/MigrateUpgradeTestBase.php
    @@ -260,4 +260,40 @@ protected function assertMigrationResults(array $expected_counts, $version) {
    +  /**
    +   * Creates an array of credentials to post to the Credential form.
    +   *
    +   * @return string[]
    +   *   The flattened $edit array suitable for BrowserTestBase::drupalPostForm().
    +   *
    +   * @see \Drupal\migrate_drupal_ui\Form\CredentialForm
    +   */
    +  protected function getCredentials() {

   It should now be @return array. The following description needs to be updated as well. Maybe "… suitable for BrowserTestBase::translatePostValues()"? Certainly, remove "flattened".

   To be honest, I am not sure #4.5 is a good idea, either. Some day, if we are adding a new test and we need to change one of the nested array values, we will be glad we did it this way. That day may never come, but if it does then we do not want to do more of the copy/paste that we are fixing in this issue.

2. I see you took the opportunity to use the new helper function in another file, MultilingualReviewPageTestBase.php. Good! That made me wonder if there are any other places we should use it, so I searched for getFormOptions(). Is there any reason not to use it in MigrateUpgradeExecuteTestBase::testMigrateUpgradeExecute()?

1.   /**
      * Creates an array of credentials for the Credential form.
      *
      * Before submitting to the Credential form the array must be processed by
      * BrowserTestBase::translatePostValues() before submitting.
      *
      * @return array
      *   An array of values to post that are suitable for
      * BrowserTestBase::translatePostValues().
      *
      * @see \Drupal\migrate_drupal_ui\Form\CredentialForm
      */
     protected function getCredentials() {
   

   Given the one-line description and the @see comment, I think we can remove the extra comment and just use "An array of values suitable for BrowserTestBase::translatePostValues()." as the @return comment. I think that fits one one line; if not, then both lines should be indented.

2.   public function testMigrateUpgradeExecute() {
       $connection_options = $this->sourceDatabase->getConnectionOptions();
       // ...
   
       $driver = $connection_options['driver'];
       $connection_options['prefix'] = $connection_options['prefix']['default'];
   
       // Get valid credentials.
       $edits = $this->translatePostValues($this->getCredentials());
   

   We can remove a few more lines here.

working on it

@benjifisher Could you please review this patch for #10

@pavnish:

Thanks for trying to finish off this issue.

If you remove the words "to post that are", then the @return comment should fit on a single line. I do not think the meaning suffers.

In addition to the blank line(s), every line I quoted in #10.2 that includes $connection_options can be removed. But do not take my word for it: check that the variables $connection_options and $driver are not used later.

@benjifisher sorry for that.
Could you please review this patch.I have also remove unused code from "testMigrateUpgradeExecute"

Yes $connection_options and $driver are used in $this->drupalPostForm(NULL, [$driver . '[database]' => 'wrong'] + $edits, t('Review upgrade'));

$driver = $connection_options['driver'];

So we can not remove this.

@pavnish:

I am not sure what you mean by this:

Yes $connection_options and $driver are used in $this->drupalPostForm(NULL, [$driver . '[database]' => 'wrong'] + $edits, t('Review upgrade'));

@benjifisher sorry for late reply .I mean that the $connection_options and $driver are using in code so we cannot remove this.

Looking at it again, I see that you are right.

We should at least rearrange those lines:

    // Ensure submitting the form with invalid database credentials gives us a
    // nice warning.
    $connection_options = $this->sourceDatabase->getConnectionOptions();
    $driver = $connection_options['driver'];
    $this->drupalPostForm(NULL, [$driver . '[database]' => 'wrong'] + $edits, t('Review upgrade'));

We can combine the first two lines, eliminating the $connection_options variable. For the purposes of this test, do we need to use the same $driver or could we just use 'mysql[database]' => 'wrong'? Or should I not worry about it--is this one of the tests we plan to remove in #3143720: Create a separate CredentialFormTest?

This test is close to the situation I described in #8.2, where we want to modify the value we get from getCredentials(). In this case, we are not modifying a nested key, so we can work with the flattened array.

OK. We will be moving the code, not removing it, but we can clean it up as part of the move if you prefer. RTBC.

Committed 4df1ca4 and pushed to 9.1.x. Thanks!