Problem/Motivation

The migrate_upgrade UI is now providing a confirmation page, showing the administrator performing the upgrade what modules are being upgraded from their legacy sites (and what modules are not). There is not currently an automated way to get this information - the closest thing is the source_provider annotation on the source plugin, but this doesn't help us with configuration-only migrations. My suggestion is adding a module key to the source plugin config for the Drupal migrations.

Proposed resolution

  1. Modify the migrate_drupal_ui confirmation form to query source and destination plugins for their providers, instead of using a hard-coded list of module/provider relationships.
  2. Add explicit provider information to all source/destination plugins lacking them.

Remaining tasks

None.

User interface changes

Contrib and custom modules will now be able to document their provider relationships, making the confirmation page more accurate.

API changes

None.

Data model changes

None.

Files: 
CommentFileSizeAuthor
#86 interdiff.txt2.98 KBquietone
#86 for_drupal_migration-2569805-86.patch71.07 KBquietone
#76 Selection_004.png12.49 KBquietone
#75 for_drupal_migration-2569805-75.patch70.53 KBquietone
#69 interdiff_67-69.txt3.59 KBJo Fitzgerald
#69 for_drupal_migration-2569805-69.patch70.29 KBJo Fitzgerald
#67 interdiff.txt34.59 KBquietone
#67 for_drupal_migration-2569805-67.patch70.33 KBquietone
#66 for_drupal_migration-2569805-65.patch107.62 KBquietone
#62 interdiff.txt420 bytesquietone
#62 for_drupal_migration-2569805-62.patch107.65 KBquietone
#61 for_drupal_migration-2569805-61.patch107.65 KBquietone
#53 interdiff.txt3.42 KBmikeryan
#53 for_drupal_migration-2569805-53.patch106.54 KBmikeryan
#50 for_drupal_migration-2569805-50-do_not_test.txt22.58 KBheddn
#48 for_drupal_migration-2569805-48.patch107.17 KBmikeryan
#47 for_drupal_migration-2569805-47.patch107.82 KBmikeryan
#41 Selection_017.png26.24 KBjuancasantito
#39 for_drupal_migration-2569805-39.patch107.21 KBmikeryan
#38 interdiff.txt24.49 KBquietone
#38 mi-identify-source-2569805-38.patch106.99 KBquietone
#37 mi-identify-source-2569805-37.patch83.2 KBquietone
#35 mi-identify-source-2569805-35.patch84.08 KBiMiksu
#34 mi-identify-source-2569805-34.patch84.08 KBquietone
#32 mi-identify-source-2569805-32-test-only.patch106.19 KBquietone
#16 mi-identify-sourcemodule.patch80.13 KBtvb
#14 for_drupal_migration-2569805-14.patch80.87 KBquietone
#13 for_drupal_migration-2569805-4.patch80.88 KBquietone
#13 interdiff-for_drupal_migration-4-14.txt358 bytesquietone
#10 for_drupal_migration-2569805-4.patch80.88 KBMiguel.kode
#3 for_drupal_migration-2569805-3.patch80.83 KBmikeryan
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,714 pass(es). View

Comments

mikeryan created an issue. See original summary.

mikeryan’s picture

Well, I went through the exercise of simply adding the module to source and destination keys in all the migration templates, which works great for the real-life purpose, but of course the tests with strict config schema checking want to see 'module' in the schema, and I'm not seeing a good way to do that - for the source side, at least, 'module' is relevant only to Drupal sources, but I don't see a way to specify that as schema that's only relevant to source plugins when used in a Drupal-to-Drupal migration context.

I'm having a vague idea that #2559345: Document source_provider and minimum_schema_version annotation properties might be relevant here - right now the 'source_provider' annotation is only used in practice for Drupal source plugins, but it is named that way to account for other use cases (e.g., WordPress plugins). We can't justify adding 'module' to the generic migrate.source.* schema, but we could add 'provider', which would override the annotation-provided provider if present, and achieve what we want here (at least on the source side).

