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.

  1. 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.
  2. 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.
  3. 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.

CommentFileSizeAuthor
#309 interdiff.2936365.297-309.txt925 bytesmikelutz
#309 2936365-309.drupal.Migrate-UI--allow-modules-to-declare-the-state-of-their-migrations.patch118.6 KBmikelutz
#297 2936365-297.patch118.59 KBquietone
#297 interdiff-293-297.txt678 bytesquietone
#293 2936365-293.patch118.56 KBquietone
#293 interdiff-289-293.txt744 bytesquietone
#289 2936365-289.patch118.54 KBquietone
#289 interdiff-287-289.txt6.45 KBquietone
#287 2936365-287.patch118.48 KBquietone
#281 2936365-281.patch118.48 KBquietone
#281 interdiff-280-281.txt469 bytesquietone
#280 interdiff-278-280.txt5.9 KBquietone
#280 2936365-280.patch118.48 KBquietone
#278 interdiff-176-278.txt21.75 KBquietone
#278 2936365-278.patch116.59 KBquietone
#278 2936365-278-fail.patch116.49 KBquietone
#276 2936365-276.patch109.38 KBquietone
#276 interdiff-274-276.txt10.84 KBquietone
#274 interdiff-265-274.txt4.35 KBquietone
#274 2936365-274.patch108.92 KBquietone
#266 2936365-265.patch107.61 KBquietone
#266 interdiff-263-265.txt974 bytesquietone
#263 2936365-263.patch107.52 KBquietone
#263 interdiff-245-263.txt15.99 KBquietone
#246 2936365-245.patch105.12 KBheddn
#246 interdiff_241-245.txt462 bytesheddn
#241 2936365-240.patch104.67 KBheddn
#240 interdiff_233-240.txt366 bytesheddn
#233 2936365-233.patch104.81 KBquietone
#233 interdiff-230-233.txt868 bytesquietone
#230 interdiff-223-230.txt1.64 KBquietone
#230 2936365-230.patch104.74 KBquietone
#223 Screen Shot 2019-04-06 at 10.07.43 am.png50.07 KBlarowlan
#223 2936365-module-migrate-ui-222.patch104.63 KBlarowlan
#223 2936365-module-migrate-ui-222-interdiff.txt47.59 KBlarowlan
#216 interdiff.2936365.213-216.txt1.26 KBmikelutz
#216 2936365-216.drupal.Migrate-UI--allow-modules-to-declare-their-migration-status.patch95.95 KBmikelutz
#213 2936365-213.patch95.95 KBquietone
#213 interdiff-202-213.txt11.86 KBquietone
#204 WithPatch.png39.12 KBquietone
#204 WithoutPatch.png51.29 KBquietone
#202 2936365-202.patch95.97 KBquietone
#202 interdiff-200-202.txt6.11 KBquietone
#200 interdiff-198-200.txt10.16 KBquietone
#200 2936365-200.patch94.06 KBquietone
#198 2936365-198.patch91.53 KBquietone
#198 interdiff-197-198.txt1 KBquietone
#197 Selection_038.png25.92 KBquietone
#197 2936365-197.patch91.63 KBquietone
#197 interdiff-195-187.txt1.79 KBquietone
#195 interdiff-193-195.txt50.75 KBquietone
#195 2936365-195.patch91.56 KBquietone
#193 interdiff-191-193.txt20.17 KBquietone
#193 2936365-193.patch80.78 KBquietone
#191 interdiff.txt6.19 KBquietone
#191 2936365-191.patch73.72 KBquietone
#188 BeforeWillNotBeUpgrade.png45.88 KBquietone
#188 BeforeWillBeUpgraded.png23.07 KBquietone
#185 2936365-185.patch71.66 KBquietone
#185 interdiff-183-185.txt1.72 KBquietone
#183 interdiff.2936365.180-183.txt3.8 KBmikelutz
#183 2936365-183.drupal.Migrate-UI--allow-modules-to-declare-their-migration-status.patch71.45 KBmikelutz
#180 interdiff.2936365.178-180.txt2.94 KBmikelutz
#180 2936365-180.drupal.Migrate-UI--allow-modules-to-declare-their-migration-status.patch71.57 KBmikelutz
#178 interdiff.2936365.174-178.txt4.5 KBmikelutz
#178 2936365-178.drupal.Migrate-UI--allow-modules-to-declare-their-migration-status.patch71.62 KBmikelutz
#174 Selection_023.png9.82 KBquietone
#174 Selection_022.png30.04 KBquietone
#174 interdiff-171-174.txt5.56 KBquietone
#174 2936365-174.patch70.77 KBquietone
#171 2936365-170.drupal.Migrate-UI--allow-modules-to-declare-their-migration-status.patch70.44 KBmikelutz
#171 interdiff.2936365.163-170.txt2.05 KBmikelutz
#169 interdiff.2936365.163-169.txt2.05 KBmikelutz
#169 2936365-169.drupal.Migrate-UI--allow-modules-to-declare-their-migration-status.patch71.69 KBmikelutz
#163 2936365-163.patch70.59 KBquietone
#163 interdiff-159-163.txt2.76 KBquietone
#159 2936365-159.patch72.08 KBquietone
#158 2936365-158.patch72.21 KBquietone
#158 interdiff-157-158.txt10.08 KBquietone
#157 interdiff-155-157.txt1.48 KBquietone
#157 2936365-157.patch72.56 KBquietone
#155 interdiff-153-155.txt38.44 KBquietone
#155 2936365-155.patch72.56 KBquietone
#153 2936365-153.patch72.26 KBquietone
#153 interdiff-151-153.txt1.12 KBquietone
#151 2936365-151.patch72.56 KBquietone
#151 interdiff-149-151.txt722 bytesquietone
#149 interdiff-145-147.txt3.32 KBquietone
#149 2936365-149.patch72.22 KBquietone
#147 interdiff-145-147.txt2.01 KBquietone
#147 2936365-147.patch72.22 KBquietone
#145 interdiff-143-145.txt36.55 KBquietone
#145 2936365-145.patch71.99 KBquietone
#143 interdiff-140-143.txt3.47 KBquietone
#143 2936365-143.patch62.74 KBquietone
#140 interdiff-138-140.txt18.7 KBquietone
#140 2936365-140.patch62.81 KBquietone
#138 2936365-138.patch63.47 KBquietone
#138 interdiff-136-138.txt5.24 KBquietone
#136 interdiff-133-136.txt5.48 KBquietone
#136 2936365-136.patch63.54 KBquietone
#133 2936365-133.patch61.75 KBquietone
#133 interdiff-131-333.txt5.19 KBquietone
#131 2936365-131.patch61.71 KBquietone
#131 interdiff-129-131.txt526 bytesquietone
#129 interdiff-126-129.txt32.44 KBquietone
#129 2936365-129.patch61.73 KBquietone
#128 interdiff.txt1.13 KBquietone
#128 2936365-128.patch63.29 KBquietone
#126 2936365-126.patch63.29 KBquietone
#122 interdiff-118-122.txt781 bytesquietone
#122 2936365-122.patch62.47 KBquietone
#118 2936365-118.patch62.69 KBquietone
#118 interdiff-116-118.txt6.19 KBquietone
#116 interdiff-114-116.txt8.69 KBquietone
#116 2936365-116.patch58.46 KBquietone
#114 interdiff-112-114.txt6.1 KBquietone
#114 2936365-114.patch65.89 KBquietone
#112 interdiff-110-112.txt12 KBquietone
#112 2936365-112.patch58.26 KBquietone
#110 2936365-110.patch51.01 KBquietone
#110 interdiff-105-110.txt1.56 KBquietone
#105 interdiff-102-104.txt1.29 KBquietone
#105 2936365-104.patch51.17 KBquietone
#102 interdiff-93-102.txt5.78 KBquietone
#102 2936365-102.patch51.2 KBquietone
#96 Before-FullWindow.png53.67 KBquietone
#96 Before-ModulesthatwillNotbeUpgraded.png26.62 KBquietone
#96 Before-FullWindow.png53.67 KBquietone
#93 interdiff-91-93.txt2.26 KBquietone
#93 2936365-93.patch47.62 KBquietone
#91 2936365-91.patch47.35 KBquietone
#84 interdiff.txt577 bytesquietone
#84 2936365-84.patch47.41 KBquietone
#82 interdiff.txt3.39 KBquietone
#82 2936365-82.patch47.42 KBquietone
#80 interdiff-77-80.txt12.07 KBquietone
#80 2936365-80.patch46.66 KBquietone
#78 Screenshot from 2018-10-08 22-14-53.png34.87 KBquietone
#78 Screenshot from 2018-10-08 22-15-28.png23.74 KBquietone
#77 2936365-77.patch50.4 KBquietone
#75 2936365-75.patch83.6 KBquietone
#65 Screenshot from 2018-09-16 20-09-06.png39.87 KBquietone
#65 Screenshot from 2018-09-16 20-08-39.png55.97 KBquietone
#64 interdiff-57-64.txt59.01 KBquietone
#64 2936365-64.patch83.77 KBquietone
#57 interdiff-55-57.txt1.22 KBquietone
#57 2936365-57.patch78.93 KBquietone
#55 interdiff-52-55.txt7.3 KBquietone
#55 2936365-55.patch78.89 KBquietone
#52 interdiff-50-52.txt1.7 KBquietone
#52 2936365-52.patch78.61 KBquietone
#50 interdiff.txt3.87 KBquietone
#50 2936365-50.patch78.94 KBquietone
#45 interdiff.txt1 KBquietone
#45 2936365-45.patch79.04 KBquietone
#43 interdiff.txt45.77 KBquietone
#43 2936365-43.patch78.72 KBquietone
#40 interdiff-38-40.txt2.8 KBquietone
#40 2936365-40.patch47.76 KBquietone
#38 interdiff-36-38.txt2.92 KBquietone
#38 2936365-38.patch43.79 KBquietone
#36 interdiff.txt31.33 KBquietone
#36 2936365-36.patch43.53 KBquietone
#35 2936365-35.patch36.05 KBquietone
#28 2936365-28.png32.6 KBmasipila
#21 2928147-21.patch36.04 KBquietone
#15 interdiff.txt1.11 KBquietone
#15 2936365-15.patch37.7 KBquietone
#13 interdiff.txt375 bytesquietone
#13 2936365-13.patch37.71 KBquietone
#11 2936365-11.patch37.9 KBquietone
#9 Screenshot from 2018-03-11 17-54-14.png10.58 KBquietone
#2 form_alter_patch.txt4.37 KBquietone
#2 form_alter_patch.txt4.37 KBquietone
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

