Make migrate_upgrade compatible with Drush 10.

Lovely to see someone happy to push forward a Drush 10 patch. Please throw up patches :)

This isn't a complete patch, but it's a start to maybe inject LoggerChannelFactoryInterface and then LoggerChannelInterface into MigrateUpgradeCommands and MigrateUpgradeDrushRunner respectively. The latter doesn't have a service since the upgrade commands acts like a factory. For bc, I added \Drush\Drush::logger(), which is also deprecated, into the migrate upgrade drush include file.

Not sure if this works at all at the moment, but might as well upload and see. Marking as Needs work.

After applying patch (#3) I get the following error on running the command:

$ drush migrate-upgrade --legacy-db-key upgrade --configure-only

[error] ArgumentCountError: Too few arguments to function Drupal\migrate_upgrade\Commands\MigrateUpgradeCommands::__construct(), 1 passed in /var/www/drupal8/web/core/lib/Drupal/Component/DependencyInjection/Container.php on line 269 and exactly 2 expected in Drupal\migrate_upgrade\Commands\MigrateUpgradeCommands->__construct() (line 42 of /var/www/drupal8/web/modules/contrib/migrate_upgrade/src/Commands/MigrateUpgradeCommands.php) #0 /var/www/drupal8/web/core/lib/Drupal/Component/DependencyInjection/Container.php(269): Drupal\migrate_upgrade\Commands\MigrateUpgradeCommands->__construct(Object(Drupal\Core\State\State))

NB: Using Drupal/Core version 8.7.10, Migrate_upgrade 8.x-3.1, and Drush 10.0.2.

Confirming this is still a problem.

After a lot of trial and error and grepping the Drush code base (because if Drush has documentation for how to upgrade from deprecated code paths to new code paths, I couldn't find it), it turned out to be pretty simple. And #3 was already super close :)

Thank you for fixing up the patch!

+++ b/tests/src/Unit/MigrateUpgradeDrushRunnerTest.php
@@ -27,7 +27,8 @@ class MigrateUpgradeDrushRunnerTest extends MigrateTestCase {
-    $runner = new TestMigrateUpgradeDrushRunner();
+    $loggerProphet = $this->prophesize('\Drupal\Core\Logger\LoggerChannelInterface');
+    $runner = new TestMigrateUpgradeDrushRunner([], $this->loggerProphet->reveal());

Do we need to fix this as well to be a direct mock of Drush\Log\Logger? I guess if it passes test, then no.

Changed to Needs work based on test bot results.

1. +++ b/drush.services.yml
   @@ -1,6 +1,6 @@
   +    arguments: ['@state', '@logger.factory']
   
   +++ b/src/MigrateUpgradeDrushRunner.php
   @@ -84,14 +86,24 @@ class MigrateUpgradeDrushRunner {
   +  public function __construct(array $options = [], Logger $logger) {
   

   Optional arguments should be at the end, but I don't think we can change the order. So maybe Logger should be NULL and then add NULL checks?

   Or if we can change the arguments of the runner, then we can swap these around. That would also require updating all the calls to MigrateUpgradeDrushRunner.

2. +++ b/src/MigrateUpgradeDrushRunner.php
   @@ -13,6 +13,8 @@ use Drupal\migrate_drupal\MigrationConfigurationTrait;
   +use Drupal\Core\Logger\LoggerChannelInterface;
   

   Testbot mentions an unused use statement. I think this is the one.

3. +++ b/src/MigrateUpgradeDrushRunner.php
   @@ -84,14 +86,24 @@ class MigrateUpgradeDrushRunner {
   +   * @var \Drupal\Core\Logger\LoggerChannelInterface
   

   And this probably should be updated to \Drush\Log\Logger

+++ b/tests/src/Unit/MigrateUpgradeDrushRunnerTest.php
@@ -27,7 +27,8 @@ class MigrateUpgradeDrushRunnerTest extends MigrateTestCase {
+    $loggerProphet = $this->prophesize('\Drupal\Core\Logger\LoggerChannelInterface');
+    $runner = new TestMigrateUpgradeDrushRunner([], $this->loggerProphet->reveal());

My bad code. I assigned $loggerProphet to a local variable and then try to use it as an object property.

Thank you -- patch #6 did the trick. All good now.

Or if we can change the arguments of the runner, then we can swap these around. That would also require updating all the calls to MigrateUpgradeDrushRunner.

I think this is preferable :) I don't think we need to provide backwards compatibility here.

Want to address your own feedback? I'm happy to review & RTBC!

Sure. I found that in #3 I needed to typehint to the PSR interface because Drush\Log\Logger doesn't implement LoggerChannelInterface.

Let's see if this passes on the test bot.

I think this is probably Major now that 8.8.0 is released.

At least the tests are running now for the patch!

+++ b/src/MigrateUpgradeDrushRunner.php
@@ -84,14 +85,24 @@ class MigrateUpgradeDrushRunner {
+  public function __construct(Logger $logger, array $options = []) {

+++ b/tests/src/Unit/MigrateUpgradeDrushRunnerTest.php
@@ -27,7 +28,8 @@ class MigrateUpgradeDrushRunnerTest extends MigrateTestCase {
+    $loggerProphet = $this->prophesize('\Drush\Log\Logger');

@@ -186,8 +188,8 @@ class TestMigrateUpgradeDrushRunner extends MigrateUpgradeDrushRunner {
+  public function __construct(Logger $logger, array $options = []) {

Type probably needs to be \Psr\Log\LogInterface in the function definition as well to both fit Drush\Log\Logger and LoggerChannelInterface. I think Drush's DrupalLogAdapter does some sort of handling to transform Drupal's logs?

I tried to look into doing a bit more of a functional test in the DrushTest rather than mocking it, but I think that's not possible without actually having Drush around to add its DrupalLogAdapter as an available logger. "ok" level is only available when using drush and not in core, but "success" should only be used once per drush command.

This patch changes the typings of Drush\Log\Logger to the generic Psr LoggerInterface.

drush 10 should be available, at least on the testbot. See https://git.drupalcode.org/project/migrate_upgrade/blob/8.x-3.x/composer...

migrate tools has more fully-baked test coverage using the same dependencies. See https://git.drupalcode.org/project/migrate_tools/blob/8.x-4.x/tests/src/... as an example.

Oh, nice! Thank you, @heddn. I think a new browser test like that would probably help raise confidence in the patch to make sure that Drush's Logger is being used appropriately.

I think this test will fail miserably, but its what I got to in the time I had. Perhaps it will let the next person move it along further.

Tested this patch using the core D7 source fixture as the source, minimal D8 install, enabled the necessary modules.

$ drush --version
Drush Commandline Tool 10.1.1
$ drush migrate-upgrade --legacy-db-key='d7_dump' --migration-prefix='' 
 [notice] Upgrading action_settings
 [notice] Upgrading book_settings
 [notice] Upgrading contact_category
 [notice] Upgrading d6_book_settings
 [notice] Upgrading d7_action
 [notice] Upgrading d7_aggregator_feed
 [notice] Upgrading d7_aggregator_settings
 [notice] Upgrading d7_blocked_ips

In Container.php line 153:
                                                                                         
  You have requested a non-existent service "logger.channel.migrate_upgrade". Did you m  
  ean this: "logger.channel.migrate_drupal"?                                             
                                                                                         

A quick fix for #18. Doesn't fix any of the tests. That will have to be another day or someone else.

NW because tests are still failing.

After applying patch #19, 'drush rq' command crashes drush 10 with the following output.......

ArgumentCountError: Too few arguments to function Drupal\migrate_upgrade\Commands\MigrateUpgradeCommands::__construct(), 1 passed in /var/www/fading/afrocentric/public_html/core/lib/Drupal/Component/DependencyInjection/Container.php on line 269 and exactly 2 expected in Drupal\migrate_upgrade\Commands\MigrateUpgradeCommands->__construct() (line 42 of /var/www/fading/afrocentric/public_html/modules/contrib/migrate_upgrade/src/Commands/MigrateUpgradeCommands.php).
[warning] Drush command terminated abnormally.

trying patch #6.................Patch does not apply as #19 covers and extends what #6 does

thanks @mradcliffe..................your suggestion made my day

I think your service container needs to be rebuilt. Try drush cr.

Just noting that I bumped into this following the official Drupal upgrade migration guide on Drupal.org, which recommends installing Drush via composer (which currently gives 10.2.x), then installing the three required migrate modules, then running drush migrate-upgrade. I had to Google the error message which eventually brought me here.

Should we update the migration guide, for now, to specify Drush 9 when running the composer require command, since Drush 10 definitely won't work at all until this issue is fixed? Otherwise it's another frustrating bump in the road for users following the guide.

Upgrade guide doc in question: https://www.drupal.org/docs/8/upgrade/upgrade-using-drush

I have added a warning under https://www.drupal.org/docs/8/upgrade/upgrade-using-drush#s-installing-d....

Thanks for reporting this @geerlingguy. I am really looking forward to the next livestream in your Migrating JeffGeerling.com from Drupal 7 to Drupal 8 - How-to video series :)

I am starting to look at this issue. Here are some initial questions/comments based on the patch from #19.

1. --- a/composer.json
   +++ b/composer.json
   @@ -3,6 +3,23 @@
   

   The changes to composer.json are not a bad thing, but they seem to be outside the scope of this issue.

2. --- a/migrate_upgrade.drush.inc
   +++ b/migrate_upgrade.drush.inc
   

   This issue is about Drush 10, so it is surprising to see changes to this file. I guess these are needed because of the other changes in the patch.

3. --- a/src/Commands/MigrateUpgradeCommands.php
   +++ b/src/Commands/MigrateUpgradeCommands.php
   @@ -184,7 +196,7 @@ class MigrateUpgradeCommands extends DrushCommands {
      public function upgradeRollback() {
        if ($date_performed = $this->state->get('migrate_drupal_ui.performed')) {
          if ($this->io()->confirm(dt('All migrations will be rolled back. Are you sure?'))) {
   -        $runner = new MigrateUpgradeDrushRunner();
   +        $runner = new MigrateUpgradeDrushRunner($this->logger, []);
   

   (and similar changes elsewhere). The second argument of the constructor defaults to []. Why not use $runner = new MigrateUpgradeDrushRunner($this->logger)?

I hope to have a more substantive comment and/or an updated patch within a week.

I have spent more time than I can really afford trying to debug the test failure with the patch in #19. I do not have a solution, but here is what I found.

I added debugging code like

    print_r(array_keys(['before' => 0] + Database::getAllConnectionInfo()));

to the test classes (migrate_upgrade/tests/src/Kernel/DrushTest.php and migrate_upgrade/tests/src/Functional/MigrateUpgradeCommandsTest.php). I added a similar line ("after" instead of "before") to migrate_upgrade/src/Commands/MigrateUpgradeCommands.php.

The kernel test passes. My debugging lines show similar "before" and "after" results: the same database keys are available before the test calls MigrateUpgradeCommands::upgrade() and when that function executes.

The functional test is the one that fails. The "before" debug output shows the expected database keys (including migrate_drupal_ui), but the "after" result shows only the default key.

Since Database::getAllConnectionInfo() returns the value of a static class property, I think that a new PHP process (or thread or whatever) is being created, which means that the static property gets lost.

The difference between the two tests is that the kernel test calls MigrateUpgradeCommands::upgrade() directly, but the functional test uses Drush\TestTraits\DrushTestTrait::drush() to run the command.

Let's see if this gets us a little closer.

Yeah, the blocker in the test is now fixed. Just need to match up assertions.

I'm confused as the failures surfacing on the test bot aren't surfacing on local. Meaning, the are green and I can't reproduce the failures. Will leave for now and hope someone else has suggestions.

I get the same failures locally that the testbot shows.

I tried hacking the test, adding

    $output = $this->getOutputFromJSON();
    $this->assertEqual(6, count($output));
    $this->assertEqual([], $output);

I got output like

1) Drupal\Tests\migrate_upgrade\Functional\MigrateUpgradeCommandsTest::testDrupal6Upgrade
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
 Array (
-    'upgrade_block_content_type' => Array (...)
-    'upgrade_block_content_body_field' => Array (...)
-    'upgrade_block_content_entity_display' => Array (...)
-    'upgrade_block_content_entity_form_display' => Array (...)
-    'upgrade_user_picture_field' => Array (...)
-    'upgrade_d6_upload_field' => Array (...)

I also added

    $output = $this->getOutputFromJSON();
    $this->assertEqual([], $output['upgrade_block_content_type']);

and got

2) Drupal\Tests\migrate_upgrade\Functional\MigrateUpgradeCommandsTest::testDrupal7Upgrade
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
 Array (
-    'original' => 'block_content_type'
-    'generated' => 'upgrade_block_content_type'

It is a little disconcerting that the -/+ markers for Expected/Actual seem to be backwards, but this seems to be saying that $this->getOutputFromJSON() really is an array with just 6 entries. Why only 6? Why these 6? Why the same 6 for the D6 test and the D7 test, including upgrade_d6_upload_field?

Maybe we are not connecting to the database we think we are connecting to. If I have time to work on this tomorrow, then I will explore that possibility.

I did a little manual testing. I started by setting up an extra database and importing the fixture, following the instructions on Step 3: Create a database and Generating database fixtures for D8 Migrate tests:

mysql ...
> CREATE DATABASE drupal_fixture CHARACTER SET utf8mb4 COLLATE utf8mb4_unicode_ci;
> GRANT SELECT, INSERT, UPDATE, DELETE, CREATE, DROP, INDEX, ALTER, CREATE TEMPORARY TABLES ON drupal_fixture.* TO 'drupal8'@'%' IDENTIFIED BY 'drupal8';
> \q
php core/scripts/db-tools.php import --database fixture_connection core/modules/migrate_drupal/tests/fixtures/drupal6.php

Then I ran the migrate:upgrade command:

drush mup --format=json --fields=* --legacy-db-key=fixture_connection --configure-only

I got many copies of the warning

[notice] Field discovery failed for Drupal core version 7. Did this site have the CCK or Field module installed? Error: The module field is not enabled in the source site.

but I figure that I can ignore those because I imported the D6 fixture.

Then I got the expected output: more lines than I care to count, but nothing for the action nor book modules. Then I enabled those modules and ran the command again. This time, the output (after the warning messages) started with

{
    "upgrade_action_settings": {
        "original": "action_settings",
        "generated": "upgrade_action_settings"
    },
    "upgrade_book_settings": {
        "original": "book_settings",
        "generated": "upgrade_book_settings"
    },

This is very different from the output I got last night when running the test. Perhaps I was right then, and we are not connecting to the correct database.

I was right: adding this code

  public function testDrupal7Upgrade() {
    $this->loadFixture(drupal_get_path('module', 'migrate_drupal') . '/tests/fixtures/drupal7.php');
    $url = $this->convertDbSpecUrl($this->sourceDatabase->getConnectionOptions());
    $this->assertEqual('', $url);

leads to this output when running the test:

2) Drupal\Tests\migrate_upgrade\Functional\MigrateUpgradeCommandsTest::testDrupal7Upgrade
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'mysql://drupal8:drupal8@database/drupal8'
+''

I am using Lando, and that is the db-url to connect to the default database.

Thanks @benjifisher. That makes sense and helps a lot. Here's another try. It passed green locally.

Re-adding some drush 8 BC logic as it doesn't hurt and I'd like to keep support for it as long as possible.

And here we re-add the export printing (in another location) for drush 8 continued support. I can't see anything else in my self-review.

Drush 10 is still not ready. Why is 8.x-3.1 green? The green version should be 8.x-3.0 because that actually works with Drush 9.

I don't understand #51. Are we trying to say that you can't use Drush 10 on your site? Or... what. It isn't clear what is intended by Drush 10 not being ready. Because it is ready and 10.2+ is already released. And Drush 9 has only a few more months of support left on it.

Or are you saying that the patches here don't make the module support drush 10?

The patch in #50 is working well for me, but I have a few suggestions for cleaning it up.

1.   +++ b/composer.json
     @@ -3,6 +3,23 @@
     ...
     +    "authors": [
     +        {
     +            "name": "Mike Ryan",
     +            "homepage":"https://www.drupal.org/u/mikeryan",
     +            "role": "Maintainer"
     +        },
     +        {
     +            "name": "Lucas Hedding",
     +            "homepage": "https://www.drupal.org/u/heddn",
     +            "role": "Maintainer"
     +        }
     +    ],
     +    "support": {
     +        "issues": "https://www.drupal.org/project/issues/migrate_upgrade",
     +        "slack": "#migrate",
     +        "source": "https://git.drupalcode.org/project/migrate_upgrade"
     +    },

   As I said in #27, this is out of scope, but that is OK as far as I am concerned. I checked the links. I have not seen the slack key before, but that does seem like the primary form of support.

2.   +++ b/migrate_upgrade.services.yml
     @@ -0,0 +1,5 @@
     +services:
     +  logger.channel.migrate_upgrade:
     +    class: Drupal\Core\Logger\LoggerChannel
     +    factory: logger.factory:get
     +    arguments: ['migrate_upgrade']

   The change record recommends the pattern

     services:
       logger.channel.mymodule:
         parent: logger.channel_base
         arguments: ['mymodule']

   Maybe it is worth doing it this way.

3.   @@ -23,14 +26,25 @@ class MigrateUpgradeCommands extends DrushCommands {
         */
        protected $state;
   
     +  /**
     +   * A PSR-compatible Logger shared between Drupal and Drush.
     +   *
     +   * @var \Psr\Log\LoggerInterface
     +   */
     +  protected $logger;

   In what sense is it "shared"? Maybe I have it wrong, but I think that Drush injects this class when we use a Drush command, and Drupal injects it when we run the unit test (not the functional test). If so, then it would be more accurate to say "A PSR-compatible Logger provided [or injected] by Drupal or Drush". Maybe "PSR-3 compatible".

4. The code looks reasonable, but I do not know the Drush output routines well enough to review the other changes in this file.

5.   +++ b/src/DrushLogMigrateMessage.php
     @@ -2,14 +2,35 @@

   Same thing here. There have not been any changes to this file since at least Comment #19.

6.   +++ b/src/MigrateUpgradeDrushRunner.php
     @@ -84,14 +84,24 @@ class MigrateUpgradeDrushRunner {
     ...
     -  public function __construct(array $options = []) {
     +  public function __construct(LoggerInterface $logger, array $options = []) {
          $this->setOptions($options);
     +    $this->logger = $logger;

   Nit: can we handle the parameters in the same order that they are defined?

7.   +++ b/tests/src/Functional/MigrateUpgradeCommandsTest.php
     @@ -0,0 +1,195 @@
     ...
     +  public function testDrupal6Upgrade() {
     ...
     +    foreach ($expected as $key => $subset) {
     +      $this->assertArraySubset([$key => $subset], $this->getOutputFromJSON(), TRUE);
     +    }

   I think we can go back to

         $this->assertArraySubset($expected, $this->getOutputFromJSON(), TRUE);

8.   +  /**
     +   * Test module using Drupal 7 fixture.
     +   */
     +  public function testDrupal7Upgrade() {

   Since the tests are so similar, can we combine them? We could use a data provider to supply the path to the fixture, the expected output (or not, since that dcoes not change) and so on.

9. Nit:

    +  /**
    +   * Prepare and execute migrations.
    +   */
    +  protected function executeMigrations() {

   Since this method sets the configure-only option, I would call the method configureMigrations() and change the one-line description to something like "Generate migration config[uration] entities".

10. Nit:

     +    $optional = array_flip($migrate_plus_migrations['upgrade_d7_url_alias']->toArray()['migration_dependencies']['optional']);

    I guess that long lines bother me more than most people. I prefer something like

         $optional = array_flip(
           $migrate_plus_migrations['upgrade_d6_url_alias']
             ->toArray()['migration_dependencies']['optional']
         );

11.   @@ -27,7 +28,8 @@ class MigrateUpgradeDrushRunnerTest extends MigrateTestCase {
      ...
         public function testIdSubstitution(array $source, array $expected) {
      -    $runner = new TestMigrateUpgradeDrushRunner();
      +    $loggerProphet = $this->prophesize('\Psr\Log\LoggerInterface');
      +    $runner = new TestMigrateUpgradeDrushRunner($loggerProphet->reveal(), []);

    I think this is the only remaining place where the constructor is called with the optional parameter explicitly specified.

Thanks for the review. I think this addresses all the review feedback in #53.

Why doesn't the testbot set the status to NW after "PHPLint Failed"?

This seems to be the problem (from the interdiff):

+++ b/tests/src/Functional/MigrateUpgradeCommandsTest.php
@@ -39,11 +39,13 @@ class MigrateUpgradeCommandsTest extends MigrateUpgradeTestBase {
...
-  public function testDrupal6Upgrade() {
-    $this->loadFixture(drupal_get_path('module', 'migrate_drupal') . '/tests/fixtures/drupal6.php');
-    $this->executeMigrations();
+  public function testDrupalConfi gureUpgrade($drupal_version) {
+    $this->loadFixture(drupal_get_path('module', 'migrate_drupal') . "/tests/fixtures/drupal{$drupal_version}.php");
+    $this->executeMigrateUpgrade(['configure-only' => NULL]);

There is a stray space in the function name.

+  public function majorDrupalVersionsDataProvider() {
+    $version = [];
+    $version['drupal 6'][] = 6;
+    $version['drupal 7'][] = 7;
+    return $version;

Why not simply

  $version = [
    'drupal 6' => 6,
    'drupal 7' => 7,
  ];

We want the values to be 6 and 7, not [6] and [7], right? Or maybe "6" and "7".

Also, I snipped the doc block, but it needs an @param comment.

re #56: I think we want it the way it is. The arguments for a data provider are an array of values. If we do it the other way, phpunit will fail and complain.

Fixed the php lint issue.

Thanks for the updates since my review in #53. I have a collection of nits and suggestions, but I will defer to you on the suggestions if you do not like them.

1.   +  public function majorDrupalVersionsDataProvider() {
     +    $version = [];
     +    $version['drupal 6'][] = 6;
     +    $version['drupal 7'][] = 7;
     +    return $version;
     +  }

   I think a corrected version of my suggestion in #56 is simpler:

     return [
       'drupal 6' => [6],
       'drupal 7' => [7],
     ];

2.   +  /**
     +   * Execute Drush migrate:upgrade command
     +   */
     +  protected function executeMigrateUpgrade(array $options = []) {
     +    $connection_options = $this->sourceDatabase->getConnectionOptions();
     +    $url = $this->convertDbSpecUrl($connection_options);
     +    $options += [
     +      'legacy-db-url' => $url,
     +      'format' => 'json',
     +      'fields' => '*',
     +    ];
     +    if (!empty($connection_options['prefix']['default'])) {
     +      $options['legacy-db-prefix'] = $connection_options['prefix']['default'];
     +    }
     +    $this->drush('migrate:upgrade', [], $options);
     +  }

   Nit: the one-line description needs ending punctuation.

3. The $options parameter should have an @param comment. Keep in mind the next point and your response.

4. It seems inconsistent to let the calling function override legacy-db-url but not legacy-db-prefix. I might not allow either to be overridden. This snippet also removes the $url parameter:

      protected function executeMigrateUpgrade(array $options = []) {
        $options += [
          'format' => 'json',
          'fields' => '*',
        ];
        $connection_options = $this->sourceDatabase->getConnectionOptions();
        $options['legacy-db-url'] = $this->convertDbSpecUrl($connection_options);
        if (!empty($connection_options['prefix']['default'])) {
          $options['legacy-db-prefix'] = $connection_options['prefix']['default'];
        }
        $this->drush('migrate:upgrade', [], $options);
      }

5. Sorry, I forgot to look at this function in my earlier review:

    +  /**
    +   * Asserts that all migrations are exported as migrate plus entities.
    +   *
    +   * @param \Drupal\migrate\Plugin\MigrationInterface[] $migrations
    +   *   The migrations.
    +   * @param \Drupal\migrate_plus\Entity\MigrationInterface[] $migrate_plus_migrations
    +   *   The migrate plus config entities.
    +   */
    +  protected function assertMigrations(array $migrations, array $migrate_plus_migrations) {
    +    // Filter to remove migrations that have an embedded data source and
    +    // therefore are always available.
    +    $available_migrations = array_unique(array_map(static function (MigrationInterface $migration) {
    +      $prefix = 'upgrade_';
    +      $migration_id = $prefix . str_replace(PluginBase::DERIVATIVE_SEPARATOR, '_', $migration->id());
    +      if ((strpos($migration->id(), $prefix) === 0)) {
    +        $migration_id = $migration->id();
    +      }
    +      return $migration_id;
    +    }, $migrations));
    +    $this->assertCount(count($available_migrations), $migrate_plus_migrations);
    +    foreach ($available_migrations as $migration_id) {
    +      $this->assertArrayHasKey($migration_id, $migrate_plus_migrations);
    +    }
    +  }

   In the one-line comment, I would use either migrate_plus or "Migrate Plus".

   It looks as though the code comment about embedded data sources was copied from somewhere else.

   I do not like the pattern of assigning $migration_id and then conditionally reassigning it. We could use if … then … else or a ternary operator or

          $migration_id = $migration->id();
          if ((strpos($migration_id, $prefix) !== 0)) {
            $migration_id = $prefix . str_replace(PluginBase::DERIVATIVE_SEPARATOR, '_', $migration_id);
          }

   Finally, if we replace array_unique() with array_flip(), then we could replace the foreach loop with an assertion that array_diff_key(...) is empty.

Addressed all the feedback in #59 and committed. Thanks for all the help on this one. Lots of good feedback to make things better.

@heddn: Sorry for the late reply. My point was the 8.x-3.1 version of this module does not work with Drush 10 without patching so it shouldn't be marked green implying it is the "stable" version.

This issue is mentioned on https://www.drupal.org/docs/8/upgrade/upgrade-using-drush, and recommends users to install drush 9 instead. I suppose that note can be removed when a new release is tagged?

Thanks to everyone for fixing this issue. Would a fresh release be worth considering? It seems like the exclude modules from the configuration synchronization feature introduced in Drupal 8.8 via something like this in settings.php:

$settings['config_exclude_modules'] = ['devel', 'stage_file_proxy'];

.... only works with Drush 10, which I would prefer, over the more complex solution Configuration Split offers.

Tests for the module are currently failing. Mainly because of core failures introduced by #2746541: Migrate D6 and D7 node revision translations to D8 and being fixed in #3125763: Hidden dependency on migrate_drupal from node module when only migrate.module is enabled. Once we can get non-failing tests again, I'm more then happy to release a new tag.

That sounds great @heddn, thanks! Both issues look epic with a lot of work getting done, so thanks also to everybody involved in these two other issues.

#3125763: Hidden dependency on migrate_drupal from node module when only migrate.module is enabled was just committed and awaits back-porting, so one down, and one to go :-)

#3125763 was committed to the 8.9.x, 9.0.x, and 9.1.x branches a little while ago.

#2746541 was committed to 8.9.x and 9.0.x more than two weeks ago. (Maybe that was before 9.1.x branched from 9.0.x.)

Both issues are now fixed on those branches. Both are waiting to be back-ported to 8.8.x.

Can someone provide a patch providing the fix that was actually committed? It looks like in #61 @heddn addressed some feedback and committed the code without a new patch.

Or is it safe to use patch #59 on the current tagged release?

I just upgraded to the latest Drupal core and Migrate releases, and can confirm they work with Drush 10:

$ drush st
Drupal version   : 8.9.0                             
Drush version    : 10.2.2                            

$ drush pm-list --type=Module --status=enabled --package=Migration
 ----------- ---------------------------------- --------- --------- 
  Package     Name                               Status    Version  
 ----------- ---------------------------------- --------- --------- 
  Migration   Migrate (migrate)                  Enabled   8.9.0    
  Migration   Migrate Drupal (migrate_drupal)    Enabled   8.9.0    
  Migration   Migrate Plus (migrate_plus)        Enabled   8.x-5.1  
  Migration   Migrate Tools (migrate_tools)      Enabled   8.x-5.0  
  Migration   Drupal Upgrade (migrate_upgrade)   Enabled   8.x-3.2  
 ----------- ---------------------------------- --------- ---------