mikeryan’s picture

FileSize
80.83 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,714 pass(es). View

Making 'provider' a general source and destination property, seems to work locally... This will need further refinement

mikeryan’s picture

Status: Needs work » Needs review

Let's see how it does with the tests (passed locally for me, but that means next to nothing...)

mikeryan’s picture

Issue tags: +Barcelona2015

Tagged for review at Barcelona sprints.

ultimike’s picture

Issue summary: View changes

Minor typo fix.

-mike

ultimike’s picture

I'm not sure I'm qualified to review this patch - I don't understand annotations well enough to explain them to someone else...

Regardless, looking at the patch, I noticed that it looks like you removed some "provider" schema? I don't get it:

+++ b/core/modules/migrate/config/schema/migrate.source.schema.yml
@@ -7,10 +7,6 @@ migrate.source.*:
-  mapping:
-    provider:
-      type: string
-      label: 'Provider'

-mike

quietone’s picture

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

@ultimike, I guess that is removed because it is for an 'empty source'?

mikeryan’s picture

Yes, that was removed because it makes no sense for the empty source.

Miguel.kode’s picture

This is a re-rolling for this patch. I solved some conflicts in :

- core/modules/contact/migration_templates/contact_category.yml
- core/modules/file/migration_templates/d6_file.yml
- core/modules/file/migration_templates/d7_file.yml
- core/modules/migrate/config/schema/migrate.data_types.schema.yml
- core/modules/node/migration_templates/d6_node_revision.yml
- core/modules/search/migration_templates/d7_search_settings.yml
- core/modules/user/migration_templates/user_profile_entity_form_display.yml
- core/modules/aggregator/migration_templates/d6_aggregator_item.yml
- core/modules/aggregator/migration_templates/d6_aggregator_feed.yml

Status: Needs review » Needs work

The last submitted patch, 10: for_drupal_migration-2569805-4.patch, failed testing.

The last submitted patch, 10: for_drupal_migration-2569805-4.patch, failed testing.

quietone’s picture

@Miguel.kode, thx for the reroll.

Looks like those errors are because a 'provider:' entry was in the migration_dependencies of user_profile_entity_form_display.yml. But as, always, lets see what testbot has to say.

quietone’s picture

Oh, that's the wrong patch. This is the correct one.

The last submitted patch, 13: for_drupal_migration-2569805-4.patch, failed testing.

tvb’s picture

There were 2 conflicts in the previous patch.

Miguel.kode’s picture

Status: Needs review » Reviewed & tested by the community

thanks @quietone . It works for me.

catch’s picture

Status: Reviewed & tested by the community » Needs review

This seems like something we could collect during discovery?

chx’s picture

I do not think I understand this issue. Why is this necessary. Plugins have providers. If the UI needs them, then get it. What's the problem?

quietone’s picture

Because of the configuration-only migrations. They use the 'variable' source plugin but the provider can be any module.

chx’s picture

Well then yes perhaps the variable source plugin needs a provider key. But the config destination plugin carries the destination provider in the first part: action.settings belongs to action. And why did we patch things like aggregator_feed?

quietone’s picture

I see what you mean about the destination provider. So I looked at all the migration templates that use the variable source plugin. There are two that don't use the config destination plugin, d6_date_formats.yml and search_page.yml. Instead they use entity:date_format and entity:search_page respectively. Not sure how to get the provider information from that.

id: d6_date_formats
label: Date format configuration
migration_tags:
  - Drupal 6
source:
  plugin: variable_multirow
  variables:
    - date_format_long
    - date_format_medium
    - date_format_short
process:
  id:
    plugin: static_map
    source: name
    map:
      date_format_long: long
      date_format_short: short
      date_format_medium: medium
  pattern: value
destination:
  plugin: entity:date_format

