For Drupal migration, identify the source module

Minor typo fix.

-mike

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

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

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

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

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

There were 2 conflicts in the previous patch.

thanks @quietone . It works for me.

This seems like something we could collect during discovery?

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?

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

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?

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.

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?

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.

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!

Hopefully, this fixes all the typos.

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

Rerolling because #2549839: Removed unused variables from D6 search settings migration was committed.

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.

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.

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

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

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.

That looks a lot better. What about #51.2?

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

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

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

Reroll

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

Self-assigning for review.

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

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

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.

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

Gorgeous, dahhlink. RTBC.

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

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

Needs a reroll.

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

Self-assigning for review.

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!

Likely needs a re-roll after #2724903: Migrated custom block body field is hidden on form and display

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

very annoying

I think that is an understatement!

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

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.

Very thorough. I'm happy with this.

Just a bit of fuzz in d6_file.yml.

patching file core/modules/file/migration_templates/d6_file.yml
Hunk #1 succeeded at 12 with fuzz 1.

I think such a simple re-roll can be returned to RTBC.

Tests are green again, back to RTBC.

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

There's no test coverage of these messages. Also is the lack of a source provider really an error? Doesn't just mean the module is not installed on the source?

How to write the tests? I'm not familiar with WebTestBase.

Tests added.

Also is the lack of a source provider really an error?

Yes, it really is an error. This is saying that the source provider is not defined, either in the migration or the source plugin annotation. It is that definition which lets us know what module on the source site is/was responsible for the data.

Here's a quick re-roll. The changes were in that huge $moduleUpgradePaths array, which may need to be reviewed and the test MigrateUpgradeTestBase the modules array was made multiline and missing the addition of 'provider_test'

Surprised to see the provider_test was missing, considering the test shouldn't pass without it. So I looked and I do see it the patch from #96.

+++ b/core/modules/migrate_drupal_ui/src/Tests/MigrateUpgradeTestBase.php
@@ -30,7 +30,7 @@
+  public static $modules = ['language', 'content_translation', 'migrate_drupal_ui', 'telephone', 'provider_test'];

It was there.

Just a sanity check....

Oh not missing from the patch, just needing to be added manually back during reroll . Those were the only two merge conflicts I addressed

Oh, right.

Looks to me like @alexpott's feedback is addressed.

So #2724903: Migrated custom block body field is hidden on form and display added to \Drupal\migrate_drupal_ui\Form\MigrateUpgradeForm::$moduleUpgradePaths - are we sure that we're not undoing that change here... asked in another way - how does core/modules/block_content/migration_templates/block_content_entity_form_display.yml get the right source provider?

Yes, this needs to be updated.

@quietone do you think we should have a test that ensures somehow that all migrations have the expected source provider since #97 didn't fail?

@alexpott, ha ha, I was thinking about that too. And trying not to think about (because I just want this patch in and that big array gone). Still it would be useful. A followup?

But then I kept thinking about such a test. So, here it is. I pulled the logic out into a new class, MigrateProviders.

1. +++ b/core/modules/migrate_drupal_ui/src/Form/MigrateUpgradeForm.php
   @@ -397,43 +398,14 @@
   +    $migrateProviders = new MigrateProviders();
   

   Any chance this could be a service and injected into the form?