quietone created an issue. See original summary.

quietone’s picture

Status: Active » Postponed
FileSize
4.37 KB
4.37 KB

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

quietone’s picture

Upload the patch twice, hiding one.

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

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

quietone’s picture

Issue tags: +Migrate UI

Add tag.

quietone’s picture

Status: Postponed » Needs review

Time to get this sorted.

heddn’s picture

Status: Needs review » Needs work

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

  • Status value
  • A description run through admin xxs. It can include a description paragraph, links, etc.

The status value would be from an allowed values list. i.e. upgradable, not upgradable, incomplete migration path

phenaproxima’s picture

Issue tags: +Needs documentation

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

quietone’s picture

Status: Needs work » Needs review
FileSize
10.58 KB

Is this a suitable entry in the info.yml for a module?

migrate:
  status: incomplete
  description: See <a href="https://www.drupal.org/project/drupal/issues/2409435">Upgrade path for Book 6.x and 7.x</a>.

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?

quietone’s picture

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

quietone’s picture

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

migrate:
  6:
    status:
    description: 

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.

Status: Needs review » Needs work

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

quietone’s picture

Status: Needs work » Needs review
FileSize
37.71 KB
375 bytes

Extra character crept into options.info.yml.

Status: Needs review » Needs work

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

quietone’s picture

Status: Needs work » Needs review
FileSize
37.7 KB
1.11 KB

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

migrate:
  -
    version: 6
    status: 
    description:
quietone’s picture

Issue summary: View changes

Update IS. For anyone reviewing you might want to read from #9 onwards.

quietone’s picture

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

masipila’s picture

Hi!

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

masipila’s picture

Status: Needs review » Needs work

Hi!

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?

  • I do.
  • If we think about our core migrations alone, there are several which have known issues.
  • So allowing the details for also 'complete' path means that the module can say that 'the upgrade path is as complete as it gets but it is not a bad idea to check this and that because x y z'.

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.

  • So how does D8 Address module declare the status that it provides the migration for D7 Addressfield?

Kicking back to NW to discuss these further.

Cheers,
Markus

quietone’s picture

Assigned: Unassigned » quietone

Assigning to myself

quietone’s picture

Status: Needs work » Needs review
FileSize
36.04 KB

Examples for the scenarios in #18

Scenario 1: Comment module
- Upgrade path exists, module name is the same

migrate:
  6:
    status: complete

Scenario 2: D7 Adressfield to D8 Address
- D7 Addressfield can be migrated to D8 Address

migrate:
  6:
    status: complete

Scenario 3: Views
- No updrade in core
- Core would link to the Known Issues doc page

migrate:
  6:
    status: incomplete
    description: some useful string

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

migrate:
  6:
    status: n/a or incomplete

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

masipila’s picture

Assigned: quietone » masipila

Assigning for myself for review.

masipila’s picture

Assigned: masipila » Unassigned
Issue summary: View changes
Status: Needs review » Needs work

Remaining 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

masipila’s picture

I can update the change record this evening.

masipila’s picture

Status: Needs work » Needs review

Change 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

quietone’s picture

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

masipila’s picture

Hi!

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

masipila’s picture

FileSize
32.6 KB

Replying 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.
Proposed UI

Cheers,
Markus

masipila’s picture

Issue summary: View changes

I 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

lomasr’s picture

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

masipila’s picture

@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

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

heddn’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll
masipila’s picture

Priority: Normal » Major

Bumping to Major. Let's try to finally land this so that contribs can declare their status.

quietone’s picture

Status: Needs work » Needs review
FileSize
36.05 KB

Reroll.

quietone’s picture

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

Status: Needs review » Needs work

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

quietone’s picture

Status: Needs work » Needs review
FileSize
43.79 KB
2.92 KB

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

Status: Needs review » Needs work

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

quietone’s picture

Status: Needs work » Needs review
FileSize
47.76 KB
2.8 KB

Well, my team lost but maybe I can get this patch to pass tests.