id: search_page
label: Search page configuration
migration_tags:
  - Drupal 6
  - Drupal 7
source:
  plugin: variable
  variables:
    - node_rank_comments
    - node_rank_promote
    - node_rank_recent
    - node_rank_relevance
    - node_rank_sticky
    - node_rank_views
  constants:
    id: node_search
    path: node
    plugin: node_search
process:
  id: 'constants/id'
  path: 'constants/path'
  plugin: 'constants/plugin'
  'configuration/rankings':
    plugin: search_configuration_rankings
destination:
  plugin: entity:search_page

And if the source provider were to be taken from the destination plugin doesn't that limit the source provider to always being the same as the destination provider?

And why did we patch things like aggregator_feed?

Not sure I understand the question.

mikeryan’s picture

Priority: Normal » Minor

And if the source provider were to be taken from the destination plugin doesn't that limit the source provider to always being the same as the destination provider?

That.

Anyway, this exists so the UI can present to the upgrader detailed info on what is being migrated to what. Had some discussions last week with webchick and xjm and we're leaning towards stripping down the review page of the UI (TMI), so this issue is not a priority atm.

benjy’s picture

Don't we already have source_provider on the source itself providing something pretty similar to part of this? Also, it would be nice if we could source_provider in the annotation with a default so no need to put them in every migration? We could then just override it for the variable migrations?

mikeryan’s picture

Issue tags: -Barcelona2015, -Needs Review

