Problem/Motivation

The Migrate Drupal UI module provides the web browser user interface for upgrading from Drupal 6 / 7 to Drupal 8. The UI is available in /upgrade. We have a pre-upgrade analysis where we analyze which modules have an upgrade path to Drupal 8. It uses the 'source_module' and 'destination_module' properties on each migration along with the system table of the source database to determine which modules will be upgraded and which will not.

Current behavior

  • If a module is enabled in the source and a migration exists then that module will be listed in 'available paths'
  • If not, the module is displayed in 'missing paths'.
  • See the screenshot below.

Issue 1: There are modules such as Help, Overlay, Toolbar and many others where we don't need any migrations at all. Currently these modules are listed under 'missing upgrade paths' without any additional information even though no actions are required from the site builder. This is not ideal user experience.

Issue 2: There are modules such as Views where there is no automatic upgrade path. If we use Views as an example, the site builder needs to configure the desired views manually in D8 after the upgrade has been executed. Currently we don't have any way how a module could provide a link for further information on how to handle the upgrade when automatic upgrade is not possible. A follow-up has been created to evaluate this further: #2932652: Migrate UI - handle extensions that can't be automatically upgraded

Issue 3: It might be possible that a module requires multiple migrations but only some of them are available. Handling this corner case ('Incomplete' upgrade path) is out of scope for this issue and will be handled separately in #2928147: Migrate UI - add 'incomplete' migration status.

Proposed resolution

We currently have two lists 'Upgrade analysis OK' and 'Upgrade analysis NOT OK'. UI improvements to these lists are part of #2922701: Migrate UI - refer to modules and add help text.

Proposed resolution for Issue 1 described above (modules that do not need migrations at all)

  • Let's move modules like Help, Overlay, Toolbar to the 'OK' list.
  • Any core / contrib module can explicitly opt-in to this category, see an example in the change record.

Proposed resolution for Issue 2 described above (modules that will never have an automatic upgrade path)
Not in the scope of this issue. See the follow-up #2932652: Migrate UI - handle extensions that can't be automatically upgraded

Remaining tasks

Ensure that handling for Views is sensible
Review
Commit

User interface changes

Yes, modules that do not need an upgrade will be moved to the 'OK' list.

API changes

N/A

Data model changes

N/A

CommentFileSizeAuthor
#183 interdiff.txt8.95 KBquietone
#183 2914974-183.patch44.53 KBquietone
#173 interdiff.txt1.18 KBquietone
#173 2914974-173.patch47.84 KBquietone
#170 interdiff.txt1.12 KBquietone
#170 2914974-170.patch48.13 KBquietone
#168 2914974-168.patch47.76 KBquietone
#166 interdiff.txt9.86 KBquietone
#166 2914974-166.patch48.29 KBquietone
#164 2914974-163.patch47.36 KBquietone
#158 interdiff.txt6.44 KBquietone
#158 2914974-158.patch48.26 KBquietone
#157 interdiff-150-155.txt3.75 KBquietone
#157 2914974-157.patch47.68 KBquietone
#155 interdiff.txt656 bytesquietone
#155 2914974-155.patch46.74 KBquietone
#150 interdiff.txt2.34 KBquietone
#150 2914974-150.patch47.64 KBquietone
#148 interdiff.txt2.76 KBquietone
#148 2914974-148.patch46.77 KBquietone
#147 2914974-147.patch46.3 KBheddn
#147 interdiff_145-147.txt1.65 KBheddn
#145 interdiff.txt1.26 KBquietone
#145 2914974-145.patch46.42 KBquietone
#143 interdiff.txt1.66 KBquietone
#143 2914974-143.patch46.46 KBquietone
#141 interdiff.txt32.65 KBquietone
#141 2914974-141.patch46.09 KBquietone
#140 interdiff.txt20.91 KBquietone
#140 2914974-140.patch35.04 KBquietone
#126 2914974-125.png25.77 KBmasipila
#118 interdiff-2914974-114-118.txt3.98 KBmaxocub
#118 2914974-118.patch15.86 KBmaxocub
#114 interdiff-2914974-110-114.txt4.64 KBmaxocub
#114 2914974-114.patch16.31 KBmaxocub
#110 interdiff_107-110.txt816 bytesheddn
#110 2914974-110.patch16.16 KBheddn
#107 interdiff_105-107.txt2.8 KBheddn
#107 2914974-107.patch16.16 KBheddn
#105 interdiff-2914974-102-105.txt1.41 KBmaxocub
#105 2914974-105.patch16.15 KBmaxocub
#102 interdiff-2914974-100-102.txt13.66 KBmaxocub
#102 2914974-102.patch15.76 KBmaxocub
#102 2914974-102-failing-patch.patch15.77 KBmaxocub
#100 interdiff.txt1.46 KBquietone
#100 2914974-100.patch12.38 KBquietone
#99 Upgrade_analysis_report_after.png19.42 KBquietone
#99 Upgrade_analysis_report_before.png33.63 KBquietone
#92 interdiff-2914974-87-92.txt1.88 KBmaxocub
#92 2914974-92.patch11.92 KBmaxocub
#87 interdiff-2914974-86-87.txt2.05 KBmaxocub
#87 2914974-87.patch11.87 KBmaxocub
#86 interdiff-2914974-83-86.txt1.39 KBmaxocub
#86 2914974-86.patch10.9 KBmaxocub
#83 interdiff.txt1002 bytesquietone
#83 2914974-83.patch10.38 KBquietone
#81 interdiff.txt1.43 KBquietone
#81 2914974-81.patch10.38 KBquietone
#78 interdiff-2914974-76-78.txt996 bytesrakesh.gectcr
#78 2914974-78.patch10.62 KBrakesh.gectcr
#76 interdiff-2914974-74-76.txt1.09 KBrakesh.gectcr
#76 2914974-76.patch10.62 KBrakesh.gectcr
#74 interdiff_72-74.txt689 bytesheddn
#74 2914974-74.patch10.85 KBheddn
#72 interdiff_71-72.txt2.01 KBheddn
#72 2914974-72.patch10.92 KBheddn
#71 interdiff_70-71.txt1.06 KBheddn
#71 2914974-71.patch10.32 KBheddn
#70 interdiff.txt2.22 KBquietone
#70 2914974-70.patch10.36 KBquietone
#66 interdiff-2914974-64-66.txt538 bytesrakesh.gectcr
#66 2914974-66.patch10.92 KBrakesh.gectcr
#64 interdiff-2914974-62-64.txt900 bytesrakesh.gectcr
#64 2914974-64.patch10.92 KBrakesh.gectcr
#62 interdiff-2914974-60-62.txt410 bytesrakesh.gectcr
#62 2914974-62.patch10.72 KBrakesh.gectcr
#61 MigrateUIUpgradePath.gif805.13 KBrakesh.gectcr
#60 interdiff-2914974-40-60.txt3.41 KBrakesh.gectcr
#60 2914974-60.patch10.74 KBrakesh.gectcr
migrationupgradeui.gif73 bytesrakesh.gectcr
#57 interdiff-2914974-54-57.txt632 bytesrakesh.gectcr
#57 2914974-57.patch10.9 KBrakesh.gectcr
#55 interdiff-2914974-52-54.txt897 bytesrakesh.gectcr
#55 2914974-54.patch10.95 KBrakesh.gectcr
#52 interdiff-50-52.txt1009 bytesJo Fitzgerald
#52 2914974-52.patch11 KBJo Fitzgerald
#50 interdiff-2914974-47-50.txt435 bytesrakesh.gectcr
#50 2914974-50.patch10.77 KBrakesh.gectcr
#47 interdiff-2914974-44-47.txt410 bytesrakesh.gectcr
#47 2914974-47.patch10.73 KBrakesh.gectcr
#45 interdiff-2914974-40-44.txt2.91 KBrakesh.gectcr
#45 2914974-44.patch10.72 KBrakesh.gectcr
#40 interdiff.txt6.52 KBquietone
#40 2914974-40.patch10.17 KBquietone
#38 interdiff.txt2.62 KBquietone
#38 2914974-38.patch5.75 KBquietone
#35 D6ModuleList.ods13.57 KBquietone
#22 interdiff.txt2.55 KBquietone
#22 2914974-22.patch3.86 KBquietone
#20 migrate_ui_handle-2914974-20.patch1.67 KBYogesh Pawar
#16 2914974-16.patch1.67 KBquietone
#12 2914974-11.patch1.56 KBquietone
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

