Drush 9 port of commands

Hello,
Thanks for this patch. We use drush 9 too wo we really need this for the beta5 version.

I think you forgot to replace just one drush_log by $this->logger on line 74 of MigrateToolsCommand.php

67	      foreach ($migration_list as $migration_id => $migration) {
68	        try {
69	          $map = $migration->getIdMap();
70	          $imported = $map->importedCount();
71	          $source_plugin = $migration->getSourcePlugin();
72	        }
73	        catch (Exception $e) {
74	          drush_log(dt('Failure retrieving information on @migration: @message',
75	            ['@migration' => $migration_id, '@message' => $e->getMessage()]));
76	          continue;
77	        }
78	        if ($names_only) {
79	          $table[] = [$migration_id];
80	        }

Thanks so much for doing this! I have received requests for these commands.

1. $this->logger()->info(dt('No migrations found.'), 'error');. The error part at the end is a mistake I think.
2. $this->logger()->error() is used in a few places. I think you want to throw an exception instead.
3. Replace drush_get_option() with $options. I added this point to my blog post.
4. To return a table, use output format system. Here is an example. The code you are porting should have used output format system even in prior versions of Drush.

This patch doesn't work for me.

1) If I use the "mi" alias instead of "migrate-import": drush mi --group=[group_name] it says:

[error] You must specify --all, --group, --tag or one or more migration names separated by commas

2) If I use the complete command with --group or even --all: drush migrate-import --all or drush migrate-import --group=[group_name], it says:

[error] You must specify --all, --group, --tag or one or more migration names separated by commas

Versions:

Drush 9.0.0-beta5
Drupal 8.4-rc2
Migrate_Tools: Tested with 4.0.0-beta1 and 4.x-dev patched with patch #4

Commands should be renamed to @command migrate:status. Then add an alias to the original name: @aliases mi,migrate-status. I will update the generator for this.

Here is a new patch. For now I didn't replace any drush_log('', 'error') by Exception (but I replaced drush_set_error() calls by Exception as recommended by Drush portng doc), just by $this->logger()->error(), but feel free to do so for a future patch.

Sorry bad patch on previous comment.

Btw, I replaced "mi" alias for migrate:import (or migrate-import) by "mim", as "mi" is considered ambiguous with all the migrate: commands by Drush 9.

Hi guys,
thanks for your work but last patch doesn't work for me, when I run `migrate:status` I can't see any migrations.
In the method migrationsList I replaced the service plugin.manager.migration with plugin.manager.config_entity_migration that is the one used by the old drush commands.
With this service all commands work as expected and I can list/import/revert my migrations.

Drush 9.0.0-beta5
Drupal 8.4.0
Migrate_Tools: Tested with 4.0.0-beta1/4.x-dev patched with patch #10

@fancescoben

In the method migrationsList I replaced the service plugin.manager.migration with plugin.manager.config_entity_migration that is the one used by the old drush commands.

In which version of original Drush commands did you see that the service used on drush_migrate_tools_migration_list() was "plugin.manager.config_entity_migration" and not "plugin.manager.migration".

I checked on both latest GIT code and beta1 archive and on both the service used in this function is "plugin.manager.migration". In fact I just did a simple copy/paster from this function and applied port instructions to it to create the migrationsList method:

function drush_migrate_tools_migration_list($migration_ids = '') {
  // Filter keys must match the migration configuration property name.
  $filter['migration_group'] = drush_get_option('group') ? explode(',', drush_get_option('group')) : [];
  $filter['migration_tags'] = drush_get_option('tag') ? explode(',', drush_get_option('tag')) : [];

  $manager = \Drupal::service('plugin.manager.migration');
  $plugins = $manager->createInstances([]);
  $matched_migrations = [];

  .../...
}

On my side #10 works for status on both beta1 and 4.x-dev with Drupal 8.4.0. Are you you are on Drupal 8.4 or even 8.3.x ? The service plugin.manager.config_entity_migration has been removed from Drupal Core since 8.3.5 (if I remember correctly) and replaced by plugin.manager.migration.

So your patch is not valid for unpatched Drupal 8.3.5+ core.

Btw, I prepare a new version of my Patch with dependency injection (instead of using \Drupal::service()).

Patch #12 does not work, I got this error:
[error] You have requested a non-existent service "plugin.manager.config_entity_migration".

Patch #10 does work for me.

Here a new version of my patch with the following changes:

• Using dependency injection instead of \Drupal::service() static calls for services instances.
• Fix output formatting of migrate-status by adding missing @field-labels and @default-fields annotation required by RowsOfField output formatter
• Code reformatting to respect Drupal coding standards
• Revert use of RowsOfField to drush_print_table for migrate:messages and migrate:fields-source are I don't know what @field-labels and @default-fields annotations to use because the output of these commands are heavily dynamic. I tried to use the FormatterManager directly as described here: https://github.com/consolidation/output-formatters (API Usage chapter), but didn't achieve to get a good output. Help wanted on this point :)

