If getMessageIterator() includes useful IDs, don't print the source_ids_hash column in drush mmsg command

Clarified title and summary that this is about the drush mmsg command.

Postponed might be a more appropriate status here, although the way I wrote this it doesn't depend on #2960209 landing first, and will happily work whether you're running a version of core that includes that fix/patch or not.

Can we just load/print the source ids if they don't exist?

Sort of. I suppose we could stop using \Drupal\migrate\Plugin\migrate\id_map\Sql::getMessageIterator() entirely and write our own version that does the same query. But that seems evil and wrong, and will break if the migration's id_map isn't being stored in SQL (not sure if that even works, but presumably if this is all so class-tastically flexible, it's intended to work).

And needs tests so needs work.

Can you be more specific? What exactly would you want tested here? It's a trivial fix to the UI. Please clarify and I'll see what I can do.

Thanks,
-Derek

Upon closer inspection, I see no existing test coverage for any of the drush commands. I have no idea where to start with that. Seems unfair to hold up this trivial fix for adding entirely new test plumbing. If you want to open a new issue for "add test coverage for migrate_tools drush commands" I'll try to help, but for now, I'm resetting the status on this. Hope you agree.

Thanks,
-Derek

Except we can't always remove the hash from getMessageIterator(). See #2714529-18: Add source and destination IDs to the data returned by getMessageIterator() and #19.

That said, patch #20 would break this, since we would no longer be able to assume consistent column aliases in that case. :( Not entirely sure the cleanest solution to that. Input welcome.

Maybe, although I've never been able to get Drush 9 working on any site where I've tried it, so I'm the wrong one to ask. ;)

Plus, it seems like the parent issue is stalled upstream, so I don't know when/if it's worth trying to fix drush to benefit from it.

Perhaps this is a more appropriate status.

It was once a simple fix. ;) Then Drush released 2 more major versions, and apparently we're trying to support all of them.

I honestly haven't looked at any of this code in ages. Not sure why patch #3 is drush-version-dependent. Not sure what else needs to happen here, if anything.

#13 claims the current UI is fine (including the incomprehensible hash value). I disagree, but I don't really care that much.

So, either "Won't fix" if we believe #13 that this UI is good as-is.

Or figure out what, if anything, needs to change about #3 to work with All The Drushes(tm).

There's also the question of "needs tests", but as I pointed out in #6 we had (still have?) 0 test coverage of the drush commands, so I don't think we should be adding all new test plumbing just for this.

YMMV... ;)

Cheers,
-Derek

+++ b/src/Commands/MigrateToolsCommands.php
@@ -499,8 +499,7 @@ class MigrateToolsCommands extends DrushCommands {
-   *
-   * @option csv Export messages as a CSV

@@ -515,15 +514,16 @@ class MigrateToolsCommands extends DrushCommands {
-    'csv' => FALSE,

@@ -537,29 +537,58 @@ class MigrateToolsCommands extends DrushCommands {
-    if ($options['csv']) {
-      fputcsv(STDOUT, array_keys($table[0]));
-      foreach ($table as $row) {
-        fputcsv(STDOUT, $row);
-      }
-      return NULL;
-    }

Removing the CSV option seems out of scope. Is that included here by accident?

Thanks! Sorry I didn't get a chance to actually test this again for real. Appreciate you taking the time to finish this off.

Cheers,
-Derek