quietone created an issue. See original summary.

phenaproxima’s picture

Priority: Major » Critical

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

quietone’s picture

Yep, forgot that.

maxocub’s picture

Status: Needs review » Postponed

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.

heddn’s picture

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.

heddn’s picture

Follow-up should be major, not critical.

heddn’s picture

xjm’s picture

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).

phenaproxima’s picture

Issue tags: +Needs followup

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.

phenaproxima’s picture

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

jhodgdon’s picture

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...

quietone’s picture

Yea, I know it is postponed. No need for testing, it will need a reroll when #2859304: Show field type migrations correctly in Migrate Drupal UI.

quietone’s picture

Issue summary: View changes

Add a note about the likely follow up issue

phenaproxima’s picture

Status: Postponed » Needs review
quietone’s picture

This is far from complete. I only added a couple of modules without migration paths, more need to be added.

quietone’s picture

The modules with no upgrade paths now appear in the available list with a destination of 'core'. Wasn't sure what to use for the destination and settled on 'core' as the best option. For me, if I was using the form and saw 'core' that would tell me that whatever is in that module is being dealt with.

The following lists the modules that still appear in the missing list. If there is no issue list, then these are the ones that still need some investigation.

Drupal 6

Drupal 7

Status: Needs review » Needs work

The last submitted patch, 16: 2914974-16.patch, failed testing. View results

quietone’s picture

#2859304: Show field type migrations correctly in Migrate Drupal UI has been committed to 8.5.x, so can't really work on this on 8.4.x. Is this now 8.5.x?

phenaproxima’s picture

Version: 8.4.x-dev » 8.5.x-dev

I think so, yes.

Yogesh Pawar’s picture

Status: Needs work » Needs review
FileSize
1.67 KB

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

phenaproxima’s picture

Status: Needs review » Needs work
+++ 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.

quietone’s picture

Status: Needs work » Needs review
FileSize
3.86 KB
2.55 KB

d6_no_upgrade_path and d7_no_upgrade_path are now static. And tests added for the UI.

quietone’s picture

Issue summary: View changes

Update IS

phenaproxima’s picture

Issue tags: -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.

maxocub’s picture

Status: Needs review » Needs work

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.
quietone’s picture