1. +++ b/src/Commands/MigrateToolsCommands.php
   @@ -0,0 +1,700 @@
   +      $group = MigrationGroup::load($group_id);
   

   You should use entity type manager to load entities

2. +++ b/src/Commands/MigrateToolsCommands.php
   @@ -0,0 +1,700 @@
   +        $configured_group_id = empty(
   +        $migration->get(
   +          'migration_group'
   +        )
   +        ) ? 'default' : $migration->get('migration_group');
   +        $migrations[$configured_group_id][$id] = $migration;
   

   i think it could be more readable on one line

3. +++ b/src/Commands/MigrateToolsCommands.php
   @@ -0,0 +1,700 @@
   +        dt(
   +          'You must specify --all, --group, --tag or one or more migration names separated by commas'
   +        )
   

   same thing here, no needs to mulitiple lines

4. +++ b/src/Commands/MigrateToolsCommands.php
   @@ -0,0 +1,700 @@
   +      drush_set_error(
   +        'MIGRATE_ERROR',
   +        dt(
   +          'You must specify --all, --group, --tag, or one or more migration names separated by commas'
   +        )
   +      );
   

   same thing here. There is a lot of things like that. Do not make a new line each time you call a function.

New patch version taking code review comments in account. There will probably still be some formatting stuff, but I had applied PHPStorm reformat action that (stupidly) apply Drupal coding standards.

@bpresles - my suggestion is to make the output of these commands less dynamic. You might need to break them into multiple commands. i have not looked into it.

