Migrate UI - handle sources that do not need an upgrade

Migrate criticals are now straight-up critical, by committer agreement.

I think we should postponed on #2859304: Show field type migrations correctly in Migrate Drupal UI because it contains new tests for the available/missing paths on the 'review upgrade' page. We will need those for testing this issue.

How does this work for modules in contrib? Just because there are missing upgrade paths doesn't mean that being on the list of missing modules in a bad thing.

Discussed in maintainers meeting. This issue will create a hard coded list of modules. And we'll provide, in another follow-up, a way for contrib modules to subscribe to that list.

Follow-up should be major, not critical.

I think #2918448: Migrate UI: Clarify that a missing upgrade path isn't necessarily a problem should be closed as duplicate.

I think it is probably critical to hardcode a list of core modules that don't require upgrade paths. Contrib "no migration needed" could be a separate scope (and not necessarily critical).

Tagging for a follow-up issue to deal with contrib modules, as I stated in #2859304-113: Show field type migrations correctly in Migrate Drupal UI.

Here is a list of modules we should consider in creating the initial list of upgrade paths to core, copied from #2859304-120: Show field type migrations correctly in Migrate Drupal UI:

admin_menu Missing
backup_migrate Missing
captcha Missing
context Missing
context_ui Missing
ctools Missing
*** date_api Missing
*** date_timezone Missing
fieldgroup Missing
*** i18nblocks Missing
*** i18ncck Missing
??? i18nstrings Missing
??? i18nsync Missing
*** i18nviews Missing
*** imageapi Missing
*** imageapi_gd Missing
*** imagecache_ui Missing
lost_character_captcha Missing
math_captcha Missing
menu_block Missing
menutrails Missing
*** nodereference Missing
nodewords Missing
nodewords_basic Missing
path_redirect Missing
pathfilter Missing
rules Missing
rules_admin Missing
token Missing
update Missing [Note: I didn't have this module on in my destination site]
*** views Missing
views_data_export Missing
*** views_ui Missing

The list in the previous comment was from a D6 to D8 migration that I tried yesterday. The D6 site had several contrib modules (obviously) and was my motivation for filing [#12314437] originally. Let me know if you have questions about the list. As one note, I didn't have the Update module enabled on my D8 site, but I did have Views, Views UI, all the multilingual modules, and ... well if you want a list of the D8 modules I had enabled when I generated that list of missing modules, let me know.

Anyway, I'm following this issue and volunteering to test patches on this same D6 migration...

#2859304: Show field type migrations correctly in Migrate Drupal UI has landed, so this is no longer postponed.

I think so, yes.

Re-rolled the patch #16 because it's failed to apply on 8.5.x branch

+++ b/core/modules/migrate_drupal_ui/src/Form/MigrateUpgradeForm.php
@@ -67,6 +67,36 @@ class MigrateUpgradeForm extends ConfirmFormBase {
+  protected static $d6_no_upgrade_path = [

If we make this and $d7_no_upgrade_path public instead of protected (or at least provide public static accessors), boom: that's one way contrib modules could extend these arrays.

Still needs tests.

I only see one architectural thing with this patch: would it be clearer to collapse $d6_no_upgrade_path and $d7_no_upgrade path into a single array? Something like this:

public static $noUpgrade = [6 => ['date_api', 'help', ...], 7 => ['blog', 'contextual', ...]]

I'm still on the fence about whether we need to add an accessor method that can mark a module as not having an upgrade path.

I don't think we need additional tests; our existing assertions will cover the changes made here. So I'm removing that tag.

I forgot to comment yesterday about the discussion on this issue we had at the migrate meeting with @heddn, @phenaproxima, @Gábor Hojtsy, @rakeshjames and myself, so here's a summary.

1. First, we should add constants to MigrateUpgradeForm to flag migrations as either Available, Unavailable, Ignored, etc. We didn't agreed yet on those constants/labels, it should be discussed here. Those ideas were mentioned:
   • 'Available', 'Unavailable', 'Ignored'
   • 'Intentionally ignored ', 'Currently unavailable', 'Available', 'Partially available'
   • 'Ready', 'Partial', 'Missing'

   Then contrib modules could override this status if they provide a migration path.

2. Second, we should add footnotes (or descriptions at the top of the tables) to each table to better explain what the different 'missing' statuses means and link to a documentation page with even more details.
3. This was not discussed yesterday, but I would also suggest that we keep the two summary elements 'Missing' (or unavailable?) and 'Available' at the top of the page and add a second table in the 'Missing' section. The other option would be to find and agree on a new icon to represent the new status.

This all sounds reasonable, but I'd just like to point out one thing: Providing links is mostly only good for people who can read English. Providing information on the page at least lets it be translated on localize.drupal.org.

So I would advocate providing at least the minimum needed for someone to understand the situation, either in hook_help() or directly on the page, and only using a link if you want to write a page-long or section-long explanation.

Therefor, I suggest the constants are 'will be upgraded', will not be upgraded' and 'incomplete'.

+1 for this. We can mark things like Date API, which have no upgrade path (because it was folded into core), as "will be upgraded", even though there is technically no update path -- because as far as the user is concerned, there is. To me, that seems like good UX that upholds the cardinal "don't make me think" rule.

These are good, human-friendly categories. Great work, @quietone.

Event is a contrib module that depends on the contrib date module.

Yeah, we shouldn't worry about event.

Hm... I was going to say that some of that list came from my original issue and I had several contrib modules installed, but it doesn't match my site.

Anyway, a few I can comment on:
- simpletest was a contrib module in D6, added to core in D7, machine name changed to "testing" in D8
- In D6, the CCK contrib module shipped with nodereference and userreference (no _ in the middle). Also optionreference. The names node_reference and user_reference come from the D7 contrib project reference I think. And option I think is core in D7? sorry I am in a hurry didn't check this.
- file/image are D7 core. In d6 the contrib modules were imagefield and filefield.
- language and list area also D7 core

So is the list in #35 from D6 or D7? Looks more 7-ish to me.

Did we ever open the follow-up for contrib to add to the list? I don't see that any where and I'm not sure if that is necessary given the current solution. Per #21, maybe that isn't necessary. Removing the tag.

Also, there was a request in #24 to collapse the number of static variables down to a single one.

And finally, the test failure was on 'tracker' not available on the list for D7.

And #38 I'm not any better on knowing about test coverage and what is in the test fixture. I'm more inclined to fix anything missing with follow-ups and continue with our current automated and manual testing approach to get the initial functionality added to core.

#24 done
#38, IMHO, its fine
Added the source module tracker on migration. Checking is that the reason test is failing.

Hitting it again

:)

Apologies for sticking my nose into your ticket, @rakesh.gectcr, but it looks like this one is nearly complete so I thought I'd help push it over the line.

Unfortunately I can't get functional tests running on my local build so I can't be certain this will fix everything, but let's see what the testbot has to say!

I have also corrected a minor coding standards issue.

@Jo Fitzgerald,
Jo, You are always welcome. In our migrations tickets, we always expect you any where. Again thank you for the amzing contributions. keep going dear mate.

just checking, not able to test the functional tests on my local. ones this pass, will work on it

.

hope this pass. So I created the inter diff from#40

@quietone
Here https://www.drupal.org/pift-ci-job/820668 The test was failing because of that.

+++ b/core/modules/migrate_drupal_ui/src/Form/MigrateUpgradeForm.php
@@ -67,6 +67,55 @@ class MigrateUpgradeForm extends ConfirmFormBase {
+  public static $noUpgrade = [

Recognizing that naming is hard, what about $noModuleUpgradePath?

I've done that in this latest patch and marked RTBC. I don't think renaming a variable should disallow that honor. All feedback is addressed. We have automated testing. There's been several rounds of manual testing.

Hmm, I guess we need a CR. So, no RTBC (yet).

+++ b/core/modules/migrate_drupal/src/Plugin/migrate/field/UserReference.php
--- a/core/modules/migrate_drupal_ui/src/Form/MigrateUpgradeForm.php
+++ b/core/modules/migrate_drupal_ui/src/Form/MigrateUpgradeForm.php

+++ b/core/modules/migrate_drupal_ui/src/Form/MigrateUpgradeForm.php
@@ -67,6 +67,55 @@ class MigrateUpgradeForm extends ConfirmFormBase {
+  public static $noModuleUpgradePath = [

Let's not use the name module. What about themes or profiles? And here's an attempt to remove standard and minimal in a more clean method.

Change record added.

If we add the no upgrade path array to the form state during the first step of the form, we then could alter it with the (modified) hook_form_alter() from the change record:

/**
 * Implements hook_form_FORM_ID_alter().
 */
function pirate_form_migrate_drupal_ui_form_alter(&$form, \Drupal\Core\Form\FormStateInterface $form_state, $form_id) {
  $no_upgrade_path = $form_state->get('no_upgrade_path');
  $no_upgrade_path[6][] = 'pirate';
  $no_upgrade_path[7][] = 'pirate';
  $form_state->set('no_upgrade_path', $no_upgrade_path);
}

I tested it manually and it works, but we should add automated testing.

Here's a simple test showing the hook_form_alter() in action.

Tests are now green, back to NR.

Some coding standard corrections.

Issue summary does not clearly explain what is going on here. Is this issue in need of a usability review or "just" hiding things?

1. +++ b/core/modules/migrate_drupal_ui/src/Form/MigrateUpgradeForm.php
   @@ -155,6 +204,9 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
    
   +    // Add the no upgrade path array to the form state so it can be altered.
   +    $form_state->set('no_upgrade_path', static::$noUpgradePath);
   

   This is not necessary. Anything with access to $form_state can simply do this:

   $form_object = $form_state->getFormObject();
   $form_object::$noUpgradePath
   

2. +++ b/core/modules/migrate_drupal_ui/src/Form/MigrateUpgradeForm.php
   @@ -507,9 +559,18 @@ public function buildConfirmForm(array $form, FormStateInterface $form_state) {
   +    $no_upgrade_path = $form_state->get('no_upgrade_path');
   +    foreach ($no_upgrade_path[$version] as $module) {
   +      $table_data[$module]['core'][$module] = $module;
        }
   

   We can just use static::$noUpgradePath. No need to bring $form_state into it.

   Also, we need to be sure that $version is a string. I think that $arr[6] and $arr['6'] are treated differently by PHP...

3. +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/MigrateUpgradeTestBase.php
   @@ -45,6 +46,20 @@
   +  public static $missing_count;
   +
   +  /**
   +   * Count of modules that will be upgraded.
   +   *
   +   * @var array
   +   */
   +  public static $available_count;
   

   Is there any particular reason these need to be public and static?

4. +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/d7/MigrateUpgrade7Test.php
   @@ -123,7 +129,27 @@ protected function getAvailablePaths() {
   +      // views_ui is added to this list by
   +      // migration_provider_test_form_migrate_drupal_ui_form_alter().
   

   Can we, or do we already, assert that this happens?

1. The problem with accessing the static property from the form object directly in the hook_form_alter() is that it has no effect. For what I understand, each time the form is rebuilt for a new step, the static property is reseted back to the default value, and then realtered from the hook. But, at the moment we use the static property to move modules from the missing table to the available table, the property is still unaltered. That was the idea behind using the $form_state, so it stored in state and not reseted each time the form is rebuilt.
2. Idem.
3. I don't understand why we would need those hardcoded counts. Couldn't we use the existing getAvailablePaths() and getMissingPaths() and count the returned array?
4. Right, this have no effect at all in the test. I thought that this list was used in the count of the available paths. We should add a dedicated assert to test the altering of the noUpgradePath property.

The problem with accessing the static property from the form object directly in the hook_form_alter() is that it has no effect. For what I understand, each time the form is rebuilt for a new step, the static property is reseted back to the default value, and then realtered from the hook. But, at the moment we use the static property to move modules from the missing table to the available table, the property is still unaltered. That was the idea behind using the $form_state, so it stored in state and not reseted each time the form is rebuilt.

Ah, okay. That makes sense. I totally realized this after I posted the comment :)

I updated the summary slightly since the problem explained included the proposed resolution, while the proposed resolution did not explain the user change. There is still an attempt at two images in the summary, only the second works, and it is not clear if that is a before or after GIF.

Also the "check if contrib modules can add to the list" is a "Needs tests" in fact :)

In this patch, I make sure that contrib modules can alter the $no_upgrade_path list with a hook_form_alter() and that we test it.
The failing patch shows that without the hook_form_alter(), the tests are failing.

Also:

• The $noUpgradePath property does not need to be plublic static anymore, so I changed it to protected.
• I make sure that the $version variable is a string so we can do $no_upgrade_path[$version] safely.
• Get rid of the harcoded missing and available counts, and use the returning arrays of getMissingPaths() and getAvailablePaths().

I think the IS is much better now, thanks @quietone! Only one question, in the first paragraph RDF is used as an example of a module that does not need an upgrade path, but we still see it in the after screenshot. Should it be added to the $no_upgrade_path array?

+++ b/core/modules/migrate_drupal_ui/src/Form/MigrateUpgradeForm.php
@@ -483,7 +482,7 @@ public function validateCredentialForm(array &$form, FormStateInterface $form_st
+        if ($version == '6') {

We should use === here, because string comparison can occasionally blow up in weird ways. #PHPWTF

Here you go! I also change another string comparison to be consistent.

+++ b/core/modules/migrate_drupal_ui/src/Form/MigrateUpgradeForm.php
@@ -661,9 +712,23 @@ public function buildConfirmForm(array $form, FormStateInterface $form_state) {
+    $no_upgrade_path = $form_state->get('no_upgrade_path');
+    foreach ($no_upgrade_path[$version] as $module) {

Super nit: Instead of $no_upgrade_path and $module might we try $no_upgrade_paths and $no_upgrade_path?

Making the form state and everything use plural vs singular? Then we avoid calling this 'module', when themes or install profiles could also use this same technique. All this is per the title of the associated CR.

+++ b/core/modules/migrate_drupal_ui/tests/modules/upgrade_form_alter_test/upgrade_form_alter_test.module
@@ -11,8 +11,8 @@
+  $form_state->set('no_upgrade_path', $no_upgrade_paths);

Typo, 's' missing.

Honestly? Looks pretty good to me. Maybe we could add a functional test of the new form_alter stuff? That's about all that might help make the case for this to land soon. But otherwise...seems pretty straightforward.

+++ b/core/modules/migrate_drupal_ui/tests/src/Functional/d6/MigrateUpgrade6Test.php
@@ -93,55 +93,61 @@ protected function getAvailablePaths() {
-  protected function getMissingPaths() {
-    return [
...
+      // To test that contrib modules can alter this list, Event is added here
+      // by a hook_form_alter().
+      // See upgrade_form_alter_test_form_migrate_drupal_ui_form_alter().
+      'event',
+    ];
...
+  protected function getMissingPaths() {
+    return [
+      // To test that contrib modules can alter this list, Event is removed from
+      // here by a hook_form_alter().
+      // See upgrade_form_alter_test_form_migrate_drupal_ui_form_alter().
+      // 'event',
...
     ];
   }

+++ b/core/modules/migrate_drupal_ui/tests/src/Functional/d7/MigrateUpgrade7Test.php
@@ -121,33 +118,40 @@ protected function getAvailablePaths() {
-  protected function getMissingPaths() {
-    return [
...
+      // To test that contrib modules can alter this list, Book is added here
+      // by a hook_form_alter().
+      // See upgrade_form_alter_test_form_migrate_drupal_ui_form_alter().
+      'book',
+    ];
+  }
...
+  protected function getMissingPaths() {
+    return [
+      // To test that contrib modules can alter this list, Book is removed from
+      // here by a hook_form_alter().
+      // See upgrade_form_alter_test_form_migrate_drupal_ui_form_alter().
+      // 'book',
...
     ];
   }

Do you mean a dedicated functional test outside of MigrateUpgradeTestBase? Because the form_alter stuff is already tested by the code above. There was a failing test only patch in #102 to show that this is tested.

Fair enough, and clear upon a closer re-read. This patch looks good to me, I mostly have documentation nits.

1. +++ b/core/modules/migrate_drupal_ui/src/Form/MigrateUpgradeForm.php
   @@ -69,6 +69,54 @@ class MigrateUpgradeForm extends ConfirmFormBase {
   +   * List of modules that do not need an upgrade path.
   +   */
   +  protected $noUpgradePaths = [
   

   This comment should also explain why this array has two sub-arrays, and what the keys mean.

2. +++ b/core/modules/migrate_drupal_ui/src/Form/MigrateUpgradeForm.php
   @@ -661,9 +712,23 @@ public function buildConfirmForm(array $form, FormStateInterface $form_state) {
   +    foreach ($no_upgrade_paths[$version] as $no_upgrade_path) {
   

   $no_upgrade_path is not a very good name for this variable. Can we call it $module or $extension instead?

3. +++ b/core/modules/migrate_drupal_ui/tests/modules/upgrade_form_alter_test/upgrade_form_alter_test.module
   @@ -0,0 +1,18 @@
   +function upgrade_form_alter_test_form_migrate_drupal_ui_form_alter(&$form, FormStateInterface $form_state, $form_id) {
   

   &$form should be type hinted. Type hint all the things! :)

4. +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/MigrateUpgradeTestBase.php
   @@ -37,6 +37,7 @@
   +    'config_translation',
        'content_translation',
        'migrate_drupal_ui',
        'telephone',
   @@ -44,7 +45,10 @@
   
   @@ -44,7 +45,10 @@
        'book',
        'forum',
        'statistics',
   +    'tracker',
   +    'update',
   

   Why were config_translation, tracker, and update added?

5. +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/d6/MigrateUpgrade6Test.php
   @@ -65,7 +65,7 @@ protected function getEntityCounts() {
   -      'tour' => 4,
   +      'tour' => 5,
   

   Why did this change?

6. +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/d6/MigrateUpgrade6Test.php
   @@ -93,55 +93,61 @@ protected function getAvailablePaths() {
   +      // See upgrade_form_alter_test_form_migrate_drupal_ui_form_alter().
   +      // 'event',
   

   We should remove the // 'event', line, and the "See" comment should begin with @see.

7. +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/d7/MigrateUpgrade7Test.php
   @@ -121,33 +118,40 @@ protected function getAvailablePaths() {
   +      // See upgrade_form_alter_test_form_migrate_drupal_ui_form_alter().
   +      // 'book',
   

   Same here.

Thanks for the review @phenaproxima!

1. Added a comment.
2. Changed to $extension
3. Added a type hint.
4. I added those modules because in the D6 fixtures, the I18profile and the Update modules are enabled, so they were showing in the missing paths table (the i18nprofile migration is in the config_translation module) and this was misleading because they do have an upgrade path. In the D7 fixtures, Tracker and Update are also enabled and they were also showing in the missing paths table while having one.
5. This is required since one more tour entity is being migrated because of the new enabled modules (I think it's the Locale tour, which is a dependency of config_translation)
6. Done.
7. Done.

Okay. I feel confident about it. RTBC once Drupal CI passes it.

@quietone: I see your point. We should indeed test that. We should not enabled the modules I added and move them from the available paths array to the missing paths array, and add a comment saying why the are missing, i.e. they are enabled on the source site but disabled on the destination site.

Left some modules uninstalled so we can test that they show up in the missing paths.

I read through the whole issue and checked the change record. I also discussed this with @quietone in IRC.

I have a couple of issues with this.

1. The issue summary currently says this:

This issue it to determine those modules that do not need an upgrade path and include them in the list of 'available paths'. These modules are not removed from the lists (they are not hidden). They are exposed so that the user, if they wish so, can check that each module enabled in the source will be upgraded, even if under the hood nothing will happen for some modules.

This is perfectly fine for modules like Help, Overlay etc. where nothing is upgraded and where nothing is needed.

2. The issue summary says

Also, contrib modules need to be able include themselves in this list.

Can we add a link to the CR saying that the CR provides an example?

3. According to the CR, we are offering a mechanism for contribs to hide their module from the lists altogether.

1. This is contradicting what we are saying in the issue summary
2. According the IRC discussion I had with @quietone, the core Views module is using this same hiding mechanism. So Views will not appear in either one of the lists.
3. I find this extremely confusing. There are actions that the site builder needs to do (i.e. manually configure the views that are needed) so hiding the module from the report is not very nice thing to do in my opinion...

Cheers,
Markus

Conceptual poroposal:

1. Modules like Help that will never have an upgrade path because there's nothing to migrate: Let's show this as 'upgrade available' like we are now doing with the latest patch.

2. Modules like Views thst will never have an upgrade path and site builder might need to take manual action: Let's show these in a separate list + link to handbook where we will explain for example that Views need to be manually configured in D8.

Cheers,
Markus

Thanks for the additional clarifications @quietone! And apologies for everyone that I jumped into this at a very late stage of this... :/

I'm not suggesting that we try to cover all of these in one and the same issue but is it so that conceptually we have the following scenarios:

Scenario 1. Upgrade path is available

• Examples: Block, Address

Scenario 2. There is nothing to be migrated

• Examples: Help, Overlay
• This is what this issue is all about if I'm understanding this correctly.
• No manual actions are supposed to be required by the site builder.

Scenario 3. There is no upgrade path and most probably never will

• Examples: Views
• Site builder should evaluate what does the missing upgrade path mean in his/her use case.
• Depending on the module, the site builder may need to take manual actions such as creating the views in D8.
• This is different from 4 in the sense that the module maintainer has explicitly declared that there will never be an automatic upgrade path.

Scenario 4. There is no upgrade path because no migrations were detected

• Examples: Color
• Site builder should evaluate what does the missing upgrade path mean in his/her use case.
• This is different from 3 in the sense that we know that at runtime, there is no migration detected for the module but we don't have any other information from the module in question.

Scenario 5. Incomplete upgrade path

• Examples: I can't figure out an example at quick thinking
• The module has some migrations but in addition to this some manual actions are required
• The follow-up is about this: #2928147: Migrate UI - add 'incomplete' migration status

Now, if we think about these from the UI design point of view. We currently have two lists: 'Upgrade path available' and 'Upgrade path missing'. #2928147: Migrate UI - add 'incomplete' migration status will eventually add a third list.

If we think about the 5 scenarios above, could we do as follows:

Scenario 1. Upgrade path is available

• Show these under 'Upgrade path available' like today.
• In other words, the list will show the Drupal 6/7 module and the corresponding Drupal 8 module(s).

Scenario 2. There is nothing to be migrated

• Show these under 'Upgrade path available' like suggested in this patch.
• EDIT / Addition: Module must alway explicitly opt-in to this category.
• EDIT / Addition: Otherwise it will appear under 'Upgrade path missing'. Alternatively we can keep this under 'Upgrade path missing' with a label 'No migration needed' or something similar..

Scenario 3. There is no upgrade path and most probably never will

• Show these under 'Upgrade path missing'.
• Instead of the word 'Missing', let's use some other word to indicate that there will never be an automatic upgrade path. This could even be a hyperlink to a given URL which then provides module-specific instructions on what the site builder could / should do manually.
• The module must always explicitily opt-in to this status and provide the URL for further instructions.

Scenario 4. There is no upgrade path because no migrations were detected

• Show these under 'Upgrade path missing' with a label 'missing' like we do today

Scenario 5. Incomplete upgrade path

• These currently appear under 'Upgrade path available'
• #2928147: Migrate UI - add 'incomplete' migration status will add better handling for these.

Cheers,
Markus

Edit: Additions marked above.

@masipila: I agree whit what you are proposing and I like your thorough analysis, as always.

Let's clarify what you would like to see happening in this patch. You think that some modules, like Views, do not belong to the 'no upgrade path' list?

Here are the current 'no upgrade path' lists for D6 & D7, we should agree on which one should stay there, which one should be removed to be added in the upcoming 'incomplete paths' list, and which one should be removed because they should not be marked as available (like Views).

Drupal 6

• blog
• color
• date_api
• date_timezone
• help
• i18n
• i18nstrings
• imageapi
• number
• openid
• php
• poll
• profile
• tracker
• trigger
• variable
• variable_admin
• views
• views_export
• views_ui

Drupal 7

• blog
• contextual
• dashboard
• date_api
• entity
• field_ui
• help
• openid
• overlay
• php
• poll
• profile
• simpletest
• syslog
• toolbar
• trigger
• views
• views_ui

Okay, here's a quickly skecthed UI mockup.

1. Let's change the title of the 'Missing upgrade paths' to simply 'Warnings'

2. Let's change the explantory text to 'The following modules will not be upgraded. Refer to Upgrade to Drupal 8 handbook for more information.'

3. In the 'Warnings' there would be two kinds of elements:

• Default would be 'Missing' like today.
• If the module explicitely opts-in, we can show 'Automatic upgrade not possible' which is a link to a page where more information will be provided on what the site builder needs to do. Views falls into this category but contribs should also be able to opt-in to this.

4. In the 'Available upgrade paths' box there would be two kinds of elements:

• Modules that have an upgrade path would be shown like today (e.g. addressfield, block in the UI mockup).
• Those modules which do not need any upgrade (e.g. help in the UI mockup) would be shown as 'No upgrade needed'.

5. For now, we can show the 'incomplete' modules in 'Available' box. Later in the follow-up #2928147: Migrate UI - add 'incomplete' migration status, we can decide what to do about them.

TODOs on the conceptual level:

Technical considerations:

• From the technical point of view the biggest open topic is probably the mechanism how core / contrib modules can opt-in to the 'Automatic upgrade not possible' or 'No upgrade needed' categories.

Cheers,
Markus

How about "28 Checked" instead of 28 Available Upgrade Paths?

Updated the issue title to be more accurate + updated issue summary.

I agree with @quietone, we should stop increasing the scope, do what we need to get this in, and create more follow ups if there's more thing we want to improve.

If we do not want to move views and other modules that can't be automatically upgraded into the available paths table, we'll remove them from the $noUpgradePaths list and leave them in the missing paths table for now.

I discussed this again briefly with @quietone and I do agree with her and @maxocub that let's limit the scope here.

#126:1-2. As quietone mentioned, these are part of #2922701: Migrate UI - refer to modules and add help text. Sorry for confusion, I blame pre-xmas stress.

#126.3: 'Automatic upgrade not possible' (case Views). This can be further evaluated in the follow-up #2932652: Migrate UI - handle extensions that can't be automatically upgraded. Let's leave Views in the 'Missing' list for now.

#126.4: I don't find it necessary to add the 'No updgrade needed' label for Help, Overlay and similar. I'm OK how they are now in the latest patch.

#126.5: This out of scope here and belongs to the follow-up #2928147: Migrate UI - add 'incomplete' migration status

Summa summarum:
The only remaining thing from my point of view under this issue is the fact that triggered me from #120 onwards and that where we show Views. Views belongs to the 'upgrade path NOT OK' list, it must not be appearing in the 'upgrade path OK' list.

Cheers,
Markus

I updated the change record draft and removed the reference to Views.

Image shows up as an available upgrade path, even when the destination image module is disabled. This leads to issues when upgrading Drupal, since it tries to migrate the user profile images to an image field, of which type does not exist.

That would be because of:

/**
 * @MigrateField(
 *   id = "image",
 *   core = {7},
 *   source_module = "image",
 *   destination_module = "file"
 * )
 */

It seems like we need to fix that maybe? And possibly move it to the image module from file? This could bump to a new issue if we think its worth it.

@heddn: I agree that is definitely a problem and this should not be committed with that in the patch. We should factor that into a new issue and not bother with that here then.

I've opened up #2934145: Image field migration plugin in wrong module