My thoughts turned to this while at Orchestra Wellington last night and I'll put my thoughts here, though some maybe outside the scope of this issue, but still about the review page. I tried to put myself in the shoes of someone who has minimal experience and wants to upgrade a Drupal 6 or Drupal 7 site. As that person, when I get to the review page I just want to know if the modules I have will be upgraded, yes or no. Not available (does that mean I didn't install something?), missing (who lost it?), ready (that's nice) or partial (Oh no!). Just "these modules will be upgraded" and "these modules will not be upgraded". And that leads me to think that the text on the page should refer only to modules, not paths and items.

Then, if a module is not to be upgraded, a link to some explanation of the situation would be helpful as has already been discussed. Here the 'partial' idea makes sense. And if the module is to upgraded I don't really care about the 'Upgrade module'. And, if I look, I see that some modules when upgraded are now 2 or even 3 modules. That is unexpected but the page tells me the module will be upgraded so it must be OK.

Now, back to being me. What do I suggest for the constants? I conferred with my partner on this taking care to avoid any of the suggestions in #25 and my own preferences. When I asked what names for the categories, he suggested 'will be upgraded', will not be upgraded' and 'incomplete'. (Which is nice because that is what I had chosen). Then I asked what about 'available'? He immediately said, 'Do I have a choice?'. Which surprised me, I am so used to seeing available on this page that I hadn't thought of 'available' as a choice, as in 'available products'. And the review page is not about choices, so available doesn't fit. Also, at first, he liked 'partial', until we realized that my explanation of that group of modules wasn't accurate. Once that was sorted out, he suggested 'incomplete'.

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

Regarding keeping the 2 summary elements, I would prefer 3 categories and 3 tables. The modules that will not be upgraded are errors from an upgrade viewpoint, at least to me. The 'partials' would be 'warnings', and the rest OK. But that is just my thoughts, it is an area I now little about, and therefor will go with whatever those more knowledgeable than me decide.

jhodgdon’s picture

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.

phenaproxima’s picture

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.

quietone’s picture

Assigned: Unassigned » quietone

Assigning to myself.

quietone’s picture

Status: Needs work » Needs review

Been pondering this again.

Mostly I am unsure of the scope. Is this just about modules that do no need an upgrade path, as in the IS. Or about the changes from #25 - #28, about constants and text on the screen? I'd like to keep this within the scope of the current IS and move the other bits to a new issue. Comments?

Setting to NR for comments.

quietone’s picture

Status: Needs review » Needs work

Moved the work on the UI text to a new issue #2922701: Migrate UI - refer to modules and add help text.

Back to NW.

quietone’s picture

date_timezone module - uses the variable 'date_default_timezone'. That variable is in the migration d6_system_date, along with other date variables, with source module of 'system'. Let's leave that and take phenaproxima's suggestion and put date_timezone in the list that 'will be upgraded'.

i18ncck - allows for translating fields, so needs a migrations
i18ncontent - allows for translating content, so needs a migration

That leaves event. Anyone know about event?

heddn’s picture

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

jhodgdon’s picture

Yeah, we shouldn't worry about event.

quietone’s picture

Assigned: quietone » Unassigned
FileSize
13.57 KB

Thanks for the replies. There is a new snag in this. The UI filters out modules that are not enabled on the source site, and in the d6 test fixture that means ~40 modules are not included at all. I've yet to look at the D7 source. Seems to me we should enable all the modules all of the necessary modules so it is easier to test. But does the test fixture have all the necessary modules? Can someone confirm that? Note that devel is listed in the system table and should be removed.

Another thing I found is that some declarations in 'source_module' do not match up to an existing module name in the D6 test db. These are listed

  • entityreference
  • file
  • image
  • language
  • list
  • node_reference
  • options
  • simpletest
  • user_reference

I know image is a D6 module. What of the others? Should they be added to the fixture?

The attached spreadsheet has the full list of module names from the system table, the status and the upgrade status.

quietone’s picture

Status: Needs work » Needs review

Setting to NR to get feedback on #35

jhodgdon’s picture

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.

quietone’s picture

Realized that the list in #35 are all from field plugins, and thanks to jhodgdon's explanation, they are a mix of d6 and d7 ones. They should not be. They should be either d6 or d7. The code the gets the field plugins was not checking the core version of the plugin. This patch fixes that.

Also, changed the source_module for the d6 nodereference and userreference migrate field plugins by removing the underscore.

I also updated the no upgrade path for both 6 and 7 as best I could based on the test fixtures, which is important here. If a module is not enabled in the test fixture it will not show up in the Missing module list. The test fixture should be sufficient to the task, but is it? Are all the necessary modules enabled or have tables been added without updating the system table? I don't know and because of my unfamiliarity with d6/d7 it would take a long time to sort out. Perhaps someone else can take a look at the dumps?

Status: Needs review » Needs work

The last submitted patch, 38: 2914974-38.patch, failed testing. View results

quietone’s picture

Status: Needs work » Needs review
FileSize
10.17 KB
6.52 KB

Updated MigrationProvidersExistTest to use the correct name for the userreference and nodereference plugin. Since the tests MigrateUpgradeTestBase only tests the module names provided in the arrays it is possible a module appears in the list and is not tested at all. Because of the an assertion is added for the number of missing and available paths.

Status: Needs review » Needs work

The last submitted patch, 40: 2914974-40.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

heddn’s picture

Issue summary: View changes
Issue tags: -Needs followup

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.

rakesh.gectcr’s picture

Assigned: Unassigned » rakesh.gectcr
rakesh.gectcr’s picture

Status: Needs work » Needs review

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

rakesh.gectcr’s picture

Status: Needs review » Needs work

The last submitted patch, 45: 2914974-44.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

rakesh.gectcr’s picture

Hitting it again

rakesh.gectcr’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 47: 2914974-47.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

rakesh.gectcr’s picture

Status: Needs work » Needs review
FileSize
10.77 KB
435 bytes

:)

Status: Needs review » Needs work

The last submitted patch, 50: 2914974-50.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Jo Fitzgerald’s picture

Status: Needs work » Needs review
FileSize
11 KB
1009 bytes

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.

Status: Needs review » Needs work

The last submitted patch, 52: 2914974-52.patch, failed testing. View results

rakesh.gectcr’s picture

@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.

rakesh.gectcr’s picture

Status: Needs work » Needs review
FileSize
10.95 KB
897 bytes

Status: Needs review » Needs work

The last submitted patch, 55: 2914974-54.patch, failed testing. View results

rakesh.gectcr’s picture

Status: Needs work » Needs review
FileSize
10.9 KB
632 bytes

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

Status: Needs review » Needs work

The last submitted patch, 57: 2914974-57.patch, failed testing. View results

rakesh.gectcr’s picture

Status: Needs work » Needs review
FileSize
10.74 KB
3.41 KB

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

rakesh.gectcr’s picture

Issue summary: View changes
FileSize
805.13 KB

rakesh.gectcr’s picture

Status: Needs review » Needs work

The last submitted patch, 62: 2914974-62.patch, failed testing. View results

rakesh.gectcr’s picture

Status: Needs work » Needs review
FileSize
10.92 KB
900 bytes

Status: Needs review » Needs work

The last submitted patch, 64: 2914974-64.patch, failed testing. View results

rakesh.gectcr’s picture

Status: Needs work » Needs review
FileSize
10.92 KB
538 bytes
quietone’s picture

+++ b/core/modules/tracker/migrations/d7_tracker_node.yml
@@ -4,6 +4,7 @@ migration_tags:
+  source_module: tracker