2. +++ b/core/modules/migrate_drupal_ui/tests/src/Kernel/ProvidersExistTest.php
   @@ -0,0 +1,125 @@
   +  public static $modules = [
   

   Is there any way to auto-discover all the core modules? So this is more dynamic?

Not sure if either of these comments make sense, so leaving at NR.

1.Yes, there is chance. It is in this patch.
2. I had a look at KernelTestBase and didn't see any existing way to say 'load all modules'. But yes, we need to find a way to do that.

All actionable feedback addressed. LGTM.

+++ b/core/modules/migrate_drupal_ui/tests/src/Kernel/ProvidersExistTest.php
@@ -0,0 +1,125 @@
+  public static $modules = [

This can be dynamically generated - see \Drupal\KernelTests\Config\DefaultConfigTest()

+++ b/core/modules/migrate_drupal_ui/src/MigrationProviders.php
@@ -0,0 +1,51 @@
+  public function getProviders($migration) {

Should be typehinted. But this class looks unnecessary to me.

Looking at MigrationProviders - I'm pondering if this should be on MigrationInterface as getSourceProvider() and getDestinationProvider() - so we don't have to introduce a whole new class etc... because this seems useful knowledge for a migration to have about itself.

1. +++ b/core/modules/migrate_drupal_ui/src/MigrationProviders.php
   @@ -0,0 +1,51 @@
   +      $plugin_definition = $migration->getSourcePlugin()->getPluginDefinition();
   

   There's part of me that would just expect getting the source provider should be:
   $migration->getSourcePlugin()->getSourceProvider()

2. +++ b/core/modules/migrate_drupal_ui/src/MigrationProviders.php
   @@ -0,0 +1,51 @@
   +    $destination_configuration = $migration->getDestinationConfiguration();
   +    $destination_module = (!empty($destination_configuration['provider'])) ? $destination_configuration['provider'] : NULL;
   +    if (!$destination_module) {
   +      $destination_plugin = $migration->getDestinationPlugin();
   +      if ($destination_plugin->getPluginId() == 'config') {
   +        // For configuration destinations pull out the module name from the
   +        // configuration destination string.
   +        $configuration_destination = (!empty($destination_configuration['config_name'])) ? $destination_configuration['config_name'] : NULL;
   +        if ($configuration_destination) {
   +          $destination_module = explode('.', $configuration_destination, 2)[0];
   +        }
   +      }
   +      else {
   +        // If not a config destination, the destination module is simply
   +        // the provider of the plugin.
   +        $destination_module = $destination_plugin->getPluginDefinition()['provider'];
   +      }
   +    }
   

   And all of this could be
   $migration->getDestinationPlugin()->getDestinationProvider()
   Or something like that.

I'm not sure but I think we could make this stuff easier to get and more API complete.

@alexpott, thanks for the tip on getting the list of all modules.

I'm pondering if this should be on MigrationInterface as getSourceProvider() and getDestinationProvider()

I pondered that as well. But decided against it since the providers are only used by migrate_drupal_ui. Maybe a better place would be in migrate_drupal? Would be good to hear what mikeryan, heddn or phenaproxima think.

I think I'm in favor of adding a getProvider() method to both MigrateSourceInterface and MigrateDestinationInterface SourcePluginBase and DestinationBase. Obviously we cannot add them to the relevant interfaces because it would be a BC break. But I'm OK with having TODOs about moving those methods to the interfaces.

After sitting here in the sprint room chatting about this for the last hour, we cannot think of any other user of the source/destination provider data. Drush doesn't use it. Only the UI needs this. So the code seems to be in the right place.

Still needs work for auto-generating the list of modules per #110, no?

Gonna take a crack at #110.1.

Sorry I missed that point in my eagerness to get this in.

Is it this simple? Let's see.

Is it this simple? Let's see.

Thanks for moving this along. One thing that strikes me about the modified test is that it is running the test for every module, which is far more than we need. We only need to run it once, for all migrations at once. And, on a quick look, this isn't picking up the test modules which are needed.

I took a different approach but didn't finish. I'll go look for it now.

system and user are installed by MigrateDrupalTestBase

I ran patch #119 with the debugger and confirmed that is is not loading the test modules.
Here is the approach I took.

Arg, named the interdiff as a patch. Sorry, testbot.

Fix coding standards errors.

#110 is now addressed and tests pass. Back to RTBC.

After sitting here in the sprint room chatting about this for the last hour, we cannot think of any other user of the source/destination provider data. Drush doesn't use it. Only the UI needs this. So the code seems to be in the right place.

Hmmm but this means that that MigrateProviders has to reach into the source plugin and destination plugin and get all the configuration out and then make assumptions about random keys. It feels like this break encapsulation. If the source plugins don't need this information then why do they have it. The only reason why drush doesn't need it is because its not validating or checking what's there - the user is picking migrations. If there was a drush command to do what migrate UI does then it would need it. I guess my question is: are you really sure?

+++ b/core/modules/migrate_drupal_ui/tests/src/Kernel/ProvidersExistTest.php
@@ -0,0 +1,56 @@
+    $module_handler = $this->container->get('module_handler');
+    $modules = system_rebuild_module_data();
+    $modules_enabled = $module_handler->getModuleList();
+    $modules_to_enable = array_keys(array_diff_key($modules, $modules_enabled));
+    $this->enableModules($modules_to_enable);

This should only enable core modules. That's why I pointed you at \Drupal\KernelTests\FileSystemModuleDiscoveryDataProviderTrait

@alexpott, yea I got that wrong. This should fix the module discovery.

Added manual testing steps.

Test d6 and d7 standard installs in the upgrade path, both were showing proper modules in source/dest. Marking as RTCB.