Migrate UI - add human-friendly module names to the Review form

I don't know if its straightforward for modules not on the system because unless their human name is provided in some migrate file (additionally to their machine name), we'll not be able to pull the human name from anywhere, unless we also build in an update XML reader in this that tries to pull that project info down to get the human name.

#2936365: Migrate UI - allow modules to declare the state of their migrations landed, so not anymore postponed.

Looks good to me except:

+++ b/core/modules/migrate_drupal_ui/tests/src/Functional/d6/MultilingualReviewPageTest.php
@@ -148,18 +148,18 @@ protected function getAvailablePaths() {
-      'block',
-      'devel',
-      'devel_generate',
-      'devel_node_access',
-      'i18n',
-      'i18ncck',
-      'i18nstrings',
-      'i18ntaxonomy',
-      'i18nviews',
-      'locale',
+      'Block',
+      'CCK translation',
+      'Devel',
+      'Devel generate',
+      'Devel node access',
+      'Internationalization',
+      'Locale',
+      'String translation',
+      'Taxonomy translation',
+      'Views',
+      'Views translation',

I really like the human readable names on the UI. However this list shows a possible major weakness of the approach, namely that module machine names like "i18nviews" turn to be "Views translation" on the UI, so there is not necessarily a match. Will users be able to trace down these names to the projects they are in? This is not a standalone module but part of a bigger project.

Two possible ideas to solve this:

(1) add the project name from the module's info data in parenthesis or as a tooltip

(2) add the machine name of the module in parenthesis or as a tooltip

I think in some cases the two will be the same (project and module) or very similar (machine name and module). In other cases though the difference will be crucial to know. Therefore the tooltip idea. I don't know if we have a programatic way to tell if there is a "significant difference".

I'm blocked on multiple fronts getting into a D8 environment right now to check it, but I would say that in general, the primary interface through which users learn/know about modules is the Modules page, so as close to what it does as we can get here, the better, most likely.

I thought about this a bit more overnight... The D8 branch of https://www.drupal.org/project/upgrade_status module has about the same problem area it's trying to surface, and has much more recently been designed by an actual designer, and reviewed by the UX team.

It just has the alphabetical list of module names. No package, no machine name in parentheticals, etc.

So maybe we should do the simplest thing that can possibly work here, and use human-readable name, falling back to machine name in case things are not found.

#22 is a valid point, but it's also valid for Upgrade Status and the Modules page as well. So maybe we should file an issue to fix it in core (or not), bring it to the UX team, and apply whatever pattern later, once it lands.

Upgrade Status does actually roll up all modules to a package level, eg. showing d.o project names instead of submodules one by one. It also attempts to do the same for custom module projects where there are submodules. This is because projects are what have d.o destinations, maintainers to work with, etc. You can only update them altogether.

Oh, fair point. Let's do that then. :)

I think the point Gabor was making was that upgrade status groups by composer level package, which makes sense for upgrade status, but probably not for migrate, so lets go ahead and implement #22 - 1

Looking good. Setting to NW for one super nit, sorry!

1. +++ b/core/modules/migrate_drupal_ui/src/Form/ReviewForm.php
   @@ -57,10 +73,13 @@ class ReviewForm extends MigrateUpgradeFormBase {
   +  public function __construct(StateInterface $state, MigrationPluginManagerInterface $migration_plugin_manager, PrivateTempStoreFactory $tempstore_private, MigrationState $migrationState, ConfigFactoryInterface $config_factory, ModuleHandler $module_handler) {
        parent::__construct($config_factory, $migration_plugin_manager, $state, $tempstore_private);
        $this->migrationState = $migrationState;
   +    $this->moduleHandler = $module_handler;
   

   Often we will provide a BC layer for constructor changes, despite it not being required per BC policy. I don't think it's necessary here, as I don't see anyone having a reason to extend this form class.

2. +++ b/core/modules/migrate_drupal_ui/src/Form/ReviewForm.php
   @@ -167,18 +190,21 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   +            '#value' => $source_module ,
   

   super nit: extra space added before the comma

Looks good to me. Thanks!

Could we get a screenshot please? :D

I'm really sorry, this keeps totally dropping off my radar for some reason... And wow, award for biggest effort that had to go into a screenshot goes to...! :)

I want pull this back a little. Here's the modules page:

Here's the core Update Manager:

Here's the Upgrade Status module (D8 version; shown because it has had much more recent design love):

Notice that in each case, the site operator is shown the human-readable version of the module's name. The only exception is that if you expand a details element on the Modules page, in tiny, tiny font, you can also see the machine name there, too.

In none of these examples are we making machine name the primary identifier, and also in none of those does a tooltip appear on hover to reveal more information. (Actually, the URLs will appear at the bottom of your browser when hovering over links in Update/Upgrade Status, so maybe that's not quite true.)

In each instance, one could argue that it can be helpful to find the module's machine name, but yet in each instance we do not show it, or at least certainly not obviously show it.

We can argue that this is not useful, and that we should adopt a nomenclature instead of "Module name [machine_name]" or a tooltip on hover that exposes the machine name or something else, and use that pattern everywhere, but we currently do not use that pattern, and I don't believe we should start a new one in this issue. (Happy to discuss in a follow-up, though.)

So, I'd really prefer for us to go back to the original request, which was extremely simple, and literally just using the module's human-readable names wherever possible. (Obviously this won't work for custom modules and the like which would fall back to machine_name)

If I'm missing something obvious—which is QUITE possible, since as I stated before, this issue unfortunately keeps dropping off my radar—I'm happy to discuss it.

Oopsie. Missed a spot.

"Needs review" for discussion.

We could handle that bit like Upgrade Status module does (https://www.drupal.org/files/project-images/UpgradeStatus8UI.png), which is:

1) A full table of all modules in alphabetical order (could indeed group sub-modules under package if you'd like; Update Status module does that: https://www.drupal.org/files/issues/2019-10-09/d8-update-manager.png)
2) Colour + icon the table rows according to their statuses
3) Allow filtering the table to show only rows with a problem

...all of which, however, is light years more complicated than just switching to the human-readable label! ;)

Wow! This looks really great, but is also a bit WAY overkill for what we need here, I think?

Is there any way to make this issue just "use human-friendly module names on the Migrate UI" (as it was originally written) and split off "and also make the styling 10,000% better and match the Upgrade Status module" to a separate follow-up? I think these changes look great, but we probably need to spend some time properly evaluating them, and in the meantime we hold up a sensible fix.

The issue summary mentions the concern in #22 that if we do just that, then we're removing useful information, so as a compromise, how about we invert the logic of #36, and show the human-readable version by default and the machine name on hover?

I am just starting to look at this issue. Since the patch in #36 was RTBC and similar to the current patch (except that machine name and label are switched) I decided to start by comparing the two.

Almost all of these comments are either small nits or out of scope. On the plus side, the nits should be easy to fix and I will not argue if you choose not to fix anything out of scope.

1. +++ b/core/modules/migrate_drupal_ui/src/Form/ReviewForm.php
   @@ -3,6 +3,8 @@
    namespace Drupal\migrate_drupal_ui\Form;
    
    use Drupal\Core\Config\ConfigFactoryInterface;
   +use Drupal\Core\Extension\Exception\UnknownExtensionException;
   +use Drupal\Core\Extension\ModuleHandler;
       

   This patch adds use statements in alphabetical order, which is the usual practice. I noticed that the patch in #36 also fixed some of the existing use statements, which were out of order. This is out of scope for this issue, but I will be even happier if you make the same fix again.

2. @@ -57,10 +73,15 @@ class ReviewForm extends MigrateUpgradeFormBase {
       *   Migration state service.
       * @param \Drupal\Core\Config\ConfigFactoryInterface $config_factory
       *   The config factory service.
   +   * @param \Drupal\Core\Extension\ModuleHandler $module_handler
   +   *   The module handler service.
       */
   -  public function __construct(StateInterface $state, MigrationPluginManagerInterface $migration_plugin_manager, PrivateTempStoreFactory $tempstore_private, MigrationState $migrationState, ConfigFactoryInterface $config_factory) {
   +  public function __construct(StateInterface $state, MigrationPluginManagerInterface $migration_plugin_manager, PrivateTempStoreFactory $tempstore_private, MigrationState $migrationState, ConfigFactoryInterface $config_factory, ModuleHandler $module_handler) {
        parent::__construct($config_factory, $migration_plugin_manager, $state, $tempstore_private);
   +    $this->state = $state;
   +    $this->pluginManager = $migration_plugin_manager;
        $this->migrationState = $migrationState;
   +    $this->moduleHandler = $module_handler;
       

   Since $this->state is already set in the parent constructor, there is no need to do it again. I do not see $this->pluginManager declared or used anywhere. I do see $this->migrationPluginManager declared and assigned in the parent constructor. I think both of those lines should be removed.

   Another out-of-scope problem that you can choose to fix or not: $this->migrationState = $migrationState; should really be $this->migrationState = $migration_state;. (Class properties should all be lowerCamelCase. Other variables can be either lowerCamelCase or snake_case, but should be consistent within afile.) If you choose to fix this, then also adjust the doc block and the declaration.

3. Same snippet: in the parent constructor, the order of the variables is
   • $config_factory
   • $migration_plugin_manager
   • $state
   • $tempstore_private

   After this patch, the order of variables in the child constructor is

   • $state
   • $migration_plugin_manager
   • $tempstore_private
   • $migrationState
   • $config_factory
   • $module_handler

   Usually, when we extend a constructor, we keep the original parameters and add new ones on the end. Is there any reason not to do that here? Again, I will not argue if you say that this is out of scope.

4. @@ -254,4 +280,49 @@ public function getConfirmText() {
   ...
   +  protected function sortByName(array $migration_state) {
   ...
   +      $source_module_name = $data['name'];
   +      if (empty($source_module_name)) {
   +        $source_module_name = $source_module;
   +      }
       

   Since we are targeting Drupal 8.8 (or maybe 8.9) we can finally use PHP 7:
   $source_module_name = $data['name'] ?: $source_module;

5. @@ -254,4 +280,49 @@ public function getConfirmText() {
   ...
   +      $destination_module_names = implode(', ', $destination_module_names);
   +      $module_names[$source_module_name]['source'] = $source_module;
   +      $module_names[$source_module_name]['destination'] = $destination_module_names;
       

   What do you think of doing it this way (and staying under 80 characters)?

         $module_names[$source_module_name]['source'] = $source_module;
         $module_names[$source_module_name]['destination']
           = implode(', ', $destination_module_names);
       

   Or perhaps

         $module_names[$source_module_name] = [
           'source' => $source_module,
           'destination' => implode(', ', $destination_module_names),
         ];
       

I will continue to look at this issue. I want to look at the patch as a whole, not just compare it to the one from #36, and I should have a look at the tests.

What do you recommend for manual testing? Can I grab one of the database fixtures used in the tests, enable the module, and be in business?

+++ b/core/modules/migrate_drupal_ui/src/Form/ReviewForm.php
@@ -254,4 +278,47 @@ public function getConfirmText() {
+   * Helper to convert the module machine_name to name.
...
+  protected function sortByName(array $migration_state) {

The first line docblock and the method name don't really seem to match. Typically a method called sortFoo will be a callback for a PHP sort method. This seems to do more than that. In particular, its input and output are fairly different.

I have finished reviewing the patch, including the tests. I have just a few more requests, mostly to the documentation blocks.

1. +++ b/core/modules/migrate_drupal_ui/src/Form/ReviewForm.php
   @@ -45,22 +47,39 @@ class ReviewForm extends MigrateUpgradeFormBase {
   ...
   +  /**
   +   * Source system data.
   +   *
   +   * @var array
   +   */
   +  protected $systemData;
       

   Since this variable is not set in the constructor, it would be helpful to mention where it is set: buildForm().

2. This is totally out of scope, but I had a look at Drupal\migrate_drupal\MigrationState:

     public function getUpgradeStates($version, array $source_system_data, array $migrations) {
       return $this->buildUpgradeState($version, $source_system_data, $migrations);
     }
       

   Why do we have one function that is just a wrapper for another function? Then, in the doc block of buildUpgradeState(),

      * @code
      * [
      *   'finished' => [
      *     'menu' => [
      *       'menu_link_content','menu_ui','system'
      *     ]
      *   ],
      * ]
      * @endcode
       

   That should be 'menu' => 'menu_link_content','menu_ui','system', right?

   If I have this right, then I will open an issue (and tag it for Novice) to fix the documentation. Maybe a second issue to get rid of one of the functions.

3. @@ -254,4 +278,52 @@ public function getConfirmText() {
   ...
   +   * @param array $migration_state
   +   *   A migration state array using module machine names.
   +   *
   +   * @return array
   +   *   An array of module data, keyed and sorted by the source module name. Each
   +   *   array of source module data contains the destination modules names in a
   +   *   sorted CSV format and the source module machine name.
       

   It will help if the @param comment includes "as returned by MigrationState::getUpgradeStates() and if the @return comment explicitly mentions the keys source and destination. Or maybe the @param comment should be something like "Keys are machine names of modules on the source site. Values are lists of machine names of modules on the destination site, in CSV format." That would clarify that this function is not changing the CSV format of the inner values.

4. @@ -86,7 +87,7 @@ public function testMigrateUpgradeReviewPage() {
   ...
        $available_paths = $this->getAvailablePaths();
   -    $available_paths = array_diff($available_paths, [$module]);
   +    $available_paths = array_diff($available_paths, [$module_name]);
       

   It is a little confusing that getAvailablePaths() used to be an array of machine names and is now an array of "friendly" names. Can you update the doc block in MigrateUpgradeTestBase to make this explicit? I guess also update the doc block of getMissingPaths().

5. +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/d6/MultilingualReviewPageTest.php
   @@ -63,81 +63,81 @@ protected function getSourceBasePath() {
      protected function getAvailablePaths() {
        return [
          // Aggregator is set not_finished in migrate_sate_not_finished_test.
       

   I know this is just context, but this looks like a typo. It should be migrate_state_not_finished_test, right? Please fix it here and in the D7 version as well.

I also did some manual testing. For the record, the instructions in #66 were not quite right. If the database key (in settings.php) is d7_dump, then the correct command is php core/scripts/db-tools import --database=d7_dump core/modules/migrate_drupal/tests/fixtures/drupal7.php.

Here are my screenshots. They show the correct labels and the sorting. The hover effect also works, although I did not work as hard as @quietone.

@quietone, thanks for the fixes! I think this looks good.

+++ b/core/modules/migrate_drupal_ui/src/Form/ReviewForm.php
@@ -254,4 +278,54 @@ public function getConfirmText() {
+      $output[$source_module_name] = [
+        'source' => $source_module,
+        'destination' => implode(', ', $destination_module_names),
+      ];

I don't think we should change the key from being the extension machine name. There's no guarantee that the human readable form of the module name is suitable for an array key or is unique. We can still key by the machine name and then do a sort by a key in the each element of the array. Also the tooltip is useful but I think for accessibility doesn't it need to say what it is?

The patch in #76 addresses the issues raised in #74.
No new issues from me.
Back to RTBC.

Do we need to put the D8 module machine name in parens after them?

I am changing the screenshots in the issue summary from links to embedded images, and moving them to the "User interface changes" section.

Usability review

We discussed this issue today: Drupal Usability Meeting - 2019-12-12.

We agreed that the third column is a good way to provide the extra information, although there are some accessibility concerns. Since we already have the follow-up issue #3088215: Use styling from Upgrade Status module on Review form for a more complete overhaul, we decided that we should use this incremental improvement for now.

I am removing the "Needs usability review" tag. Since this issue was RTBC before #80 pointed out that the review was pending, I am setting the status back to RTBC. (I have not personally reviewed the changes since #74.)

Reroll from #76.

The link in #92 ("Rerolling patches") is broken. There is a comment on https://www.drupal.org/patch/reroll suggesting a diff (not an interdiff) for rerolls, and that comment was added to the body of https://www.drupal.org/documentation/git/interdiff#reroll

I am reviewing this today.

I already see one little nit to pick, but nothing serious so far.

@quietone, thanks for the continued work on this! I reviewed the patch, especially the changes since my review in #73.

As usual, I apologize for the minor complaints. I will make up for that by giving a prompt review of an updated patch.

1.   +++ b/core/modules/migrate_drupal_ui/src/Form/ReviewForm.php
     @@ -119,22 +139,24 @@ public function buildForm(array $form, FormStateInterface $form_state) {
     ...
          if (isset($display[MigrationState::NOT_FINISHED])) {
     -      foreach ($display[MigrationState::NOT_FINISHED] as $source_module => $destination_modules) {
     +      $output = $this->prepareOutput($display[MigrationState::NOT_FINISHED]);
     +      foreach ($output as $source_module => $data) {
              $missing_count++;
              // Get the migration status for this $source_module, if a module of the
              // same name exists on the destination site.
              $missing_module_list['module_list'][] = [
     -          'source_module' => [
     +          'source_module_name' => [
                  '#type' => 'html_tag',
                  '#tag' => 'span',
     -            '#value' => $source_module,
     +            '#value' => $data['source_module_name'],

   We no longer need the key in the foreach loop. If we remove $source_module =>, then we should probably adjust the comment.

   I also wonder whether it is worth using '#type' => 'html_tag'. We could get the same markup using #prefix and #suffix, and then the three entries for each row would be more consistent. If you like it better the way it is, or think this is out of scope, then I will not argue.

2.   @@ -160,20 +187,22 @@ public function buildForm(array $form, FormStateInterface $form_state) {
     ...
          if (isset($display[MigrationState::FINISHED])) {
     -      foreach ($display[MigrationState::FINISHED] as $source_module => $destination_modules) {
     +      $output = $this->prepareOutput($display[MigrationState::FINISHED]);
     +      foreach ($output as $source_module => $data) {

   Same comments here (key is not needed, '#type' => 'html_tag' could be simplified).

3.   @@ -254,4 +288,57 @@ public function getConfirmText() {
     ...
     +  /**
     +   * Prepare the migration state data for output.
     +   *
     +   * Each source and destination module_name is changed to the human readable
     +   * name, the destination modules are put into a CSV format and everything is
     +   * sorted.
     +   *
     +   * @param array $migration_state
     +   *   A migration state array where the keys are machine names of modules on
     +   *   the source site. Values are lists of machine names of modules on the
     +   *   destination site, in CSV format.
     +   *
     +   * @return array
     +   *   An array of module data, keyed and sorted by the source module name. Each
     +   *   array of source module data contains the destination modules names in a
     +   *   sorted CSV format and the source module machine name.
     +   */

   Can we make a few changes to the doc block? I like the Oxford comma, and I think that the core committers mostly agree, so please add a comma after "put into a CSV format". Consider shortening "A migration state array" to "An array". For the @return comment, use "source module machine name" in the first sentence and consider using list format for the three keys of the array.

4.   +    foreach ($migration_state as $source_machine_name => $destination_modules) {
     +      $data = unserialize($this->systemData['module'][$source_machine_name]['info']);
     +      $source_machine_name_name = $data['name'] ?: $source_machine_name;
     ...
     +      $output[$source_machine_name] = [
     +        'source_module_name' => $source_machine_name_name,

   Please change the variable name from $source_machine_name_name to $source_module_name or explain why you think the current name is better.

5.   +    usort($output, function ($a, $b) {
     +      return strcmp($a['source_module_name'], $b['source_module_name']) <=> 0;
     +    });

   Either return strcmp($a['source_module_name'], $b['source_module_name']); or return $a['source_module_name'] <=> $b['source_module_name'];.

6.   +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/MultilingualReviewPageTestBase.php
     @@ -72,6 +72,7 @@ public function testMigrateUpgradeReviewPage() {
          // does not need any. Test with a module that is was in both Drupal 6 and
          // Drupal 7 core.
          $module = 'help';
     +    $module_name = 'Help';

   I guess it is out of scope, and I do not want to get bogged down in arguing over whether it should be "is" or "was", but "is was" is clearly wrong.

7.   +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/d6/MultilingualReviewPageTest.php
     @@ -60,83 +60,82 @@ protected function getSourceBasePath() {
         */
        protected function getAvailablePaths() {
          return [
     -      // Aggregator is set not_finished in migrate_sate_not_finished_test.
     -      'aggregator',
     -      'blog',
     -      'blogapi',
     -      'book',
     ...
     +      'Aggregator',
     +      'Block translation',
     +      'Blog',
     +      'Blog API',
     +      'Book',

   I just want to note that we are not adding modules to the list (specifically Block translation). We are changing the order of the list, since it used to be sorted by machine name and is now being sorted by label. I already checked this when I reviewed the patch in #72.

8.   --- /dev/null
     +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/d7/NoMultilingualReviewPageTest.php
     @@ -0,0 +1,179 @@

   I am a little confused about the history of this test. The interdiff attached to #98 and the comment there suggest that this is a modified version of NoMultilingualTest.php, which was removed in #2966856: Hide and disable Drupal Migrate Multilingual. Can you clarify this?

9.   +  /**
     +   * {@inheritdoc}
     +   */
     +  public static $modules = [
     +    'file',
     +    'language',
     +    'content_translation',
     +    'config_translation',
     +    'migrate_drupal_ui',
     +    'telephone',
     +    'aggregator',
     +    'book',
     +    'forum',
     +    'statistics',
     +  ];

   Is there any reason not to sort the list of modules? It is easier to spot duplicates in a sorted list, so that can help prevent errors down the line. It is also easier to check whether a particular module is on the list.

@quietone, thanks for the prompt update! I am afraid that we need a few fixes for the fixes. :-(

Thanks for looking up the usage of the serial/Oxford comma. I did not realize it was official.

1.   +++ b/core/modules/migrate_drupal_ui/src/Form/ReviewForm.php
     @@ -119,31 +139,31 @@ public function buildForm(array $form, FormStateInterface $form_state) {
     ...
     +          'source_module_name' => [
     +            '#prefix' => '<div class = "upgrade-analysis-report__status-icon upgrade-analysis-report__status-icon--error">',
     +            '#suffix' => $data['source_module_name'],
     +          ],

   and

     @@ -160,29 +180,29 @@ public function buildForm(array $form, FormStateInterface $form_state) {
     ...
     +          'source_module_name' => [
     +            '#prefix' => '<div class = "upgrade-analysis-report__status-icon upgrade-analysis-report__status-icon--checked">',
     +            '#suffix' => $data['source_module_name'],
     +          ],

   You asked whether this is the right way to use #prefix and #suffix. The first one should be something like this. Note that I removed spaces around = inside the tag; I think that is the standard. Also, you can use <span> instead of <div> and revert the changes to the test from #101 to #103. (If you do that, then it will save me the trouble of another round of manual testing since we are changing the markup.)

     'source_module_name' => [
       '#prefix' => '<div class="upgrade-analysis-report__status-icon upgrade-analysis-report__status-icon--error">',
       '#suffix' => '</div>',
       '#plain_text' => $data['source_module_name'],
     ],

2. Now that the render array is more consistent, I notice a different inconsistency:

    +          'source_machine_name' => [
    +            'source_machine_name' => [
    +              '#plain_text' => $data['source_machine_name'],
                 ],
               ],
               'destination_module' => [
    -            '#plain_text' => $destination_modules,
    +            '#plain_text' => $data['destination'],

   We can get rid of one level of nesting for 'source_machine_name', right?

3.   @@ -254,4 +274,57 @@ public function getConfirmText() {
     ...
     +   * @return array
     +   *   An indexed array of module data, sorted by the source module name. Each
     +   *   array of contains the source module name, the source module machine name,
     +   *   and the the destination module names in a sorted CSV format.
     +   */

   There is something missing after "Each array of", or we can remove "of", and there is an extra "the" before "the destination module". I was wrong (part of #100.3) to say that the first sentence should end with "source module machine name".

4. Thanks for clarifying the history of the test (#100.8).

Regarding #prefix and #suffix, I guess I fell into the trap of "if I know this, then everyone must know this". If I taught you something new, then that is my good deed for the day!

The updates in #101 and #105 fix all the issues I found in #101 and #104, so back to RTBC. The changes to the test in #103 were reverted: see #104.1.

The testbot disagrees, but the failure is on an unrelated test: https://www.drupal.org/pift-ci-job/1637535

There was 1 failure:

1) Drupal\Tests\node\Functional\NodeCreationTest::testAuthoredDate
Failed asserting that '1586018408' matches expected 1586014808.

/var/www/html/core/modules/node/tests/src/Functional/NodeCreationTest.php:201

Let's give it another chance.

1586018408 - 1586014808 = 1 hour, BTW

This patch seems to contain a lot more than what's necessary to fix the scope described in the title and IS. Can we update the IS and/or the title, and rein in the out-of-scope changes a bit? (They can be moved to related issues.) Thanks!

Updated summary with fresh screenshots based on patch on #105.

Updated screenshots from patch 116 to summary. Wonder if the column header be "Drupal 9" instead of "Drupal 8" ?

I compared ReviewForm.php in (9.1.x with the patch from #116) to 9.1.x and to (8.9.x with the patch from #105). I also looked at the patch in #119. I still owe you a review of the tests.

I see just one problem with the patch in #116, and I am setting the status to NW for this:

@@ -55,12 +71,13 @@ class ReviewForm extends MigrateUpgradeFormBase {
...
-   * @param \Drupal\Core\Config\ConfigFactoryInterface $config_factory
-   *   The config factory service.
+   * @param \Drupal\Core\Extension\ModuleHandler $module_handler
+   *   The module handler service.

We should add the new @param comment without removing the one for $config_factory.

1. I am updating the title. The previous title was consistent with the original approach (replacing machine name with friendly name) as well as the current approach (using both in separate columns) but we may as well be specific.
2. I am updating the remaining/completed tasks in the issue summary. In general, I do not think it is useful to list "Review" and "Commit" as steps there.
3. I am adding the "Needs followup" issue tag. See the list below.
4. The change in #119 is out of scope, so I am including it in the list of followup issues. As the "before" screenshot shows, the form already says "Drupal 8". That should be fixed, but it is not part of the current issue.

I think all of us agree that writing patches is more fun than creating and updating issues here on d.o. What @xjm is reminding us, in #110, is that we have to keep the final reviewer in mind. Think of it as a single-responsibility principle for issues, or think of it as keeping your commits/patches atomic. Each issue should do just one thing.

Follow-up issues (all related to ReviewForm.php)

1. Sort use statements alphabetically.
2. Various fixes to the constructor:
   
   1. Make order of parameters match the constructor in the parent class.
   2. Use snake_casefor parameters and lowerCamelCase for properties.
   3. Make @param comments in the same order as the function declaration.
3. Simplify buildForm() by using '#prefix' and '#suffix'.
4. Update "Drupal 8" in comments, table header, and elsewhere so that is shows the correct version.

Perhaps (1) and (2) can be combined. Also, (2) will require corresponding changes to the create() method.

I have a few suggestions for improving the change in #119, but I will save them for the to-be-created issue.

Thanks for the patch in #124, fixing the problem I noticed in #121.

I reviewed the changes in the tests. Other than metadata (line counts and commit hashes) the only differences between #105 and #124 are

1. Revert the change that is deferred to #3134308: Change 'is was' to 'is' in comments.
2. Add return type void to the setUp() method.

Of these, (1) removes an out-of-scope change and (2) is required now that the patch targets 9.1.x (and is an update to code added by this patch, not existing code).

I am adding the followup issues as related issues.

I would still like to see followups for #121.2 and #121.3, but I will not hold up this issue for those. Maybe we can discuss at the Migrate API meeting on Slack today.

I am setting the status back to RTBC.

@quietone:

I just realized that. I was about to say ...

I missed the new child issues related to #121.2, #121.3, and #121.5. There is no need to list that last one as both related and a child issue.

Thanks, just a few more small things.

1. +++ b/core/modules/migrate_drupal_ui/src/Form/ReviewForm.php
   @@ -44,6 +46,20 @@ class ReviewForm extends MigrateUpgradeFormBase {
   +   * @var array
   
   @@ -254,4 +284,58 @@ public function getConfirmText() {
   +   * @param array $migration_state
   ...
   +   * @return array
   

   We can almost always be more specific than array in data type documentation, e.g. is it string[]? array[]?string[][]? The more specific, the better.

2. +++ b/core/modules/migrate_drupal_ui/src/Form/ReviewForm.php
   @@ -57,10 +73,13 @@ class ReviewForm extends MigrateUpgradeFormBase {
   -  public function __construct(StateInterface $state, MigrationPluginManagerInterface $migration_plugin_manager, PrivateTempStoreFactory $tempstore_private, MigrationState $migrationState, ConfigFactoryInterface $config_factory) {
   +  public function __construct(StateInterface $state, MigrationPluginManagerInterface $migration_plugin_manager, PrivateTempStoreFactory $tempstore_private, MigrationState $migrationState, ConfigFactoryInterface $config_factory, ModuleHandler $module_handler) {
   

   We should add BC for this (make the new parameter optional and defaulted to NULL, and trigger a deprecation error and set it from \Drupal if NULL is passed).

3. +++ b/core/modules/migrate_drupal_ui/src/Form/ReviewForm.php
   @@ -119,22 +139,24 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   -        // Get the migration status for this $source_module, if a module of the
   +        // Get the migration status for each source module, if a module of the
   

   Looks out of scope.

4. +++ b/core/modules/migrate_drupal_ui/src/Form/ReviewForm.php
   @@ -254,4 +284,58 @@ public function getConfirmText() {
   +   * Each source and destination module_name is changed to the human readable
   

   Nit: human-readable.

5. +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/MigrateUpgradeTestBase.php
   @@ -145,7 +145,7 @@ protected function assertUpgradePaths(WebAssert $session, array $available_paths
       *
       * @return string[]
   -   *   An array of available upgrade paths.
   +   *   An array of source modules names that will be upgraded.
       */
      abstract protected function getAvailablePaths();
    
   @@ -153,7 +153,7 @@ protected function assertUpgradePaths(WebAssert $session, array $available_paths
   
   @@ -153,7 +153,7 @@ protected function assertUpgradePaths(WebAssert $session, array $available_paths
       * Gets the missing upgrade paths.
       *
       * @return string[]
   -   *   An array of missing upgrade paths.
   +   *   An array of source modules names that will not upgraded.
       */
   

   This still looks out of scope. Unless it's meant to indicate that the data is being changed from the machine names to the UI labels? If so, that's not clear here. I'd make it more specific, i.e., "An array of the human-readable names (UI labels) of the modules that will be upgraded.

   Also, nit: "source modules names" isn't quite grammatical.

6. +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/d7/MultilingualReviewPageTest.php
   @@ -140,39 +140,39 @@ protected function getAvailablePaths() {
   -      // Action is set not_finished in migrate_sate_not_finished_test.
   -      // Aggregator is set not_finished in migrate_sate_not_finished_test.
   ...
   -      // Block is set not_finished in migrate_sate_not_finished_test.
   ...
   +      // Action is set not_finished in migrate_state_not_finished_test.
   +      // Aggregator is set not_finished in migrate_state_not_finished_test.
   ...
   +      // Block is set not_finished in migrate_state_not_finished_test.
   

   Technically fixing "sate" to "state" is also not in scope, but OK I guess since it's part of and directly relevant to the hunk being updated.

This has been on my list for a long time, apologies to @quietone for how long it has taken me to look at it in detail.
It looks really good to me, and I applaud your persistence @quietone for sticking with an issue that on the surface seems straight forward, but yet somehow we're at comment #132.
There's only one thing that I have a question about here, and I might be wrong -

+++ b/core/modules/migrate_drupal_ui/src/Form/ReviewForm.php
@@ -254,4 +284,58 @@ public function getConfirmText() {
+            $destination_module_names[] = 'Core';

Does this need to be translatable? i.e. should it be $this->t('Core').
Also, we can continue here, which would avoid the else and therefore reduce some complexity (less indentation etc) - but don't worry about that unless we need to add translation support here.

The patch needs a reroll now that #3133305: Use \Drupal::VERSION to get the version in Migrate UI rather than hardcoding it has landed, so back to NW. I guess that means we should also address #132.

1.   +++ b/core/modules/migrate_drupal_ui/src/Form/ReviewForm.php
     @@ -254,4 +284,58 @@ public function getConfirmText() {
     ...
     +   * @param array $migration_state
     +   *   An array where the keys are machine names of modules on
     +   *   the source site. Values are lists of machine names of modules on the
     +   *   destination site, in CSV format.
     +   *
     +   * @return string[][]
     +   *   An indexed array of arrays that contain module data, sorted by the source
     +   *   module name. Each sub-array of contains the source module name, the
     +   *   source module machine name, and the the destination module names in a
     +   *   sorted CSV format.
     +   */
     +  protected function prepareOutput(array $migration_state) {

   Follow-up to #130.1: @param array can also be changed to @param string[]. Back to NW for this.

   I just noticed the stray "of" in the second sentence of the @return comment. Please fix that, too.

2. I am not sure how the BC policy applies to @internal classes as discussed in #130.2 and #131.

3. This patch fixes #130.3 and #130.4.

4. I do not have time right now to review the changes to the test classes. I also have not thought about #132.