This shouldn't be necessary, the annotation on the source plugin d7_tracker_node has defined source_module. The same is true for d7_tracker_user. Or, am I missing something?

The same is true for d7_tracker_user

Status: Needs review » Needs work

The last submitted patch, 66: 2914974-66.patch, failed testing. View results

rakesh.gectcr’s picture

Status: Needs work » Needs review

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

quietone’s picture

I've not thought about why adding source_module to the two tracker migrations would allow the test to pass but since the source_module is in the annotation of the plugin used by those migrations I looked elsewhere.

The tracker module has an upgrade path and should not be in the noUpgrade path list. The tracker module is not enabled in the test fixture so it should be listed in the missing paths on the overview page. I've made a new patch based on that.

heddn’s picture

Issue tags: +Needs change record
FileSize
10.32 KB
1.06 KB
+++ 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).

heddn’s picture

Issue tags: -Needs change record
FileSize
10.92 KB
2.01 KB
+++ 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.

Status: Needs review » Needs work

The last submitted patch, 72: 2914974-72.patch, failed testing. View results

heddn’s picture

Status: Needs work » Needs review
FileSize
10.85 KB
689 bytes

Status: Needs review » Needs work

The last submitted patch, 74: 2914974-74.patch, failed testing. View results

rakesh.gectcr’s picture

Status: Needs work » Needs review
FileSize
10.62 KB
1.09 KB

Status: Needs review » Needs work

The last submitted patch, 76: 2914974-76.patch, failed testing. View results

rakesh.gectcr’s picture

Status: Needs work » Needs review
FileSize
10.62 KB
996 bytes
quietone’s picture

Status: Needs review » Needs work

I think the changes made in #72 to remove standard and minimal is out of scope. There is an existing issue to improve the finding of, and removal of profiles from the system_data. #2918185: Don't display install profiles in the Migrate UI Before, the install profiles were completely removed from the list before calculating the unmigrated modules, so they never were displayed. But now we have install profiles included in the no upgrade path to avoid their display and that is new.

Setting to NW to remove the change to the profiles, and let that work happen in the other issue.

quietone’s picture

I've been a bit busy lately and maybe I missed something. There is a change record for a hook that is not in the patch? And I thought the method for contrib modules to declare their migration status was to be implemented in #2928147: Migrate UI - add 'incomplete' migration status? Will someone please clarify.

quietone’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
10.38 KB
1.43 KB

Updated the IS, it now shows the list of modules that should appear as 'missing'.

The attached patch removes the changes made for profiles and themes. Profiles are removed before processing and the themes are simply ignored. And note that the patch for handling profiles is now RTBC, #2918185: Don't display install profiles in the Migrate UI.

I completely missed that the hook is a form_alter. Not having used that hook before I tested using it to alter the display. To do this I added the hook to the address module (because it was there in my test environment) then went to /upgrade. It did not work. Some debugging and I see that the hook is called (at least I got that correct) but it is not called after clicking 'Review upgrade' on the database credential page, which is when it is needed. Also, note that the form is @internal.

TODOs
1) Is the work allowing contrib to declare their upgrade status to be done here or in #2928147: Migrate UI - add 'incomplete' migration status. Actually, I don't care, just confused by what I see as a change in plan.
2) If doing it here, then it needs to work. My testing shows it does not.

Status: Needs review » Needs work

The last submitted patch, 81: 2914974-81.patch, failed testing. View results

quietone’s picture

Status: Needs work » Needs review
FileSize
10.38 KB
1002 bytes

And adjust the counts accordingly

Status: Needs review » Needs work

The last submitted patch, 83: 2914974-83.patch, failed testing. View results

quietone’s picture

Status: Needs work » Needs review

The tests really did pass, setting to NR

maxocub’s picture

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.

maxocub’s picture

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

The last submitted patch, 86: 2914974-86.patch, failed testing. View results

The last submitted patch, 86: 2914974-86.patch, failed testing. View results

Status: Needs review » Needs work

The last submitted patch, 87: 2914974-87.patch, failed testing. View results

Jo Fitzgerald’s picture

Status: Needs work » Needs review

Tests are now green, back to NR.

maxocub’s picture

Assigned: rakesh.gectcr » Unassigned
FileSize
11.92 KB
1.88 KB

Some coding standard corrections.

Gábor Hojtsy’s picture

Status: Needs review » Needs work
Issue tags: +Needs issue summary update

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

quietone’s picture

Issue summary: View changes
Status: Needs work » Needs review
Issue tags: -Needs issue summary update

Updated the IS.
This just puts modules in the 'correct' list and therefor does not need a usability review.

phenaproxima’s picture

Status: Needs review » Needs work
  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?

maxocub’s picture

  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.
phenaproxima’s picture

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 :)

Gábor Hojtsy’s picture

Issue summary: View changes
Issue tags: +Needs issue summary update

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 :)

quietone’s picture

@Gábor Hojtsy, thank you.

I've added before and after screenshots of the missing path section of the page to the IS. Does it need the available path section? The modules listed in the noUpgradePath are moved to the available path section with an upgrade module of 'core'.

But I did notice something while making the screen shots. Overlay is not enabled on the D7 source site but appears in the available paths. I'm pretty sure that should not happen. It should only show modules enabled.

quietone’s picture

Status: Needs work » Needs review
FileSize
12.38 KB
1.46 KB

This now only adds modules with noUpgradePath to the available list if they are enabled on the source.

Status: Needs review » Needs work

The last submitted patch, 100: 2914974-100.patch, failed testing. View results

maxocub’s picture

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?

The last submitted patch, 102: 2914974-102-failing-patch.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

phenaproxima’s picture

+++ 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

maxocub’s picture

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

heddn’s picture

Status: Needs review » Needs work
+++ 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.

heddn’s picture

Status: Needs review » Needs work

The last submitted patch, 107: 2914974-107.patch, failed testing. View results

maxocub’s picture

+++ 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.

heddn’s picture

Status: Needs work » Needs review
FileSize
16.16 KB
816 bytes
phenaproxima’s picture

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.

maxocub’s picture

+++ 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.

phenaproxima’s picture

