Problem/Motivation
Modules need a way to declare their migration status and give a short description on the Migrate Upgrade review page, /upgrade. The short description can have a link to a documentation page for example for known issues.
The Review form lists all modules on the source site in either a 'will be upgraded' or 'will not be upgraded' list. However, determining which section to add a module too is not robust. If the module has migrations, it will be displayed in the 'will be upgraded' list, even if it only has one migration and needs more than one to be complete. We need a mechanism that can explain this situation in the UI.
The mechanism needs to be flexible to accommodate the many scenarios possible. See #71 and #72 for details.
This work here is to only create and implement such a system, to make the first step, for what is a complicated problem. Changes may be made to the UI but are expected to be minimal. However, a UX review is planned. When that feedback is given it needs to be decided what can be done in this issue and what must be moved to followups.
Note: in #2914974: Migrate UI - handle sources that do not need an upgrade we defined a list of modules that do not need any migrations (e.g. Overlay, Help) or the contrib functionality has moved into core and there is not a D8 core module with the same name. These modules are listed in ReviewForm::$noUpgradePaths
In a migrate meeting, using a hook or an event was discussed. A possible solution, using hook_form_alter, was implemented in #2914974: Migrate UI - handle sources that do not need an upgrade and removed after a review #176. That review asked some questions, listed below, which should be resolved 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?
Proposed resolution
Allow Drupal 8 modules to declare their migration state in module_name/migrations/state/migrate_drupal.yml. This will allow the Review form (at /upgrade) to display accurate information. See Change Record for details and usage examples.
A deprecation notice is displayed when a module has at least one migration and does not have a migrate_drupal.yml file.
A brief description:
In 'finished' is a list of all source module/destination module pairs for migrations that are complete in the module. In 'not_finished' is a list of all the migrations this module intends to provide but either do not exist or are incomplete for any reason. Any migrations discovered in the module that do not have a corresponding entry in the migrate_drupal.yml file will be assumed 'not finished'.
The 'not_finished' is needed because more than one module can have migrations for a source module and at least one of the modules uses the 'finished' key. The 'not_finished' one will ensure that the source module is listed in the 'will not be upgraded' list. Say Module A and Module B provide migrations for Module C. Module A is finished and uses 'finished'. Then, if Module B has no migrations, it needs to use 'not_finished'.
History
The first proposal which came from a migrate meeting was to add the migrate status of the module and a description string to be displayed on the Review Page to info.yml . This was tweaked a few times. See #7 and #8. See #10. Eventually, the description was removed as to difficult to maintain an html string in .info but the version key remained.
After framework manager review the status information moved from .info.yml to module.migrate.yml, the format stays the same. And later it moved to migrate_drupal.yml since this is only for drupal sources. See comment #124.
All the logic to determine whether a source module will be upgraded or not is now in a service (See suggestion in #137.9, migrate_drupal.migration_state.
Discussion of the logic are in #72 and #77
Remaining tasks
Decide on the format of the new 'migrate' key and values in .info.yml. Suggested formats are in #11 and #15. Those have been abandoned in favor of 'migrations_finished' and 'migrations_not_finished'.Decide on the allowed values for status. Suggestions are in #7, #9. Currently proposed values are 'complete', 'incomplete' and 'n/a'. The UI impact with a screenshot is in #28.Decide if adding a key for source version is ok.As pointed out in #10, the status needs to be declared by source version.- Review
-
UX Review -See #106 - #11 Update change record</li>- Update handbook documentation
- Commit
- Possible followup: Allow modules to add text to the page via a hook.
User interface changes
The difference between the before and after is that on the after screenshots the modules in the Drupal 8 column will be listed in a single line separated by commas whereas in the before screenshots they are on separate lines. That's all.
The new Review page also has a new help text string to give some information or direction to the user on what to do about the modules in the 'will not be upgraded' section. The new help text is
The new site is missing modules corresponding to the old site's modules. Unless they are installed prior to the upgrade, configuration and/or content needed by them will not be available on your new site. Read the checklist to help decide what to do.", [':review' => 'https://www.drupal.org/docs/8/upgrade/upgrade-using-web-browser#pre-upgr...']),
See #87
Screens shots using the Drupal 6 Ubercart test database from Commerce Migrate.
Before
Module that will not be upgraded section
After
Modules that will not be upgraded section
Screens shots using the Drupal 6 test fixture.
Before
Modules that will be upgraded
After
Modules that will be upgraded
The checklist referred to is https://www.drupal.org/docs/8/upgrade/upgrade-using-web-browser#pre-upgr...
API changes
No API changes. A new migrations/state/MODULE_NAME.migrate_drupal.yml
file may be added in each module. See the change record for format information.
Data model changes
None.
Release notes snippet
There pre-upgrade analysis in Migrate Drupal UI displays a list of legacy modules that will be upgraded and will not be upgraded. Included in those that will be upgraded are modules that do not need an upgrade, such as the Help module. In order for this analysis to be correct, some Drupal 8 modules must now provide information in a MODULE_NAME.migrate_drupal.yml
file in the module's migrations/state
directory.
Comment | File | Size | Author |
---|---|---|---|
#309 | interdiff.2936365.297-309.txt | 925 bytes | mikelutz |
#309 | 2936365-309.drupal.Migrate-UI--allow-modules-to-declare-the-state-of-their-migrations.patch | 118.6 KB | mikelutz |
#204 | WithPatch.png | 39.12 KB | quietone |
#204 | WithoutPatch.png | 51.29 KB | quietone |
Comments
Comment #2
quietone CreditAttribution: quietone as a volunteer commentedIn #2914974: Migrate UI - handle sources that do not need an upgrade comment #184 phenaproxima asked that a patch containing the code implementing the form alter hook method that was removed from that issue be posted in a followup, marked postponed. So, posting a patch here. This patch is made from the patch 2914974-173.patch removing the chunks not related to the hook.
And postponing on #2914974: Migrate UI - handle sources that do not need an upgrade and #2928147: Migrate UI - add 'incomplete' migration status.
Comment #3
quietone CreditAttribution: quietone as a volunteer commentedUpload the patch twice, hiding one.
Comment #5
quietone CreditAttribution: quietone as a volunteer commentedAdd tag.
Comment #6
quietone CreditAttribution: quietone as a volunteer commentedTime to get this sorted.
Comment #7
heddnDiscussed in migrate maintainers meeting w/ alexpott, masipila, quieteone, phenaproxima, maxocub:
Add a new set of migrate keys to .info.yml to track a module's migratable-ness.
The status value would be from an allowed values list. i.e. upgradable, not upgradable, incomplete migration path
Comment #8
phenaproxima+1 on that approach, but it will need documentation before we can land it :) This is the kind of thing that absolutely should be blocked on docs.
Comment #9
quietone CreditAttribution: quietone as a volunteer commentedIs this a suitable entry in the info.yml for a module?
And for display, something like this:
Does anyone see a need to include the 'Details' column for modules with an complete upgrade path?
What about the allowed values for that status? So far we have 'upgradable', 'not upgradable', and 'incomplete'. My current thought is 'complete', 'incomplete' and 'n/a'. The 'n/a' is because when I was faced with marking a module not upgradable for testing purposes it was natural to type 'n/a'.
Thinking out load here, I wonder if it would be easier for people if we accepted version strings plus 'n/a'. Then anything with 'alpha', 'beta' or 'rc' is considered 'incomplete'.
What should happen if the status for a module is 'complete' but there are no migrations? Or if there are migrations but the status is 'n/a', or whatever term is decided for that concept?
Comment #10
quietone CreditAttribution: quietone as a volunteer commentedThere is a problem with this approach because it doesn't allow the status to be defined per source. The UI needs to know the migration status based on the source, not the destination. For example, the D7 color migration is incomplete but for Drupal 6 is it n/a. And looking forward to d8->d9, there will be working migrations for D7 user to D 9 user, but not yet D8 user to D9 user. They both can't have the same migrate status.
Comment #11
quietone CreditAttribution: quietone as a volunteer commentedHere is a draft. The info.yml file has a index for the version number so that the status between Drupal 6 and Drupal 7 sources is available.
Not sure I like the format of that, I just did something quickly so I could test.
A test has been added to check that all modules have the migrate information in the info.yml. The status values used are 'complete', 'incomplete', and 'n/a' but only the 'n/a' value is actually used in the ReviewForm.
Any module in noUpgradePaths that also has a status of 'n/a' has been removed from noUpgradePaths. NoUpradePaths now contains only those modules that don't have a corresponding Drupal 8 module.
Comment #13
quietone CreditAttribution: quietone as a volunteer commentedExtra character crept into options.info.yml.
Comment #15
quietone CreditAttribution: quietone as a volunteer commentedAnother stray character and I forgot to update the test after a change to the info.yml format.
Assuming there is agreement about adding version, maybe this format is better.
Comment #16
quietone CreditAttribution: quietone as a volunteer commentedUpdate IS. For anyone reviewing you might want to read from #9 onwards.
Comment #17
quietone CreditAttribution: quietone as a volunteer commentedWhat do you think about adding the UI changes to implement 'incomplete' here? Conceptually it all goes together, it will uncover any problems now and not later and all the changes to the .info.yml will happen in one patch, not two. There is existing code at #2928147: Migrate UI - add 'incomplete' migration status.
Comment #18
masipila CreditAttribution: masipila as a volunteer commentedHi!
Quick comments.
I definitely like the syntax in #15 over the syntax in #11
About the statuses. I propose we evaluate these lables with concrete scenarios.
The current labels in the web UI are 'will be upgraded' and 'will not be upgraded'.
Scenario 1: Comment module
- Upgrade path exists, module name is the same
Scenario 2: D7 Adressfield to D8 Address
- D7 Addressfield can be migrated to D8 Address
Scenario 3: Views
- No updrade in core
- Core would link to the Known Issues doc page
Scenario 4: Overlay
- No upgrade needed as the module doesn't store any data
Scenario 5: random contrib that has D8 version but no automatic migrations
Scenario 6: some other random contrib, does not have D8 version at all
I guess these 6 scenarios cover all aspects of this. Many of them fall into the same category than the others so I'm not suggesting 6 different lables. I'm only listing these here so that when we discuss thos lables we keep the all different aspects in mind.
I don't have suggestions right now, gotta go to bed now, but I'll keep this in my mind and maybe my brain will background process this while I'm sleeping :)
Cheers,
Markus
Comment #19
masipila CreditAttribution: masipila as a volunteer commentedHi!
The unconscious background processing seemed to work here :) Two comments:
1. @quietone asked in #9 if anyone sees a need to include the 'Details' column for modules with an complete upgrade path?
2. @quietone was thinking out loud in #10 that there is a problem with this approach because it doesn't allow the status to be defined per source. The UI needs to know the migration status based on the source, not the destination.
Kicking back to NW to discuss these further.
Cheers,
Markus
Comment #20
quietone CreditAttribution: quietone as a volunteer commentedAssigning to myself
Comment #21
quietone CreditAttribution: quietone as a volunteer commentedExamples for the scenarios in #18
Scenario 1: Comment module
- Upgrade path exists, module name is the same
Scenario 2: D7 Adressfield to D8 Address
- D7 Addressfield can be migrated to D8 Address
Scenario 3: Views
- No updrade in core
- Core would link to the Known Issues doc page
Scenario 4: Overlay
- No upgrade needed as the module doesn't store any data
There isn't a D8 overlay module so nothing to declare here. It is included in a separate list, noUpgradePath.
Scenario 5: random contrib that has D8 version but no automatic migrations
Scenario 6: some other random contrib, does not have D8 version at all
Will be listed in the 'will not be upgraded' section.
--
Rerolled the patch so no interdiff. The screenshot in #9 is still valid. Note that the patch is using statuses of 'complete', 'incomplete' and 'n/a'.
Comment #22
masipila CreditAttribution: masipila as a volunteer commentedAssigning for myself for review.
Comment #23
masipila CreditAttribution: masipila as a volunteer commentedRemaining tasks are as follows:
1. Decide on the format of the new 'migrate' key and values in .info.yml. Suggested formats are in #11 and #15. Currently proposed format is the one presented in #11. I'm OK with this.
2. Decide on the allowed values for status. Suggestions are in #7, #9. Currently proposed values are 'complete', 'incomplete' and 'n/a'. I'm OK with these. These statuses cover all scenarios that I was able to think in #18.
3. Decide if adding a key for source version is ok. As pointed out in #10, the status needs to be declared by source version.
4. Review: I checked the patch and it looks good to me. I also tested it manually. Screenshot is in #9 as @quietone pointed out in her previous comment above.
5. Documentation updates and change record. The change record draft is out-of-date. Kicking back to NW for that. The handbook doc pages need to also be updated.
Question: Do we want to add the Details text / links to Known issues page for the core modules in this same patch or in a follow-up?
I updated the Issue Summary.
Cheers,
Markus
Comment #24
masipila CreditAttribution: masipila as a volunteer commentedI can update the change record this evening.
Comment #25
masipila CreditAttribution: masipila as a volunteer commentedChange record updated: https://www.drupal.org/node/2929443.
I will update this Migrate API handbook documentation page once this issue lands. I don't want to update it earlier as it would be very confusing to the contrib module maintainers if our handbook would give instructions to uncommitted stuff.
Back to NR. To me, the only remaining open topic is this question:
Do we want to add the Details text / links to Known issues page for the core modules in this same patch or in a follow-up?
Cheers,
Markus
Comment #26
quietone CreditAttribution: quietone as a volunteer commented@masiplia, thanks you. The summary in #23 is great.
Some questions
1) How to reflect the different statuses in the UI? complete - will be upgraded, incomplete - will not be upgraded or a new section, n/a - will be upgraded or a new section?
2) What to do if when there are multiple migrations with the same source module and destination module and with a different status?
Comment #27
masipila CreditAttribution: masipila as a volunteer commentedHi!
1. How about this for the UI
Let's keep the two sections that we currently have ('will be upgraded' and 'will not be upgraded').
Status 'n/a' makes the module to appear in the 'will not be upgraded' section with a red symbol.
Status 'complete' makes the module to appear in the 'will be upgraded' section with green symbol.
Status 'incomplete' makes the module to appear in the 'will be upgraded' list with yellow symbol. Free text explanation would typically be provided in the .info.yml file.
2. I would argue that this is so rare corner case that we can simply have two entries in the UI in the cases where the source module is 'split' between two destination modules. The free text can be used to explain that the functionality is split into two modules in D8.
Markus
Comment #28
masipila CreditAttribution: masipila as a volunteer commentedReplying to myself to #27.2. I checked the D7-D8 core upgrade analysis. Core seems to have the following 1:N relations where one source module has multiple destination modules:
D7 block -- D8 block + block_content
D7 locale -- D8 language + system
D7 menu -- D8 menu_link_content + menu_ui + system
D7 node -- D8 comment + content_translation + node
D7 system -- D8 file + system
D7 taxonomy -- D8 core + taxonomy
So it is not that rare corner case. However, I still feel that the most elegant approach is to show these as separate rows in the section. It's not just the different upgrade status value that we must be able to handle, it's also possible that the free text details are different.
Here's my proposal for the UI as a screenshot.
Cheers,
Markus
Comment #29
masipila CreditAttribution: masipila as a volunteer commentedI updated the IS to clarify the scope of this issue and that the actual change to Migrate Drupal UI will be handled separately in #2928147: Migrate UI - add 'incomplete' migration status.
This issue is now NR waiting for feedback. @quietone's patch #21 handles the scope of this issue as far as I can see. I haven't touched the patch myself but I've been involved in the conceptual side of this issue to the extent that I would like somebody else to RTBC this.
Cheers,
Markus
Comment #30
lomasr CreditAttribution: lomasr at gai Technologies Pvt Ltd for gai Technologies Pvt Ltd commented#26 . I think also considering contrib modules. There can to "no upgrade found" and "Upgrade found" with current status "complete" or "in progress". Please suggest.
Comment #31
masipila CreditAttribution: masipila as a volunteer commented@lomasr, the contrib modules would declare their upgrade status as follows in the .info.yml
Status 'complete' makes the module to appear in the 'will be upgraded' section with green symbol.
Status 'incomplete' makes the module to appear in the 'will be upgraded' list with yellow warning symbol. Free text explanation would typically be provided in the .info.yml file. This status can be used for example in a situation where some aspects of the migration are covered but some things are not automatically migrated yet.
Status 'n/a' makes the module to appear in the 'will not be upgraded' section with a red warning symbol. Free text explanation can be provided in the .info.yml file.
Cheers,
Markus
Comment #33
heddnComment #34
masipila CreditAttribution: masipila as a volunteer commentedBumping to Major. Let's try to finally land this so that contribs can declare their status.
Comment #35
quietone CreditAttribution: quietone as a volunteer commentedReroll.
Comment #36
quietone CreditAttribution: quietone as a volunteer commentedThis add a a test to check that the messages in the Details column are displayed. It is only on the MigrateUpgrade6ReviewPageTest. The format of the data in the .info file is changed to the suggestion in #15, which masipila preferred in #18. Changing the format of that array changed all the code in the ReviewForm.
Still needs a test for Drupal 7.
Comment #38
quietone CreditAttribution: quietone as a volunteer commentedA few fixes that should have been in the previous patch. I was eager to submit it before watching a rugby semi-final and it really wasn't ready.
Comment #40
quietone CreditAttribution: quietone as a volunteer commentedWell, my team lost but maybe I can get this patch to pass tests.
Comment #41
maxocub CreditAttribution: maxocub for Acquia commentedThere's some missing href attributes here.
This could be a single if statement.
Ditto.
Comment #42
quietone CreditAttribution: quietone as a volunteer commentedI'll do this this weekend.
Comment #43
quietone CreditAttribution: quietone as a volunteer commentedFirst, fixed all the points raised by maxocub on #41.
And apologies for not making a patch at this point and uploading it. I just continued on.
Then took up masipila's suggestion in #28 to have one line per source module/destination module pair which allows each to have their own status. That decision required quite a lot of changes to the forms and the tests. Primarily, the changes were to allow the tests to know how many times a source module appears on the review page for the 'checked' status and the new 'warning' (for incomplete) status. In order to provide that count per module, the source arrays for those tests have been changed to include a count for each source module. Those arrays of module lists are modified in the tests so that code needed to be adjusted for the new format of the array.
Tests are added to check that the correct message appears in the details section. I'd prefer to do these by count as well but couldn't get that to work so settled on just making sure that the detail messages are displayed at least once.
Since core will have 'completed' and 'n/a' status but eventually not 'incomplete' a test module has been added that has an incomplete status.
The description messages in *.info.yml are a bit improved as well.
Comment #45
quietone CreditAttribution: quietone as a volunteer commentedCorrect the call parameters.
Comment #46
quietone CreditAttribution: quietone as a volunteer commentedYou may notice that the i18n source module is marked as checked and warning on the Review page, that is fixed in #2988235: Remove i18n from d6 $noUpgradePaths. I'm not marking this as postponed since we continue to review and perhaps improve the messages for the details column.
Comment #47
quietone CreditAttribution: quietone as a volunteer commentedUnassigning for all the reviewers to comment!
Comment #48
maxocub CreditAttribution: maxocub commentedGreat work with those tests @quietone!
Super nit, but there's a few extra lines.
This comment needs work.
I also have a few remarks/questions.
config_translation
andcontent_translation
. This is good and accurate. I guess the follow-up #2928147: Migrate UI - add 'incomplete' migration status will be to add a third summary at the top and a third table?Back to NW for the nits above.
Comment #49
quietone CreditAttribution: quietone as a volunteer commented@maxocub, thanks! Getting the tests correct was tedious.
Comment #50
quietone CreditAttribution: quietone as a volunteer commentedFixes
1. I thought I fixed all those extra lines. Thanks for spotting them.
2. Oh, didn't mean to leave that is.
Re commetns
1. Thanks. Yes, that makes sense to do this in two steps. This is big enough with all the changes to the info.yml files.
2/3. Yes, we will need to keep an eye on those.
Comment #51
maxocub CreditAttribution: maxocub commented#2500509: Upgrade path for RDF 7.x landed, this needs a re-roll. After that, I think it's pretty close to the finish line.
Comment #52
quietone CreditAttribution: quietone as a volunteer commentedRerolled for changes from #2500509: Upgrade path for RDF 7.x and #2988235: Remove i18n from d6 $noUpgradePaths.
Comment #53
quietone CreditAttribution: quietone as a volunteer commentedRemove needs reroll tag.
Comment #54
maxocub CreditAttribution: maxocub commentedSorry for not finding those in my earlier reviews, but this is such a big patch.
We usually put the opening brackets on the same line as the variable.
Closing bracket too much indented.
Extra period.
Is the status of the Views migration should really be 'incomplete'? Or should it be with the missing upgrade paths?
Comment #55
quietone CreditAttribution: quietone as a volunteer commentedNo worries, I expected it to take a few rounds to get this to RTBC, especially since I was focusing on getting it to work and not checking code style. This time I found a few more things as well.
1-3 Fixed
4. Glad you pointed this out. Because Views has no migrations and is enabled on both sites it gets put into the $missing_modules list and for these it looks for the description (from info.yml) for a destination module with the same name as the source module. Not an obvious thing. My first reaction is we need yet another status to express that a module should have a migration path but will never have one, for whatever reason.
5. Todo.
Comment #57
quietone CreditAttribution: quietone as a volunteer commentedAssuming I fixed this correctly (haven't run the tests locally because of the time). The errors occur because the available/missing module arrays are being adjusted even though no modules have been installed or uninstalled. So, sometime in the past something changed in regard to the aggregator module and the upgrade tests. I suspect that previously the aggregator module was only enabled after the initial migration and that had been replaced by createContentPostUpgrade. I really don't know and can't research it now. I do know that there are no module changes and thus testing the ReviewPage post the initial upgrade and before the incremental is not needed. And, in fact, the ReviewPage is well tested in the MigrateUpgradeReview tests.
Comment #58
masipila CreditAttribution: masipila as a volunteer commentedWe had quite long chat with @quietone this morning my time and evening her time on #54.4 and #55.4 i.e. what should be the views.info.yml migration 'status' value.
We had to end the chat before we came to the final conclusions so I promised to write a summary here so that we can continue. When I started to do this, I realized that I first need to update the issue summary because
.
Now, let's try to wrap up this whole thing once more. Let's start by looking at the UI mockup from comment #28:
1. We have two different lists in the Migrate Drupal UI: 'Modules that will be upgraded' and 'Modules that will not be upgraded'
1A. Modules that will be upgraded. This list includes two kinds of modules:
1B. Modules that will not be upgraded.
2. What are we trying to achieve here
See comments #18 - #21 above. I'll use the same scenario numbers for the sake of clarity.
Scenario 1: Comment module
Scenario 2: D7 Adressfield to D8 Address
Scenario 4: Overlay
Scenario 5A: random contrib that has D8 version but no automatic migrations because the migration has not (yet) been developed
Scenario 5B: random contrib that has D8 version but does not **need** any migrations because the module does not store any config or content data
Scenario 3: Views
Cheers,
Markus
p.s. This whole thing is about to cause a brain meltdown to both @quietone and me. Please bare with us, we're talking about conceptually quite deep s#it here :)
p.p.s. I'm open for all sorts of ideas here i.e. feel free to comment everything in this comment and in the issue summary update I made. Hopefully I did not make any brainfarts in this comment or the IS update.
Comment #59
quietone CreditAttribution: quietone as a volunteer commented@masipila, thanks that helps a lot. Thinking out loud here
Migration status: complete, will be upgraded, green
Scenario 1: Comment module
Scenario 2: D7 Adressfield to D8 Address
Migration status: incomplete, will not be upgraded, red
Scenario 5A: random contrib that has D8 version but no automatic migrations because the migration has not (yet) been developed
Scenario 3: Views
Migration status: n/a, will be upgrade, green
Scenario 4: Overlay (currently n/a ignored since no migrations are discovered).
Scenario 5B: random contrib that has D8 version but does not **need** any migrations because the module does not store any config or content data
This removes the warning status because the site migrator is probably looking for a yes/no answer for each module migration. The 'no' category can have a link to more info for those interested. This does mean that a module may appear in both lists, but with different destination modules. Hmm. That would be accurate but perhaps confusing? Or just check if a source module has any destination that is incomplete, then the will be upgraded ones are not displayed only the will not be upgraded? But then the display would not show any description message for the completed parts for that source module.
Oh universe, looks like we need more scenarios for when there are multiple destination_modules for a source_module. Sigh.
Comment #60
masipila CreditAttribution: masipila as a volunteer commentedThanks for summarizing this even further @quietone, I think that we are starting to be conceptually closer and closer!
1. Migration status: complete
I'm not 100% sure if you suggested that the 'will be upgraded' list would not have the additional information / description column. Just in case you did, I would keep the additional info / description column in the UI also for the 'will be upgraded' list so that modules can show additional remarks there even if the module is included in the 'will be upgraded' list. It could very well be that a module has some conceptual differences between D6/D7 - D8 that the site builder need to understand and it would be very good to have a link in the UI to a doc page for these kind of use cases.
2. About your thinking out loud UI mockup
I love your idea of having an explicit third list for modules that will be upgraded but which do not need any data migrations. However, I would order the three lists so that the two 'OK' lists are next to each other. However, do we need the yellow symbol for those modules where part of the migrations has been developed and some are still not ready? I'm mainly thinking about the multilingual migrations here.
So the UI would be something like this:
A. Modules that will not be upgraded (red symbol for each row)
Criteria for module to be included on this list:
B. Modules that will be upgraded (green symbol if status is 'complete' and yellow symbol if symbol is 'incomplete')
Criteria for module to be included on this list:
C. Modules that will be upgraded without a need for data migration (green symbol for each row)
Criteria for module to be included on this list:
3. Modules with multiple destination_modules for one source_module
D7 core migration had the following 1:N scenarios the last time I checked in #28.
I suggest we use D7 node as a concrete example for this scenario. How would D7 node behave here?
Comment #61
quietone CreditAttribution: quietone as a volunteer commentedDiscussed this on the migrate meeting call today. There was a general drive to keep this simple.
First, there was concern over the translatability and length of the description strings (they can contain html) in info.yml. So, instead of using that, add help text to the Review Page that explains the form and directs to a handbook page with more information . And have a followup for deciding how contrib modules can provide their specific details.
Second, have one line per source_module and not many as it is now since the person using the form just wants to know if module X is migrated or not. This means the status of all destination_modules for source_module X are used to determine what category X is listed in. Something like if they are all complete then mark green, if one is incomplete, mark yellow etc.
Third, with one line per module and smart code to decide on the status use three sections, red/yellow/green, to display them.
Fourth, another scenario need so to be added. When legacy module X has been renamed to module Y and needs no migrations. In this case module X will appear as not being migrated/red. There is currently no mechanism for module Y to declare that it is the replaced for module X.
And a note that currently there is no checks that the information provided in info.yml is correct, such as if info.yml has n/a but the module contains migrations.
Comment #62
heddnI also think we assume only one module will provide the details on all of this. There's an alter hook for the .info file so if someone needs to modify the status of a module's migration status, they can use the alter hooks already provided. I'm looking at you, views module.
Comment #63
maxocub CreditAttribution: maxocub commentedBased on comments #58-#62, this needs work.
Comment #64
quietone CreditAttribution: quietone as a volunteer commentedFirst, lets look at #58 to see what, if anything, has changed due to the migrate meeting as reported in #61 and #62.
Scenario 5A: random contrib that has D8 version but no automatic migrations because the migration has not (yet) been developed
masipila suggests a new migrate status string. For that I've added 'active'. It is merely a suggestion and I took it from the set of statuses used in the issue queue.
Scenario 5B: random contrib that has D8 version but does not **need** any migrations because the module does not store any config or content data.
This is still 'n/a'.
Views: Yet another new status string for this, "won't fix". Again taken from the statuses used in the issue queue. This is for any module that should have migration but none will be created.
The case of one source_module having multiple destination_modules, discussed in #59 and #60, is handled (or at least attempted) There is now logic to determine what the migrate status of the source_module that depends on the migrate status of all the destination modules.
#61-1, 2 and 3 have been implemented.
As suggested in #62 system_info_alter is used to add the description information. That has been added for Views and in the two test modules, migrate_status_active_test and migrate_status_incomplete_test.
This patch implements the above but will fail tests, I have deliberately only been working on MigrateUpgrade6ReviewPageTest and that passes locally.
There are now 5 migrate statuses; complete, incomplete, n/a, 'won't fix' and active. See migrate_drupal_ui/src/Form/ReviewForm.php for documentation on those.
Even with the failures it would be helpful to get some initial feedback on the direction and how this looks. Don't bother with a detailed code review, some things like the logic for computing the migrate status is deliberately explicit at this early stage.
TODO:
The case where a legacy module X has been renamed to module Y and needs no migrations, #61.4
Comment #65
quietone CreditAttribution: quietone as a volunteer commentedforgot the screen shots. I don't know why the green check is missing from the top section of the report. Hasn't been high priority to look at that.
Comment #67
maxocub CreditAttribution: maxocub commentedI think this is going in the right direction, although I'm still not entirely certain about some aspects of the approach.
I'm questioning the fact that we have (for now) 5 statuses, but our UI can only display 3 types of statuses (red, orange & green). I understand why we need "complete" and "n/a" statuses for the "will be upgraded" list and the "won't fix" and "active" statuses for the "won't be upgraded" list, but the end user will ever only see 3 categories. Since the goal of this issue is for contrib modules to be able to declare their migration status, I think that having those 5 statuses is a good thing. Having only 3 statuses would confuse contrib maintainers since they would not always find a fitting status for their module.
I'm also questioning the fact for a contrib module to be able to add its migration status and a description will need to do 2 things: Add the status in its .info.yml file and implement a hook for the description (to be able to make it translatable). Ideally, it would be nice to be able to do this in only one place. I don't have a better idea right now, except than doing it all in a hook.
Comment #68
quietone CreditAttribution: quietone as a volunteer commentedThanks maxocub. I am with you on preferring to have one place to add migration information.
Just thought I would list the proposed statuses:
complete - all migrations are complete, GREEN
incomplete - at least one migration exists, but need more, YELLOW
n/a - no migrations needed, GREEN
won't fix - ideally would have migrations but policy decision not to do so (views), RED
active - needs migrations none exist, RED
Comment #69
heddnI would like to bikeshed that one for a while. Let me explain. If a contrib module decides it is far easier to not provide a migrate path for some reason, they have good reasons. Maybe they want it to be green not red. Yes, there /could/ be a migration, but it is far easier to re-do things. Take pathauto as an example. So now we leave the contrib author in a quandry. Will they be lying and saying "n/a" to make it show green or red, "won't fix"?
We want to show things that are migrated (or a migration is not possible), not migrated (and needs manual steps to redo the configuration or copy over content) or WIP (work in progress). Instead of 5 phases, can we try these phases:
Migration path finished - this would entail modules that need a path or don't. If the module doesn't need a migration, then it is complete.
Migration path partially completed - any module that is in progress
Migration path not started - any module that doesn't have any migrations
OK, enough rambling. What does all this mean? What are my succinct thoughts?
How do we tell when something is complete vs WIP vs not started? By seeing if the module has a migrate templates. If it has a template, then it is WIP. If it has none, then it is not started. Then for complete we check in its info.yml file. If it has an entry in there that says the module is complete, then it is complete.
I think this also helps us with the addressfield => address example. Where we have a module rename. In these cases we'd have either a WIP or complete, depending on the status discovery. If there isn't any migrations discovered, it automatically falls into the "not started" column.
Where does the description column come into things? I think we don't worry about that right now too much. For complete, it is isn't necessary. For all other cases, we could possibly add a hook for folks to explain more completely. Or something. But let's keep it KISS for now.
Comment #70
maxocub CreditAttribution: maxocub commentedI like it.
Comment #71
quietone CreditAttribution: quietone as a volunteer commentedI agree, 5 categories is too many but it was what I came up with at the time.
I want to summarize this into a table and then look at the different scenarios.
So, in this arrangement we have:
The two unknown's shouldn't happen but if they do make a sensible choice.
Now apply that to the scenarios, not considering translations.
Scenario 1: Legacy module the same as D8 module
Example: Comment module
Status: Finished or partial accepted
Scenario 2: Legacy module provides a migrate field plugin, module name change is OK.
Example: D7 Adressfield to D8 Address
Status: Finished or partial accepted
Scenario 3: Legacy module could have migrations but policy is to not make them.
Example: Views
Status: TBD
Scenario 4A: Legacy core module doesn't require migrations no equivalent D8 module
Example: Overlay
Status: Finished
Scenario 4B: Core module doesn't require migrations and no legacy equivalent
Example: Big Pipe
Status: Finished
Scenario 5B: Contrib that has D8 version but does not need any migrations
Status: Finished
Scenario 5B: Contrib that has D8 version that needs migrations but are not yet been developed
Status: Not started
Scenario 5C: Contrib that has D8 version that has a migration and more to develop
Status: Partial
Scenario 5D: Contrib that has D8 version and all migrations finished
Status: Finished
Scenario 6: Contrib has multiple migrations with multiple destination_modules defined
Example: Commerce Migrate
Status: ? Commerce Migrate won't set a status because the destination is another module, Commerce, and it is Commerce that needs to declare it's status. This may be a good argument for using hooks, where Commerce can modify it's status depending on if commerce_migrate is installed?
Yes, there can be good reasons to not develop migrations and to redo things, i.e Views. I think the challenge here is how to inform the upgrader. If Views and modules like it are listed in the GREEN section then any detail on that line is not visible unless action is taken. And that doesn't seem so good to me. Not sure where to put that information where it will be seen.
Edit: fix header and first column of table
Comment #72
maxocub CreditAttribution: maxocub commentedDiscussed at migrate maintainers meeting with @heddn @mikelutz @maxocub and the consensus that an entry info.yml to mark green/finished would probably cover the scenarios in #71.
Something like, just some brainstormed ideas:
Here's the scenarios from #71:
Scenario 1:
comment
Scenario 2:
D7 addressfield to D8 address
Scenario 3:
Views
No migration templates in the module, so it shows up in red.
Scenario 4a:
Overlay
There's no D8 module, so no .info.yml to add the status too, so I think our $noUpgradePath hardcoded list will have to do.
Scenario 4b:
Bigpipe
There's no D6 or D7 equivalent, so it will never show up in the UI. Nothing to do.
Scenario 5a:
Pirate (Contrib that has D8 version but does not need any migrations)
or just: (?)
Scenario 5b:
Contrib that has D8 version that needs migrations but are not yet been
No migration templates so it will shows up in red.
Scenario 5c:
Contrib that has D8 version that has a migration and more to develop
Migration templates are found, but no 'migrations_finished' in its .info.yml file, so it shows up in yellow.
Scenario 6:
Commerce migrate
This one I'm not sure what to do
Comment #73
heddnFor #6, I wonder if the following could be added into the various commerce_migrate_* modules info.yml files:
Comment #74
quietone CreditAttribution: quietone as a volunteer commentedAlways like a simpler approach!
On a early morning, not completely awake review on the new approach, I see there are two missing scenarios
Scenario 4C:
XYZZY
There's no D8 module, the module is enabled on the source, so RED.
And another more serious one
Scenario 7
A, B
Several modules provide the migrations for one source module.
The proposal does look like it will work when there is a one to one relationship between the source module and the destination module providing the migrations.
However, I don't see how this will work when migrations exist in several d8 modules for the same source. For example, consider the menu migrations, they exist in system module and in menu_ui both with a source_module of menu. If only the system module migration was complete then 'migrations_finished: 'menu: menu' would be in the system module and nothing would be in menu_ui. Then the UI would show menu in GREEN, which would be wrong. To display correct information the UI needs to know the migration for legacy menu are not yet written and they are to be in the menu_ui module. The same will be true for the i18n translation migrations.
This look promising for modules like commerce_migrate that provide migrations for other modules but like the case above some other module working on commerce migrations needs to have the ability to say no, migrations for Ubercart products, as an example, is not complete and my module is doing that.
So, right now I think there is need for a 'migrations_not_finished' so that modules can declare that they are also providing migrations, whether they currently exist or not, for the same source module.
Make sense?
Comment #75
quietone CreditAttribution: quietone as a volunteer commentedRerolling.
Comment #77
quietone CreditAttribution: quietone as a volunteer commentedThis patch implements, or attempts to implement, the approach in #72. There are two categories, 'will be upgraded' and 'will not be upgraded' and there are no details, the extra explanatory text supplied by a module.
There is one change to the approach in #72, that is the addition of the property "migrations_not_finished' to the info.yml. Why? Well 'migrations_not_finished' is necessary, I think, to allow for the situation where more that one Drupal 8 module is providing the migrations for source module 'A'. For example, block provides migrations for block, and block_content provides migrations for block. If block has an entry 'migrations_finished' in block.info.yml and block_content does not declare anything in info.yml then 'block' will show as will be upgraded. Which is not true. At least both block and block_content must have an entry
for block to be in the will be upgraded list. That is why 'migrations_not_finished' is an allowed option.
This patch is to show the approach and some tests will fail. I've used MigrateUpgrade6ReviewPageTest, and hopefully that will pass tests, for testing and have not looked at the other tests. There are two screen shots to show how it looks. The 'will not be upgraded' section now shows which destination modules are associated with that source module. It will need some explanation to say that there are migrations for source to these destinations but some migrations are still to be developed.
So far, I like this approach and it is much simpler for contrib.
One thing I would like to know is where to add some validation code for the changes to info.yml.
Oh, no interdiff since this is a totally different approach than the previous patch.
Comment #78
quietone CreditAttribution: quietone as a volunteer commentedForgot to upload the screenshots
Comment #80
quietone CreditAttribution: quietone as a volunteer commentedHopefully, this will improve the tests.
Comment #82
quietone CreditAttribution: quietone as a volunteer commentedAnother try. This still needs work.
Comment #84
quietone CreditAttribution: quietone as a volunteer commentedRemove duplicate entry in the available paths.
Comment #85
quietone CreditAttribution: quietone as a volunteer commentedA bit of an IS update.
Comment #86
quietone CreditAttribution: quietone as a volunteer commentedThe work to do here is to review all the migration properties in all the yml and make sure that what is displayed on the review page is really correct. Adding this information to the .info files caused many changes in the review page and it will take some time to double check all that.
If you want to review this have a look at the screen shots in #78. The will be upgraded looks much the same but the will not be upgraded now also includes the destination modules, specifically the destination modules that have completed their part of migrating that module. It is a way of indicating that the migration of these source modules is incomplete. If this is the way to go then some help text on the page is needed to explain all that to the user.
I will add that I still like this approach. It remains to be seen if it truly handles the edge cases.
Comment #87
heddnLooks good for a start. Tagging for UX input. The "not upgraded" screenshot seems to beg for telling the user to install modules and look for upgrade candidates, etc. Or whatever we decide on for language.
Comment #88
heddnComment #89
heddnComment #90
quietone CreditAttribution: quietone as a volunteer commentedOf the UI issues, this is arguably the most useful to the community. Therefor, tagging migrate critical.
Added the latest screenshots to the IS, hoping that makes this easier for review.
Comment #91
quietone CreditAttribution: quietone as a volunteer commentedNeeds a reroll.
Comment #93
quietone CreditAttribution: quietone as a volunteer commentedNow, fix the tests.
Comment #94
quietone CreditAttribution: quietone as a volunteer commentedComment #95
quietone CreditAttribution: quietone as a volunteer commentedStill working on improving the IS for reviews.
Also removed the section on scope as the referenced issue is out of date and I just closed it.
And brought the CR up to date.
Comment #96
quietone CreditAttribution: quietone as a volunteer commentedAdding before and after screenshots etc.
Comment #97
quietone CreditAttribution: quietone as a volunteer commentedComment #98
quietone CreditAttribution: quietone as a volunteer commentedDiscussed at migrate meeting and decided the CR and the IS needed updated. The CR needs an example and the IS needs to clarify that this issue is the first step in defining a new system and followups are expected.
Comment #99
Gábor HojtsyFirst of all based on the screenshots, the updated page looks much better, it seems to tell people that the migrations will not happen because the target modules are not installed.
Is the next step to install the target module? If so, the text could be updated above the table. Or are there other possible next steps?
Comment #100
quietone CreditAttribution: quietone as a volunteer commentedThanks, glad this is moving in the right direction.
From my point of view the next step is to ask 'does the target site need the data from this module?' If no, there is nothing to do and they can proceed. If yes, then they need to install it and try again. If the module is now in the 'will be upgraded' then they can move on to the next module to examine. If however, it is still in the 'will not be upgraded' then they need to know what to do next. That could be a page on d.o. that explains in more detail the options, such a reviewing if that data is needed and checking the module issue queue.
It would be nice to ultimately have a way for contrib modules to specify their own help page that could be added to the review page but that is for later.
Comment #101
quietone CreditAttribution: quietone as a volunteer commentedI was able to attend the UX meeting today and we discussed the Review form, what the user should see and the general flow of an upgrade (well said by webchick). It was a thorough discussion of what needs to be done now to get this issue moving and still have an eye for future improvements.
First, the group agreed the page new page is an improvement. Yay!
The resulting action items are:
1) Add a suggestion on the Review page to install modules listed in the D8 column of the 'will not be upgraded' section. There were two options here, a) add a sentence/phrase to the paragraph at the start of the section or b) add a succinct phrase in parenthesis after Drupal 8, i.e. Drupal 8 (text here), although creating such a phrase is unlikely. The details of that text were not discussed, suggestions welcomed! This addresses the issue raised in #87.
2) Allow the user to enable all modules in a similar way that Extension does. A follow up is needed for this.
3) Use Module name not machine name in the list. A follow up is needed for this as well.
Comment #102
quietone CreditAttribution: quietone as a volunteer commentedStarting working on #101.1 and quickly decided that adding text to the help message for that section is the way to go. Why? Because during the meeting I didn't see the obvious. Where there are Drupal 8 modules listed that means there are migrations for these source/destination module pairs but they are not complete. Where there is a empty Drupal 8 module list that is where the likely action is to enable a module, but which module? We can't assume that the name is the same as the Drupal 6 or Drupal 7 module name. I'm thinking of AddressField/Address here. Perhaps in a followup the work can be done to put the names of the core modules.
This patch adds a suggestion to check that all needed modules are enabled on the destination. The text is now
The other changes here are comments and adding i18nprofile to the info yml in config_translation.
Comment #103
quietone CreditAttribution: quietone as a volunteer commentedIssues made for
#101.2 #3024681: Migrate UI - allow install of modules in the will not be upgraded list
#101.3 #3024682: Migrate UI - add human-friendly module names to the Review form
Comment #105
quietone CreditAttribution: quietone as a volunteer commentedThose errors make sense since I didn't update those tests for the changes to i18nprofile
Comment #106
quietone CreditAttribution: quietone as a volunteer commentedThis now needs reviews and more reviews.
1) Review new text in the help display for modules that will not be upgraded. See #102.
2) Review the info yml settings and the Review page for accuracy. This alone will take some time and is tedious. I'd recommend doing a few modules at a time and reporting back.
Comment #107
Gábor HojtsyI would love if we could shorten the text and add spacing between the table and the text so the text is more prominent and looks more intentional.
This is the current text:
'There are no modules installed on your new site to replace these modules, check that all modules you need are enabled. If you proceed with the upgrade now, configuration and/or content needed by these modules will not be available on your new site. For more information, see <a href=":review">Review the pre-upgrade analysis</a> in the <a href=":migrate">Upgrading to Drupal 8</a> handbook.'
1. Is it important that the upgrading to Drupal 8 handbook is where the checklist is?
2. Can we integrate the suggestion to enable/install the module into what happens if that does not happen, so we don't need to mention it twice?
3. Can we shorten the text otherwise while retaining the meaning?
'The new site is missing modules corresponding to the old site\'s modules. Unless they are installed, configuration and/or content needed by them will not be available on your new site. <a href=":review">Read the checklist</a> to help decide what to do.'
Is this still explaining the same thing? Was all meaningful information retained? Is it simple enough English? If so, then great :) We should keep it short and to the point. After all what we want to say is "Modules are missing. Most common is to install them. But really just read the fine print". So the faster they get to know about checklist the better IMHO.
Comment #108
Gábor HojtsyMaybe "Unless they are installed" should be "Unless they are installed prior to the upgrade" to make it super clear that it is a "do it and then come back here" vs. a "this process will somehow make it happen if I click the right checkbox". Until #3024681: Migrate UI - allow install of modules in the will not be upgraded list builds it into the process that is.
Comment #109
jhodgdon+1 for the proposed text revision in #107/108.
If you keep the original text shown in #107, nitpick: the first comma should instead be a ; (because it is splicing together two complete sentences). Also the original text uses two different words in the first sentence for the same concept, about modules being "installed" and "enabled". That makes me wonder if the two words have two different meanings (which they don't in D8), which makes me think (bad for usability). The new text does not suffer from either of these problems.
Comment #110
quietone CreditAttribution: quietone as a volunteer commentedGábor Hojtsy and jhodgdon, thanks for the feedback!
This patch changes the text to as suggested in 107/108. Personally, I like it.
'The new site is missing modules corresponding to the old site\'s modules. Unless they are installed prior to the upgrade, configuration and/or content needed by them will not be available on your new site. <a href=":review">Read the checklist</a> to help decide what to do.', [':review' => 'https://www.drupal.org/docs/8/upgrade/upgrade-using-web-browser#pre-upgrade-analysis']),
Now to review a few .info.yml files.
Comment #111
quietone CreditAttribution: quietone as a volunteer commentedThe format of the migrate section in info.yml needs to change to allow multiple destination modules for the same source since, of course, we can't have the duplicate keys. For example in the language module for D6 it needs to be:
because the language module provides more that one migration with a source_module of locale. As you an see the value is a CSV list.
I discovered this after deciding the only way to check all the info yml is to write a test to validate them. It is still a work in progress but will be needed.
Comment #112
quietone CreditAttribution: quietone as a volunteer commentedAdding a test to check the declarations in .info.yml to the discovered migrations. The test is really rough. This will fail ReviewPage tests.
Comment #114
quietone CreditAttribution: quietone as a volunteer commentedWorking to fix the tests.
Comment #116
quietone CreditAttribution: quietone as a volunteer commentedThis should fix the tests and also remove a test file I left in the previous patch.
Comment #117
quietone CreditAttribution: quietone as a volunteer commentedBack to NW as this should have tests to prove that the status declared in the info is working as expected.
Comment #118
quietone CreditAttribution: quietone as a volunteer commentedA first attempt at a test that checks that the migrate status information in .info.yml matches discovered migrations. Well, for the migration_finished values. The values in migration_not_finished can't be verified because the migrations may or may not exist.
Comment #120
quietone CreditAttribution: quietone as a volunteer commentedThis is a failure I've not seen before
Comment #121
quietone CreditAttribution: quietone as a volunteer commentedDiscussed in the migrate meeting, the result being that it is time for a framework manager to take a look, adding tag. The last time was back in #7.
Also, a reminder that this is just used by the Migrate UI, to provide useful information to the use about what will be upgraded.
Removing the Needs UX review, since the recommendations from that meeting have been implemented (#106 - #110).
Comment #122
quietone CreditAttribution: quietone as a volunteer commentedThe test module names in the test were still the old ones.
Comment #123
heddnAssigning to myself for review this week.
Comment #124
larowlanThis is looking good
nit: their
Let's use "" here instead of \'
personal preference, but you could
continue
here and drop the else to reduce complexitysame here, you could continue in both the if and elseif and avoid using elseif, which is a known code smell.
whitespace
I can see from comment #7 that the plan to put this info in the .info.yml files was discussed before with @alexpott however, I have some concern about the memory overhead of the amount of information we're going to have in memory for every request, when really it only applies in a small set of scenarios. For that reason adding needs profiling tag, would be good to know what sort of memory overhead (if any) this adds to a typical request. If its a bit, we might need to look at separate yml files for each module with a migration naming pattern.
Removing the framework manager tag.
Comment #125
mikelutzDiscussed in maintainers meeting, we feel this would probably be most appropriate in a separate yml file regardless of performance profiling, since this data is not useful except in the rare cases of running a migration.
Comment #126
quietone CreditAttribution: quietone as a volunteer commentedNeeds a rerolll.
Comment #128
quietone CreditAttribution: quietone as a volunteer commentedMove i18nsync in the tests.
Comment #129
quietone CreditAttribution: quietone as a volunteer commentedNow move the migrate status information from info.yml to module.migrate.yml. The patch include fixes for all the items in #124.
Comment #131
quietone CreditAttribution: quietone as a volunteer commentedForgot to remove i18nsync from the missingpaths list.
Comment #132
quietone CreditAttribution: quietone as a volunteer commentedComment #133
quietone CreditAttribution: quietone as a volunteer commentedFix some comments in ReviewForm and update the CR.
This is ready for ready.
Comment #134
heddnCleaning up some tags. We don't need profiling (we're using yml files directly now), nor usability (that was already done around #101). I don't think it needs documentation as the CR is pretty clear about what is needed. Or at the least, it shouldn't block commit here.
So with that, marking RTBC as all feedback since last RTBC is now addressed.
Comment #135
Gábor HojtsyI looked at committing this and tried to understand the not finished vs finished concept, but neither the change record nor the comments linked helped me understand it. It is probably my limitation not the explanation, but it may be true for others not deep in the weeds of migrate as well. Can we put examples in the change record and explain some scenarios. Also explain that 'not finished' means the development of them is not finished, not the execution for example :) Also a combination of not finished ones may show up as finished still? How? These are unclear to me from trying understand from the docs.
Comment #136
quietone CreditAttribution: quietone as a volunteer commented@Gábor Hojtsy, thank you!
Added scenarios to the CR and reworked it a bit too and the Doc block for Review Form is updated as well. Hopefully, all the migrations_finished/migrations_not_finished are explained as meaning the migrations exist or do not exist.
As I was working on this I realized that now that the data is in .migrate.yml that the properties 'migrations_finished' and 'migrations_not_finished' can be changed to 'finished' and 'not_finished' since it is already obvious that the data is about migrate. Any objections?
Comment #137
heddnI see your point about removing the prefix 'migrate_' from the yml file since they are already in a migrate.yml file. I think that make sense, so let's do it. Some more feedback on the doc comments below. I also reviewed the CR.
Nit: "is a list of all the..."
We're missing the word "a".
Nit: the word "module" is not a proper noun and could be lower case.
And it "is" only when
lowercase "not"
This could use some wordsmithing. Not exactly sure what we were trying to communicate.
Are the pair of source/destination only optional if one of these do not exist?
This seems wrong. Should this be source module?
...and more migrations need to be created to finish the upgrade path.
Can this whole "status checking" logic move into a service that returns the two categories of statuses? I'd like us to externalize into a service so that the drush runners can also use the same logic in a pre-audit drush command.
Comment #138
quietone CreditAttribution: quietone as a volunteer commented@heddn, thanks.
Only time to do the doc fixes, #137.1 -> 8.
Comment #140
quietone CreditAttribution: quietone as a volunteer commentedRemove migrations_ from migration_finished and migrations_not_finished.
Comment #141
quietone CreditAttribution: quietone as a volunteer commentedComment #143
quietone CreditAttribution: quietone as a volunteer commentedFix the two tests. One where there were still occurrences of migrations_finished or migrations_not_finished and the other is Upgrade7 test which needs to be adjusted for the recent commit of #3008030: Migrate D7 i18n fields label and description.
Comment #145
quietone CreditAttribution: quietone as a volunteer commentedTypo in i18nfield is causing the failure, I think.
This moves the logic to a new service, MigrationState (is there a better name?) and includes some cleanup and renaming, to give the arrays descriptive names. And I tried to make 'State' is used instead of status everywhere except for the final output array (the one used in ReviewForm) where I used Category as it seemed to fit with the two 'categories' (will not be upgraded and will be upgraded) on the Review Form. The logic has not changed.
The services provides two methods getUpgradeCategory and getMigrationStates. getUpgradeCategory get an array with keys 'finished' and 'not_finished'. Is there need for an input argument so that it gets just one of the categories as well? getMigrationStates gets the raw data from the migrate.yml file. Is this needed to be public? Seemed like a good idea at the time but I am not sure.
Comment #147
quietone CreditAttribution: quietone as a volunteer commentedNow, to fix the tests.
The profile migration for Drupal 7 is not finished, there is a d6_profile_values but not a similar one for D7. Someone should double check that. The logic is 'tightened' up by ensuring the 'finished' has been declared in .migrate.yml for a source module to be 'finished'. Previously, it was the last else after all the other tests which did work but it should be stricter.
Comment #149
quietone CreditAttribution: quietone as a volunteer commentedAnother attempt to fix the tests and rename some variables.
Comment #151
quietone CreditAttribution: quietone as a volunteer commentedChange the profile to will be upgraded for D7.
Comment #152
quietone CreditAttribution: quietone as a volunteer commentedThis is ready for review. But I am concerned about the state of Profile in D7, the D7 fixture does not have values in a profile_values table nor a d7_profile_values migration and of course not test of that upgrade path. So, that really should be 'not_finished'. Correct me if I am wrong.
Comment #153
quietone CreditAttribution: quietone as a volunteer commentedYes, changed the state of profile migration in D7 to not_finished. Having some trouble running all the tests locally so let's see what happens.
Comment #154
heddnYou're going to hate me, but this a migrate_drupal API here. So we might want to use that in the naming of this yml file. i.e.
filter.migrate_drupal.yml
Let's put this service in migrate_drupal. That way drush can use it without needing an UI.
This seems at odds.
Why did we decide on error and checked? What does each contain?
I saw your comments and thoughts about naming. I think we should always use state for category. I mildly prefer state over category as category is an overloaded term. So let's standardize on that?
This isn't needed, just use
$this
Comment #155
quietone CreditAttribution: quietone as a volunteer commentedFixes for 154-1 -> 8.
154.4 Error and checked came to be used because it is the name of the icon displayed for will be upgraded and will not be upgraded. Those references should all be gone now.
154.5 Change status and category to use state everywhere so lots of name and/or comment changes.
Comment #157
quietone CreditAttribution: quietone as a volunteer commentedFixing errors when changing from 'status' to 'state'.
Comment #158
quietone CreditAttribution: quietone as a volunteer commentedJust tidying up the test and adding comments.
Comment #159
quietone CreditAttribution: quietone as a volunteer commentedReroll, just changes to migrate_drupal.services.yml.
Comment #160
quietone CreditAttribution: quietone as a volunteer commentedReady for review again!
Comment #161
heddnOne small question and a couple small nits.
Are these needed here?
Stray asterisk here.
Nit: maybe drop the word 'all'.
Comment #163
quietone CreditAttribution: quietone as a volunteer commented1. No. That was me testing something and should have been removed awhile bak.
2. fixed
3. fixes
Comment #164
quietone CreditAttribution: quietone as a volunteer commentedUpdate IS and CR to new names> Added the new service to the IS.
Comment #165
heddnNo more concerns. Onward.
Comment #166
Gábor HojtsyOnly found these two minor things now. Looks good to me otherwise.
*.migrate_drupal.yml would read better IMHO in the first two cases as in the last case.
modules.
Comment #167
mikelutzComment #168
mikelutzComment #169
mikelutzMade some documentation adjustments.
Comment #170
mikelutzComment #171
mikelutzLets try this again...
Comment #172
heddnIf this comes back green, all feedback is addressed.
Comment #173
quietone CreditAttribution: quietone as a volunteer commentedTalked to webchick on Slack about this
The initial work on the meaning of finished/not_finished, from comment #72, has this scenario.
And that is not how the code logic works, IIRC.
TODO:
New after screenshots the ones in the IS are out of date
Check the logic for Scenario 5A and confirm it's consequence that all contrib needs to add migrate_drupal.yml
Comment #174
quietone CreditAttribution: quietone as a volunteer commentedThere are now updated after screenshots. The checklist referred to in the screenshot is https://www.drupal.org/docs/8/upgrade/upgrade-using-web-browser#pre-upgr...
Note that the 'will not be upgraded' section does not have a list of destination modules. That was removed a while ago because a list of destination modules when the source module isn't going to upgraded properly doesn't make sense and will cause more confusion than not having it. However the heading 'Drupal 8' still remained and this patch removes that heading.
There are also comment updates and the CR is updated both to address the case of a Drupal 8 module that is a successor to a legacy version and does not provide migrations.
Comment #175
heddnSome real simple docs fixes, nothing more.
or Drupal 7 module is to declare...
"A" is at end of previous line and on new line.
Let's be explicit and say it is the machine name of these two things.
Comment #176
mikelutzComment #177
mikelutzComment #178
mikelutzI was struggling to understand that entire docblock, and lost it at the word 'irregardless'. I ended up rewriting a good chunk of it to improve readability.
Comment #179
heddnSo close.
Nit: 80 chars.
Since there is a finite list of values we can use here of 6 or 7, let's use those instead. It will be more clear.
Comment #180
mikelutzComment #181
heddnLooking good again.
Comment #182
quietone CreditAttribution: quietone as a volunteer commentedAh, more proof that I am not a skilled writer. Thanks, the improvements read better.
Unfortunately there is one item that needs a fix.
Sorry, but the tense isn't correct here. To be finished the migrations must be provided, not going to be provided at some future time. This is an important distinction because the ones that are going to be provided must be in the 'not finished' section.
Comment #183
mikelutzTalked with quietone on slack, and redefined some terms in the documentation.
Comment #184
quietone CreditAttribution: quietone as a volunteer commentedYes, 'sets' is a suitable term here and the changed text reads well and is correct. Thanks mikelutz.
The new wording used in the doc should be reflected in the change record as well. I can update the cr and fix the nits tomorrow.
I did find a few nits, none of which changes the meaning.
I think 'definition' can me removed and the meaning is retained.
s/This/A/
s/The migration/A migration/
Are the parenthesis needed?
s/migrations set/migration sets/
Comment #185
quietone CreditAttribution: quietone as a volunteer commented1. Not fixed. When reading again I though it was fine.
2-5 Fixed.
Updated the CR to add the 'set' terminology.
Comment #186
mikelutzLGTM. I think this is ready to go again.
Comment #187
heddnLooks good to me.
Comment #188
quietone CreditAttribution: quietone as a volunteer commentedNew before screenshots because webchick pointed out that one of them was wrong, so I remade both of them.
Comment #189
quietone CreditAttribution: quietone as a volunteer commentedremove update CR from the IS.
That just leaves the documentation and commit.
Comment #190
webchickDiscussed the patch again with the Migrate team today (this time summarizing my own comments; I'm really sorry that I had to run the other day).
I was a bit confused by the before/after screenshots, because I expected them to demonstrate the impact of this patch. For example, "before, this module was erroneously showing as complete / not complete, and now it's fixed." Since the point of this patch is to add a mechanism to allow modules to self-register this information. @quietone pointed out that you don't see a difference in the screenshots because we're only dealing with core, and we've already worked around outliers in other ways in other core patches (so identical screenshots == yay, it's working). Fair enough.
But then the size of this patch was quite alarming, 70K+ with changes required to every single core module. So the question shifted to "what happens when 8.7.0 ships and none of the contrib modules have one of these files yet?" A situation we don't want to have happen is that a site's migration test in 8.6 shows 7 outstanding modules, and then in 8.7 suddenly shows 40 of them. We're nearly 4 years from the initial release of Drupal 8.0.0, and that's a really tough pill to swallow to ask every single contrib module to suddenly add one of these files, lest they get bogus bug reports about missing migration paths.
IOW, ideally, we would not see hunks like this, because the mIgration system would be smart enough to derive "this is fine":
...and instead, we'd only see hunks like this, where there's some weird outlier:
This would allow contributed modules, such as pirate, which are currently false-flagging as "not ok" to instead say "it's cool, man." (And vice-versa.) But would not require every contributed module to suddenly have to do a thing. (The CR would note this as an optional new API that modules experience migration reporting weirdness could utilize.)
Completely unrelated to that, but found while I was grepping for an example, what's up with this?
How is profile: user both finished and not finished? :)
Comment #191
quietone CreditAttribution: quietone as a volunteer commented@webchick, good find in user.migrate_drupal.yml. That was me doing some testing.
@mikelutz, I had to get this out of my head so I could sleep. This is very rough and I can see many improvements to make but it is enough for now. The method buildForm is a reduced version of the original since the work to build $table_data is already done. I even started to a test module but ran out of energy to complete the test.
And separately I noticed that the unsetting of the profiles was in the ReviewForm and that needs to be in the service, so it is moved.
Comment #193
quietone CreditAttribution: quietone as a volunteer commentedSome improvements to the new ValidateMigrationStateTest. And finish the changes started in 191. Specifically, where instead of marking a source module as will not be upgraded if it is not declared in a migrate_drupal.yml file, the previous method is used.
There is still work to do here, at least to add a sensible message in the trigger message when the previous logic is used.
Comment #195
quietone CreditAttribution: quietone as a volunteer commentedWhile working on the change that webchick suggested I noticed an error in the logic of determining the state, or rather it worked by fluke. And fixing that lead me down a path that became a rewrite of the logic. It is simpler, has an additional check so hopefully better.
The new check is to test that all the destination modules for a given source module are installed. That test lead to a problem when the destination module is set to 'core', which it is for migrate field plugins and for modules that don't need an upgrade path. For these it was always just assumed that the modules needed would be enabled. That is not always true and is most noticeable with the i18n modules, some of the i18n ones don't have an upgrade path either and that caused them to show up as 'will be upgraded' all the time. The solution for that was to remove them from the noUpgradePaths list and put them into a migrate_drupal.yml and change the destination module to config_translation or content_translation.
With that done it, there was no reason not to remove noUpgradePath and put them all into a migrate_drupal.yml. After all, this is what is expected of contrib. For now, these are in system.migrate_drupal.yml. Further work is needed to move as many as possible to a module.
Another change is the list of D8 modules for the will not be upgrade is restored. The reason being that the help page can than say that a possible solution is to make sure that all those modules are enabled.
The patch adds deprecation notice to the method that is doing the previous logic to determine the state.
TODO;
New after screenshot of the will not be upgraded section.
Move the noupgradepath declarations from system.migrate_drupal.yml to more relevant modules.
Comment #197
quietone CreditAttribution: quietone as a volunteer commentedNow to, hopefully, fix the upgrade text. The extra failures in 8.7.x are due to the new trigger_error on the old method of calculating the migration state.
Updated the after screenshot.
TODO
Move the noupgradepath declarations from system.migrate_drupal.yml to more relevant modules.
Update CR
Comment #198
quietone CreditAttribution: quietone as a volunteer commentedThis checks the review page output before and after the patch is applied to check that the legacy method to determine if a module is upgraded or not is used when there are not declarations in migrate_drupal.yml. The concern being that with this patch we don't want people to suddenly see lots more modules as will not be upgraded than before. This test was done using the Ubercart 6 test fixture from Commerce Migrate.
Before applying the patch there were 19 modules that will not be upgraded and 42 that will be upgraded. After applying the patch there are 19 modules that will not be upgraded and 36 that will be upgraded. Will explain the difference in a day or two.
Comment #200
quietone CreditAttribution: quietone as a volunteer commentedThought it would be better to use the old method as a whole than the reduced version that was there.
Comment #202
quietone CreditAttribution: quietone as a volunteer commentedMake some corrections to the source_module definition for language migrations. Add @legacy to tests.
Comment #203
quietone CreditAttribution: quietone as a volunteer commentedI went to the Review page with and without this patch using the Ubercart 6 test fixture from Commerce Migrate to check that the concern from #190 about suddenly showing lots of will not be migrated is addressed and the user doesn't get a shock when this is committed.
Without the patch there were 20 modules that will not be upgraded and 41 modules that will be upgraded. With the patch there were 19 modules that will not be upgraded and 36 modules that will be upgraded. The numbers will not be exact since this new method is by definition more accurate.
The differences in the will not be upgraded are i18ncontent and phone without the patch and i18ntaxonomy with the patch. Here, i18ncontent is correctly listed as will not be upgraded but not phone, it will be. With the patch i18ntaxonomy is correct.
The differences in the will be upgraded are date, i18ntaxonomy, language, search, update, upload and userreference without the patch and i18ncontent, phone with the patch. In the will not be upgraded, date, language, search, update, upload and userreference are all disabled on the source and should not be listed, this is a bug. i18ntaxonomy is in the list because those translation migrations are in the taxonomy module which are discovered, that is incorrect. I think those migrations need to be moved. With the patch phone is correct. I think i18n content should not be here and that needs looking into.
Still this does appear to address the point from #190.
Comment #204
quietone CreditAttribution: quietone as a volunteer commentedAdd new before/after screenshots of the will not be upgraded.
Comment #205
quietone CreditAttribution: quietone as a volunteer commentedI did look into that and it is fine. I simply forgot that content_translation was enabled.
This is Ready for review.
Comment #206
heddnWow, a lot to digest in there. But it all looks good. The only thing I could find is the need for a period.
Needs a period to break up 2 sentences. This should be:
That can be fixed on commit. Onward.
Comment #207
heddnIs this still a candidate for 8.7 backport?
Comment #208
webchickSpoke about this to @quietone, and my previous concerns have now been addressed. Basically, the logic works like:
1) If there is a yml entiry for legacy module foo in any yml, the use that entry or entries
2) If there is no a yml entry for foo, then use the old way of determining the status
So there won't be a dramatic drop-off of modules listed as being migrated; instead ones without the YAML file declaring a "definitive" answer, it'll just be as wrong as it was before. ;)
This removes my product management concerns, and Gábor and I would also advocate for this from a PM POV even though we're past beta, because it's very strategically important from the standpoint of helping more D7 sites move to D8, and we don't want to lose 6 months off that schedule.
Escalating to critical, per a committer discussion.
Comment #209
webchickHowever, I can't make that call, so sending to release managers for review.
Comment #210
larowlanI really wanted to commit this, but I've got a few issues - I really appreciate all the work that's gone into this, but we have one shot to get this API right and if we rush it for 8.7 we've got a short period to validate it. I'm available to help with the items I've identified here - let me know.
any reason we're using CSV syntax here instead of native yaml features - ie arrays?
nit: not sure we need the local variable here, but don't feel strongly about it
this is quite a long sentence - suggest
If migrations for any of those three modules are not complete or if any of them are not installed on the destination site then the Drupal 6 Menu module cannot be listed as upgraded
. Also nit but 'enabled' is not a concept anymore in Drupal 8, just installed/uninstalledany reason we don't use `arguments` in the service definition here? Feels like we could be doing dependency injection here.
we have a module handler available as $this->moduleHandler, so should use that?
this kind of array return makes me itchy.. can we use a value object instead. Its a bit of a mystery meat api and a good indication that we need to refactor
same here :(
do we need the local $state variable here?
we have constants for finished/not_finished we should use them instead of the strings
same here re constants
yeah, if we used YAML arrays instead of CSV it would be cleaner and we'd avoid this
do we have a deprecation test for this? I'm guessing from the @legacy tags implicit coverage, but not an explicit test. Our deprecation policy requires one.
I don't think we need to define these in two places, let's put them on an interface and use that everywhere
Comment #211
larowlanI have 45 mins spare to address those items I flagged
Comment #212
larowlanwill finish in morning
Comment #213
quietone CreditAttribution: quietone as a volunteer commented@larowlan, thx for the review. All good points. I'm not even thinking about 8.7. I just want to get this done and making the time for it now.
1. Because it seemed the quickest solution at the time. Fixed, using yml.
2. Fixed
3. Fixed
4. Because I don't know how to do that.
5. Fixed
6, 7. Not so fond of it myself but this is all the legacy stuff that will go soon so it was low priority to improve. How to implement what you suggest?
8. Fixed. not needed, it is removed.
9. Fixed
10. Fixed
11. Fixed, much better, thx.
12. No. Needs test.
13. Fixed
If test pass then 4 items todo, #210-4, 6, 7 and 12. Well, 3 really 6 and 7 are the same point.
Then the CR needs updating since the syntax of the migrate_drupal.yml file has changed. And this should have a follow up to pull out the legacy code.
Comment #214
quietone CreditAttribution: quietone as a volunteer commentedCR updated using yml syntax for multiple destination modules.
Comment #216
mikelutzA blinders on attempt to resolve the test failures.
Comment #217
heddnWe shouldn't need to implement this here. Just using the constants would be enough.
Still need to fix issues from 210 4,6,7. I'm inclined to push 6 and 7 as a follow-up material.
Comment #218
mikelutzI agree on 6 and 7, but I'm writing up a case for it and doing some minor refactoring right now.
Comment #219
xjmComment #220
mikelutzI'm sorry, @larowlan, I didn't mean to steal this from you. I've added a few additional comments.
nit: These should probably go on separate lines
Let's add these two public functions to the interface.
I realise the indexed array is a bit yucky, but I would argue that these return values are on protected methods and are purely internal to this implementation of the service. They aren't part of the API at all, so I'm not sure we NEED a custom value object that would only be used internally to this one class, particularly on the deprecated internal method.
We should use the new format for deprecation errors here.
[FieldDiscoveryInterface::DRUPAL_6, FieldDiscoveryInterface::DRUPAL_7]
Comment #221
quietone CreditAttribution: quietone as a volunteer commented1. I have no strong preference and like you say, it is a nit, since YAML accepts both. Just want to note that there are several migrations that use this style, this one from statistics_node_counter.yml. At least, this patch isn't introducing a new style.
From reading the YAML spec it doesn't seem that this is necessary. And I tested it last night by adding spaces in one of the migrate_drupal.yml sequences and the Validation test still passed.
Comment #222
mikelutzAh, like I said, blinders on trying to get tests to pass. If the trim was only there for the spaces around the commas back when it was a csv format, then that should just reduce to
Comment #223
larowlanAddressed the issues I identified and the subsequent comments
Discussed with quietone, this doesn't consider that some of these modules may not be enabled.
Added a unit-test and confirmed solid coverage now that the service is unit-testable.
Having issues with teardown in the functional tests locally, but I think they're file permissions related, will keep an eye on this to see what bot says
Edit:interdiff is against 202
I also:
Comment #224
quietone CreditAttribution: quietone as a volunteer commentedUpdated CR and IS with the proper yml syntax for the migrate_drupal.yml file.
Comment #225
quietone CreditAttribution: quietone as a volunteer commentedThat's odd I didn't make the change to status and assigned.
Comment #226
heddnI had about 30 minutes this morning to do a quick review. Some of my points could be easily dismissed if I read things too quickly. Still leaving it in NR for other feedback. Although the last point does leave me wondering if we missed some conversions to the new yaml discovery method.
This comment doesn't match up with the variable name. If these don't hae a yml file, then a state file is not required? And we use legacy method of detection.
Shouldn't this be assertEquals?
I think there's a trait for this?
There's something odd that these are all marked as legacy tests. I'm fine with solving them in a follow-up if we missed a few source/destination modules, but a little digging into why these need to be marked legacy would be good.
Comment #227
masipila CreditAttribution: masipila as a volunteer commentedCan we please have an update to the issue summary? The Proposed Resolution has quite a few questions and it is quite difficult to understand the scope by reading the IS...
Comment #228
masipila CreditAttribution: masipila as a volunteer commentedMaybe the proposed resolution could simply be: allow Drupal 8 modules to declare their migration status in module_name.migrate_drupal.yml. See Change Record for details and usage examples.
And then a subheading for ideas that were considered but not implemented.
This way it would be super clear what is in and what is not.
Cheers,
Markus
Comment #229
quietone CreditAttribution: quietone as a volunteer commentedThanks masipila. The IS has been updated.
Comment #230
quietone CreditAttribution: quietone as a volunteer commented226.1 Fixed
226.2 Fixed
226.3 Can't find such a thing. This does use coreModuleListDataProvider from FileSystemModuleDiscoveryDataProviderTrait but I don't think that is what you mean.
226.4 The tests are marked legacy because they run deprecated code. But this still needs a follow up to remove the deprecated code.
There are whitespace error fixes in the patch as well.
TODO: make a followup to remove deprecated code.
Comment #231
quietone CreditAttribution: quietone as a volunteer commentedMore tweaks to the IS, as masipila suggested and I missed.
Comment #233
quietone CreditAttribution: quietone as a volunteer commentedI was so interested in the music my husband is playing today that I didn't realize I ran the wrong test.
This fixes the test.
Comment #234
quietone CreditAttribution: quietone as a volunteer commentedImprove the accuracy of the title.
Comment #235
heddnSo, is all we need a new follow-up to remove deprecated code? If that's all, then I think all feedback is addressed and this could be good to go.
Comment #236
quietone CreditAttribution: quietone as a volunteer commentedFollow up made, #3046710: Remove deprecated code from MigrationState
Comment #237
heddnLooking good.
Comment #238
effulgentsia CreditAttribution: effulgentsia at Acquia commentedHi, I'm trying to understand this patch, but I'm stumped by:
Is this saying that there's an
action.module
for Drupal 6 and Drupal 7? But is there?Comment #239
heddnre #238: System is the source module in D6/D7. And the destination is action. So in that case, we need to remove the extra
action: action
mapping. I did a quick review of the rest of the mappings and that looks to be a lone exception. Back to NW for a quick update.Comment #240
heddnAnd back to NR with the fix.
Comment #241
heddnComment #242
heddnComment #243
effulgentsia CreditAttribution: effulgentsia at Acquia commentedWhat's the meaning of these mappings being here? Is the idea that there isn't any data in these modules to migrate? But even if that's the case, shouldn't the mappings be in the corresponding destination modules (where possible)? For example, say the source site has views_ui.module enabled but the destination site does not. By the mapping being here, would that prevent the Migrate UI from informing the user that they need views_ui.module enabled on the destination site? Same question for blog.module. By the mapping being here, does that prevent discovery that https://www.drupal.org/project/blog exists for Drupal 8?
Comment #245
heddnViews UI could move to that project. Blog doesn't have a specific needed upgrade, even with that module you've linked. The data/content migration is covered by node. But there are also other tihngs like date_api, etc that are all dumped into system.migrate_drupal.yml. I'd much rather push moving these out to more logical locations in a follow-up.
Also, fixed the action test failures. And updated the CR to mention the core special use case for destinations.
Comment #246
heddnComment #247
quietone CreditAttribution: quietone as a volunteer commentedMade a patch and failed on upload because heddn beat me to it. Posting my comment anyway.
Yea, that will cause a failure because the declarations are linked to at least one migration. The action_settings migration has a source of action and destination of action which will be discovered. So the change is to have that migration use the correct source_module.
Regarding #243 the short answer is no. The mapping can be in any module. The destination module displayed as the D8 destination module is not the module the mapping is in, it is the destination module in the mappping. The list in question was originally a hard coded list in the ReviewForm in migrate_drupal_ui module and moved to system because that module will be enabled. Perhaps a better place would be migrate_drupal. Since moving it I am unsure about moving these to the most relevant modules but, in any case, it should be a followup.
Comment #248
effulgentsia CreditAttribution: effulgentsia at Acquia commentedWhat would be the consequence of just removing them from this patch entirely and adding them into their proper place in follow-ups? I'm guessing that they would then show up in the "Modules that will not be upgraded" list, and we don't want that? As a mitigation for that, what about retaining the
ReviewForm::$noUpgradePaths
variable until those followups are complete? That's a protected variable of an @internal class, which I feel better about having questionable information in it, rather than a public/documented YAML file saying stuff that isn't true.Comment #249
heddnKeeping them in
ReviewForm
means we have a much more complicated test and official API. One that is still in both worlds. Where we have to merge things from a form class and what is discovered via yaml. And to boot, I would argue that the information is true. And we should be keeping them in the system yml file. But I'd rather have that discussion in a follow-up. Rather than bog down this issue.We should try to address that whole question in a follow-up.
Comment #250
effulgentsia CreditAttribution: effulgentsia at Acquia commentedAlso, sorry if this seems like nit-picking, but how would you feel about moving the
MODULE_NAME.migrate_drupal.yml
files out of the root directory of the project? I don't like the project root directory being a dumping ground for things that don't really need to be front-and-center in the attention space of people looking at a project's code. It looks to me like this YAML file is highly connected to themigrations
directory. In a way, one could think of it as metadata about what's in there, kind of like the way that theconfig/schema
directory provides metadata about theconfig
directory. Therefore, what about moving those files intomigrations/status/MODULE_NAME.status.yml
? Perhaps it would then even make sense to split them up into four files:?
Comment #251
effulgentsia CreditAttribution: effulgentsia at Acquia commentedHow is it true? Poll.module did not move into core. It has data. Nothing in core migrates it (or does it?). In what way is it true to say that that is a finished migration with
core
as the destination?Or is the idea that the
poll -> core
migration is finished (i.e., nothing more needs to be done), but that doesn't preclude there being apoll -> poll
migration that https://www.drupal.org/project/poll could flag as not_finished? But how would a site owner who does not have D8's poll module installed know that?Comment #252
heddnI like that idea. Not sure we need to go as far as multiple files. But moving it seems like a decent suggestion. We're calling it 'state', so maybe a state folder?
Comment #253
heddnHmm, I guess it is safer to state that this API and the mappings won't make anything worse. I hadn't noticed poll on the list. Poll definitely does have its own migration path, contrib. I wonder though if my point should be that this patch doesn't make anything worse. It just makes the data more accessible for us to fix in follow-ups.
For true, I was thinking about views and blog when I wrote that part.
Comment #254
effulgentsia CreditAttribution: effulgentsia at Acquia commentedIn the patch, we have
MigrationState::$noUpgradePaths
. Given that we have that, are the entries insystem.migrate_drupal.yml
redundant, or is there different behavior between them being there vs. not? (I haven't fully reviewed the code in that class yet to know how that variable affects what the class returns.)Comment #255
quietone CreditAttribution: quietone as a volunteer commentedFirst reaction, I see your point about moving to a sub-directory. Yes, to migrations/state.
A definite no to separate files. It would be an ever changing list of files (d8 ones will need to be added, d6 ones removed etc) but mostly because it will make it harder to maintain and increase the chance for a mistake. As a migrator you will want to look at the full set of the migrations in your module at once to see the complete picture.
Agree.
Comment #256
quietone CreditAttribution: quietone as a volunteer commentedRe #254 The array noUpgradePaths is maintained because it is needed when the legacy code is executed. It is needed when a source module is enabled on the source and there is no declaration for it in any migrate_drupal.yml file.
Comment #257
effulgentsia CreditAttribution: effulgentsia at Acquia commentedRight. So then, would there be any behavior difference at all between
calendarsignup: core
oroverlay: core
being insystem.migrate_drupal.yml
or not? If there's no behavior difference, then let's remove all those "The following do not need an upgrade path" items from the YML. If there is a behavior difference, what is it?From a semantics standpoint, I can understand adding entries if there are migrations that are actually performed. For example, I guess you could argue that
blog: node
is an accurate entry, because there are migrations defined innode/migrations
that migrate all node types and nodes, which includes blog ones, so that's an example of actual blog source data getting migrated into a node destination. But for many of the "The following do not need an upgrade path" entries, such as calendarsignup and overlay, I'm confused as to what they mean.Comment #258
effulgentsia CreditAttribution: effulgentsia at Acquia commented+1 to just moving the YML files into
migrations/state
without splitting them up or renaming them.Comment #259
effulgentsia CreditAttribution: effulgentsia at Acquia commentedI guess the difference is triggering a deprecation error, per:
I see. So, in order to avoid that deprecation error, every source module that exists in our test fixtures needs representation in at least one
*.migrate_drupal.yml
so we usesystem.migrate_drupal.yml
as a dumping ground for them. I think that's super confusing, but at least now I understand the rationale.Comment #260
effulgentsia CreditAttribution: effulgentsia at Acquia commentedDoes the deprecation error in #259 mean that if the source site has modules that are no longer wanted on the destination site, but that aren't listed in any of the destination site's
*.migrate_drupal.yml
files, that they will see that deprecation error (wherever deprecation errors are normally seen, such as in test runs)? So for example, if a D6 or D7 site is runningjquery_update
, then in order to not get that deprecation error there needs to be some*.migrate_drupal.yml
file on the destination site that says:jquery_update: SOME_ARBITRARY_DESTINATION_MODULE
? And that's true for every such module that's enabled on the source site?Comment #261
quietone CreditAttribution: quietone as a volunteer commentedIt is not to avoid the deprecation error, not strictly speaking. The purpose is for those modules to not appear in the will not be upgraded list. If those declarations are removed a whole bunch of modules will show up as will not be upgraded that do not need one.
Comment #262
effulgentsia CreditAttribution: effulgentsia at Acquia commentedJust leaving a quick message here to say that if there's another committer who understands this patch well enough to feel good about committing it before 8.7's RC, then go for it, I certainly don't want to stand in the way of that. I think it's unlikely I'll get to that point myself, but I'll keep looking at it tomorrow to see how far I can get. It's obvious that a lot of good work went into this; it's just some pretty tricky stuff.
Comment #263
quietone CreditAttribution: quietone as a volunteer commentedComplicated indeed!
Here is my attempt to move the migrate_drupal.yml file to migrations/state as discussed from #250 to #255.
Comment #265
mikelutzI'll review with @webchick tomorrow at DrupalCon sprints and hopefully @xjm if she's around for RM signoff. If I can get it to where they are happy, great, if not, then it would be fair to push it back to 8.8.
Thanks for the review, Alex, everything you've said makes good sense.
Comment #266
quietone CreditAttribution: quietone as a volunteer commentedDidn't update one of the tests to find the files in the new directory.
Comment #267
effulgentsia CreditAttribution: effulgentsia at Acquia commentedI do not experience that in my local testing. I installed a D7 site, with Overlay module enabled. I installed a D8 site with #266 applied and Migrate UI installed. On the D8 site, I went to
/upgrade
, and when I got to the Review page,overlay
was listed as expected in the "MODULES THAT WILL BE UPGRADED" section. I then editedsystem.migrate_drupal.yml
down to just:In other words, I removed all of the "# The following do not need an upgrade path." entries, including the one for
overlay
. When I refreshed the Review page, overlay was still listed in the "MODULES THAT WILL BE UPGRADED" section. The only difference was that when I stepped through MigrationState::buildUpgradeState() in the debugger, I started reaching the:line.
Other than triggering that deprecation error, nothing was functionally different by the presence or absence of all those entries in
system.migrate_drupal.yml
.Comment #268
effulgentsia CreditAttribution: effulgentsia at Acquia commentedI'm concerned that the deprecation error in #267 is happening too aggressively. If I have any module on my source site that I don't have migrations for on my destination site, I get that deprecation error. For example, I have Webform on my D7 site, but I don't even have Webform in my D8 site's codebase, and I get that deprecation error. Now because webform is not listed in
MigrationState::$noUpgradePaths
, it correctly shows up in the "MODULES THAT WILL NOT BE UPGRADED" list. That's great! But I don't think the additional deprecation error is helpful. I think it would be better to limit the deprecation error to only the case that a migration is discovered (e.g., I have webform or some other module that contains a webform migration set installed on my D8 site), but there isn't a corresponding entry for it in that module's*.migrate_drupal.yml
file.Comment #269
effulgentsia CreditAttribution: effulgentsia at Acquia commentedAnother concern I have with this patch is that it does not clarify that just because a module shows up in the "Modules that will be upgraded" list does not mean that it's a complete migration. For example, I have a D7 source site with Menu module installed. I have a D8 destination site without menu_link_content installed. Because I have system module installed, Menu shows up in the "Modules that will be upgraded" list, because
system.migrate_drupal.yml
identifies that themenu: system
migration is finished. But without menu_link_content being installed on my site, I don't really get a proper migration of D7's menus.I don't know if there's anything we can really do about that. Migrate UI can't know information about destination modules that aren't installed (and might not even be in the codebase at all). However, it leads to some weirdnesses. For example, suppose I was in a situation where source module A had two destination modules X and Y. Suppose X has an A -> X migration and declares is as finished, and Y does not yet have an A -> Y migration, but is working on one, so declares it as not finished. Now, if on my destination site I have X and Y installed, then A shows up in the "Modules that will not be upgraded" list, but if I uninstall Y, then A shows up in the "Modules that will be upgraded" list. Does that mean that if I want the A -> X migration to run, then I need to first uninstall Y in order to trick the system into putting A into the "Modules that will be upgraded" list?
Comment #270
effulgentsia CreditAttribution: effulgentsia at Acquia commentedThanks! I listed my concerns (from what I've reviewed so far, which isn't everything) in #257, #268, and #269, but I'm happy to be overruled on them if @webchick, @xjm, or another committer thinks it's okay to commit despite them.
Comment #271
quietone CreditAttribution: quietone as a volunteer commentedeffulgentsia, thanks for testing and reviews!
Re #267 Yes.This is essentially what webchick wanted. That, in the absence of an entry in migrate_drupal.yml the legacy code would be executed. For modules in the noUpgradePath this should be the same on the display whether they are listed in noUpgradePaths or the system.migrate_drupal.yml
#268I see your point but seems weird to execute legacy code but only show a message some of the time.
#269Yes, there are cases that are not easily solved. This relies on the quality and accuracy of the data in migrate_drupal.yml. How to solve the problem of an upgrade path for module A which has multiple destination modules X and Y. The simplest is that both X and Y have a migration entry like A: [X, Y] (because maintainer or A know the ecosystem and knows this). Your menu example is a good one and one that needs to be corrected. For this, adding a destination of 'menu_link_content' to the menu should fix that case because a check is done to determine if the declared destination modules are installed.
No, the UI is providing information about what it thinks will happen when you upgrade, it doesn't prevent migrations from running.
Comment #272
quietone CreditAttribution: quietone as a volunteer commentedOn further reflection and sleep:
RE #269 and the menu. Congratulations, you found what may the only migration where the configuration migration is in a different module, system instead of menu_link_content (but then that is content not configuration). If that should be moved is outside the scope of this issue, but worth investigating.
So, to resolve this for core the MigrationState service needs to access all the migrate_drupal.yml files whether the module is enable or not. I don't know how to do that but let's say it is doable, what happens for the menu case? The service already checks if the destination module is installed so it would show menu in the will not be upgraded section and list all the destination modules required. The user can then make a choice to enable all the destination modules or not. For users of the UI, they should enable them all, and try again. If they are a user with knowledge of migrate and they know that they want the menu configuration but not the migration from menu_link_content they can make an informed choice and upgrade without enabling menu_link_content.
But the menu example illustrates the larger problem that the MigrationState service can only report on the information is has. It can't know what exists in contrib modules not installed on the site. This is not communicated to the user and should be added to the page. Add text to clearly state that the information presented is based on the currently installed modules only and that core and/or contrib modules may need to be installed for their site to be completely upgraded. Whatever else is decided, this change would be the most helpful to the user.
Re #269
This has the consequence of making the ValidateMigrateStateTest fail because it checks that declared migrations are in the same module as discovered migrations. Need to think on that.
Comment #273
heddnProbably needs to be NW.
Comment #274
quietone CreditAttribution: quietone as a volunteer commentedThis patch is to address the point in #268 so that a deprecation message is shown only when a migration is found but the module does not have a state file.
Comment #276
quietone CreditAttribution: quietone as a volunteer commentedIn #257 a question was asked about the "The following do not need an upgrade path" section in system.migrate_drupal.yml and the corresponding noUpgradePath array. The noUpgradePath was there to support the legacy code but it really isn't needed since that information is in system.migrate_drupal.yml. I left it in as a convenience so the legacy code wasn't changed but it is better to remove it now. This patch does that and fixes the failing test.
The next thing to look at is menu migrations. See #269
Comment #278
quietone CreditAttribution: quietone as a volunteer commentedNow, time to address the quirks of any upgrade path that requires more than one destination module to be enabled, such as menu. To address that this adds all the needed destination modules to the declaration in system.migrate_drupal.yml. The declarations in the source module where the actual migration is has been left. They could be removed but just haven't thought it through if it is better to remove them or not. That will be next.
This patch modifies MigrationStateUnitTest to use a data provider, which makes testing more cases easier. Tests are added for menu and for i18n block. The fail patch has a test which uses a declaration of 'menu: system' in system.migrate_drupal.yml. The passing tests uses a declaration of 'menu: system, menu_link_content, menu_ui' and thus is able to report that menu will not be upgraded unless all three modules are enabled.
Fixed a typo in a $discoveredBySource and this still needs comments.
Comment #280
quietone CreditAttribution: quietone as a volunteer commentedAdd another test to MigrationStateUnitTest specifically checking the modules with no upgrade path, including i18n ones. Add some comments.
Assuming this passes tests I think that the points raised by effulgentsia are addressed, with the exception of what to do when in the situation where source module A had two destination modules X and Y. In core, we can deal with that by adding entries in system.migrate_drupal.yml. For contrib all we could do would be to add explanatory text at the top of the page but what to say for a situation that is unlikely and not confuse the user? "Hey, there is a small chance that a contribute module on your source site needs several modules installed so that data is available on your new site. This process has no way to determine that, please review all your source modules." I'm not intending that to be the text it is just a lot to convey without putting people off.
Ideally, and likely, is that a maintainer or the migrator of the legacy module will know what is required and add the need for both modules, X and Y, in A/migrations/state/A.migrate_drupal.yml.
Comment #281
quietone CreditAttribution: quietone as a volunteer commentedFixing whitespace error.
This is back to review now. The recent patches have been to address all the feedback from effulgentsia beginning in #243. That was to change the deprecation notice to show only when there are migrations and no state file. The removed the noUpgradePath array as it really isn't needed anymore and was causing confusion. Then improvements to the situation where there are several destination modules, particularly menu. This also effects the i18n source modules.
Comment #282
effulgentsia CreditAttribution: effulgentsia at Acquia commentedThanks, @quietone!
+1 to this! Because system actually contains a menu -> system migration, and is also aware that it does not migrate all of what's in menu, I think it makes perfect sense for it to take on the responsibility of identifying the other destination modules that are needed.
These, however, I think belong in
content_translation.migrate_drupal.yml
. System module doesn't contain any migrations for i18nblocks or i18nmenu, so I don't think it bears the responsibility of knowing what does. Whereas content_translation does contain those migrations and is aware of its migration dependencies on these other destination modules.Per the i18nblocks example above, it doesn't need to be in A, especially if there is no A module in the destination version (just like there is no i18nblocks module in D8), but I think in most cases there is a "primary" destination module that could take on that responsibility, just like "content_translation" is that primary module for the i18nblocks example.
I like your idea in #247 of moving these to
migrate_drupal.migrate_drupal.yml
. Could we do that in this patch please, rather than in a followup? And then in followups, move individual entries to their more appropriate homes if there are any? I feel strongly about this, because I think all modules, including System module, should be very careful about making statements about source modules that they haven't taken on functionality responsibility for. For example, system.module hasn't taken on any responsibility for the functionality that was in Overlay, or php.module, or most of the other things in that list. I understand that at some point perhaps there was a decision made for Migrate Drupal's UI to put Overlay, php.module, and the others, in the "will be upgraded" list rather than the "will not be upgraded" list, but I think it needs to be made clear that it's the Migrate Drupal module that's making that decision, and not System module that's making it. Perhaps in followups, some of those entries (for example, jquery_ui) will make sense to put back into System module, but I want to make sure that anything we put there is based on something that is true about what System module does.Comment #283
quietone CreditAttribution: quietone as a volunteer commented@effulgentsia, thanks for the prompt reply.
1. Glad we agree
I had written individual responses for 2, 3 and 4 and have removed them because I see them as instances of the same problem, that is, providing useful information to the user for a) modules that do not require migrations and b) when modules are not installed and migration state declarations are not available and meeting the intention that modules "should be very careful about making statements about source modules that they haven't taken on functionality responsibility for". I don't disagree and we (the migrate maintainers) did think carefully on the problem and favored solutions that gave the user helpful information.
However, for me next step is to go back to basic and reach an agreement on these questions. What should the UI display when it finds, for example, jquery_ui enabled on the source? What should it display when i18nblocks is enabled on the source when content_translation is not installed or when block and content_translation are not installed or when block, block_content and content_translation are not installed? And for contrib module A enabled on the source for all combinations of Module X and Module Y being installed.
Comment #284
effulgentsia CreditAttribution: effulgentsia at Acquia commentedThanks. Good questions in #283. I'll try to respond with my thoughts on that in the next day or two.
Meanwhile, I began updating the CR. Here's what I got to so far: https://www.drupal.org/node/2929443/revisions/view/11380551/11393892. If I got anything wrong there, please feel free to correct it.
I would also like to update some of the usage scenarios (after responding to #283), and will comment again here after I've done that.
Comment #285
quietone CreditAttribution: quietone as a volunteer commentedVery nice improvements to the CR, much easier for the reader to understand. What should be done at some point is change 'pair' to 'set'. I just don't have time for that now.
Regarding scenarios, this is an list that may be of use. It should be a compilation of the scenarios discussed between #18 and #21.
Scenario 1: Legacy module the same as D8 module
Example: Comment module
Section: Will be upgraded
Scenario 2: Legacy module provides a migrate field plugin, module name change is OK.
Example: D7 Adressfield to D8 Address
Section: Will be upgraded
Scenario 3: Legacy module needs migration but policy is to not make them.
Example: Views
Section: Will not be upgraded
Scenario 4A: Legacy core module doesn't require migrations no equivalent D8 module
Example: Overlay
Section: Will be upgraded
Scenario 4B: D8 Core module doesn't require migrations and no legacy equivalent
Example: Big Pipe
Section: None
Scenario 5A: Contrib that has D8 version that needs migrations but are not yet been developed
Example: TBD
Section: Will not be upgraded
Scenario 5B: Contrib that has D8 version but does not need any migrations
Example: TBD
Section:
Scenario 6: Module has multiple migration with multiple destination_modules defines
Examples:
D7 block -- D8 block + block_content
D7 locale -- D8 language + system
D7 menu -- D8 menu_link_content + menu_ui + system
D7 node -- D8 comment + content_translation + node
D7 system -- D8 file + system
D7 taxonomy -- D8 core + taxonomy
Section: Will be upgraded
Comment #286
effulgentsia CreditAttribution: effulgentsia at Acquia commentedSorry for taking so long to respond to #283, but here it is...
jQuery UI did move into core, and there is no data from D6's jquery_ui module that is missing from the migrations that we have in core, so that should be listed in the "will be upgraded" list. For this example,
jquery_ui: core
is an accurate migration set (even though it's a set with 0 migrations) and belongs in the "finished" list (because there's nothing left to finish for it). If we had acore/migrations/state/core.migrate_drupal.yml
file, that would be the most correct location for this migration set, because it'score.libraries.yml
that contains the library definition forjquery.ui
. Since we don't have that,system.migrate_drupal.yml
is also a reasonable location, becausesystem
is the module most closely associated with "core".I want to contrast this, however, with Overlay. Overlay did not move into core; it just got deleted. Furthermore, on a D7 site that's using Overlay, there is data associated with it for which there are no migrations in core (for example, which roles have access to the 'access overlay' permission). Therefore, in my opinion, Overlay should appear in the "will not be upgraded" list. However, in HEAD, it currently appears in the "will be upgraded" list and it is outside the scope of this issue to change that, and core product managers might disagree with me and prefer Overlay to remain in the "will be upgraded" list (in order to focus the "will not be upgraded" list on things that need real attention rather than trivialities like Overlay). For cases like this, I think it's fine to have a mechanism to put things into the list that isn't strictly the most accurate one. I think that mechanism should be either
migrate_drupal.migrate_drupal.yml
ormigrate_drupal_ui.migrate_drupal.yml
, because bothmigrate_drupal
andmigrate_drupal_ui
are enabled on the destination site (they have to be to see this UI, right?), and I think it's fine for these modules to have code (including entries in YAML files, such asoverlay: core
) that adjusts the UI into whatever is decided is best.In any one of these situations, I think
i18nblocks
should be listed in the "will not be upgraded" list, because you need all 3 destination modules for it to be fully upgraded. I think that having:within
content_translation.migrate_drupal.yml
and no entries for i18nblocks withinblock.migrate_drupal.yml
orblock_content.migrate_drupal.yml
achieves that, because then if you don't havecontent_translation
enabled, then regardless of whether or not you haveblock
orblock_content
enabled, there are no migration state entries found fori18nblocks
, which then puts it in the "will not be upgraded list". And if you do havecontent_translation
enabled, then the above entries ensure that i18nblocks can only move to the "will be upgraded list" if all 3 of those modules are enabled.This only works if
block
andblock_content
don't themselves provide some of the migrations for i18nblocks (independent of content_translation). If they did, then they would need to add entries for that in their own.migrate_drupal.yml
files. And that would then run the risk of i18nblocks getting incorrectly placed in the "will be upgraded" list, even if content_translation weren't enabled. That's a hypothetical that's basically the same as the contrib moodule A to destination modules X and Y example, so let's cover that below.I think there are two sub-scenarios of this to consider.
In one scenario, module X and module Y each migrate only some of the data from module A. For example, menu_ui and menu_link_content each migrate only some of the data from menu. In this case, the ideal UI for this would be for module A to be in the "will not be upgraded" list unless both X and Y are enabled. The key to achieving this is that modules X and Y know that they are only doing some of what module A did. The question is do they know which other modules do the rest? If they do, then they can represent that. For example, modules X and Y could each write:
in their
.migrate_drupal.yml
files. In this way, X is saying "I am finished the A -> X migrations", and also "I don't have any A -> Y migrations, but I am finished with those too, because I don't plan to ever have them, but Y is relevant to A". And similarly, Y is saying the converse of that. By X and Y each acknowledging that the other one exists (and is relevant to A), A gets correctly listed in the "will not be upgraded" list unless both X and Y are enabled.The above doesn't work for situations where X and Y don't know about each other. For example, while X and Y might each know that they only do a part of A, they might not know which modules out there do the rest. I don't think this is a situation that we have a good answer for. Hopefully it's pretty rare.
The other scenario is that modules X and Y are each complete replacements for A, but are alternate approaches. For example, this might not yet be the case, but suppose that at some point, D8 has both Panels and Layout Builder, and that each have complete migrations of D7 Panels. In this case, I think that Panels could have
panels: panels
in its.migrate_drupal.yml
file, and layout_builder could havepanels: layout_builder
in its.migrate_drupal.yml
file. They don't have to cross-reference each other, and this way, Panels will show up in the "will be upgraded" list if either of the destination modules are enabled.----
What do you think? Any disagreements with any of the above?
Comment #287
quietone CreditAttribution: quietone as a volunteer commented@effulgentsia, thanks for the detailed review.
I have no disagreements. Just one point, that if the choice of where to put the migration state declaration is between migrate_drupal.migrate_drupal.yml and migrate_drupal_ui.migrate_drupal.yml it should be migrate_drupal.migrate_drupal.yml since this is providing a service and could be used via drush.
This patch is just a reroll.
Comment #289
quietone CreditAttribution: quietone as a volunteer commentedThis should fix the test and move most of the contents os system.migrate_drupal.yml to migrate_drupal.migrate_drupal.yml based on #286.
Comment #291
quietone CreditAttribution: quietone as a volunteer commentedOn retesting the patch in #289 passed.
Comment #292
heddnsuper small nit:
s/system/migrate_drupal/
Comment #293
quietone CreditAttribution: quietone as a volunteer commentedI reworded the comment to address #292. But that really needs a followup to review the entries in migrate_drupal.migrate_drupal.yml and move them to a more appropriate module. Well, I suggest a followup because it is a refinement to the work done here to create a way for modules to declare their migration status. The followup is #3053167: Move state entries out of migrate_drupal.migrate_drupal.yml.
Comment #294
heddnI think this is ready again then. All feedback address once more.
Comment #296
Gábor HojtsyLooks like the fail is the result of #3024460: Migrate D7 comment type language settings and is legitimate.
Comment #297
quietone CreditAttribution: quietone as a volunteer commentedAdd the migration from #3024460: Migrate D7 comment type language settings to content_translation migration state file.
Comment #298
mikelutzSimple enough adjustment around the other patch getting in. Back to RTBC pending tests.
Comment #300
heddnComment #301
quietone CreditAttribution: quietone as a volunteer commentedSorry to do this but this needs to be postponed while #2979966: Migrate D7 i18n taxonomy term language gets looked at.
Comment #302
quietone CreditAttribution: quietone as a volunteer commentedI was being overly cautious. This can go back to RTBC
Comment #303
quietone CreditAttribution: quietone as a volunteer commentedRetested as well for good measure.
Comment #304
Gábor Hojtsy@webchick added the "Needs release manager review" tag with
I think if it keeps working as-is before, there is no specific reason for release management to mandate feedback. Also @larowlan and @effulgentsia have been deeply involved. I don't think @effulgentsia's concerns have all been validated to be resolved. I'll ask him to verify.
I added a release notes snippet and reorganized the existing issue summary text to conform to the drupal.org issue template.
Comment #305
catchJust to confirm I also think this is fine from a release management standpoint.
Comment #306
DamienMcKennaIs this still under consideration for backporting to 8.7.x?
Comment #308
effulgentsia CreditAttribution: effulgentsia at Acquia commentedI reviewed #297 and I think it looks great and ready for commit. I re-queued the tests on all database engines, and there's a single test failure on each of them (independent of the database engine).
Comment #309
mikelutzSimple enough adjustment, I think it can stay RTBC
Comment #310
heddn+1 on RTBC
Comment #312
Gábor HojtsyThanks all! I committed to 8.8. I believe this will truly improve the experience of people using migrate.
We discussed the potential for backport to 8.7 with @effulgentsia, @catch, @xjm and @webchick. It would be useful for contrib developers to be able to adopt the new YML files ahead of 8.8 also. On the other hand the ways of disruption (behaviour changes that may be problematic on 8.7, eg. form alters breaking, namespace collisions, etc) are not explained in the change notice or in the issue summary. It would be possible to make the decision based on that information.
Comment #313
mikelutzOf course, contrib is welcome to adopt the new yml files now and still be BC with 8.7. They will simply be ignored unless you are on 8.8
Comment #314
xjmThe change record for this is also really long (which makes it hard to evaluate the potential disruption) and seems more like API documentation. Does the patch contain the same docs as are in the CR?
Comment #315
quietone CreditAttribution: quietone as a volunteer commentedThe patch has similar documentation, in the MigrationState class.
Comment #316
quietone CreditAttribution: quietone as a volunteer commentedIt seems that what is left to do here is
from #312, Update the IS with regard to:
On the other hand the ways of disruption (behaviour changes that may be problematic on 8.7, eg. form alters breaking, namespace collisions, etc) are not explained in the change notice or in the issue summary. It would be possible to make the decision based on that information.
from #314
There is a concern the CR is long and like API documentation. Does that mean the CR needs to be changed?
Comment #317
Gábor Hojtsy@quietone: if there is a docs section where the CR can be moved to as documentation of this API in the first place (or if the code has that docs, then point to api.d.o?) that would help make the CR less heavy. I personally don't have a strong feeling either way. This close to Drupal 8.8 it is probably not that useful to backport anyway, compared to two months ago.
Comment #318
Gábor HojtsyComment #320
Gábor HojtsyTagging for release notes as per the release notes snippet in the summary. Also as a potential highlight since this improves auditability of the migrations.
Comment #321
pameeela CreditAttribution: pameeela commentedRemoved release notes tag as this isn't disruptive but should be included as a highlight.
Comment #322
Wim LeersFixed screenshot link in issue summary.