I uploaded the interdiff of patches (between #10 and #15-2, and between #15-2 and #18).

The patch in #18 does the job but with some notices: Notice: Undefined index: execute-dependencies in MigrateToolsCommands.php on line 673

1. +++ b/src/Commands/MigrateToolsCommands.php
   @@ -0,0 +1,701 @@
   +  public function __construct(
   

   We should call the parent constructor even, right now, that one is doing nothing.

2. +++ b/src/Commands/MigrateToolsCommands.php
   @@ -0,0 +1,701 @@
   +    $log = new Drush9LogMigrateMessage($this->logger());
   

   Right now we are duplicating code from the the legacy migrate_tools.drush.inc. This is hard to maintain. IMO, we should deprecate the procedural functions. For example:

   /**
    * @param string $migration_names
    * 
    * @deprecated Use \Drupal::service('migrate_tools.commands')->status() instead.
    */
   function drush_migrate_tools_migrate_status($migration_names = '') {
     \Drupal::service('migrate_tools.commands')->status($migration_names, [
       'tag' => drush_get_option('tag'),
       'names-only' => drush_get_option('names-only'),
     ]);
   }
   

   Same for all commands. Then we would face the problem with the message logger because we need a different one for drush < 9. My suggestion would be to add protected $migrateMessage property and set it in constructor

   if (Drush::getMajorVersion() >= 9) {
         $this->migrateMessage = new Drush9LogMigrateMessage($this->logger());
       }
       else {
         $this->migrateMessage = new DrushLogMigrateMessage();
       }
   

   When the BC layer will be removed, we remove also this part and we inject the log service.

   EDIT: Lately, I found that $this->logger() returns nothing in this stage. Weird.

   EDIT2: Then we can use this lazy getter:

     /**
      * Gets the migrate message logger.
      *
      * @return \Drupal\migrate\MigrateMessageInterface
      *   The migrate message service.
      */
     protected function getMigrateMessage() {
       if (!isset($this->migrateMessage)) {
         if (Drush::getMajorVersion() >= 9) {
           $this->migrateMessage = new Drush9LogMigrateMessage($this->logger());
         }
         else {
           $this->migrateMessage = new DrushLogMigrateMessage();
         }
       }
       return $this->migrateMessage;
     }
   

Hi @claudiu.cristea, not sure why you've hidden patch #21 as it fixed a trivial error notice.

I added the remarks you made in #23. Instead of doing the private method perhaps we could inject the logger in the command?

Marking for tests.

Forgot a use statement, added now.

1. +++ b/migrate_tools.drush.inc
   @@ -261,179 +178,51 @@ function drush_migrate_tools_migrate_import($migration_names = '') {
    function _drush_migrate_tools_execute_migration(MigrationInterface $migration, $migration_id,  array $options = []) {
   ...
   +  \Drupal::service('migrate_tools.commands')->executeMigration(
   +    $migration,
   +    $migration_id,
   +    ['execute-dependencies' => drush_get_option('execute-dependencies')]
   +  );
   

   This function was kind of "protected" (a helper) and we don't need to keep it anymore. Anyway a call to a service protected method would have failed.

2. +++ b/migrate_tools.drush.inc
   @@ -261,179 +178,51 @@ function drush_migrate_tools_migrate_import($migration_names = '') {
   +  \Drupal::service('migrate_tools.commands')->stop($migration_names);
   ...
   +  \Drupal::service('migrate_tools.commands')->resetStatus($migration_names);
   

   We pass $migration_id, right?

3. +++ b/migrate_tools.drush.inc
   @@ -447,71 +236,8 @@ function drush_migrate_tools_migrate_fields_source($migration_id) {
    function drush_migrate_tools_migration_list($migration_ids = '') {
   ...
   +  \Drupal::service('migrate_tools.commands')->migrationsList($migration_ids, [
   +    'group' => drush_get_option('group'),
   +    'tag' => drush_get_option('tag'),
   +  ]);
    }
   

   Again, how is suppose to work this call to a protected method? This function is also an internal/helper we should remove it from there.

4. +++ b/src/Commands/MigrateToolsCommands.php
   @@ -0,0 +1,718 @@
   +      $this->output()->writeln(dt('Group: @name', ['@name' => $group_name]));
   

   I wonder if the group name is placed in the correct position, given that all the rows are grouped together. Shouldn't the group be also a row (I don't know if we can do rowspan)?

5. +++ b/src/Commands/MigrateToolsCommands.php
   @@ -0,0 +1,718 @@
   +  protected function migrationsList($migration_ids = '', array $options = []) {
   

   We are using this method at least in 3 Drush commands, so let's move it after the commands, just before executeMigration().

6. +++ b/src/Drush9LogMigrateMessage.php
   @@ -0,0 +1,40 @@
   +    $this->logger->$type($message);
   

   This looks too dynamic.

EDIT: Please use .txt for interdiff so it won't get tested. Please read https://www.drupal.org/documentation/git/interdiff on how to create an interdiff.

More...

+++ b/src/Commands/MigrateToolsCommands.php
@@ -0,0 +1,718 @@
+        try {
+          $map = $migration->getIdMap();
+          $imported = $map->importedCount();
+          $source_plugin = $migration->getSourcePlugin();
+        }
+        catch (\Exception $e) {
+          $this->logger()->error(
...
+        }
+        if ($names_only) {
+          $table[] = ['id' => $migration_id];
+        }
+        else {
+          try {

This is wrong because we'll not benefit from 'names-only' speed. We should move if ($names_only) {...} outside so we don't parse the migration statistics if we don't need them. We only output the migration ID, right?

EDIT: And more...

+++ b/src/Commands/MigrateToolsCommands.php
@@ -0,0 +1,718 @@
+            $this->output->writeln($e->getMessage());
+            $this->logger()->error(
+              dt(
+                'Could not retrieve source count from @migration: @message',
+                ['@migration' => $migration_id, '@message' => $e->getMessage()]
+              )
+            );

We directly output the exception message and also drop that to the logger?

EDIT2: additional

+++ b/src/Commands/MigrateToolsCommands.php
@@ -0,0 +1,718 @@
+          $migrate_last_imported_store = \Drupal::keyValue(

The key-value store service should be injected too.

Using the Drush 9 Commands class in .drush.inc seems to be a good idea at first, but the fact is with Drush 8 the service declared in drush.servcices.yml won't be detected and usable at all. As well as the "Drush\Commands\DrushCommands" and "Drush\Drush" classes don't exist at all.

So the migrate_tools.drush.inc should be kept as is without any modification from its original version.

Find attached a new patch with the following changes:

- Injecting KeyValue store service.
- Added column "Group" to the Status table.
- Removed the $this->output->writeln($e->getMessage()) in status() method.
- Calculating the migrations statistics only if the names-only options is not passed.
- Moved migrationsList() method just before executeMigration() method.
- Rewritten Drush9LogMigrateMessage class so that it extends the MigrateMessage class and work similarly except that it uses the logger instance passed in the constructor (instead of the default 'migrate' logger)
- Reverted the migrate_tools.drush.inc file completely as using the Drush 9 Commands service won't work on Drush 8 at all. This may mean code duplication, but the drush.inc file is just a legacy file only kept for now for Drush 8 compatibility and so doesn't need to be maintained and will be removed anyway at the end.

New patch with the following changes:

• Fix Drush9LogMigrateMessage to not use the map of MigrateMessage as it's not compatible with Drush logger levels (so now the class doesn't extends MigrateMessage as it has nothing in common with it anymore), but keep the same philosophy
• Removed Drush version check on getMigrateMessage() method of MigrateToolsCommand class, as this service class is only compatible with Drush 9 or later and will not work on older versions

There's a tiny typo in your patch, you've got the "mi" alias in as "mim".

@pobster i think you miss the #10 comment

Btw, I replaced "mi" alias for migrate:import (or migrate-import) by "mim", as "mi" is considered ambiguous with all the migrate: commands by Drush 9.

You should add a diff when you post a patch so we can see your changes.

#34 should be ignored in favour of #33

An interdiff is a courtesy, not a requirement. Regardless of what way round it was, you shouldn't change the alias in one place and not the other ~ if you're going to change an alias, then change the alias for everyone. It's only going to lead to confusion where it differs.

Bugger, ignore that one ~ I committed the patch by mistake... This instead.

@pobster
I didn't change the alias in the drush.inc legacy file, because it's a legacy file and so should not break compatibility with current users. Current users, in Drush 8, use "mi" as alias for migrate-import.

So I changed the alias in new Drush 9 commands class, as users will expect possible changes/new stuff because of Drush 9. So in my pint of view, changing the alias in legacy .drush.inc file is not a good idea and it should only be done on new Drush 9 commands class. Just my point of view.

Ha, since when did Drupal ever worry about backwards compatibility?!

I'd consider it more breaking to be using a module, and for a drush command to break simply by updating the version of drush which is installed. You wouldn't expect updating drush to cause that kind of mayhem? Far better to have a command "break" when you update the related module, at least then your automated pipelines don't have such mysterious failures ~ especially in cases where a developer could test something on their machine, and yet once deployed the server might see failures due to the difference. It's far, far, far better to simply break it here first ~ it's the typical Drupal "ripping off the band-aid" approach to updates. After all ... this is exactly the reason you have to make this change in the first place right? You have an exact match on the command you're attempting to run, and yet you can't run it due to updates in drush which don't care about breaking backwards compatibility.

TL:DR; Drupal doesn't give a toss about legacy ;o)

@pobster
Major releases of Drupal don't care about backward compatibility, but minor releases do. That's why there are a lot of deprecated APIs on Drupal 8 (even deprecated API from alpha/beta or dev versions of Drupal 8) that will survive all the life span of Drupal 8 (so until Drupal 9).

As for the migrate_tools.drush.inc file, at the end this file will be removed all together, it's only useful for Drush 8 (it's totally ignored by Drush 9), and so replacing the alias for migrate-import is not a good idea. Adding the new "mim" alias to the existing "mi" one is ok though, but not replacing it altogether.

Breaking the compatibility for a major release is not a problem, as it's expected and generally practiced on the software industry (this is what Drupal, Drush, Symfony and most software projects do). But updating an old API (here drush 8 commands) to reflect a new one, breaking compatibility with old usage is not common at all (if not never seen) and is a bad practice.

In a nutshell: You may ADD the "mim" alias in the migrate_tools.drush.inc file, in addition to the "mi" existing one and display a deprecated messaged when "mi" is used, for helping to the transition, but removing the "mi" alias altogether to replace it by the new one, in this legacy .drush.inc file is clearly not a good idea.

I'll post a patch that does just that (add the "mim" alias additionally to the existing "mi" one and display a deprecated message when the "mi" alias is used).

Here the patch with the following changes:

- Puts back the "mi" alias alongside the new one "mim" on migrate_tools.drush.inc.
- Display a "The 'mi' alias is deprecated and will no longer work with Drush 9. Consider the use of 'mim' alias instead." message if 'mi' is used.

Major releases of Drupal don't care about backward compatibility, but minor releases do.

From over the years using Drupal I can give you probably more than a dozen instances where this isn't true, but sure ~ your spirit hasn't been broken yet, so let's just keep it that way ;o) Anyways, the patch looks good to me ~ thanks for your update.

@bpresles

The patch basically works for me without any problems so far, despite a tiny drawback:

I was used to write "mi" for starting imports and tried to do so. But I didn't get a deprecation notice, instead it just didn't work:

Command "mi" is ambiguous (migrate:status, migrate:import and 5 more)

PS: I haven't tried a plain "drush mi" call for running all updates, I trigger them individually by ID, like "drush mi db_spareparts --update". Now instead, I have to use "drush migrate-import db_spareparts --update", but that's ok for me

Command "mi" is ambiguous (migrate:status, migrate:import and 5 more)

Thats Synfony console being annoying. Not much we can do, beside pick another brief alias that it likes better. Maybe mim?

Now I see that the patch already declared mim alias and shows a deprecation message for drush8 users when they use mi. Nice work!

I could only find one minor issue whuch we can fix whenever - $this->logger->error(dt('No migrations found.'));. That should be the method logger() [with parenthesis], not the property.

After talking with heddn, we decided to merge the latest patch. Drush9 users rejoice!

This merge increased the maintenance burden for our Drush commands, by duplicating some code/logic. Please help out at #2921721: Consolidate CLI functionality into a service in order to get this in good shape.

Great! Is there a new beta planned? Would be nice to have this released asap