Status: Needs review » Needs work

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.

maxocub’s picture

Status: Needs work » Needs review
FileSize
16.31 KB
4.64 KB

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.
phenaproxima’s picture

Status: Needs review » Reviewed & tested by the community

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

quietone’s picture

Status: Reviewed & tested by the community » Needs work

In the D7 fixtures, Tracker and Update are also enabled and they were also showing in the missing paths table while having one.

Correct me if I am wrong but adding tracker to the installed module list means we are not testing the case where a module with a migration path is enabled on the source but disabled on the destination. Seems to me we should. Setting to NW to get feedback.

maxocub’s picture

@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.

maxocub’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
15.86 KB
3.98 KB

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

quietone’s picture

@maxocub, thank for making the patch! And great comments explaining why modules are in the missing paths or available path list.

I'd RTBC but I made a couple of patches here.

RTBC+1

masipila’s picture

Status: Needs review » Needs work
Issue tags: +Needs issue summary update

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

masipila’s picture

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

quietone’s picture

My thoughts on #120

2. Sure add a link to the IS, and the CR has the example
3.1 - Yes, we should 'hide' anything. When a module is enabled on the source, it should be in either the missing or available list.
3.2 - I must not have been clear on IRC. Testing with the patch and Views enabled on the source, Views appears in one of the lists, the available list. When Views, or any module is disabled in the source, it doesn't show up in either list, which is correct.
3.3 - It seems to me this problem here is how best to show Views to the person doing the upgrade. As masipila points out, if it is in the available list, as it is now, the user is given the false impression that there isn't work to do to migrate Views when, in fact, there is. It follows then that it should be in the 'missing' list but with additional information on the page (where?) directing them to a doc page about how to upgrade Views. See #27 for suggestions on adding such information.

And from #121
1. Yes
2. Since there is already an issue to add a third category, 'Incomplete' can we just modify the existing 'missing' path section to add the information? See #2928147: Migrate UI - add 'incomplete' migration status.

masipila’s picture

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

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

Cheers,
Markus

Edit: Additions marked above.

maxocub’s picture

@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
quietone’s picture

Thanks masipila for the analysis! I think that should be moved to the IS. Maybe I can do that later.

Regarding the list of modules in #124, I reckon there are more to add. Remember, that we are testing against databases that do not have all their modules enabled and those modules are therefor filtered out. For example, the D6 fixture has date_popup and filefield_meta in the system tables but these are not in the list. See #35, #38 and #42.

Maybe the test needs to modify the system_table to set status true for all the modules?

masipila’s picture

FileSize
25.77 KB