maxocub’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/config_translation/config_translation.info.yml
    @@ -7,3 +7,12 @@ core: 8.x
    +    description: See <a = 'https://www.drupal.org/docs/8/upgrade/known-issues-when-upgrading-from-drupal-6-or-7-to-drupal-8'>Details</a>
    ...
    +    description: See <a = 'https://www.drupal.org/docs/8/upgrade/known-issues-when-upgrading-from-drupal-6-or-7-to-drupal-8'>Details</a>
    
    +++ b/core/modules/content_translation/content_translation.info.yml
    @@ -7,3 +7,12 @@ package: Multilingual
    +    description: See <a = 'https://www.drupal.org/docs/8/upgrade/known-issues-when-upgrading-from-drupal-6-or-7-to-drupal-8'>Details</a>
    ...
    +    description: See <a = 'https://www.drupal.org/docs/8/upgrade/known-issues-when-upgrading-from-drupal-6-or-7-to-drupal-8'>Details</a>
    
    +++ b/core/modules/rdf/rdf.info.yml
    @@ -4,3 +4,11 @@ description: 'Enriches your content with metadata to let other applications (e.g
    +    description: See <a = 'https://www.drupal.org/docs/8/upgrade/known-issues-when-upgrading-from-drupal-6-or-7-to-drupal-8'>Details</a>
    

    There's some missing href attributes here.

  2. +++ b/core/modules/migrate_drupal_ui/src/Form/ReviewForm.php
    @@ -200,6 +198,21 @@ public function buildForm(array $form, FormStateInterface $form_state) {
    +          if (isset($migrate_info['version'])) {
    +            if ($migrate_info['version'] == $version) {
    

    This could be a single if statement.

  3. +++ b/core/modules/migrate_drupal_ui/src/Form/ReviewForm.php
    @@ -242,6 +255,18 @@ public function buildForm(array $form, FormStateInterface $form_state) {
    +      if (isset($migrate_info['status'])) {
    +        if (strtolower($migrate_info['status']) === 'n/a') {
    

    Ditto.

quietone’s picture

Assigned: Unassigned » quietone

I'll do this this weekend.

quietone’s picture

Status: Needs work » Needs review
FileSize
78.72 KB
45.77 KB

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

Status: Needs review » Needs work

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

quietone’s picture

Status: Needs work » Needs review
FileSize
79.04 KB
1 KB

Correct the call parameters.

quietone’s picture

Issue summary: View changes

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

quietone’s picture

Assigned: quietone » Unassigned

Unassigning for all the reviewers to comment!

maxocub’s picture

Status: Needs review » Needs work

Great work with those tests @quietone!

  1. +++ b/core/modules/color/color.info.yml
    @@ -4,3 +4,11 @@ description: 'Allows administrators to change the color scheme of compatible the
    +
    
    +++ b/core/modules/config/config.info.yml
    @@ -5,3 +5,11 @@ package: Core
    +
    
    +++ b/core/modules/contact/contact.info.yml
    @@ -5,3 +5,11 @@ package: Core
    +
    
    +++ b/core/modules/contextual/contextual.info.yml
    @@ -4,3 +4,11 @@ description: 'Provides contextual links to perform actions related to elements o
    +
    
    +++ b/core/modules/datetime_range/datetime_range.info.yml
    @@ -6,3 +6,11 @@ version: VERSION
    +
    
    +++ b/core/modules/dynamic_page_cache/dynamic_page_cache.info.yml
    @@ -4,3 +4,11 @@ description: 'Caches pages for any user, handling dynamic content correctly.'
    +
    
    +++ b/core/modules/file/file.info.yml
    @@ -6,3 +6,11 @@ version: VERSION
    +
    
    +++ b/core/modules/migrate_drupal_ui/tests/modules/migration_status_incomplete_test/migration_status_incomplete_test.info.yml
    @@ -0,0 +1,12 @@
    +
    

    Super nit, but there's a few extra lines.

  2. +++ b/core/modules/migrate_drupal_ui/src/Form/ReviewForm.php
    @@ -271,14 +302,25 @@ public function buildForm(array $form, FormStateInterface $form_state) {
    +      // Get the migration status for this  information for $source_module.
    +      //this is looking at the destination for a module named the same as the source
    

    This comment needs work.

I also have a few remarks/questions.

  1. I see you use the 'incomplete' status for config_translation and content_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?
  2. The RDF upgrade path is RTBC #2500509: Upgrade path for RDF 7.x, if it gets in first, we'll have to update its status here.
  3. The Search module is mark as complete but we still have an issue for it #2587063: Variable to config: search_active_modules [d7]. I'll investigate what's left to do there.

Back to NW for the nits above.

quietone’s picture

@maxocub, thanks! Getting the tests correct was tedious.

quietone’s picture

Status: Needs work » Needs review
FileSize
78.94 KB
3.87 KB

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

maxocub’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll

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

quietone’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
78.61 KB
1.7 KB
quietone’s picture

Issue tags: -Needs reroll

Remove needs reroll tag.

maxocub’s picture

Status: Needs review » Needs work

Sorry for not finding those in my earlier reviews, but this is such a big patch.

  1. +++ b/core/modules/migrate_drupal_ui/src/Form/ReviewForm.php
    @@ -307,33 +350,55 @@ public function buildForm(array $form, FormStateInterface $form_state) {
    +          $class =
    +            [
    +              'upgrade-analysis-report__status-icon',
    +              'upgrade-analysis-report__status-icon--warning',
    +            ];
    ...
    +          $class =
    +            [
                   'upgrade-analysis-report__status-icon',
                   'upgrade-analysis-report__status-icon--checked',
    ...
    +            ];
    

    We usually put the opening brackets on the same line as the variable.

  2. +++ b/core/modules/migrate_drupal_ui/src/Form/ReviewForm.php
    @@ -307,33 +350,55 @@ public function buildForm(array $form, FormStateInterface $form_state) {
    +            '#attributes' => [
    +              'class' => $class
    +              ],
    

    Closing bracket too much indented.

  3. +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/MigrateUpgradeTestBase.php
    @@ -104,25 +104,47 @@ protected function tearDown() {
    +    // Test the available migration paths with a warning status..
    

    Extra period.

  4. +++ b/core/modules/views/views.info.yml
    @@ -6,3 +6,12 @@ version: VERSION
    +migrate:
    +  -
    +    version: 6
    +    status: incomplete
    +    description: See <a href="https://www.drupal.org/docs/8/upgrade/known-issues-when-upgrading-from-drupal-6-or-7-to-drupal-8">Known issues when upgrading from Drupal 6 or 7 to Drupal 8</a>
    +  -
    +    version: 7
    +    status: incomplete
    +    description: See <a href="https://www.drupal.org/docs/8/upgrade/known-issues-when-upgrading-from-drupal-6-or-7-to-drupal-8">Known issues when upgrading from Drupal 6 or 7 to Drupal 8</a>
    

    Is the status of the Views migration should really be 'incomplete'? Or should it be with the missing upgrade paths?

  5. If it's not already done, we should open a follow-up for the documentation and remove the 'Needs documentation' tag.
quietone’s picture

Status: Needs work » Needs review
FileSize
78.89 KB
7.3 KB

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

Status: Needs review » Needs work

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

quietone’s picture

Status: Needs work » Needs review
FileSize
78.93 KB
1.22 KB

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

masipila’s picture

Issue summary: View changes

We 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

  • it was still talking about the 'missing upgrade paths' and 'available upgrade paths' even though we now have 'modules that will be upgraded' and 'modules that will not be upgraded' lists
  • we now have the list of modules that will not need an upgrade, see ReviewForm::$noUpgradePaths

.

Now, let's try to wrap up this whole thing once more. Let's start by looking at the UI mockup from comment #28:
UI mockup

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:

  • Those that have a migration (e.g. Comment). This determination is based on declarations in the plugin class annotations.
  • Those that don't need an upgrade / migration i.e. which are listed in the ReviewForm::$noUpgradePaths list.

1B. Modules that will not be upgraded.

  • This list contains all other modules of the D6/D7 source site than those included in the 'Modules that will be upgraded' section.

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

  • This will appear in the 'Will be upgraded' list with status 'complete' and green symbol.
  • The appearance on the 'will be upgraded' list is based on the migration plugin annotations
  • The green 'complete' symbol is based on the definition in the D8 module's .info.yml

Scenario 2: D7 Adressfield to D8 Address

  • This will appear in the 'Will be upgraded' list with status 'complete' and green symbol.
  • The appearance on the 'will be upgraded' list is based on the 'source_module' and 'destination_module' annotations
  • The green 'complete' symbol is based on the definition in the D8 module's .info.yml

Scenario 4: Overlay

  • This will appear in the 'Will be upgraded' list despite the fact that nothing is actually migrated
  • This is because Overlay is listed in the ReviewForm::$noUpgradePaths list.

Scenario 5A: random contrib that has D8 version but no automatic migrations because the migration has not (yet) been developed

  • If the module maintainer does nothing in D8 .info.yml, the module will be appearing in the 'Will not be upgraded' list because there is no migration plugin that would have the 'source_module' annotation (and the module is not whitelisted in the ReviewForm::$noUpgradePaths list.
  • While the migration is being developed, the module maintainer could add a link to the module's issue that is used for building the migration but the status symbol should still be red and the module should appear in the 'will not be upgraded' list.
  • We need to have a status code in the .info.yml file that can be used for this scenario

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

  • If the module maintainer does nothing in D8 .info.yml, the module will be appearing in the 'Will not be upgraded' list because there is no migration plugin that would have the 'source_module' annotation (and the module is not whitelisted in the ReviewForm::$noUpgradePaths list.
  • Now, the whole point of this issue that we are working on is to provide a mechanism that the contrib module maintainer could move the module to the 'OK' list i.e. 'Modules that will be upgraded' list.
  • We need to have a status code in the .info.yml file that can be used for this scenario
  • This means that we have two kinds of 'n/a', one for scenario 5A and another for 5B. One will keep the module in the 'will not be upgraded' list and another will move it to the 'will be upgraded' list.

Scenario 3: Views

  • This core module belongs to the 'Will not be upgraded' list and red symbol and a link to the Known Issue page
  • The correct status in the D8 views.info.yml can be determined after we know how we are going to handle 5A and 5B above.

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.

quietone’s picture

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

masipila’s picture

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

  • Scenario 5A: random contrib that has D8 version but no automatic migrations because the migration has not (yet) been developed
  • Scenario 3: Views
  • (Drupal 8 module with the same name can provide additional info in .info.yml with a link to additional info.)

Criteria for module to be included on this list:

  • Anything else than the modules included in sections B and C below.

B. Modules that will be upgraded (green symbol if status is 'complete' and yellow symbol if symbol is 'incomplete')

  • Scenario 1: Comment module
  • Scenario 2: D7 Adressfield to D8 Address
  • (Drupal 8 module can provide additional info in .info.yml with a link to additional info.)

Criteria for module to be included on this list:

  • Migration is discovered for this module

C. Modules that will be upgraded without a need for data migration (green symbol for each row)

  • Scenario 4: Overlay
  • 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

Criteria for module to be included on this list:

  • Module is included in the ReviewForm::$noUpgradePaths
  • Or D8 module with the same name has status 'n/a' in .info.yml

3. Modules with multiple destination_modules for one source_module

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.

D7 core migration had the following 1:N scenarios the last time I checked in #28.

  • 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

I suggest we use D7 node as a concrete example for this scenario. How would D7 node behave here?

quietone’s picture

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

heddn’s picture

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

maxocub’s picture

Status: Needs review » Needs work

Based on comments #58-#62, this needs work.

quietone’s picture

Status: Needs work » Needs review
FileSize
83.77 KB
59.01 KB

First, 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

quietone’s picture

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

Status: Needs review » Needs work

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

maxocub’s picture

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

quietone’s picture

Thanks 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

heddn’s picture

won't fix - ideally would have migrations but policy decision not to do so (views), RED

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

maxocub’s picture

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

I like it.

quietone’s picture

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

Migration templates Finished partially completed not started
Yes GREEN YELLOW unknown - YELLOW
No GREEN unknown - RED RED

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

maxocub’s picture

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

migrations_finished:
  6:
    <d6_module_name>: <d8_module_name>
  7:
    <d7_module_name>: <d8_module_name>

Here's the scenarios from #71:

Scenario 1:
comment

migrations_finished:
  6:
    comment: comment
  7:
    comment: comment

Scenario 2:
D7 addressfield to D8 address

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

migrations_finished:
  6:
    pirate: pirate
  7:
    pirate: pirate

or just: (?)

migrations_finished: true

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

heddn’s picture

For #6, I wonder if the following could be added into the various commerce_migrate_* modules info.yml files:

migrations_finished:
  7:
    commerce_payment: commerce_payment
    commerce_payment_ui: commerce_payment
quietone’s picture

Always 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?

quietone’s picture

Status: Needs work » Needs review
FileSize
83.6 KB

Rerolling.

Status: Needs review » Needs work

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

quietone’s picture

Status: Needs work » Needs review
FileSize
50.4 KB

This 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

migrations_finished:
 block: block

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.

quietone’s picture

Forgot to upload the screenshots

Status: Needs review » Needs work

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

quietone’s picture

Status: Needs work » Needs review
FileSize
46.66 KB
12.07 KB

Hopefully, this will improve the tests.

Status: Needs review » Needs work

The last submitted patch, 80: 2936365-80.patch, failed testing. View results

quietone’s picture

Status: Needs work » Needs review
FileSize
47.42 KB
3.39 KB

Another try. This still needs work.

Status: Needs review » Needs work

The last submitted patch, 82: 2936365-82.patch, failed testing. View results

quietone’s picture

Status: Needs work » Needs review
FileSize
47.41 KB
577 bytes

Remove duplicate entry in the available paths.

quietone’s picture

Issue summary: View changes

A bit of an IS update.

quietone’s picture

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

heddn’s picture

Issue tags: +Needs UX review

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

heddn’s picture

Status: Needs review » Needs work
heddn’s picture

Issue tags: +Needs usability review
quietone’s picture

Issue summary: View changes
Issue tags: +Migrate critical

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

quietone’s picture

Status: Needs work » Needs review
FileSize
47.35 KB

Needs a reroll.

Status: Needs review » Needs work

The last submitted patch, 91: 2936365-91.patch, failed testing. View results

quietone’s picture

Status: Needs work » Needs review
FileSize
47.62 KB
2.26 KB

Now, fix the tests.

quietone’s picture

Issue summary: View changes
quietone’s picture

Issue summary: View changes

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

quietone’s picture

Adding before and after screenshots etc.

quietone’s picture

Issue summary: View changes
quietone’s picture

Issue summary: View changes

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

Gábor Hojtsy’s picture

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

But the screens should provide some information or direction to the user on what to do about the modules in the 'will not be upgraded' section. After all, if a module they need is listed here they need a next step right on this page. How best to inform them of what to do? A single help text somewhere on the page or something else?

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?

quietone’s picture

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

quietone’s picture

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

quietone’s picture

Starting 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

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 Review the pre-upgrade analysis in the Upgrading to Drupal 8 handbook.'

The other changes here are comments and adding i18nprofile to the info yml in config_translation.

quietone’s picture

Status: Needs review » Needs work

The last submitted patch, 102: 2936365-102.patch, failed testing. View results

quietone’s picture

Status: Needs work » Needs review
FileSize
51.17 KB
1.29 KB

Those errors make sense since I didn't update those tests for the changes to i18nprofile

quietone’s picture

Issue summary: View changes

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

Gábor Hojtsy’s picture

Status: Needs review » Needs work

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

Gábor Hojtsy’s picture

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

jhodgdon’s picture

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

quietone’s picture

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

quietone’s picture

Issue summary: View changes

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

  6:
    locale: language, system
    taxonomy: language
    system: language

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.

quietone’s picture

Status: Needs work » Needs review
FileSize
58.26 KB
12 KB

Adding a test to check the declarations in .info.yml to the discovered migrations. The test is really rough. This will fail ReviewPage tests.

Status: Needs review » Needs work

The last submitted patch, 112: 2936365-112.patch, failed testing. View results

quietone’s picture

Status: Needs work » Needs review
FileSize
65.89 KB
6.1 KB

Working to fix the tests.

Status: Needs review » Needs work

The last submitted patch, 114: 2936365-114.patch, failed testing. View results

quietone’s picture

Status: Needs work » Needs review
FileSize
58.46 KB
8.69 KB

This should fix the tests and also remove a test file I left in the previous patch.

quietone’s picture

Status: Needs review » Needs work

Back to NW as this should have tests to prove that the status declared in the info is working as expected.

quietone’s picture

Status: Needs work » Needs review
FileSize
6.19 KB
62.69 KB

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

Status: Needs review » Needs work

The last submitted patch, 118: 2936365-118.patch, failed testing. View results

quietone’s picture

This is a failure I've not seen before

00:02:37 Build timed out (after 110 minutes). Marking the build as aborted.
00:02:37 Build was aborted
quietone’s picture

Issue summary: View changes
Issue tags: -Needs UX review +Needs framework manager review

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

quietone’s picture

Status: Needs work » Needs review
FileSize
62.47 KB
781 bytes

The test module names in the test were still the old ones.

heddn’s picture

Assigned: Unassigned » heddn

Assigning to myself for review this week.

larowlan’s picture

Assigned: heddn » Unassigned
Issue tags: -Needs framework manager review +needs profiling

This is looking good

  1. +++ b/core/modules/migrate_drupal_ui/src/Form/ReviewForm.php
    @@ -230,23 +302,35 @@ public function buildForm(array $form, FormStateInterface $form_state) {
    +    // modules and there migration status. The destination is not used yet but
    

    nit: their

  2. +++ b/core/modules/migrate_drupal_ui/src/Form/ReviewForm.php
    @@ -262,7 +347,7 @@ public function buildForm(array $form, FormStateInterface $form_state) {
    +      '#description' => $this->t('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']),
    

    Let's use "" here instead of \'

  3. +++ b/core/modules/migrate_drupal_ui/src/Form/ReviewForm.php
    @@ -402,4 +487,101 @@ public function getConfirmText() {
    +              $migration_status = static::CHECKED;
    +            }
    +            else {
    

    personal preference, but you could continue here and drop the else to reduce complexity

  4. +++ b/core/modules/migrate_drupal_ui/src/Form/ReviewForm.php
    @@ -402,4 +487,101 @@ public function getConfirmText() {
    +                  $migration_status = static::ERROR;
    

    same here, you could continue in both the if and elseif and avoid using elseif, which is a known code smell.

  5. +++ b/core/modules/options/options.info.yml
    @@ -7,3 +7,9 @@ core: 8.x
    \ No newline at end of file
    

    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.

mikelutz’s picture

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

quietone’s picture

Needs a rerolll.

Status: Needs review » Needs work

The last submitted patch, 126: 2936365-126.patch, failed testing. View results

quietone’s picture

Status: Needs work » Needs review
FileSize
63.29 KB
1.13 KB

Move i18nsync in the tests.

quietone’s picture

Now move the migrate status information from info.yml to module.migrate.yml. The patch include fixes for all the items in #124.

Status: Needs review » Needs work

The last submitted patch, 129: 2936365-129.patch, failed testing. View results

quietone’s picture

Status: Needs work » Needs review
FileSize
526 bytes
61.71 KB

Forgot to remove i18nsync from the missingpaths list.

quietone’s picture

Issue summary: View changes
quietone’s picture

Fix some comments in ReviewForm and update the CR.

This is ready for ready.

heddn’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs documentation, -Needs usability review, -needs profiling

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

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Needs work

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

quietone’s picture

Status: Needs work » Needs review
FileSize
63.54 KB
5.48 KB

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

heddn’s picture

Status: Needs review » Needs work

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

  1. +++ b/core/modules/migrate_drupal_ui/src/Form/ReviewForm.php
    @@ -15,28 +15,57 @@
    + * The migration status displayed on the Review page is list of all the enabled
    

    Nit: "is a list of all the..."

    We're missing the word "a".

  2. +++ b/core/modules/migrate_drupal_ui/src/Form/ReviewForm.php
    @@ -15,28 +15,57 @@
    + * 6 Menu Module is upgraded by having migrations in three Drupal 8 modules;
    ...
    + * Module should be listed in the will be upgraded category. If any one of the
    

    Nit: the word "module" is not a proper noun and could be lower case.

  3. +++ b/core/modules/migrate_drupal_ui/src/Form/ReviewForm.php
    @@ -15,28 +15,57 @@
    + * menu_link_content, menu_ui and system. And it only when all those migrations
    

    And it "is" only when

  4. +++ b/core/modules/migrate_drupal_ui/src/Form/ReviewForm.php
    @@ -15,28 +15,57 @@
    + * pair. A path marked 'migrations_Not_finished' is used when the migrations
    

    lowercase "not"

  5. +++ b/core/modules/migrate_drupal_ui/src/Form/ReviewForm.php
    @@ -15,28 +15,57 @@
    + * when more that one migration is needed they are incomplete you do not need to
    

    This could use some wordsmithing. Not exactly sure what we were trying to communicate.

  6. +++ b/core/modules/migrate_drupal_ui/src/Form/ReviewForm.php
    @@ -15,28 +15,57 @@
    + * - migrations_not_finished: (optional) The upgrade path that is, or will be,
    + * provided by this module for these source_module/destination_module pairs
    + * do exist.
    

    Are the pair of source/destination only optional if one of these do not exist?

  7. +++ b/core/modules/migrate_drupal_ui/src/Form/ReviewForm.php
    @@ -15,28 +15,57 @@
    + *   - source_module: A list of destination module
    

    This seems wrong. Should this be source module?

  8. +++ b/core/modules/migrate_drupal_ui/src/Form/ReviewForm.php
    @@ -49,13 +78,14 @@
    + * but either no migrations exist or some do and there are more migrations to
    + * create.
    

    ...and more migrations need to be created to finish the upgrade path.

  9. +++ b/core/modules/migrate_drupal_ui/src/Form/ReviewForm.php
    @@ -231,23 +333,35 @@ public function buildForm(array $form, FormStateInterface $form_state) {
    +    // Get the migrate status information from the .migrate.yml file and build
    +    // an array of source modules and their migration status. The destination is
    +    // not used yet but can be later for validating the source/destination pairs
    +    // with the actual source/destination pairs in the migrate plugins.
    +    $system_info = $this->getMigrateInfo();
    +
    +    $this->destinationSystemMigrateInfo = [];
    +    $properties = ['migrations_finished', 'migrations_not_finished'];
    

    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.

quietone’s picture

Status: Needs work » Needs review
FileSize
5.24 KB
63.47 KB

@heddn, thanks.

Only time to do the doc fixes, #137.1 -> 8.

Status: Needs review » Needs work

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

quietone’s picture

Remove migrations_ from migration_finished and migrations_not_finished.

quietone’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 140: 2936365-140.patch, failed testing. View results

quietone’s picture

Status: Needs work » Needs review
FileSize
62.74 KB
3.47 KB

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

Status: Needs review » Needs work

The last submitted patch, 143: 2936365-143.patch, failed testing. View results

quietone’s picture

Status: Needs work » Needs review
FileSize
71.99 KB
36.55 KB

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

Status: Needs review » Needs work

The last submitted patch, 145: 2936365-145.patch, failed testing. View results

quietone’s picture

Status: Needs work » Needs review
FileSize
72.22 KB
2.01 KB

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

Status: Needs review » Needs work

The last submitted patch, 147: 2936365-147.patch, failed testing. View results

quietone’s picture

Status: Needs work » Needs review
FileSize
72.22 KB
3.32 KB

Another attempt to fix the tests and rename some variables.

Status: Needs review » Needs work

The last submitted patch, 149: 2936365-149.patch, failed testing. View results

quietone’s picture

Status: Needs work » Needs review
FileSize
722 bytes
72.56 KB

Change the profile to will be upgraded for D7.

quietone’s picture

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

quietone’s picture

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

heddn’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/file/file.migrate.yml
    --- /dev/null
    +++ b/core/modules/filter/filter.migrate.yml
    

    You'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

  2. +++ b/core/modules/migrate_drupal_ui/src/Form/ReviewForm.php
    --- /dev/null
    +++ b/core/modules/migrate_drupal_ui/src/MigrationState.php
    

    Let's put this service in migrate_drupal. That way drush can use it without needing an UI.

  3. +++ b/core/modules/migrate_drupal_ui/src/MigrationState.php
    @@ -0,0 +1,427 @@
    + * be provided by, their migrations by their .migrate.yml file. If a module will
    + * not be providing migrations then do not create a .migrate.yml file.
    ...
    + * Note that a module with migrations must provide at least one of either
    + * 'finished' or 'not_finished'.
    

    This seems at odds.

  4. +++ b/core/modules/migrate_drupal_ui/src/MigrationState.php
    @@ -0,0 +1,427 @@
    +   *   An associative array of data keyed by the display categories, 'error'
    +   *   and 'checked'.
    

    Why did we decide on error and checked? What does each contain?

  5. +++ b/core/modules/migrate_drupal_ui/src/MigrationState.php
    @@ -0,0 +1,427 @@
    +  public function getUpgradeCategory($version, array $source_system_data, array $migrations) {
    ...
    +  public function getMigrationStates($version) {
    

    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?

  6. +++ b/core/modules/migrate_drupal_ui/src/MigrationState.php
    @@ -0,0 +1,427 @@
    +    /** @var \Drupal\migrate_drupal_ui\MigrationState $migrate_status */
    +    $migration_state = \Drupal::service('migrate_drupal_ui.migration_state');
    

    This isn't needed, just use $this

quietone’s picture

Status: Needs work » Needs review
FileSize
72.56 KB
38.44 KB

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

Status: Needs review » Needs work

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

quietone’s picture

Status: Needs work » Needs review
FileSize
72.56 KB
1.48 KB

Fixing errors when changing from 'status' to 'state'.

quietone’s picture

Just tidying up the test and adding comments.

quietone’s picture

Reroll, just changes to migrate_drupal.services.yml.

quietone’s picture

Ready for review again!

heddn’s picture

Status: Needs review » Needs work

One small question and a couple small nits.

  1. +++ b/core/modules/language/migrations/d6_language_negotiation_settings.yml
    @@ -7,7 +7,7 @@ source:
    +  source_module: locale
    
    +++ b/core/modules/language/migrations/d6_language_types.yml
    @@ -7,7 +7,7 @@ source:
    +  source_module: locale
    
    +++ b/core/modules/language/migrations/d7_language_types.yml
    @@ -13,7 +13,7 @@ source:
    +  source_module: locale
    

    Are these needed here?

  2. +++ b/core/modules/migrate_drupal/src/MigrationState.php
    @@ -0,0 +1,421 @@
    + * migrations for the same* pair. A path marked 'not_finished' is used when all
    

    Stray asterisk here.

  3. +++ b/core/modules/migrate_drupal/tests/src/Kernel/ValidateMigrationStateTest.php
    @@ -0,0 +1,152 @@
    +    // Sort and make all the array unique.
    

    Nit: maybe drop the word 'all'.

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

quietone’s picture

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

1. No. That was me testing something and should have been removed awhile bak.
2. fixed
3. fixes

quietone’s picture

Issue summary: View changes

Update IS and CR to new names> Added the new service to the IS.

heddn’s picture

Status: Needs review » Reviewed & tested by the community

No more concerns. Onward.

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Needs work

Only found these two minor things now. Looks good to me otherwise.

  1. +++ b/core/modules/migrate_drupal/src/MigrationState.php
    @@ -0,0 +1,421 @@
    + * Retrieves migrate info from .migrate_drupal.yml files.
    ...
    + * will provide, by adding them to their .migrate_drupal.yml file.
    ...
    +   * Gets migration state information from *.migrate_drupal.yml.
    

    *.migrate_drupal.yml would read better IMHO in the first two cases as in the last case.

  2. +++ b/core/modules/migrate_drupal/src/MigrationState.php
    @@ -0,0 +1,421 @@
    + *   - source_module: A list of destination module
    

    modules.

mikelutz’s picture

Assigned: Unassigned » mikelutz
mikelutz’s picture

mikelutz’s picture

Assigned: mikelutz » Unassigned
Status: Needs work » Needs review
FileSize
71.69 KB
2.05 KB

Made some documentation adjustments.

mikelutz’s picture

mikelutz’s picture

heddn’s picture

Status: Needs review » Reviewed & tested by the community

If this comes back green, all feedback is addressed.

quietone’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs screenshots

Talked to webchick on Slack about this

quietone:
They are great questions - keeps it grounded. I am 99% sure that this errs on the side that all source modules need a migration so assume 'will not be upgraded' unless proven otherwise. That is safer than assuming a module will be upgraded. There is a list of core modules (overlay) that do not need an upgrade path but that method can't be done for contrib and it would be silly for a module to have to declare that it does not need a migration (just busy work for the module maintainer). So, maybe it does need to assume that the source module does not need an upgrade unless declared in migrate_drupal.yml.
webchick [8:36 AM]
I actually agree that an assumption of “not upgrading” is better.
And all overlay has to do is declare 7: overlay in its “finished” category, yes?
(like it’s a copy/paste/modify job to get rid of the error vs. piles of work for the maintainer?)
But there are BC concerns then I guess in that suddenly huge swaths of contrib are going to be declared as “not migratable” even though they are? Hm.

The initial work on the meaning of finished/not_finished, from comment #72, has this scenario.

Scenario 5a:
Pirate (Contrib that has D8 version but does not need any migrations)

migrations_finished:
  6:
    pirate: pirate
  7:
    pirate: pirate

or just: (?)

migrations_finished: true

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

quietone’s picture

Issue summary: View changes
Status: Needs work » Needs review
Issue tags: -Needs screenshots
FileSize
70.77 KB
5.56 KB
30.04 KB
9.82 KB

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

heddn’s picture

Status: Needs review » Needs work

Some real simple docs fixes, nothing more.

  1. +++ b/core/modules/migrate_drupal/src/MigrationState.php
    @@ -16,32 +16,33 @@
    + * or Drupal 7 module to declare the state of the upgrade paths for Drupal 6
    

    or Drupal 7 module is to declare...

  2. +++ b/core/modules/migrate_drupal/src/MigrationState.php
    @@ -16,32 +16,33 @@
    + * and/or Drupal 7. The upgrade path is either 'finished' or 'not_finished'. A
    + * A marked 'finished' is used for source module/destination module pairs that
    

    "A" is at end of previous line and on new line.

  3. +++ b/core/modules/migrate_drupal/src/MigrationState.php
    @@ -16,32 +16,33 @@
    + *   - source module: A comma separated list of destination modules.
    ...
    + *   - source_module: A comma separated list of destination modules.
    

    Let's be explicit and say it is the machine name of these two things.

mikelutz’s picture

mikelutz’s picture

Assigned: Unassigned » mikelutz
mikelutz’s picture

Assigned: mikelutz » Unassigned
Status: Needs work » Needs review
FileSize
71.62 KB
4.5 KB

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

heddn’s picture

Status: Needs review » Needs work

So close.

  1. +++ b/core/modules/migrate_drupal/src/MigrationState.php
    @@ -0,0 +1,451 @@
    + * current major version destination modules it is providing migrations for. We see this in
    

    Nit: 80 chars.

  2. +++ b/core/modules/migrate_drupal/src/MigrationState.php
    @@ -0,0 +1,451 @@
    + *   <legacy_version>:
    ...
    + *   <another_legacy_version>:
    ...
    + *   <legacy_version>:
    

    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.

mikelutz’s picture

heddn’s picture

Status: Needs review » Reviewed & tested by the community

Looking good again.

quietone’s picture

Status: Reviewed & tested by the community » Needs review

Ah, more proof that I am not a skilled writer. Thanks, the improvements read better.

Unfortunately there is one item that needs a fix.

+++ b/core/modules/migrate_drupal/src/MigrationState.php
@@ -0,0 +1,451 @@
+ * This path can be marked 'finished', which indicates that all migrations that
+ * are going to be provided by this destination module for this upgrade path
+ * have been written and are complete. The path may also be marked

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.

mikelutz’s picture

quietone’s picture

Yes, '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.

  1. +++ b/core/modules/migrate_drupal/src/MigrationState.php
    @@ -21,45 +21,42 @@
    + * consist of one or more migrations sets. Each migration set definition
    

    I think 'definition' can me removed and the meaning is retained.

  2. +++ b/core/modules/migrate_drupal/src/MigrationState.php
    @@ -21,45 +21,42 @@
    + * This migration set can be marked 'finished', which indicates that all
    

    s/This/A/

  3. +++ b/core/modules/migrate_drupal/src/MigrationState.php
    @@ -21,45 +21,42 @@
    + * migration set have been written and are complete. The migration set may also
    

    s/The migration/A migration/

  4. +++ b/core/modules/migrate_drupal/src/MigrationState.php
    @@ -21,45 +21,42 @@
    + * migrations to complete the set. (Note: Other modules may still provide
    + * additional finished or not_finished migrations for the same migration set.)
    

    Are the parenthesis needed?

  5. +++ b/core/modules/migrate_drupal/src/MigrationState.php
    @@ -21,45 +21,42 @@
    + * #  migrations set that this module provides and are complete.
    

    s/migrations set/migration sets/

quietone’s picture

1. Not fixed. When reading again I though it was fine.
2-5 Fixed.

Updated the CR to add the 'set' terminology.

mikelutz’s picture

LGTM. I think this is ready to go again.

heddn’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me.

quietone’s picture

Issue summary: View changes
FileSize
23.07 KB
45.88 KB

New before screenshots because webchick pointed out that one of them was wrong, so I remade both of them.

quietone’s picture

Issue summary: View changes

remove update CR from the IS.

That just leaves the documentation and commit.

webchick’s picture

Status: Reviewed & tested by the community » Needs review

Discussed 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":

+++ b/core/modules/action/action.migrate_drupal.yml
@@ -0,0 +1,7 @@
+finished:
+  6:
+    system: action
+    action: action
+  7:
+    system: action
+    action: action

...and instead, we'd only see hunks like this, where there's some weird outlier:

+++ b/core/modules/config_translation/config_translation.migrate_drupal.yml
@@ -0,0 +1,22 @@
+finished:
+  6:
+    i18nprofile: config_translation
+  7:
+    i18n_variable: config_translation
+not_finished:
+  6:
+    # language content comment settings.
+    locale: language
+    i18n: config_translation
+    # field labels and descriptions, synchronized fields.
+    i18ncck: config_translation
+    #
+    i18ntaxonomy: config_translation
+  7:
+    # language content comment settings.
+    locale: language
+    i18n: config_translation
+    # field labels and descriptions, field options.
+    i18n_field: config_translation
+    # localized. vocabulary language settings, taxonomy term language.
+    i18n_taxonomy: config_translation

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?

+++ b/core/modules/user/user.migrate_drupal.yml
@@ -0,0 +1,10 @@
+finished:
+  6:
+    profile: user
+    user: user
+  7:
+    profile: user
+    user: user
+not_finished:
+  7:
+    profile: user

How is profile: user both finished and not finished? :)

quietone’s picture

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

Status: Needs review » Needs work

The last submitted patch, 191: 2936365-191.patch, failed testing. View results

quietone’s picture

Status: Needs work » Needs review
FileSize
80.78 KB
20.17 KB

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

Status: Needs review » Needs work

The last submitted patch, 193: 2936365-193.patch, failed testing. View results

quietone’s picture

Status: Needs work » Needs review
FileSize
91.56 KB
50.75 KB

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

Status: Needs review » Needs work

The last submitted patch, 195: 2936365-195.patch, failed testing. View results

quietone’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
1.79 KB
91.63 KB
25.92 KB

Now 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

quietone’s picture

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

Status: Needs review » Needs work

The last submitted patch, 198: 2936365-198.patch, failed testing. View results

quietone’s picture

Status: Needs work » Needs review
FileSize
94.06 KB
10.16 KB

Thought it would be better to use the old method as a whole than the reduced version that was there.

Status: Needs review » Needs work

The last submitted patch, 200: 2936365-200.patch, failed testing. View results

quietone’s picture

Status: Needs work » Needs review
FileSize
6.11 KB
95.97 KB

Make some corrections to the source_module definition for language migrations. Add @legacy to tests.

quietone’s picture

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

quietone’s picture

Issue summary: View changes
FileSize
51.29 KB
39.12 KB

Add new before/after screenshots of the will not be upgraded.

quietone’s picture

I think i18n content should not be here and that needs looking into.

I did look into that and it is fine. I simply forgot that content_translation was enabled.

This is Ready for review.

heddn’s picture

Status: Needs review » Reviewed & tested by the community

Wow, a lot to digest in there. But it all looks good. The only thing I could find is the need for a period.

+++ b/core/modules/migrate_drupal/src/MigrationState.php
@@ -0,0 +1,605 @@
+ * are included in the analysis modules that are uninstalled are ignored.

Needs a period to break up 2 sentences. This should be:

are included in the analysis. Modules that are uninstalled are ignored.

That can be fixed on commit. Onward.

heddn’s picture

Is this still a candidate for 8.7 backport?

webchick’s picture

Priority: Major » Critical

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

webchick’s picture

However, I can't make that call, so sending to release managers for review.

larowlan’s picture

Status: Reviewed & tested by the community » Needs work

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

  1. +++ b/core/modules/language/language.migrate_drupal.yml
    @@ -0,0 +1,7 @@
    +    locale: language, system
    ...
    +    locale: language, system
    

    any reason we're using CSV syntax here instead of native yaml features - ie arrays?

  2. +++ b/core/modules/migrate_drupal/src/MigrationConfigurationTrait.php
    @@ -210,4 +210,22 @@ protected function getLegacyDrupalVersion(Connection $connection) {
    +    /** @var \Drupal\migrate_drupal\MigrationState $migration_state */
    +    $migration_state = \Drupal::service('migrate_drupal.migration_state');
    ...
    +    /** @var \Drupal\migrate_drupal\MigrationState $migration_state */
    +    $migration_state = \Drupal::service('migrate_drupal.migration_state');
    

    nit: not sure we need the local variable here, but don't feel strongly about it

  3. +++ b/core/modules/migrate_drupal/src/MigrationState.php
    @@ -0,0 +1,605 @@
    + * see this in core where the Drupal 6 Menu module is upgraded by having
    ...
    + * And it is only when all those migrations are complete, in all three modules,
    + * and menu is enabled on the source, and three destination modules are enabled
    

    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/uninstalled

  4. +++ b/core/modules/migrate_drupal/src/MigrationState.php
    @@ -0,0 +1,605 @@
    +  public function __construct() {
    +    $this->fieldPluginManager = \Drupal::service('plugin.manager.migrate.field');
    +    $this->moduleHandler = \Drupal::service('module_handler');
    +  }
    

    any reason we don't use `arguments` in the service definition here? Feels like we could be doing dependency injection here.

  5. +++ b/core/modules/migrate_drupal/src/MigrationState.php
    @@ -0,0 +1,605 @@
    +    $module_handler = \Drupal::service('module_handler');
    

    we have a module handler available as $this->moduleHandler, so should use that?

  6. +++ b/core/modules/migrate_drupal/src/MigrationState.php
    @@ -0,0 +1,605 @@
    +    list($declared_by_source, $destination_modules_declared) = $this->getDeclaredBySource($version);
    

    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

  7. +++ b/core/modules/migrate_drupal/src/MigrationState.php
    @@ -0,0 +1,605 @@
    +        $return = $this->getStateLegacyMethod($migrations, $source_module, $version, $source_system_data, $discovered_upgrade_paths);
    ...
    +          list($migration_state, $destination_modules) = $return;
    

    same here :(

  8. +++ b/core/modules/migrate_drupal/src/MigrationState.php
    @@ -0,0 +1,605 @@
    +          $state = $migration_state;
    +          $upgrade_state[$state][$source_module] = implode(', ', $destination_modules);
    

    do we need the local $state variable here?

  9. +++ b/core/modules/migrate_drupal/src/MigrationState.php
    @@ -0,0 +1,605 @@
    +            if (!in_array('not_finished', $declared_by_source[$source_module]) && in_array('finished', $declared_by_source[$source_module])) {
    

    we have constants for finished/not_finished we should use them instead of the strings

  10. +++ b/core/modules/migrate_drupal/src/MigrationState.php
    @@ -0,0 +1,605 @@
    +    $states = ['finished', 'not_finished'];
    

    same here re constants

  11. +++ b/core/modules/migrate_drupal/src/MigrationState.php
    @@ -0,0 +1,605 @@
    +                $tmp = array_map('trim', explode(',', $destination));
    

    yeah, if we used YAML arrays instead of CSV it would be cleaner and we'd avoid this

  12. +++ b/core/modules/migrate_drupal/src/MigrationState.php
    @@ -0,0 +1,605 @@
    +    @trigger_error(sprintf("The legacy module '%s', is not listed in any migrate_drupal.yml file, see https://www.drupal.org/node/2929443", $module), E_USER_DEPRECATED);
    

    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.

  13. +++ b/core/modules/migrate_drupal_ui/src/Form/ReviewForm.php
    @@ -5,24 +5,46 @@
    +  const FINISHED = 'finished';
    ...
    +  const NOT_FINISHED = 'not_finished';
    

    I don't think we need to define these in two places, let's put them on an interface and use that everywhere

larowlan’s picture

Assigned: Unassigned » larowlan

I have 45 mins spare to address those items I flagged

larowlan’s picture

will finish in morning

quietone’s picture

Status: Needs work » Needs review
FileSize
11.86 KB
95.95 KB

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

quietone’s picture

CR updated using yml syntax for multiple destination modules.

Status: Needs review » Needs work

The last submitted patch, 213: 2936365-213.patch, failed testing. View results

mikelutz’s picture

heddn’s picture

Status: Needs review » Needs work
+++ b/core/modules/migrate_drupal_ui/src/Form/ReviewForm.php
@@ -27,22 +28,7 @@
+class ReviewForm extends MigrateUpgradeFormBase implements MigrationStateInterface {

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

mikelutz’s picture

Assigned: larowlan » mikelutz

I agree on 6 and 7, but I'm writing up a case for it and doing some minor refactoring right now.

xjm’s picture

Issue tags: +Needs release note
mikelutz’s picture

Assigned: mikelutz » larowlan

I'm sorry, @larowlan, I didn't mean to steal this from you. I've added a few additional comments.

  1. +++ b/core/modules/language/language.migrate_drupal.yml
    @@ -0,0 +1,7 @@
    +    locale: [language, system]
    ...
    +    locale: [language, system]
    

    nit: These should probably go on separate lines

  2. +++ b/core/modules/migrate_drupal/src/MigrationState.php
    @@ -0,0 +1,589 @@
    +  public function getUpgradeStates($version, array $source_system_data, array $migrations) {
    ...
    +  public function getMigrationStates() {
    

    Let's add these two public functions to the interface.

  3. +++ b/core/modules/migrate_drupal/src/MigrationState.php
    @@ -0,0 +1,589 @@
    +    list($declared_by_source, $destination_modules_declared) = $this->getDeclaredBySource($version);
    ...
    +        $return = $this->getStateLegacyMethod($migrations, $source_module, $version, $source_system_data, $discovered_upgrade_paths);
    +        if ($return) {
    +          list($migration_state, $destination_modules) = $return;
    

    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.

  4. +++ b/core/modules/migrate_drupal/src/MigrationState.php
    @@ -0,0 +1,589 @@
    +    @trigger_error(sprintf("The legacy module '%s', is not listed in any migrate_drupal.yml file, see https://www.drupal.org/node/2929443", $module), E_USER_DEPRECATED);
    

    We should use the new format for deprecation errors here.

  5. +++ b/core/modules/migrate_drupal/tests/src/Kernel/ValidateMigrationStateTest.php
    @@ -0,0 +1,173 @@
    +    $versions = ['6', '7'];
    

    [FieldDiscoveryInterface::DRUPAL_6, FieldDiscoveryInterface::DRUPAL_7]

quietone’s picture

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

  nid:
    -
      plugin: migration_lookup
      migration: [d6_node, d7_node]
      source: nid
3. Very much agree.
4. Oops, thought I did that.

And my own nit,
+++ b/core/modules/migrate_drupal/src/MigrationState.php
@@ -0,0 +1,589 @@
+                if (!is_array($destination)) {
+                  $destination = [trim($destination)];
+                } else {
+                  $destination = array_map('trim', $destination);
+                }

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.

mikelutz’s picture

Ah, 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

if (!is_array($destination)) {
  $destination = [$destination];
}
larowlan’s picture

Assigned: larowlan » Unassigned
Issue summary: View changes
Status: Needs work » Needs review
FileSize
47.59 KB
104.63 KB
50.07 KB

Addressed the issues I identified and the subsequent comments

+++ b/core/modules/migrate_drupal/src/MigrationState.php
@@ -0,0 +1,589 @@
+    if (array_key_exists($module, $this->migratedSourceModules)) {

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:

  • Got rid of the double processing of migrations/field migrations
  • Reduced the public api (only usages were in tests)
  • Got rid of the trait changes and injected the service into ReviewForm (less api changes)
  • A few other cleanups
quietone’s picture

Assigned: Unassigned » larowlan
Issue summary: View changes
Status: Needs review » Needs work

Updated CR and IS with the proper yml syntax for the migrate_drupal.yml file.

quietone’s picture

Assigned: larowlan » Unassigned
Status: Needs work » Needs review

That's odd I didn't make the change to status and assigned.

heddn’s picture

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

  1. +++ b/core/modules/migrate_drupal/tests/src/Kernel/StateFileExists.php
    @@ -0,0 +1,103 @@
    +   * Modules that do no have a migrate_drupal.yml file.
    ...
    +  protected $stateFileRequired = [
    

    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.

  2. +++ b/core/modules/migrate_drupal/tests/src/Kernel/StateFileExists.php
    @@ -0,0 +1,103 @@
    +    $this->assert(count($this->stateFileRequired, count($has_state_file)));
    

    Shouldn't this be assertEquals?

  3. +++ b/core/modules/migrate_drupal/tests/src/Kernel/ValidateMigrationStateTest.php
    @@ -0,0 +1,170 @@
    +  protected function enableAllModules() {
    

    I think there's a trait for this?

  4. +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/MultilingualReviewPageTestBase.php
    --- a/core/modules/migrate_drupal_ui/tests/src/Functional/d6/MultilingualReviewPageTest.php
    +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/d6/MultilingualReviewPageTest.php
    
    +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/d6/MultilingualReviewPageTest.php
    @@ -11,6 +11,8 @@
    + * @group legacy
    
    --- a/core/modules/migrate_drupal_ui/tests/src/Functional/d6/NoMultilingualReviewPageTest.php
    +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/d6/NoMultilingualReviewPageTest.php
    
    +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/d6/NoMultilingualReviewPageTest.php
    @@ -12,6 +12,8 @@
    + * @group legacy
    
    +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/d6/NoMultilingualTest.php
    --- a/core/modules/migrate_drupal_ui/tests/src/Functional/d6/Upgrade6Test.php
    +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/d6/Upgrade6Test.php
    
    +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/d6/Upgrade6Test.php
    @@ -12,6 +12,8 @@
    + * @group legacy
    
    --- a/core/modules/migrate_drupal_ui/tests/src/Functional/d7/MultilingualReviewPageTest.php
    +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/d7/MultilingualReviewPageTest.php
    
    +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/d7/MultilingualReviewPageTest.php
    @@ -11,6 +11,8 @@
    + * @group legacy
    

    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.

masipila’s picture

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

masipila’s picture

Maybe 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

quietone’s picture

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

Thanks masipila. The IS has been updated.

quietone’s picture

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

quietone’s picture

Issue summary: View changes

More tweaks to the IS, as masipila suggested and I missed.

Status: Needs review » Needs work

The last submitted patch, 230: 2936365-230.patch, failed testing. View results

quietone’s picture

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

quietone’s picture

Title: Migrate UI - allow modules to declare their migration status » Migrate UI - allow modules to declare the state of their migrations

Improve the accuracy of the title.

heddn’s picture

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

quietone’s picture

heddn’s picture

Status: Needs review » Reviewed & tested by the community

Looking good.

effulgentsia’s picture

Hi, I'm trying to understand this patch, but I'm stumped by:

+++ b/core/modules/action/action.migrate_drupal.yml
@@ -0,0 +1,7 @@
+finished:
+  6:
+    system: action
+    action: action
+  7:
+    system: action
+    action: action

Is this saying that there's an action.module for Drupal 6 and Drupal 7? But is there?

heddn’s picture

Status: Reviewed & tested by the community » Needs work

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

heddn’s picture

heddn’s picture

effulgentsia’s picture

+++ b/core/modules/system/system.migrate_drupal.yml
@@ -0,0 +1,92 @@
+finished:
+  6:
+    # The following do not need an upgrade path.
+    blog: core
...
+    views_ui: core
+  7:
+    # The following do not need an upgrade path.
+    blog: core
...
+    views_ui: core

What'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?

Status: Needs review » Needs work

The last submitted patch, 241: 2936365-240.patch, failed testing. View results

heddn’s picture

Status: Needs work » Needs review

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

heddn’s picture

FileSize
462 bytes
105.12 KB
quietone’s picture

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

effulgentsia’s picture

I'd much rather push moving these out to more logical locations in a follow-up.

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

heddn’s picture

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

effulgentsia’s picture

Also, 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 the migrations directory. In a way, one could think of it as metadata about what's in there, kind of like the way that the config/schema directory provides metadata about the config directory. Therefore, what about moving those files into migrations/status/MODULE_NAME.status.yml? Perhaps it would then even make sense to split them up into four files:

  • migrations/status/MODULE_NAME.d6.finished.yml
  • migrations/status/MODULE_NAME.d6.unfinished.yml
  • migrations/status/MODULE_NAME.d7.finished.yml
  • migrations/status/MODULE_NAME.d7.unfinished.yml

?

effulgentsia’s picture

And to boot, I would argue that the information is true.

How 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 a poll -> 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?

heddn’s picture

I 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?

heddn’s picture

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

effulgentsia’s picture

In the patch, we have MigrationState::$noUpgradePaths. Given that we have that, are the entries in system.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.)

quietone’s picture

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

The mappings won't make anything worse.

Agree.

quietone’s picture

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

effulgentsia’s picture

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.

Right. So then, would there be any behavior difference at all between calendarsignup: core or overlay: core being in system.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 in node/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.

effulgentsia’s picture

+1 to just moving the YML files into migrations/state without splitting them up or renaming them.

effulgentsia’s picture

would there be any behavior difference at all between calendarsignup: core or overlay: core being in system.migrate_drupal.yml or not?

I guess the difference is triggering a deprecation error, per:

+++ b/core/modules/migrate_drupal/src/MigrationState.php
@@ -0,0 +1,600 @@
+        // If there is not a declared state for this source module then use the
+        // legacy method for determining the migration state.
+        if (!isset($this->stateBySource[$version][$source_module])) {
+          @trigger_error(sprintf("Using migration plugin definitions to determine the migration state of the module '%s' is deprecated in Drupal 8.7. Add the module to a migrate_drupal.yml file. See https://www.drupal.org/node/2929443", $source_module), E_USER_DEPRECATED);

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 use system.migrate_drupal.yml as a dumping ground for them. I think that's super confusing, but at least now I understand the rationale.

effulgentsia’s picture

Does 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 running jquery_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?

quietone’s picture

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

effulgentsia’s picture

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

quietone’s picture

Complicated indeed!

Here is my attempt to move the migrate_drupal.yml file to migrations/state as discussed from #250 to #255.

Status: Needs review » Needs work

The last submitted patch, 263: 2936365-263.patch, failed testing. View results

mikelutz’s picture

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

quietone’s picture

Status: Needs work » Needs review
FileSize
974 bytes
107.61 KB

Didn't update one of the tests to find the files in the new directory.

effulgentsia’s picture

If those declarations are removed a whole bunch of modules will show up as will not be upgraded that do not need one.

I 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 edited system.migrate_drupal.yml down to just:

finished:
  6:
    menu: system
    system: system
  7:
    menu: system
    system: system

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:

@trigger_error(sprintf("Using migration plugin definitions to determine the migration state of the module '%s' is deprecated in Drupal 8.7. Add the module to a migrate_drupal.yml file. See https://www.drupal.org/node/2929443", $source_module), E_USER_DEPRECATED);

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.

effulgentsia’s picture

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

effulgentsia’s picture

Another 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 the menu: 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?

effulgentsia’s picture

I'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! 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.

quietone’s picture

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

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?

No, the UI is providing information about what it thinks will happen when you upgrade, it doesn't prevent migrations from running.

quietone’s picture

On 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

For this, adding a destination of 'menu_link_content' to the menu should fix

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.

heddn’s picture

Status: Needs review » Needs work

Probably needs to be NW.

quietone’s picture

Status: Needs work » Needs review
FileSize
108.92 KB
4.35 KB

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

Status: Needs review » Needs work

The last submitted patch, 274: 2936365-274.patch, failed testing. View results

quietone’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
10.84 KB
109.38 KB

In #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

Status: Needs review » Needs work

The last submitted patch, 276: 2936365-276.patch, failed testing. View results

quietone’s picture

Status: Needs work » Needs review
FileSize
116.49 KB
116.59 KB
21.75 KB

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

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

quietone’s picture

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

quietone’s picture

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

effulgentsia’s picture

Thanks, @quietone!

  1.     b/core/modules/system/migrations/state/system.migrate_drupal.yml
    @@ -0,0  1,112 @@
         menu:
           - system
           - menu_link_content
           - menu_ui
    
  2. +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.

  3.     b/core/modules/system/migrations/state/system.migrate_drupal.yml
    @@ -0,0  1,112 @@
         i18nblocks:
           - block
           - block_content
           - content_translation
         i18nmenu:
           - content_translation
           - menu_link_content
    

    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.

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

    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.

  5.     b/core/modules/system/migrations/state/system.migrate_drupal.yml
    @@ -0,0  1,112 @@
         # The following do not have an upgrade path.
    

    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.

quietone’s picture

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

effulgentsia’s picture

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

quietone’s picture

Very 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

effulgentsia’s picture

Sorry for taking so long to respond to #283, but here it is...

What should the UI display when it finds, for example, jquery_ui enabled on the source?

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 a core/migrations/state/core.migrate_drupal.yml file, that would be the most correct location for this migration set, because it's core.libraries.yml that contains the library definition for jquery.ui. Since we don't have that, system.migrate_drupal.yml is also a reasonable location, because system 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 or migrate_drupal_ui.migrate_drupal.yml, because both migrate_drupal and migrate_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 as overlay: core) that adjusts the UI into whatever is decided is best.

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?

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:

i18nblocks:
  - content_translation
  - block
  - block_content

within content_translation.migrate_drupal.yml and no entries for i18nblocks within block.migrate_drupal.yml or block_content.migrate_drupal.yml achieves that, because then if you don't have content_translation enabled, then regardless of whether or not you have block or block_content enabled, there are no migration state entries found for i18nblocks, which then puts it in the "will not be upgraded list". And if you do have content_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.

and no entries for i18nblocks within block.migrate_drupal.yml or block_content.migrate_drupal.yml

This only works if block and block_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.

And for contrib module A enabled on the source for all combinations of Module X and Module Y being installed.

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:

A:
- X
- Y

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 have panels: 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?

quietone’s picture

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

Status: Needs review » Needs work

The last submitted patch, 287: 2936365-287.patch, failed testing. View results

quietone’s picture

Status: Needs work » Needs review
FileSize
6.45 KB
118.54 KB

This should fix the test and move most of the contents os system.migrate_drupal.yml to migrate_drupal.migrate_drupal.yml based on #286.

Status: Needs review » Needs work

The last submitted patch, 289: 2936365-289.patch, failed testing. View results

quietone’s picture

Status: Needs work » Needs review

On retesting the patch in #289 passed.

heddn’s picture

super small nit:

+++ b/core/modules/migrate_drupal/migrations/state/migrate_drupal.migrate_drupal.yml
@@ -1,4 +1,94 @@
+# modules they are all listed here since system is always installed.

s/system/migrate_drupal/

quietone’s picture

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

heddn’s picture

Status: Needs review » Reviewed & tested by the community

I think this is ready again then. All feedback address once more.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 293: 2936365-293.patch, failed testing. View results

Gábor Hojtsy’s picture

Looks like the fail is the result of #3024460: Migrate D7 comment type language settings and is legitimate.

Drupal\Tests\migrate_drupal\Kernel\ValidateMigrationStateTest::testMigrationState
No migration state found for version '7' with source_module 'locale' and destination_module 'content_translation' declared in module 'content_translation'
Failed asserting that false is true.

quietone’s picture

Add the migration from #3024460: Migrate D7 comment type language settings to content_translation migration state file.

mikelutz’s picture

Status: Needs review » Reviewed & tested by the community

Simple enough adjustment around the other patch getting in. Back to RTBC pending tests.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 297: 2936365-297.patch, failed testing. View results

heddn’s picture

Status: Needs work » Reviewed & tested by the community
quietone’s picture

Status: Reviewed & tested by the community » Postponed

Sorry to do this but this needs to be postponed while #2979966: Migrate D7 i18n taxonomy term language gets looked at.

quietone’s picture

Status: Postponed » Reviewed & tested by the community

I was being overly cautious. This can go back to RTBC

quietone’s picture

Retested as well for good measure.

Gábor Hojtsy’s picture

@webchick added the "Needs release manager review" tag with

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

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.

catch’s picture

Just to confirm I also think this is fine from a release management standpoint.

DamienMcKenna’s picture

Is this still under consideration for backporting to 8.7.x?

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 297: 2936365-297.patch, failed testing. View results

effulgentsia’s picture

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

mikelutz’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
118.6 KB
925 bytes

Simple enough adjustment, I think it can stay RTBC

heddn’s picture

+1 on RTBC

  • Gábor Hojtsy committed c1734c6 on 8.8.x
    Issue #2936365 by quietone, mikelutz, heddn, larowlan, masipila,...
Gábor Hojtsy’s picture

Version: 8.8.x-dev » 8.7.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)
Issue tags: +Needs issue summary update

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

mikelutz’s picture

Of 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

xjm’s picture

The 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?

quietone’s picture

The patch has similar documentation, in the MigrationState class.

quietone’s picture

It 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?

Gábor Hojtsy’s picture

Status: Patch (to be ported) » Fixed
Issue tags: -needs backport to 8.7.x

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

Gábor Hojtsy’s picture

Version: 8.7.x-dev » 8.8.x-dev

Status: Fixed » Closed (fixed)

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

Gábor Hojtsy’s picture

Tagging for release notes as per the release notes snippet in the summary. Also as a potential highlight since this improves auditability of the migrations.

pameeela’s picture

Issue tags: -8.8.0 release notes

Removed release notes tag as this isn't disruptive but should be included as a highlight.

Wim Leers’s picture

Issue summary: View changes

Fixed screenshot link in issue summary.