Yes, ideally the configuration provider would only need to be set where the plugin lacks one (or for some reason you want to override the plugin's default provider).

xjm’s picture

xjm’s picture

Version: 8.0.x-dev » 8.1.x-dev
Priority: Minor » Normal

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.0-beta1 was released on March 2, 2016, which means new developments and disruptive changes should now be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

benjy’s picture

I still think we should use source_provider here. That mean all the content sources are already correct? And then we can just set the source_provider in the migration yml for config migrations.

Status: Needs review » Needs work

The last submitted patch, 16: mi-identify-sourcemodule.patch, failed testing.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.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
106.19 KB

some discussions last week with webchick and xjm and we're leaning towards stripping down the review page of the UI (TMI)

Yes, simplifying it would be helpful.

This patch is a reroll. And it include benjy's idea (#29) of using the source_provider for content migrations and adding provider to config migrations. And chx's idea (#21) of getting the destination provider from the config destination plugin. But does not (yet?) add a provider key to the variable source plugin as chx suggested.

It also includes a modified MigrateUpgradeForm which uses this information. (I really want to get rid of that big array.) During testing I added some error messages to help find the migrations that lacked a provider or had one defined in the migration and the source plugin. Since this is a test patch, I have left that in. Actually checking all the migrations was a bit fiddly and I need to get outside!

Status: Needs review » Needs work

The last submitted patch, 32: mi-identify-source-2569805-32-test-only.patch, failed testing.

quietone’s picture

Status: Needs work » Needs review
FileSize
84.08 KB

Hopefully, this fixes all the typos.

iMiksu’s picture

Checked if patch needed a reroll. Some offset spotted. Updating patch.

patching file core/modules/user/migration_templates/d6_user_role.yml
Hunk #1 succeeded at 36 (offset -5 lines).
patching file core/modules/user/migration_templates/d7_user_role.yml
Hunk #2 succeeded at 35 (offset -5 lines).
mikeryan’s picture

Assigned: Unassigned » mikeryan
quietone’s picture

quietone’s picture

Needs updating since #2669978: Migrate D7 Menu Links removed menu_links.yml and menu.yml and created d6 and d7 versions of those migrations. Added MigrateUpgradeForm, which was useful as it reported problems with the providers in the user migrations. But I'm not sure how best to present these problem to users of the UI.

mikeryan’s picture

I had assigned this to myself for review, but having contributed the original patch here it's not appropriate for me to RTBC. I have, however, run an upgrade with this patch applied (after rerolling, as attached) and confirmed that it works properly for reals.

heddn’s picture

Assigned: Unassigned » heddn

Taking some notes for my review. I will need to look at:

  • Run a D6/D7 migration and see how the results look.
  • Need to look at error messages - does the text make sense?
  • Pay special attention to the list to see if it the results are sane/accurate.
juancasantito’s picture

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

The wording is confusing. I'd prefer to see something more explicit, like 'missing upgrade path'. That make it clear that the path doesn't exist. I'd also like some explanation why the path doesn't exist. Taxonomy originally was on the list. I enabled that module and then it disappeared from the missing list. So, part of a solution is that the destination module might not be enabled. But it could also just be that an upgrade doesn't exist. For example, color module is enabled in D7 & D8, but there is no migrate templates so no path exists.

Lastly, there's a problem with config only changes for things like contrib modules. Screenshot attached. We need to also handle that either by altering something in this patch to it moves out of the missing or explicitly state that the list is a best effort aggregation and it might be lying. Because a migration path does exist for google analytics.

juancasantito’s picture

Assigned: heddn » Unassigned
quietone’s picture

Status: Needs work » Needs review

@juancasantito, thanks for the review. Yes, work needs to be done on the UI to make these things clearer to users. But that needs to be addressed in a separate issue. Here we are just making it possible to identify the source and destination provider automatically.

I was just looking at the existing issues concerning the UI and there isn't one addressing this specific points you raise. I suggest making a new issue that includes your ideas of how to resolve the problems.

Therefor setting back to NR to see what else people have to say.

mikeryan’s picture

Assigned: Unassigned » mikeryan
Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs change record

Contrib modules will, as core migrations do, add 'provider' to be included in the list in the UI.

There needs to be a change record, assigning to myself to add. Juan, can you open the follow-up for the UI side?

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 39: for_drupal_migration-2569805-39.patch, failed testing.

mikeryan’s picture

Issue tags: +Needs reroll
mikeryan’s picture

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

Rerolled

mikeryan’s picture

Add a draft change record. Also rerolled for the md_entity patch, and fixed a few newline issues. This is now ready for hopefully-final review, testbots willing.

mikeryan’s picture

Assigned: Unassigned » heddn

@heddn: Could you review this one?

Thanks.

heddn’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
22.58 KB

Adding a review only patch. It has only the changes apart from the provider/source_provider changes.

heddn’s picture

Status: Reviewed & tested by the community » Needs work

Needs work for two points of clarification.

  1. +++ b/core/modules/migrate_drupal_ui/src/Form/MigrateUpgradeForm.php
    @@ -1020,26 +400,75 @@ public function buildConfirmForm(array $form, FormStateInterface $form_state) {
    +        if ($source_plugin_provider && $source_provider) {
    +          drupal_set_message(t('Source provider exists in source plugin and migration @migration_id.', ['@migration_id' => $migration_id]), 'error');
    +
    +        }
    

    I'm not sure this is enterable code. How can there not be a source module and there be one of these?

  2. +++ b/core/modules/migrate_drupal_ui/src/Form/MigrateUpgradeForm.php
    @@ -1020,26 +400,75 @@ public function buildConfirmForm(array $form, FormStateInterface $form_state) {
    +        // Source plugin provider and destination provider.
    

    This comment doesn't help. We are dealing with destination at this point. Not source.

heddn’s picture

Assigned: heddn » Unassigned
mikeryan’s picture

OK, I've streamlined the logic in that area, and eliminated the error for having both providers specified (seems to me configuration should override annotation).

heddn’s picture

Status: Needs review » Needs work

That looks a lot better. What about #51.2?

mikeryan’s picture

Status: Needs work » Needs review

What about #51.2?

That comment was removed.

heddn’s picture

Status: Needs review » Reviewed & tested by the community

Sorry, I didn't see that in the interdiff. Ready for prime time.

alexpott’s picture

+++ b/core/modules/image/migration_templates/d7_image_styles.yml
@@ -16,3 +16,4 @@ process:
 destination:
   plugin: entity:image_style
+  provider: image

I don't see why the destination needs to have the provider in the template - this should be able to be determined by the plugin. The image style entity type is provided by the image module.

alexpott’s picture

Also do we have test coverage that the UI is displaying the correct information?

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 53: for_drupal_migration-2569805-53.patch, failed testing.

mikeryan’s picture

I don't see why the destination needs to have the provider in the template - this should be able to be determined by the plugin.

Hmm, at one point this was going to go into this patch but somehow we forgot it...

Also do we have test coverage that the UI is displaying the correct information?

I had thought we had it - and since the output of the UI should be exactly the same as it was before, the tests wouldn't need changing - but it seems #2647470: Write tests skipped right over them. I've added #2834450: Tests needed for migrate_drupal_ui confirmation form to address this (we don't really want to add them as part of this patch, which seems quite large enough already - right?).

quietone’s picture

Status: Needs work » Needs review
FileSize
107.65 KB

Reroll

quietone’s picture

Correct the destination provider for d7_user_role.yml.

I don't see why the destination needs to have the provider in the template - this should be able to be determined by the plugin.

How? For example for d6_comment_field.yml and d7_comment_field.yml

destination:
  plugin: entity:field_storage_config
  provider: comment
mikeryan’s picture

Assigned: Unassigned » mikeryan
mikeryan’s picture

Assigned: mikeryan » Unassigned

Not sure why I assigned it to myself for review, my fingerprints are all over this patch...

I don't see why the destination needs to have the provider in the template - this should be able to be determined by the plugin.

How? For example for d6_comment_field.yml and d7_comment_field.yml

True, it's necessary for configuration entities - but content entities can determine the provider automatically.

phenaproxima’s picture

Assigned: Unassigned » phenaproxima

Self-assigning for review.

quietone’s picture

OK. Patch no longer applies to 8.3 so this is a reroll.

quietone’s picture

Now get the destination provider from the plugin and remove the key from the migration yml files as needed.

phenaproxima’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/migrate_drupal_ui/src/Form/MigrateUpgradeForm.php
    @@ -23,634 +23,9 @@ class MigrateUpgradeForm extends ConfirmFormBase {
    +  protected $moduleUpgradePaths = [];
    

    Do we still need this variable?

  2. +++ b/core/modules/migrate_drupal_ui/src/Form/MigrateUpgradeForm.php
    @@ -1024,26 +400,60 @@ public function buildConfirmForm(array $form, FormStateInterface $form_state) {
    +      // Determine the source provider, looking at configuration first.
    +      $source_module = (!empty($migration->getSourceConfiguration()['provider'])) ? $migration->getSourceConfiguration()['provider'] : NULL;
    +      if (!$source_module) {
    +        $source_module = (!empty($migration->getSourcePlugin()
    +          ->getPluginDefinition()['source_provider'])) ? $migration->getSourcePlugin()
    +          ->getPluginDefinition()['source_provider'] : NULL;
    +      }
    

    Can this be refactored a bit so we're not repeatedly calling getSourceConfiguration() and getPluginDefinition()? I think it will be a little more readable that way.

  3. +++ b/core/modules/migrate_drupal_ui/src/Form/MigrateUpgradeForm.php
    @@ -1024,26 +400,60 @@ public function buildConfirmForm(array $form, FormStateInterface $form_state) {
    +      $destination_module = (!empty($migration->getDestinationConfiguration()['provider'])) ? $migration->getDestinationConfiguration()['provider'] : NULL;
    +      if (!$destination_module) {
    +        if ($migration->getDestinationPlugin()->getPluginId() == 'config') {
    +          $configuration_destination = (!empty($migration->getDestinationConfiguration()['config_name'])) ? $migration->getDestinationConfiguration()['config_name'] : NULL;
    

    Same here. Too many calls to getDestinationConfiguration(). Maybe we could just call $migration->getDestinationPlugin() and use its getConfiguration() and getPluginDefinition() methods.

Jo Fitzgerald’s picture

  1. Removed $moduleUpgradePaths - it only ever has values assigned, but never read.
  2. Refactored getSourceConfiguration() and getPluginDefinition()
  3. Refactored getDestinationConfiguration() (and getDestinationPlugin)
phenaproxima’s picture

Status: Needs review » Reviewed & tested by the community

Gorgeous, dahhlink. RTBC.

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

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

xjm’s picture

Version: 8.4.x-dev » 8.3.x-dev
Status: Reviewed & tested by the community » Needs review
Issue tags: +Needs screenshots, +rc deadline
+++ b/core/modules/user/src/Plugin/migrate/source/d6/Role.php
+++ b/core/modules/user/src/Plugin/migrate/source/d6/User.php
+++ b/core/modules/user/src/Plugin/migrate/source/d6/UserPicture.php
+++ b/core/modules/user/src/Plugin/migrate/source/d6/UserPictureFile.php
+++ b/core/modules/user/src/Plugin/migrate/source/d7/User.php

Interesting that these are provided by Migrate itself; why is that? Other than these additions, all the changes are to the two alpha experimental modules.

If a module fails to include this key, what happens? E.g., many contrib modules will not have them. I don't see the fallback if any described on the change record: https://www.drupal.org/node/2831566

I'm assuming it means the information can't be used by Migrate UI, but what are the actual implications of that? What actually happens? Can someone test manually and provide screenshots of that situation?

For now, I'm moving this back to 8.3.x with an RC deadline, but only because of the changed expectation and the few small changes to sources provided by Migrate itself. It might still be safe to backport this in a patch release even with Migrate in beta, especially given the indirect nature of the change, but on the other hand this is also the kind of thing that's good to ship in minors anyway. So leaving tagged with rc deadline for now, and we can re-evaluate when RC comes around as needed.

Thanks everyone! I'm also adding updated test runs against both branches.

xjm’s picture

Also, the issue summary should be updated with the answers to those questions. Thanks!

Status: Needs review » Needs work

The last submitted patch, 69: for_drupal_migration-2569805-69.patch, failed testing.

quietone’s picture

Status: Needs work » Needs review
FileSize
70.53 KB

Needs a reroll.

quietone’s picture

FileSize
12.49 KB
+++ b/core/modules/block_content/src/Plugin/migrate/source/d6/Box.php
+++ b/core/modules/block_content/src/Plugin/migrate/source/d7/BlockCustom.php
+++ b/core/modules/book/src/Plugin/migrate/source/d6/Book.php
+++ b/core/modules/menu_link_content/src/Plugin/migrate/source/MenuLink.php
+++ b/core/modules/node/src/Plugin/migrate/source/d6/Node.php
+++ b/core/modules/node/src/Plugin/migrate/source/d6/NodeRevision.php
+++ b/core/modules/node/src/Plugin/migrate/source/d6/NodeType.php
+++ b/core/modules/taxonomy/src/Plugin/migrate/source/d6/TermNodeRevision.php

There are also changes to these source plugins as well.

Attached a screen shot of what happens when source_provider is not in the annotation of d6_user_role and provider is not in the migration template for taxonomy_settings.

phenaproxima’s picture

Assigned: Unassigned » phenaproxima

Self-assigning for review.

xjm’s picture

Version: 8.3.x-dev » 8.4.x-dev
Issue tags: -rc deadline

Since 8.3.x is now in commit freeze for its release candidate phase and Migrate is in beta, this disruptive change should now be targeted for 8.4.x. Thanks!

mikeryan’s picture

Version: 8.4.x-dev » 8.3.x-dev
Issue tags: -Needs screenshots
+++ b/core/modules/user/src/Plugin/migrate/source/d6/Role.php
+++ b/core/modules/user/src/Plugin/migrate/source/d6/User.php
+++ b/core/modules/user/src/Plugin/migrate/source/d6/UserPicture.php
+++ b/core/modules/user/src/Plugin/migrate/source/d6/UserPictureFile.php
+++ b/core/modules/user/src/Plugin/migrate/source/d7/User.php

Interesting that these are provided by Migrate itself; why is that? Other than these additions, all the changes are to the two alpha experimental modules.

These migrate_drupal-based source plugins are provided by the user module.

If a module fails to include this key, what happens? E.g., many contrib modules will not have them. I don't see the fallback if any described on the change record: https://www.drupal.org/node/2831566

I'm assuming it means the information can't be used by Migrate UI, but what are the actual implications of that? What actually happens? Can someone test manually and provide screenshots of that situation?

If a module does not indicate its provider, then the UI does not know that that provider (source module) has a migration path and will incorrectly claim that the migration path is missing for that module (which is what is happening now). The migration will actually run - only the reporting will be off (@quietone provided a screenshot).

Since 8.3.x is now in commit freeze for its release candidate phase and Migrate is in beta, this disruptive change should now be targeted for 8.4.x. Thanks!

This patch does not touch the beta-experimental migrate API, it only affects the Drupal-to-Drupal migration path, which is still alpha experimental. I don't see anything disruptive here - modules previously mistakenly shown as not having migration paths will continue to be shown the same way, except now they actually have the capability of indicating their source providers.

mikeryan’s picture

Updated the issue summary.

I think this is a blocker for Drupal-to-Drupal beta stability - marking migrate-critical.

joelpittet’s picture

mikeryan’s picture

Issue tags: +Migrate UI
phenaproxima’s picture

Looks quite good. +1 what @mikeryan said -- this is not API disruptive, but it fixes some very annoying stuff in the UI. I love the fact that we can finally remove that big stupid mapping array.

I found only a few little points:

  1. +++ b/core/modules/migrate_drupal_ui/src/Form/MigrateUpgradeForm.php
    @@ -1032,26 +393,60 @@ public function buildConfirmForm(array $form, FormStateInterface $form_state) {
    +        drupal_set_message(t('Source provider not found for @migration_id.', ['@migration_id' => $migration_id]), 'error');
    

    This should be using $this->t().

  2. +++ b/core/modules/migrate_drupal_ui/src/Form/MigrateUpgradeForm.php
    @@ -1032,26 +393,60 @@ public function buildConfirmForm(array $form, FormStateInterface $form_state) {
    +        else {
    +          $destination_module = $destination_plugin->getPluginDefinition()['provider'];
    +        }
    

    Why don't we do something similar to this when determining the source module? If there's a reason for that, we should probably add a comment as to why.

  3. +++ b/core/modules/migrate_drupal_ui/src/Form/MigrateUpgradeForm.php
    @@ -1032,26 +393,60 @@ public function buildConfirmForm(array $form, FormStateInterface $form_state) {
    +        drupal_set_message(t('Destination provider not found for @migration_id.', ['@migration_id' => $migration_id]), 'error');
    

    Should be using $this->t().

quietone’s picture

Assigned: phenaproxima » quietone
Status: Needs review » Needs work

very annoying

I think that is an understatement!

The source provider for the d6 field migrations is 'content'. Surely that is not right.

mikeryan’s picture

The source provider for the d6 field migrations is 'content'. Surely that is not right.

The actual machine name of the D6 module we all know and love as "CCK" is, actually, "content". So... it is right, and don't call me Shirley...

quietone’s picture

Status: Needs work » Needs review
FileSize
71.07 KB
2.98 KB

Hah, I don't think I ever looked at the .info file before. Thanks.

Fixed #83.1 and #83.3. For #83.2, I hope the added comments are sufficient.