Reduce method and control structure length/complexity in Migrate UI

I've been working on this and should be able to upload a patch in a day or two.

A bit of a delay for family reasons but here is a patch.

Tested locally, looks good!

run() is being refactored in #2687851: Refactor run() method on Migrate UI batch and remove the $operation parameter.

Restoring RTBC.

No, the tests are passing. Restoring RTBC.

Patch applies with offset of 40 lines. Updating patch.

And of course forgot to update status.

Rerolled patch.

Reroll looks good.

I'm not keen on the current approach - we don't really lose any complexity. I think it is worth asking what needs to be on the confirmation page by way of information. At the moment the big thing that is displaying is a table where one of the columns is always going to be "Missing". That suggests that maybe this table is not the best way of doing this. Maybe just an item list of the modules enabled in the source site that have no migration path. Also there are modules we know have no migration path - i.e. overlay and are never likely to - therefore are we just giving the user too much information? What I'm getting at is that some missing migrations are important and some are not - that is a tricky problem.

So for example - the missing module list code could be:

    $unmigrated_source_modules = array_filter(array_diff_key($system_data['module'], $table_data), function ($module_data) {
      return $module_data['status'];
    });
    $missing_items = array_map(function ($source_module) {
      $info = unserialize($source_module['info']);
      return $info['name'];
    }, $unmigrated_source_modules);
    sort($missing_items);
    $missing_count = count($missing_items);
    $form['missing_module_list'] = [
      '#theme' => 'item_list',
      '#items' => $missing_items,
    ];

This also has the advantage of use the real module name from Drupal 6 / 7. Another thing is the use of functional code like array_map() and array_filter() to manipulate the arrays of data. This can result in simpler code that does less too. We could also consider removing disabled modules from the system data when we get it as we don't need them. That said I'm not sure why we're putting the system data in form state - probably when the form we also being used for rollback and incremental. I'm not sure that having one form do everything is really the way to go here.

tldr; I think we need to look at:

• Does this really have to be a single form. Maybe we should have 1 form that presents the options (atm only migrate - in the future maybe rollback and incremental). And then each process should get it's own form. These forms might share a base form for common functionality but not be the same form.
• We need to look at the information we actually give the user and decide what is relevant

Some past discussion on the "missing" upgrade paths: #2569771: [meta] Highlighting of source modules with no upgrade paths (and child issues).

#2569805: For Drupal migration, identify the source module is addressing the hard-coded list of module upgrade paths.

Needed a reroll.

Assigning for review.

OK, this patch that is up for review is only a reroll.
I see that @alexpott's comments in #23 haven't been addressed yet, nor discussed.

Does this really have to be a single form. Maybe we should have 1 form that presents the options (atm only migrate - in the future maybe rollback and incremental). And then each process should get it's own form. These forms might share a base form for common functionality but not be the same form.

I'm still not too much familiar with this upgrade form so I don't have an opinion yet on that point, but I can see why we need to think about that. If we don't do that here, we should open a follow up with a plan.

We need to look at the information we actually give the user and decide what is relevant

I must say I agree that the 'Missing upgrade paths' table isn't really useful as a table and could be just a list of the missing upgrade paths. Same for the 'Available upgrade paths', I don't think that the 'Destination' column is that useful, it could also be just a list of the available upgrade paths.

Back to needs work because of #23

Does this really have to be a single form.

Restructing the form is probably a good idea. I looked at that ages ago but was too new to really pursue it. And like you I don't know if that should be done here or in another issue (either new or existing). Does that even fit with the plan for the UI? What is the plan for the UI?

We need to look at the information we actually give the user and decide what is relevant

How do we do that? I've seen a fair amount of comments about that over the year but no solid answers.

Even so, the attached patch is a quick fix to get rid of the destination column in the form table.

Here is an attempt to address the issues raised in #23.

• Used the suggested code to get the list of missing modules and available modules.
• Created a base form
• Reduce the amount of information displayed.

The failing tests pass locally and are unrelated to the patch. Tried a retest and still failing.

And here is screenshot of the modified upgrade confirm page.

Looks like the tests are passing. Setting to NR.

1. +++ b/core/modules/migrate_drupal_ui/src/Form/MigrateUpgradeFormBase.php
   @@ -775,8 +730,8 @@ public function buildOverviewForm(array $form, FormStateInterface $form_state) {
            '#markup' => '<p>' . $this->t('Upgrade a site by importing it into a clean and empty new install of Drupal 8. You will lose any existing configuration once you import your site into it. See the <a href=":url">online documentation for Drupal site upgrades</a> for more detailed information.', [
   -          ':url' => 'https://www.drupal.org/upgrade/migrate',
   -        ]),
   +            ':url' => 'https://www.drupal.org/upgrade/migrate',
   +          ]),
   

   Extra indent added.

2. +++ b/core/modules/migrate_drupal_ui/src/Form/MigrateUpgradeUpgradeForm.php
   @@ -0,0 +1,430 @@
   +      '#description' => $this->t('The following items will not be upgraded. For more information see <a href=":migrate">Upgrading from Drupal 6 or 7 to Drupal 8</a>.', array(':migrate' => 'https://www.drupal.org/upgrade/migrate')),
   

   s/items/modules/

   The linked page isn't really helpful in interpreting the "Missing upgrade paths" list. I think we're better off describing the likely explanations in-place here, something like:

   Modules may be listed here for various reasons, including:

   • The module has no data which requires migration. No action is required.
   • The Drupal 8 module which corresponds to the module in your source site is not enabled. If you wish the data to be migrated, you need to enable the corresponding Drupal 8 module before performing the upgrade.
   • The Drupal 8 module which corresponds to the module in your source site does not (yet) support migration. Check the module issue queue to see if migration support is in progress.
   • The Drupal 8 module which corresponds to the module in your source site is not present. If you wish the data to be migrated, you need to install and enable the corresponding Drupal 8 module before performing the upgrade.

1. Fixed
2. Modified using the text you provided. And added a screen shot.

But, if I understand the user interface standards, maybe the sentence, "The Drupal 8 module which corresponds to the module in your source site is not present" should be modified to not refer to Drupal 8? Maybe "The module on this site that corresponds to the module on the source site is not present."?

Got called away and forgot to upload the screenshot.

#2569805: For Drupal migration, identify the source module is going to require a major reroll of this, and I think that's the more important issue. Postponing on that.

The issue summary should be updated to reflect that this issue was redirected from just simplifying the code to simplifying the UI itself.

Now that #2918761: Break up MigrateUpgradeForm into smaller forms has been committed this can proceed.

The two point in the original IS have been done, that is noted in the IS. I read through the issue and didn't find any followup to make.

Closing as out of date