Okay, here's a quickly skecthed UI mockup.

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:

  • The list of modules @maxocub provided in the previous comment needs to be analyzed i.e. which modules belong to 'Automatic upgrade not possible' and which belong to 'No upgrade needed'.
  • I'm not 100% happy to the label 'Available upgrade paths' because now that is not quite accurate label as are also showing the 'No upgrade needed' modules in this box.
  • 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

    heddn’s picture

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

    masipila’s picture

    Title: Migrate UI - handle sources that never have an upgrade path » Migrate UI - handle sources that do not need an upgrade and sources that can't be automatically upgraded
    Issue summary: View changes
    Issue tags: -Needs issue summary update

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

    quietone’s picture

    masipila, Thanks for updating the IS.

    I am struggling to keep the various issues regarding this page separate. This started out with just adding noUpgradePath functionality, then the hook was added and now there is discussion on changing the text. What is the scope of this?

    And some thoughts on some of the points in 126.
    #126.1 - There is already an issue changing the titles of the missing and available paths section. How does this fit with the work done there? #2922701: Migrate UI - refer to modules and add help text
    #126.2 - This change is already made in the latest patch of #2922701: Migrate UI - refer to modules and add help text.
    #126.3 - This is 'module not upgraded' in the other issue. I'm not convinced that 'Automatic upgrade not possible' is better but I do agree that the user needs more information. And note that in #27 there is a recommendation to not use links. Oh, and there is a module for migrating views, so someone thinks it is possible.
    #126.4 - If different text is displayed for the modules then, I think, some help text is needed to explain what it means. But part of me wonders if the distinction needs to be made. As someone using the UI to upgrade, do I really care about the difference between help and taxonomy? I doubt it, I would just want to know that help and taxonomy will upgrade and I don't need to do anything afterwards.
    #126-5 - I think this is moot. There is currently no mechanism to identify a module as having an incomplete migration status.

    maxocub’s picture

    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.

    masipila’s picture

    Issue summary: View changes

    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

    masipila’s picture

    Title: Migrate UI - handle sources that do not need an upgrade and sources that can't be automatically upgraded » Migrate UI - handle sources that do not need an upgrade
    masipila’s picture

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

    quietone’s picture

    Thanks maxocub and masiplia for taking my concerns into account. The updated CR is much much better. I made a few tweaks as well.

    quietone’s picture

    Assigned: Unassigned » quietone

    I've been working on this, although not ready to post yet.

    heddn’s picture

    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.

    heddn’s picture

    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.

    Gábor Hojtsy’s picture

    @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.

    heddn’s picture

    quietone’s picture

    Assigned: quietone » Unassigned
    Status: Needs work » Needs review
    FileSize
    35.04 KB
    20.91 KB

    This adds a completely new test so that all modules in the source can be enabled and we can see what is listed in the available and missing sections of the review form. I did that so we are sure to properly handle the modules in the text fixtures, that is, have an accurate noUpgradePath array for d6 and d7. This assumes that the test fixtures include all the modules that have been moved to D8 core. The new test is a modified copy of MigrateUpgradeTestBase.php and still duplicates a lot of that code, so there is more clean up to do.

    I did find that d6_node_translation and d7_node_translation migrations have 2 source_module declarations. Not sure if that is intentional or not, but I removed the source_module statement from the yml, leaving the one in the source plugin annotation, and adjusted the test accordingly.

    quietone’s picture

    Cleanup of the duplicate code by making a new base class for these tests.

    Status: Needs review » Needs work

    The last submitted patch, 141: 2914974-141.patch, failed testing. View results
    - codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

    quietone’s picture

    Status: Needs work » Needs review
    FileSize
    46.46 KB
    1.66 KB

    Add @group statement to MigrateUpgradeTest.php and a couple of comment changes.

    Status: Needs review » Needs work

    The last submitted patch, 143: 2914974-143.patch, failed testing. View results
    - codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

    quietone’s picture

    Status: Needs work » Needs review
    FileSize
    46.42 KB
    1.26 KB

    Corrections for the profile module. It is installed on both d6 and d7 and has an upgrade path, in ProfileField.php.

    Still don't see the solution to the d6 file count changing.

    Status: Needs review » Needs work

    The last submitted patch, 145: 2914974-145.patch, failed testing. View results
    - codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

    quietone’s picture

    Status: Needs work » Needs review
    FileSize
    46.77 KB
    2.76 KB

    Fixed a few comments and the file entity count error. The method getSourceBase() needs to be declared in MigrateUpgrade6Test.

    Status: Needs review » Needs work

    The last submitted patch, 148: 2914974-148.patch, failed testing. View results

    quietone’s picture

    Status: Needs work » Needs review
    FileSize
    47.64 KB
    2.34 KB

    Rename the test running and added a missing comma.

    Status: Needs review » Needs work

    The last submitted patch, 150: 2914974-150.patch, failed testing. View results

    quietone’s picture

    Status: Needs work » Needs review

    Well, I wasn't sure about that anyway. I can't reproduce the error. Can anyone tell me how to reproduce it and a hint for a fix?

    quietone’s picture

    Status: Needs review » Needs work
    neclimdul’s picture

    +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/MigrateUpgradeTest.php
    @@ -2,35 +2,21 @@
    -abstract class MigrateUpgradeTestBase extends BrowserTestBase {
    +abstract class MigrateUpgradeTest extends MigrateUpgradeTestBase {
    

    The is the failure. Because it lost the Base and end ends with "Test" this test is trying to be run as a full test but it is still abstract so it can't be instantiated.

    quietone’s picture

    Status: Needs work » Needs review
    FileSize
    46.74 KB
    656 bytes

    @neclimdul, thanks.

    Another try, I want that class, MigrateUpgradeTest, to be abstract in the same way as the new class MigrateUpgradeReviewPageTestBase. So I spent more time comparing them and it turns out that it had an @group statement which was messing things up. And I did some renaming.

    Status: Needs review » Needs work

    The last submitted patch, 155: 2914974-155.patch, failed testing. View results

    quietone’s picture

    FileSize
    47.68 KB
    3.75 KB

    Well, that is embarrassing. Ignore the patch in #155. I have no idea how I managed to mess up so well.

    Try again.

    Edit. Sigh the interdiff is named incorrectly it is between 150 and 157 and I forgot to set to NR for the testbot.

    quietone’s picture

    Status: Needs work » Needs review
    FileSize
    48.26 KB
    6.44 KB

    Improve comments and tweaked the query that enables modules in the source db.

    phenaproxima’s picture

    Assigned: Unassigned » phenaproxima

    I'll give this a review; @heddn has volunteered to pick up any follow-ups that come out of my review.

    heddn’s picture

    heddn’s picture

    phenaproxima’s picture

    Status: Needs review » Needs work

    Still some work to do here. Kudos on the extremely extensive test coverage! It's a bit hard to wrap my head around, but it looks quite thorough.

    1. +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/MigrateUpgradeReviewPageTestBase.php
      @@ -0,0 +1,123 @@
      +   * The upgrade review form displays two types of information, one is possible
      +   * error messages for migrations that do not have a source_module defined and
      +   * the other is a list of modules and their upgrade status. In this test,
      +   * there should be no errors displayed of the first type because the relevant
      +   * test modules are not installed. The second type of information, the list
      +   * of modules and their upgrade status, is tested with all the modules in the
      +   * source test fixture enabled, except test and example modules, but including
      +   * simpletest. The test fixture is assumed to have the modules that have since
      +   * moved to Drupal 8 core. This is done to test that the noUpgradePath array
      +   * defined in MigrateUpgradeForm contains a complete list of modules moved
      +   * from previous versions of Drupal to Drupal 8 and that the modules are
      +   * displayed in the correct list, either the available or missing list.
      

      This needs a little bit of word-smithing, for clarity.

    2. +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/MigrateUpgradeTestBase.php
      @@ -284,6 +118,33 @@ protected function translatePostValues(array $values) {
      +   * @param $session
      

      $session should be type hinted, in every doc block and method signature where it is used.

    heddn’s picture

    Issue tags: +Needs reroll

    Needs a reroll. I cannot do that right now.

    quietone’s picture

    Status: Needs work » Needs review
    Issue tags: -Needs reroll
    FileSize
    47.36 KB

    1. Yea, I am not a wordsmith. But I had another attempt.
    2. Not sure I got this right.

    Sadly, the interdiff failed because it needed a reroll.

    phenaproxima’s picture

    Status: Needs review » Needs work

    Much improved, thanks! All I found were docs problems, really. :)

    1. +++ b/core/modules/migrate_drupal_ui/src/Form/MigrateUpgradeForm.php
      @@ -69,6 +69,101 @@ class MigrateUpgradeForm extends ConfirmFormBase {
      +   * This property is an array where the keys are the version of Drupal from
      +   * which we are upgrading and the values are arrays of extensions that do not
      

      Can we rephrase this a bit? "...where the keys are the major Drupal core version from which we are upgrading,...". There should be a comma after "upgrading". Also let's say "extension names" rather than just "extensions".

    2. +++ b/core/modules/migrate_drupal_ui/src/Form/MigrateUpgradeForm.php
      @@ -355,7 +453,7 @@ public function buildCredentialForm(array $form, FormStateInterface $form_state)
      +          ':input[name="version"]' => ['value' => '7'],
      

      Hooray for strict(ish) typing!

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

      :)

    4. +++ b/core/modules/migrate_drupal_ui/src/Form/MigrateUpgradeForm.php
      @@ -661,9 +759,23 @@ public function buildConfirmForm(array $form, FormStateInterface $form_state) {
      +      if (in_array($version, $definition['core'])) {
      

      In case anyone asks, we should *not* pass TRUE as the third argument here. Because the field plugin might have {"core" = "6"} or {"core" = 6} in its annotation, and we want both cases to work. It's not ideal, but it should be OK.

    5. +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/MigrateUpgradeExecuteTestBase.php
      @@ -2,107 +2,32 @@
      +abstract class MigrateUpgradeExecuteTestBase extends MigrateUpgradeTestBase {
      

      Can we update the doc comment for this class? "MigrateUpgradeExecuteTestBase" is a mouthful; it should be explained.

    6. +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/MigrateUpgradeReviewPageTestBase.php
      @@ -0,0 +1,124 @@
      +/**
      + * Provides a base class for testing the Upgrade review page.
      + */
      +abstract class MigrateUpgradeReviewPageTestBase extends MigrateUpgradeTestBase {
      

      Thanks for the clear doc comment :) Can we rephrase it, though, to "...testing the review step of the Upgrade form"?

    7. +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/MigrateUpgradeReviewPageTestBase.php
      @@ -0,0 +1,124 @@
      +  /**
      +   * Modules to enable.
      +   *
      +   * @var array
      +   */
      +  public static $modules = [
      

      Should be {@inheritdoc}.

    8. +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/MigrateUpgradeReviewPageTestBase.php
      @@ -0,0 +1,124 @@
      +   * that the review page works correctly for all Drupal 6 and Drupal 7 modules
      

      Should be "...all contributed Drupal 6 and Drupal 7 modules..."

    9. +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/MigrateUpgradeReviewPageTestBase.php
      @@ -0,0 +1,124 @@
      +   * that have moved to core, ie Views, and for modules that were in Drupal 6 or
      

      Should be "e.g.,"

    10. +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/MigrateUpgradeReviewPageTestBase.php
      @@ -0,0 +1,124 @@
      +   * Drupal 7 core but are not in Drupal 8 core, ie Overlay. To do this all
      

      Should be "e.g.," And can we start a new paragraph at "To do this..."?

    11. +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/MigrateUpgradeReviewPageTestBase.php
      @@ -0,0 +1,124 @@
      +   * noUpgradePath is correct, because there will be no migrations with a
      

      Should be "...the $noUpgradePath property of the update form class are correct, since there will be no available migrations which declare those modules as their source_module."

    12. +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/MigrateUpgradeReviewPageTestBase.php
      @@ -0,0 +1,124 @@
      +   * source_module declared for that module. It is assumed that the modules in
      

      Let's remove "the modules in".

    13. +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/MigrateUpgradeReviewPageTestBase.php
      @@ -0,0 +1,124 @@
      +    // Enable all modules in the source except test and example modules but
      +    // include simpletest.
      

      Should have a comma after "example modules".

    14. +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/MigrateUpgradeTestBase.php
      @@ -267,6 +118,34 @@ protected function translatePostValues(array $values) {
      +  protected function assertUpgradePaths($session, $available_paths, $missing_paths) {
      

      All three parameters should be type hinted.

    15. +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/MigrateUpgradeTestBase.php
      @@ -267,6 +118,34 @@ protected function translatePostValues(array $values) {
      +      $session->elementExists('xpath', "//span[contains(@class, 'checked') and text() = '$available']");
      +      $session->elementNotExists('xpath', "//span[contains(@class, 'warning') and text() = '$available']");
      

      I don't know XPath, but is it at all possible to combine these into a single query? Like "span class contains 'checked' and not 'warning', with text $available?

    16. +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/d6/MigrateUpgrade6ReviewPageTest.php
      @@ -0,0 +1,134 @@
      + * @group migrate_drupal_ui
      

      Let's add this to the migrate_drupal_6 group as well. (You can put any number of @group annotations on a class.)

    17. +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/d6/MigrateUpgrade6ReviewPageTest.php
      @@ -0,0 +1,134 @@
      +      // Modules that have no upgrade path.
      

      Can we mention that these are copied out of the upgrade form? That knowledge might help us in the future.

    18. +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/d7/MigrateUpgrade7ReviewPageTest.php
      @@ -0,0 +1,126 @@
      +      // Modules that have no upgrade path.
      

      Same here?

    19. +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/d7/MigrateUpgrade7Test.php
      @@ -134,9 +135,22 @@ protected function getAvailablePaths() {
      +      // Modules that have no upgrade path.
      

      And here?

    quietone’s picture

    Status: Needs work » Needs review
    FileSize
    48.29 KB
    9.86 KB

    Thanks phenaproxima.

    This got tedious after a while, and the sun came out and it was getting hot. Anyway, most done. I don't know Xpath either but will see what I can find.

    1. Fixed
    2. Fixed
    3. Fixed
    4. Fixed
    5. Fixed
    6. Fixed
    7. Fixed
    8. Fixed
    9. Better?
    10. Still to do, I don't know Xpath either
    11. Fixed
    12. Fixed
    13. Fixed
    14. Fixed

    Status: Needs review » Needs work

    The last submitted patch, 166: 2914974-166.patch, failed testing. View results

    quietone’s picture

    Status: Needs work » Needs review
    FileSize
    47.76 KB

    Something got committed such that this needed a reroll.

    The last submitted patch, 164: 2914974-163.patch, failed testing. View results

    quietone’s picture

    Somehow lost the the install of 'upgrade_form_alter_test' for d6 and d7 tests.

    The last submitted patch, 168: 2914974-168.patch, failed testing. View results
    - codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

    Status: Needs review » Needs work

    The last submitted patch, 170: 2914974-170.patch, failed testing. View results
    - codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

    quietone’s picture

    Status: Needs work » Needs review
    FileSize
    47.84 KB
    1.18 KB

    More problems with that reroll.

    phenaproxima’s picture

    Status: Needs review » Reviewed & tested by the community

    Cool. Proceed!

    quietone’s picture

    Assigned: phenaproxima » Unassigned

    I think it is safe to unassign phenaproxima as it is RTBC now.

    larowlan’s picture

    Are we sure that form_alter is the API we want to go with here?

    The CR is titled Provide a Method to Intentionally Mark Modules, Themes and Profiles as not Upgradable but we're not really providing a method.

    Did we give consideration to a dedicated API? E.g. we could put entries in .info.yml or we could put .migrate.info.yml files in modules.

    How likely is it that contrib modules will need to make this change?

    There isn't much discussion here on why that approach was chosen, so just trying to understand the reasoning.

    quietone’s picture

    @larowlan, thanks for the review. Your comments are about a small part of this patch. Did you have time to review the rest of it? There is a new test method for the UI and refactoring of the test classes.

    larowlan’s picture

    Hi @quietone if you're saying that letting other modules interact with this is not the key facet of this patch, and updating the message to handle core modules is, I'm happy to move that discussion to a follow up and leave the change notice unpublished - thoughts?

    quietone’s picture

    @larowlan, yes, that is correct it is not the key part of the patch. The key part is getting the available and missing modules lists to be correct.

    I checked with heddn and we agree to move that part. I will reroll the patch, removing anything to do with the alter form.

    larowlan’s picture

    I think the change record is the only thing?

    quietone’s picture

    Created a new issue to address how other modules can change where they appear on the migrate upgrade form. #2936365: Migrate UI - allow modules to declare their migration status.

    I think the change record is the only thing?

    Thanks for the reminder. I have moved the CR from this issue to the one I just created.
    There are code changes, the hook is used to alter the form in the test to prove the hook worked.

    Gábor Hojtsy’s picture

    Status: Reviewed & tested by the community » Needs work

    Sounds like this needs work based on previous comments.

    quietone’s picture

    Status: Needs work » Needs review
    FileSize
    44.53 KB
    8.95 KB

    Removed everything related to allowing other modules to add to the noUpgradePath list, which include the test module, upgrade_form_alter_test. Also found that entityreference was missing from the d7 noUpgradePath, that has been restored. Tests passing locally.

    phenaproxima’s picture

    Issue tags: +Needs followup

    Can we open a postponed follow-up, with a patch containing the parts that were removed in #183? Just so that we can easily bikeshed/re-implement the alteration stuff in the future, if needed?

    Other than that, it's RTBC.

    quietone’s picture

    The patch is posted to #2936365: Migrate UI - allow modules to declare their migration status, created in #181, and postponed on this issue and #2928147: Migrate UI - add 'incomplete' migration status so that all the migration status options are available before we decide on how to let other modules declare their status.

    phenaproxima’s picture

    Status: Needs review » Reviewed & tested by the community

    Slam dunk.

    larowlan’s picture

    Adding review credits

    * @phenaproxima and myself for reviews
    * @jhodgdon for guidance/solution design
    * @Gabor Hojtsy for IS updates and committer reviews
    * @neclimdul for diagnosing test failures

    larowlan’s picture

    Status: Reviewed & tested by the community » Fixed
    Issue tags: -Needs followup

    Fixed on commit

    diff --git a/core/modules/migrate_drupal_ui/tests/src/Functional/MigrateUpgradeReviewPageTestBase.php b/core/modules/migrate_drupal_ui/tests/src/Functional/MigrateUpgradeReviewPageTestBase.php
    index 2c927df..eb72bdd 100644
    --- a/core/modules/migrate_drupal_ui/tests/src/Functional/MigrateUpgradeReviewPageTestBase.php
    +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/MigrateUpgradeReviewPageTestBase.php
    @@ -4,7 +4,6 @@
     
     use Drupal\migrate_drupal\MigrationConfigurationTrait;
     use Drupal\Tests\migrate_drupal\Traits\CreateTestContentEntitiesTrait;
    -use Drupal\Tests\WebAssert;
     
     /**
      * Provides a base class for testing the review step of the Upgrade form.
    diff --git a/core/modules/migrate_drupal_ui/tests/src/Functional/MigrateUpgradeTestBase.php b/core/modules/migrate_drupal_ui/tests/src/Functional/MigrateUpgradeTestBase.php
    index 170a725..8496b11 100644
    --- a/core/modules/migrate_drupal_ui/tests/src/Functional/MigrateUpgradeTestBase.php
    +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/MigrateUpgradeTestBase.php
    @@ -7,6 +7,7 @@
     use Drupal\Tests\BrowserTestBase;
     use Drupal\Tests\migrate_drupal\Traits\CreateTestContentEntitiesTrait;
     use Drupal\Tests\WebAssert;
    +
     /**
      * Provides a base class for testing migration upgrades in the UI.
      */
    diff --git a/core/modules/migrate_drupal_ui/tests/src/Functional/d6/MigrateUpgrade6ReviewPageTest.php b/core/modules/migrate_drupal_ui/tests/src/Functional/d6/MigrateUpgrade6ReviewPageTest.php
    index 22669f9..a526de7 100644
    --- a/core/modules/migrate_drupal_ui/tests/src/Functional/d6/MigrateUpgrade6ReviewPageTest.php
    +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/d6/MigrateUpgrade6ReviewPageTest.php
    @@ -2,7 +2,6 @@
     
     namespace Drupal\Tests\migrate_drupal_ui\Functional\d6;
     
    -use Drupal\migrate_drupal_ui\Form\MigrateUpgradeForm;
     use Drupal\Tests\migrate_drupal_ui\Functional\MigrateUpgradeReviewPageTestBase;
     
     /**
    

    Committed as 5fcde4a and pushed to 8.6.x

    Cherry-picked as c9cb478 and pushed to 8.5.x

    Thanks, see you in the follow up.

    quietone’s picture

    @larowlan, thanks for fixing that on commit.

    • larowlan committed 5fcde4a on 8.6.x
      Issue #2914974 by quietone, rakesh.gectcr, maxocub, heddn, Jo Fitzgerald...

    • larowlan committed c9cb478 on 8.5.x
      Issue #2914974 by quietone, rakesh.gectcr, maxocub, heddn, Jo Fitzgerald...
    rakesh.gectcr’s picture

    Status: Fixed » Closed (fixed)

    Automatically closed - issue fixed for 2 weeks with no activity.