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
Comment | File | Size | Author |
---|---|---|---|
#183 | interdiff.txt | 8.95 KB | quietone |
#183 | 2914974-183.patch | 44.53 KB | quietone |
#173 | interdiff.txt | 1.18 KB | quietone |
#173 | 2914974-173.patch | 47.84 KB | quietone |
#170 | interdiff.txt | 1.12 KB | quietone |
Comments
Comment #2
phenaproximaMigrate criticals are now straight-up critical, by committer agreement.
Comment #3
quietone CreditAttribution: quietone as a volunteer commentedYep, forgot that.
Comment #4
maxocub CreditAttribution: maxocub as a volunteer commentedI 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.
Comment #5
heddnHow 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.
Comment #6
heddnFollow-up should be major, not critical.
Comment #7
heddnI think #2918448: Migrate UI: Clarify that a missing upgrade path isn't necessarily a problem should be closed as duplicate.
Comment #8
xjmI 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).
Comment #9
phenaproximaTagging 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.
Comment #10
phenaproximaHere 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
Comment #11
jhodgdonThe 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...
Comment #12
quietone CreditAttribution: quietone as a volunteer commentedYea, 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.
Comment #13
quietone CreditAttribution: quietone as a volunteer commentedAdd a note about the likely follow up issue
Comment #14
phenaproxima#2859304: Show field type migrations correctly in Migrate Drupal UI has landed, so this is no longer postponed.
Comment #15
quietone CreditAttribution: quietone as a volunteer commentedThis is far from complete. I only added a couple of modules without migration paths, more need to be added.
Comment #16
quietone CreditAttribution: quietone as a volunteer commentedThe 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
Comment #18
quietone CreditAttribution: quietone as a volunteer commented#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?
Comment #19
phenaproximaI think so, yes.
Comment #20
yogeshmpawarRe-rolled the patch #16 because it's failed to apply on 8.5.x branch
Comment #21
phenaproximaIf 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.
Comment #22
quietone CreditAttribution: quietone as a volunteer commentedd6_no_upgrade_path and d7_no_upgrade_path are now static. And tests added for the UI.
Comment #23
quietone CreditAttribution: quietone as a volunteer commentedUpdate IS
Comment #24
phenaproximaI 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.
Comment #25
maxocub CreditAttribution: maxocub as a volunteer commentedI 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.
Then contrib modules could override this status if they provide a migration path.
Comment #26
quietone CreditAttribution: quietone as a volunteer commentedMy 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.
Comment #27
jhodgdonThis 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.
Comment #28
phenaproxima+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.
Comment #29
quietone CreditAttribution: quietone as a volunteer commentedAssigning to myself.
Comment #30
quietone CreditAttribution: quietone as a volunteer commentedBeen 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.
Comment #31
quietone CreditAttribution: quietone as a volunteer commentedMoved the work on the UI text to a new issue #2922701: Migrate UI - refer to modules and add help text.
Back to NW.
Comment #32
quietone CreditAttribution: quietone as a volunteer commenteddate_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?
Comment #33
heddnEvent is a contrib module that depends on the contrib date module.
Comment #34
jhodgdonYeah, we shouldn't worry about event.
Comment #35
quietone CreditAttribution: quietone as a volunteer commentedThanks 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
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.
Comment #36
quietone CreditAttribution: quietone as a volunteer commentedSetting to NR to get feedback on #35
Comment #37
jhodgdonHm... 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.
Comment #38
quietone CreditAttribution: quietone as a volunteer commentedRealized 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?
Comment #40
quietone CreditAttribution: quietone as a volunteer commentedUpdated 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.
Comment #42
heddnDid 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.
Comment #43
rakesh.gectcrComment #44
rakesh.gectcr#24 done
#38, IMHO, its fine
Added the source module tracker on migration. Checking is that the reason test is failing.
Comment #45
rakesh.gectcrComment #47
rakesh.gectcrHitting it again
Comment #48
rakesh.gectcrComment #50
rakesh.gectcr:)
Comment #52
jofitz CreditAttribution: jofitz at ComputerMinds commentedApologies 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.
Comment #54
rakesh.gectcr@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.
Comment #55
rakesh.gectcrComment #57
rakesh.gectcrjust checking, not able to test the functional tests on my local. ones this pass, will work on it
Comment #58
rakesh.gectcr.
Comment #60
rakesh.gectcrhope this pass. So I created the inter diff from#40
Comment #61
rakesh.gectcrComment #62
rakesh.gectcrComment #64
rakesh.gectcrComment #66
rakesh.gectcrComment #67
quietone CreditAttribution: quietone as a volunteer commentedThis 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
Comment #69
rakesh.gectcr@quietone
Here https://www.drupal.org/pift-ci-job/820668 The test was failing because of that.
Comment #70
quietone CreditAttribution: quietone as a volunteer commentedI'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.
Comment #71
heddnRecognizing 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).
Comment #72
heddnLet'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.
Comment #74
heddnComment #76
rakesh.gectcrComment #78
rakesh.gectcrComment #79
quietone CreditAttribution: quietone as a volunteer commentedI 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.
Comment #80
quietone CreditAttribution: quietone as a volunteer commentedI'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.
Comment #81
quietone CreditAttribution: quietone as a volunteer commentedUpdated 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.
Comment #83
quietone CreditAttribution: quietone as a volunteer commentedAnd adjust the counts accordingly
Comment #85
quietone CreditAttribution: quietone as a volunteer commentedThe tests really did pass, setting to NR
Comment #86
maxocub CreditAttribution: maxocub as a volunteer commentedIf 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:
I tested it manually and it works, but we should add automated testing.
Comment #87
maxocub CreditAttribution: maxocub as a volunteer commentedHere's a simple test showing the hook_form_alter() in action.
Comment #91
jofitz CreditAttribution: jofitz at ComputerMinds commentedTests are now green, back to NR.
Comment #92
maxocub CreditAttribution: maxocub as a volunteer commentedSome coding standard corrections.
Comment #93
Gábor HojtsyIssue summary does not clearly explain what is going on here. Is this issue in need of a usability review or "just" hiding things?
Comment #94
quietone CreditAttribution: quietone as a volunteer commentedUpdated the IS.
This just puts modules in the 'correct' list and therefor does not need a usability review.
Comment #95
phenaproximaThis is not necessary. Anything with access to $form_state can simply do this:
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...
Is there any particular reason these need to be public and static?
Can we, or do we already, assert that this happens?
Comment #96
maxocub CreditAttribution: maxocub as a volunteer commentedComment #97
phenaproximaAh, okay. That makes sense. I totally realized this after I posted the comment :)
Comment #98
Gábor HojtsyI 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 :)
Comment #99
quietone CreditAttribution: quietone as a volunteer commented@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.
Comment #100
quietone CreditAttribution: quietone as a volunteer commentedThis now only adds modules with noUpgradePath to the available list if they are enabled on the source.
Comment #102
maxocub CreditAttribution: maxocub as a volunteer commentedIn this patch, I make sure that contrib modules can alter the
$no_upgrade_path
list with ahook_form_alter()
and that we test it.The failing patch shows that without the
hook_form_alter()
, the tests are failing.Also:
$noUpgradePath
property does not need to beplublic static
anymore, so I changed it toprotected
.$no_upgrade_path[$version]
safely.getMissingPaths()
andgetAvailablePaths()
.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?Comment #104
phenaproximaWe should use === here, because string comparison can occasionally blow up in weird ways. #PHPWTF
Comment #105
maxocub CreditAttribution: maxocub as a volunteer commentedHere you go! I also change another string comparison to be consistent.
Comment #106
heddnSuper 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.
Comment #107
heddnComment #109
maxocub CreditAttribution: maxocub as a volunteer commentedTypo, 's' missing.
Comment #110
heddnComment #111
phenaproximaHonestly? 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.
Comment #112
maxocub CreditAttribution: maxocub as a volunteer commentedDo 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.
Comment #113
phenaproximaFair enough, and clear upon a closer re-read. This patch looks good to me, I mostly have documentation nits.
This comment should also explain why this array has two sub-arrays, and what the keys mean.
$no_upgrade_path is not a very good name for this variable. Can we call it $module or $extension instead?
&$form should be type hinted. Type hint all the things! :)
Why were config_translation, tracker, and update added?
Why did this change?
We should remove the
// 'event',
line, and the "See" comment should begin with @see.Same here.
Comment #114
maxocub CreditAttribution: maxocub as a volunteer and commentedThanks for the review @phenaproxima!
Comment #115
phenaproximaOkay. I feel confident about it. RTBC once Drupal CI passes it.
Comment #116
quietone CreditAttribution: quietone as a volunteer commentedCorrect 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.
Comment #117
maxocub CreditAttribution: maxocub as a volunteer commented@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.
Comment #118
maxocub CreditAttribution: maxocub as a volunteer commentedLeft some modules uninstalled so we can test that they show up in the missing paths.
Comment #119
quietone CreditAttribution: quietone as a volunteer commented@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
Comment #120
masipila CreditAttribution: masipila as a volunteer commentedI 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 is perfectly fine for modules like Help, Overlay etc. where nothing is upgraded and where nothing is needed.
2. The issue summary says
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.
Cheers,
Markus
Comment #121
masipila CreditAttribution: masipila as a volunteer commentedConceptual 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
Comment #122
quietone CreditAttribution: quietone as a volunteer commentedMy 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.
Comment #123
masipila CreditAttribution: masipila as a volunteer commentedThanks 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
Scenario 2. There is nothing to be migrated
Scenario 3. There is no upgrade path and most probably never will
Scenario 4. There is no upgrade path because no migrations were detected
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
Scenario 2. There is nothing to be migrated
Scenario 3. There is no upgrade path and most probably never will
Scenario 4. There is no upgrade path because no migrations were detected
Scenario 5. Incomplete upgrade path
Cheers,
Markus
Edit: Additions marked above.
Comment #124
maxocub CreditAttribution: maxocub as a volunteer commented@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
Drupal 7
Comment #125
quietone CreditAttribution: quietone as a volunteer commentedThanks 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?
Comment #126
masipila CreditAttribution: masipila as a volunteer commentedOkay, 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:
4. In the 'Available upgrade paths' box there would be two kinds of elements:
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:
Cheers,
Markus
Comment #127
heddnHow about "28 Checked" instead of 28 Available Upgrade Paths?
Comment #128
masipila CreditAttribution: masipila as a volunteer commentedUpdated the issue title to be more accurate + updated issue summary.
Comment #129
quietone CreditAttribution: quietone as a volunteer commentedmasipila, 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.
Comment #130
maxocub CreditAttribution: maxocub as a volunteer commentedI 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.
Comment #131
masipila CreditAttribution: masipila as a volunteer commentedI 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
Comment #132
masipila CreditAttribution: masipila as a volunteer commentedComment #133
masipila CreditAttribution: masipila as a volunteer commentedI updated the change record draft and removed the reference to Views.
Comment #134
quietone CreditAttribution: quietone as a volunteer commentedThanks maxocub and masiplia for taking my concerns into account. The updated CR is much much better. I made a few tweaks as well.
Comment #135
quietone CreditAttribution: quietone as a volunteer commentedI've been working on this, although not ready to post yet.
Comment #136
heddnImage 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.
Comment #137
heddnThat would be because of:
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.
Comment #138
Gábor Hojtsy@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.
Comment #139
heddnI've opened up #2934145: Image field migration plugin in wrong module
Comment #140
quietone CreditAttribution: quietone as a volunteer commentedThis 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.
Comment #141
quietone CreditAttribution: quietone as a volunteer commentedCleanup of the duplicate code by making a new base class for these tests.
Comment #143
quietone CreditAttribution: quietone as a volunteer commentedAdd @group statement to MigrateUpgradeTest.php and a couple of comment changes.
Comment #145
quietone CreditAttribution: quietone as a volunteer commentedCorrections 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.
Comment #147
heddnFixed code standards only. Leaving at NW.
Comment #148
quietone CreditAttribution: quietone as a volunteer commentedFixed a few comments and the file entity count error. The method getSourceBase() needs to be declared in MigrateUpgrade6Test.
Comment #150
quietone CreditAttribution: quietone as a volunteer commentedRename the test running and added a missing comma.
Comment #152
quietone CreditAttribution: quietone as a volunteer commentedWell, 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?
Comment #153
quietone CreditAttribution: quietone as a volunteer commentedComment #154
neclimdulThe 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.
Comment #155
quietone CreditAttribution: quietone as a volunteer and commented@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.Comment #157
quietone CreditAttribution: quietone as a volunteer commentedWell, 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.
Comment #158
quietone CreditAttribution: quietone as a volunteer commentedImprove comments and tweaked the query that enables modules in the source db.
Comment #159
phenaproximaI'll give this a review; @heddn has volunteered to pick up any follow-ups that come out of my review.
Comment #160
heddnComment #161
heddnComment #162
phenaproximaStill 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.
This needs a little bit of word-smithing, for clarity.
$session should be type hinted, in every doc block and method signature where it is used.
Comment #163
heddnNeeds a reroll. I cannot do that right now.
Comment #164
quietone CreditAttribution: quietone as a volunteer commented1. 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.
Comment #165
phenaproximaMuch improved, thanks! All I found were docs problems, really. :)
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".
Hooray for strict(ish) typing!
:)
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.
Can we update the doc comment for this class? "MigrateUpgradeExecuteTestBase" is a mouthful; it should be explained.
Thanks for the clear doc comment :) Can we rephrase it, though, to "...testing the review step of the Upgrade form"?
Should be {@inheritdoc}.
Should be "...all contributed Drupal 6 and Drupal 7 modules..."
Should be "e.g.,"
Should be "e.g.," And can we start a new paragraph at "To do this..."?
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."
Let's remove "the modules in".
Should have a comma after "example modules".
All three parameters should be type hinted.
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?
Let's add this to the migrate_drupal_6 group as well. (You can put any number of @group annotations on a class.)
Can we mention that these are copied out of the upgrade form? That knowledge might help us in the future.
Same here?
And here?
Comment #166
quietone CreditAttribution: quietone as a volunteer commentedThanks 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.
Comment #168
quietone CreditAttribution: quietone as a volunteer commentedSomething got committed such that this needed a reroll.
Comment #170
quietone CreditAttribution: quietone as a volunteer commentedSomehow lost the the install of 'upgrade_form_alter_test' for d6 and d7 tests.
Comment #173
quietone CreditAttribution: quietone as a volunteer commentedMore problems with that reroll.
Comment #174
phenaproximaCool. Proceed!
Comment #175
quietone CreditAttribution: quietone as a volunteer commentedI think it is safe to unassign phenaproxima as it is RTBC now.
Comment #176
larowlanAre 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.
Comment #177
quietone CreditAttribution: quietone as a volunteer commented@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.
Comment #178
larowlanHi @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?
Comment #179
quietone CreditAttribution: quietone as a volunteer commented@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.
Comment #180
larowlanI think the change record is the only thing?
Comment #181
quietone CreditAttribution: quietone as a volunteer commentedCreated 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 the state of their migrations.
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.
Comment #182
Gábor HojtsySounds like this needs work based on previous comments.
Comment #183
quietone CreditAttribution: quietone as a volunteer commentedRemoved 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.
Comment #184
phenaproximaCan 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.
Comment #185
quietone CreditAttribution: quietone as a volunteer commentedThe patch is posted to #2936365: Migrate UI - allow modules to declare the state of their migrations, 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.
Comment #186
phenaproximaSlam dunk.
Comment #187
larowlanAdding 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
Comment #188
larowlanFixed on commit
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.
Comment #189
quietone CreditAttribution: quietone as a volunteer commented@larowlan, thanks for fixing that on commit.
Comment #192
rakesh.gectcr