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.

Manual Testing

This issue is only present in the UI as drush doesn't utilize this provider array/config.

To test the manual update:

  1. Create a D6 database from a standard install and back it up
  2. Create a D7 Database from standard install and back it up
  3. Create D8 install off standard, install migrate_drupal_ui
  4. Visit /upgrade and test the upgrade path.
  5. Ensure there are no regressions in the module destinations provided for d6 and d7 migrations
Files: 
CommentFileSizeAuthor
#210 interdiff.txt1.91 KBquietone
#210 2569805-210.patch114.09 KBquietone
#207 interdiff.txt7.8 KBquietone
#207 2569805-207.patch114.05 KBquietone
#206 interdiff.txt1.03 KBquietone
#206 2569805-206.patch115.88 KBquietone
#204 interdiff.txt1.28 KBquietone
#204 2569805-204.patch115.5 KBquietone
#202 interdiff.txt426 bytesquietone
#202 2569805-202.patch115.5 KBquietone
#200 interdiff.txt11.79 KBquietone
#200 2569805-200.patch115.5 KBquietone
#199 interdiff.txt12.46 KBquietone
#199 2569805-199.patch117.62 KBquietone
#198 interdiff.txt515 bytesquietone
#198 2569805-198.patch121.86 KBquietone
#196 interdiff.txt7.42 KBquietone
#196 2569805-196.patch121.22 KBquietone
#194 interdiff.txt794 bytesquietone
#194 2569805-194.patch115.21 KBquietone
#192 2569805-192.patch115.12 KBquietone
#190 2569805-190.patch114.91 KBJo Fitzgerald
#190 interdiff-187-190.txt1.41 KBJo Fitzgerald
#187 interdiff.txt6.23 KBquietone
#187 2569805-187.patch114.9 KBquietone
#186 interdiff.txt577 bytesquietone
#186 2569805-186.patch111.37 KBquietone
#184 interdiff.txt26.48 KBquietone
#184 2569805-184.patch110.65 KBquietone
#180 2569805-180.patch135.42 KBJo Fitzgerald
#178 interdiff.txt81.24 KBquietone
#178 2569805-178.patch114.75 KBquietone
#168 2569805-168.patch89.14 KBJo Fitzgerald
#168 interdiff-166-168.txt774 bytesJo Fitzgerald
#166 2569805-166.patch89.15 KBJo Fitzgerald
#166 interdiff-163-166.txt1.53 KBJo Fitzgerald
#163 interdiff.txt4.77 KBquietone
#163 2569805-163.patch89.15 KBquietone
#161 2569805-161.patch89.53 KBJo Fitzgerald
#161 interdiff-159-161.txt1.55 KBJo Fitzgerald
#159 interdiff.txt5.84 KBquietone
#159 identify-source-2569805-160.patch89.53 KBquietone
#155 interdiff-145-153.txt17.63 KBquietone
#155 identify-source-2569805-153.patch90.56 KBquietone
#138 identify-source-2569805-138.patch89.31 KBAdita
#138 interdiff.txt1.38 KBAdita
#136 new_after.png130.08 KBmaxocub
#134 interdiff.txt4.09 KBquietone
#134 identify-source-2569805-134.patch89.23 KBquietone
#134 identify-source-2569805-134-test-only.patch84.55 KBquietone
#133 migrate_after.png58.39 KBmaxocub
#133 migrate_before.png51.66 KBmaxocub
#131 identify-source-2569805-127-reroll.patch85.75 KBjoelpittet
#127 interdiff.txt2.17 KBquietone
#127 identify-source-2569805-127.patch84.47 KBquietone
#124 interdiff.txt2.41 KBquietone
#124 identify-source-2569805-124.patch85.62 KBquietone
#121 interdiff-108-121.patch3.87 KBquietone
#121 identify-source-2569805-121.patch85.62 KBquietone
#119 2569805-119.patch83.61 KBheddn
#119 interdiff_117-119.txt649 bytesheddn
#145 interdiff.txt925 bytesAdita
#145 identify-source-2569805-145.patch89.4 KBAdita

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.

phenaproxima’s picture

Status: Needs review » Reviewed & tested by the community

Very thorough. I'm happy with this.

Status: Reviewed & tested by the community » Needs work

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

Jo Fitzgerald’s picture

Status: Needs work » Needs review
FileSize
71.06 KB

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.
Jo Fitzgerald’s picture

Status: Needs review » Reviewed & tested by the community

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

Status: Reviewed & tested by the community » Needs work

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

Jo Fitzgerald’s picture

Status: Needs work » Reviewed & tested by the community

Tests are green again, back to RTBC.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ 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?

quietone’s picture

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

quietone’s picture

Assigned: quietone » Unassigned
quietone’s picture

Status: Needs work » Needs review
FileSize
76.27 KB
4.17 KB

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.

joelpittet’s picture

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'

quietone’s picture

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

joelpittet’s picture

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

quietone’s picture

Oh, right.

phenaproxima’s picture

Status: Needs review » Reviewed & tested by the community

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

alexpott’s picture

Status: Reviewed & tested by the community » Needs review

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?

quietone’s picture

Yes, this needs to be updated.

alexpott’s picture

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

quietone’s picture

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

quietone’s picture

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

heddn’s picture

  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.

quietone’s picture

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.

heddn’s picture

Status: Needs review » Reviewed & tested by the community

All actionable feedback addressed. LGTM.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ 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.

quietone’s picture

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

phenaproxima’s picture

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.

heddn’s picture

Status: Needs work » Reviewed & tested by the community

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.

tstoeckler’s picture

Status: Reviewed & tested by the community » Needs work

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

heddn’s picture

Gonna take a crack at #110.1.

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

heddn’s picture

Status: Needs work » Needs review
FileSize
2.36 KB
83.59 KB

Is it this simple? Let's see.

heddn’s picture

Status: Needs work » Needs review
FileSize
2.36 KB
83.59 KB

Is it this simple? Let's see.

Status: Needs review » Needs work

The last submitted patch, 116: 2569805-116.patch, failed testing.

heddn’s picture

Status: Needs work » Needs review
FileSize
649 bytes
83.61 KB
quietone’s picture

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

quietone’s picture

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

quietone’s picture

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

Status: Needs review » Needs work

The last submitted patch, 121: interdiff-108-121.patch, failed testing.

quietone’s picture

Status: Needs work » Needs review
FileSize
85.62 KB
2.41 KB

Fix coding standards errors.

heddn’s picture

Status: Needs review » Reviewed & tested by the community

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

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

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

quietone’s picture

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

timodwhit’s picture

Issue summary: View changes

Added manual testing steps.

timodwhit’s picture

Status: Needs review » Reviewed & tested by the community

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

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 127: identify-source-2569805-127.patch, failed testing.

joelpittet’s picture

Status: Needs work » Needs review
FileSize
85.75 KB

The only diff in the reroll that made it not apply:

-    'd7_theme_settings' => [
-      'source_module' => 'system',
-      'destination_module' => 'system',

So I added the provider: system to
core/modules/system/migration_templates/d7_theme_settings.yml

mikeryan’s picture

If there was a drush command to do what migrate UI does then it would need it.

There is one - drush migrate-upgrade (in the migrate_upgrade module).

maxocub’s picture

Status: Needs review » Needs work
FileSize
51.66 KB
58.39 KB

I'm really sorry for putting this back to NW as I was about to RTBC it, but I have 2 nitpicks and 1 concern.

+++ b/core/modules/migrate_drupal_ui/tests/src/Kernel/ProvidersExistTest.php
@@ -0,0 +1,58 @@
+  use FileSystemModuleDiscoveryDataProviderTrait;
+  /**

1. Blank line missing.

+++ b/core/modules/migrate_drupal_ui/tests/src/Kernel/ProvidersExistTest.php
@@ -0,0 +1,58 @@
+  public static $modules = ['migrate_drupal_ui', 'provider_test'];

2. 'migrate_drupal_ui' is not needed, it is already provided by coreModuleListDataProvider().

And for the concern, it's about the 'Available upgrade paths' table. After applying the patch, I see a few 'migrate' in the 'destination' column that were not there before, and it's a bit confusing. Is that because of plugins provided by the migrate plugin?

Before:

After:

quietone’s picture

@maxocub, thanks for testing this and spotting that problem. The nitpicks are fixed. As for the 'migrate' destinations, the test has been modified to check for invalid destinations and a provider of 'core' has been added to the destinations in the migrate module.

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

maxocub’s picture

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

Screenshot after applying latest patch:

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/modules/migrate_drupal_ui/src/Form/MigrateUpgradeForm.php
    @@ -11,6 +11,7 @@
    +use Drupal\migrate_drupal_ui\MigrationProviders;
     use Symfony\Component\DependencyInjection\ContainerInterface;
     
     /**
    @@ -21,658 +22,6 @@ class MigrateUpgradeForm extends ConfirmFormBase {
    
    @@ -21,658 +22,6 @@ class MigrateUpgradeForm extends ConfirmFormBase {
       use MigrationConfigurationTrait;
     
       /**
    -   * Mapping of known migrations and their source and destination modules.
    -   *
    

    I get that this used to be implemented in migrate_drupal_ui only, but is this not a kind of information that other uses could be built on top of, command line or other UIs? Why are we putting this in migrate_drupal_ui? I see this was discussed in #111, #113, etc. @heddn posted in conclusion in #113 that the migrate team did think this was the right place, but did not indicate who was part of the discussion. Other than this the rest two are minor notes:

  2. +++ b/core/modules/migrate_drupal_ui/src/MigrationProviders.php
    @@ -0,0 +1,52 @@
    +/**
    + * Gets the source and destination provider for a migration.
    + */
    +class MigrationProviders {
    +
    +  /**
    +   * @param \Drupal\migrate\Plugin\MigrationInterface $migration
    +   *   The migration entity.
    

    The method documentation seems to be on the class(?).

  3. +++ b/core/modules/migrate_drupal_ui/tests/modules/provider_test/provider_test.info.yml
    @@ -0,0 +1,6 @@
    +description: 'Add a migration missing a source and destination and a source plugin missing a source provide.'
    

    provider (at the end)

Adita’s picture

@Gábor Hojtsy #2 and #3 fixed

Adita’s picture

Status: Needs work » Needs review
heddn’s picture

re #137.1: the conversation was with @phenaproxima, myself and maxocub. mikeryan unforunately wasn't at the table at the time. He brings up a good point in #132 that migrate_upgrade is the equivilent to migrate_upgrade_ui for drush.

@mikeryan any thoughts on moving this to MigrateSourceInterface and MigrateDestinationInterface? While migrate_upgrade *could* use it, do you plan to do that?

mikeryan’s picture

Actually, I wasn't thinking it through - drush migrate-upgrade isn't going to be reporting on providers the way the UI does, so I think this can stick to migrate_drupal_ui.

heddn’s picture

Status: Needs review » Reviewed & tested by the community

I think #138 can be marked RTBC then. No non-migrate_drupal_ui modules seem to need the source or destination interface changes. All feedback from #137 is addressed.

tstoeckler’s picture

+++ b/core/modules/migrate_drupal_ui/src/MigrationProviders.php
@@ -0,0 +1,55 @@
+  /**
+   * /**
+   * Gets the source and destination provider for a migration.

This commenting is strange/incorrect.

Can probably fixed on commit, but would also be nice to make less work for committers, I guess ;-)

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/modules/migrate_drupal_ui/src/MigrationProviders.php
    @@ -0,0 +1,55 @@
    +  /**
    +   * /**
    +   * Gets the source and destination provider for a migration.
    

    Indeed, the formatting of this is wrong.

  2. +++ b/core/modules/migrate_drupal_ui/src/MigrationProviders.php
    @@ -0,0 +1,55 @@
    +   *   And indexed array containing the source and destination provider.
    

    "And"?

    Since we need to fix this anyway, let's elaborate on the structure of the array. I would say something like:

    "An indexed array containing the source and destination provider module names as strings. If either cannot be determined, NULL will be used in its place."

    (Or some better English :)

Adita’s picture

this patch change the wrong formatting, and I changed the commentary for @return

Adita’s picture

Status: Needs work » Needs review
heddn’s picture

Status: Needs review » Reviewed & tested by the community

Docs only changes. Back to RTBC.

alexpott’s picture

fwiw I'm still not happy with the decisions taken with respect to API design. The way in which MigrationProviders reaches into a migration and gets the configuration out of its source and destination plugins is for me completely the wrong way around. It's not just about use-case it is about ensure that the right parts of the system has access to the right things. What we have here is a reliance on an ArrayPI (the plugin definition). We know from experience that this causes problems when we want to refactor and make changes further down the line because there is no contract (interface) between the migration and its plugins to be able to provide this information to the MigrationProviders class.

phenaproxima’s picture

Status: Reviewed & tested by the community » Needs review

I propose the following solution, if we're adamant about getting rid of MigrationProviders:

  1. Define a new interface, MigrationProviderInterface (or similar), with one method: getProvider().
  2. Implement it on SourcePluginBase and DestinationBase, with the logic currently in MigrationProviders.
  3. Extend the implementation on the Config destination, to include the logic currently in MigrationProviders for config stuff.
  4. Remove MigrationProviders and have the UI do something like this:
    $source_plugin = $migration->getSourcePlugin();
    if ($source_plugin instanceof MigrationProviderInterface) {
      $source_provider = $source_plugin->getProvider();
    }
    

    ...and ditto for the destination.

How does that sound? I'll write the patch if we have general buy-in from everybody.

heddn’s picture

+1 on #149

mikeryan’s picture

+1 for me as well. I haven't looked closely at this patch in a while, but yes, only the source and destination plugins should be interpreting their own configuration...

quietone’s picture

Assigned: Unassigned » quietone

I'd like to have a go at implementing #149.

quietone’s picture

What directory does MigrationProviderInterface go in?

mikeryan’s picture

Status: Needs review » Needs work
quietone’s picture

Status: Needs work » Needs review
FileSize
90.56 KB
17.63 KB

During the migrate call we agreed on the directory, and I didn't write it down. And I don't remember either! So, to keep this moving along here is a patch.

mikeryan’s picture

src/Plugin was what we decided - and it's what you did here, so we're good.

heddn’s picture

Status: Needs review » Reviewed & tested by the community

This solves the slight dilema of where to put the provider logic. Its an interface, so it has to go in migrate_drupal. Tests are basically the same, just moved into a new location. We're using the approach outlined in #149. This looks good to go again.

phenaproxima’s picture

Status: Reviewed & tested by the community » Needs work

This is really close, but not quite RTBC. There are some implementation details that need to be corrected...

  1. +++ b/core/modules/migrate/src/Plugin/MigrationProviderInterface.php
    @@ -0,0 +1,15 @@
    +interface MigrationProviderInterface {
    

    This will need a doc comment.

  2. +++ b/core/modules/migrate/src/Plugin/MigrationProviderInterface.php
    @@ -0,0 +1,15 @@
    +   * @return string | NULL
    +   *   The provider.
    

    Should be string|null, and maybe we should explain the return value a little more.

  3. +++ b/core/modules/migrate/src/Plugin/migrate/destination/Config.php
    @@ -149,4 +149,22 @@ public function rollback(array $destination_identifier) {
    +    // Determine the destination provider, looking in configuration first.
    +    $destination_configuration = $this->migration->getDestinationConfiguration();
    

    The destination does not need to reach into the migration anymore. It should just use $this->configuration.

  4. +++ b/core/modules/migrate/src/Plugin/migrate/destination/Config.php
    @@ -149,4 +149,22 @@ public function rollback(array $destination_identifier) {
    +    return $destination_module;
    

    I think this should first call parent::getProvider(), and then compute the provider from the config name if the parent did not specify a provider.

  5. +++ b/core/modules/migrate/src/Plugin/migrate/destination/DestinationBase.php
    @@ -110,4 +111,29 @@ protected function setRollbackAction(array $id_map, $update_action = MigrateIdMa
    +   * Gets the destination provider.
    +   *
    +   * @return string | NULL
    +   *   The destination provider.
    +   */
    +  public function getProvider() {
    

    Should be @inheritdoc.

  6. +++ b/core/modules/migrate/src/Plugin/migrate/destination/DestinationBase.php
    @@ -110,4 +111,29 @@ protected function setRollbackAction(array $id_map, $update_action = MigrateIdMa
    +      if ($this->pluginId == 'config') {
    +        // For config destinations get the module name from the configuration.
    +        $configuration = (!empty($this->configuration['config_name'])) ? $this->configuration['config_name'] : NULL;
    +        if ($configuration) {
    +          $destination_module = explode('.', $configuration, 2)[0];
    +        }
    +      }
    

    All this should be ripped out of here. This logic only applies to the Config destination, so it should be living only in Config::getProvider(). And it's already there, so this can just flat-out be removed.

quietone’s picture

Status: Needs work » Needs review
FileSize
89.53 KB
5.84 KB

1-2. Fixed
3,5,6. Arg, I thought I fixed that! Clearly, I was thinking ahead to my weekend away.
4. Fixed. But what strikes me about this is that it will be possible to have a destination provider key that refers to a different module than what is referred to in the config_name field. For example,

destination:
  plugin: config
  config_name: dblog.settings
  provider: mymodule

The provider key takes precedence and 'mymodule' will be the destination.

I also found 5 config migrations with a destination provider key. Since that is not necessary, I have removed them.

quietone’s picture

Assigned: quietone » Unassigned
Jo Fitzgerald’s picture

Minor coding standards tweaks.

alexpott’s picture

Status: Needs review » Needs work

This is looking really nice.

  1. +++ b/core/modules/migrate_drupal_ui/src/Form/MigrateUpgradeForm.php
    @@ -1067,6 +432,12 @@ public function buildConfirmForm(array $form, FormStateInterface $form_state) {
    +    $system_data = [];
    +    // Fetch the system data at the first opportunity.
    +    if (empty($system_data)) {
    +      $system_data = $form_state->get('system_data');
    +    }
    

    This can be just $system_data = $form_state->get('system_data');

  2. +++ b/core/modules/migrate/src/Plugin/MigrationProviderInterface.php
    @@ -0,0 +1,20 @@
    +   *   source plugin the provider is expected to be the name of the module in
    

    extension? I.e. could this be a theme?

  3. +++ b/core/modules/migrate/src/Plugin/migrate/destination/Config.php
    @@ -149,4 +149,22 @@ public function rollback(array $destination_identifier) {
    +    if (in_array($destination_module, ['migrate', 'migrate_drupal', 'migrate_drupal_ui'])) {
    

    in_array - let's use the third param and make it TRUE.

  4. +++ b/core/modules/migrate/src/Plugin/migrate/destination/Config.php
    @@ -149,4 +149,22 @@ public function rollback(array $destination_identifier) {
    +      $configuration_destination = (!empty($this->configuration['config_name'])) ? $this->configuration['config_name'] : NULL;
    +      if ($configuration_destination) {
    +        $destination_module = explode('.', $configuration_destination, 2)[0];
    +      }
    +      else {
    +        $destination_module = NULL;
    +      }
    

    You can have a bit less logic here if you do:

          if (!empty($this->configuration['config_name'])) {
            $destination_module = explode('.', $this->configuration['config_name'], 2)[0];
          }
          else {
            $destination_module = NULL;
          }
    
  5. +++ b/core/modules/migrate/src/Plugin/migrate/destination/DestinationBase.php
    @@ -110,4 +111,15 @@ protected function setRollbackAction(array $id_map, $update_action = MigrateIdMa
    +    $destination_module = (!empty($this->configuration['provider'])) ? $this->configuration['provider'] : NULL;
    +    if (!$destination_module) {
    +      $destination_module = $this->pluginDefinition['provider'];
    +    }
    +    return $destination_module;
    

    Could be:

      public function getProvider() {
        return !empty($this->configuration['provider']) ? $this->configuration['provider'] : NULL;
      }
    
  6. +++ b/core/modules/migrate/src/Plugin/migrate/source/SourcePluginBase.php
    @@ -541,4 +542,15 @@ public function postRollback(MigrateRollbackEvent $event) {
    +    $source_module = (!empty($this->configuration['provider'])) ? $this->configuration['provider'] : NULL;
    +    if (!$source_module) {
    +      $source_module = (!empty($this->pluginDefinition['source_provider'])) ? $this->pluginDefinition['source_provider'] : NULL;
    +    }
    +    return $source_module;
    

    I think this might be neater like:

      /**
       * {@inheritdoc}
       */
      public function getProvider() {
        if (!empty($this->configuration['provider'])) {
          return $this->configuration['provider'];
        }
        elseif (!empty($this->pluginDefinition['source_provider']) {
          return $this->pluginDefinition['source_provider'];
        }
        return NULL;
      }
    

    Less logic to hold in one's head. Also should be trust provider or source_provider first?

quietone’s picture

1. Fixed
2.

extension? I.e. could this be a theme?

Not sure.
3. Fixed
4. Fixed
5. Fixed, with a slightly modified solution.
6. Fixed

quietone’s picture

Status: Needs work » Needs review
heddn’s picture

Status: Needs review » Needs work
+++ b/core/modules/migrate/src/Plugin/migrate/destination/Config.php
@@ -157,14 +157,7 @@
+      $destination_module = (!empty($this->configuration['config_name'])) ? explode('.', $this->configuration['config_name'], 2)[0] : NULL;

+++ b/core/modules/migrate/src/Plugin/migrate/destination/DestinationBase.php
@@ -115,11 +115,7 @@
+    return (!empty($this->configuration['provider'])) ? $this->configuration['provider'] : $this->pluginDefinition['provider'];

Nit: extra paren around !empty isn't necessary.

For #162.2, it was discussed by @phenaproxima and @alexpott in IRC that 'extension' in place of module would suffice.

So close. I'm itching to RTBC this thing.

Jo Fitzgerald’s picture

Status: Needs work » Needs review
FileSize
1.53 KB
89.15 KB

Let's get this finished...

heddn’s picture

Status: Needs review » Needs work

@Jo Fitzgerald that only caught one of my parenthesis nits. Can you grab the other too in DestinationBase?

Jo Fitzgerald’s picture

Status: Needs work » Needs review
FileSize
774 bytes
89.14 KB

Whoops! Rushed that one, sorry. #Embarrassed

phenaproxima’s picture

Status: Needs review » Reviewed & tested by the community

All appears to be right with the universe. Or at least, with this patch.

neclimdul’s picture

+++ b/core/modules/migrate/src/Plugin/migrate/destination/Config.php
@@ -149,4 +149,15 @@ public function rollback(array $destination_identifier) {
+  /**
+   * {@inheritdoc}
+   */
+  public function getProvider() {
+    $destination_module = parent::getProvider();
+    // The migrate modules are not valid destination modules.
+    if (in_array($destination_module, ['migrate', 'migrate_drupal', 'migrate_drupal_ui'], TRUE)) {
+      $destination_module = !empty($this->configuration['config_name']) ? explode('.', $this->configuration['config_name'], 2)[0] : NULL;
+    }
+    return $destination_module;
+  }

Passing on my thoughts from IRC, this breaks the plugin interface so badly I would suggest another method.

Additionally I can't put together why so many of the plugins are explicitly listing their provider when its autogenerated?

quietone’s picture

It is because the Variable source plugin and the Config destination plugin are a bit different and they are widely used. The auto generated provider for Config is 'migrate' and for Variable it is 'migrate_drupal'. But for say, the d6_dblog_settings migration, the source as far as the UI is concerned is dblog and the destination is also dblog.

neclimdul’s picture

So... sounds like provider isn't the right property to be using the UI. Because as far as Plugins are concerned migrate and migrate_drupal are the providers.

quietone’s picture

Status: Reviewed & tested by the community » Needs work

Ok, so the providers, as used here, are trying to identify the modules that need to be installed so that the data exists (as in the case of the source_provider) and what modules needs to be installed so the data can be saved. Then we can rename the source_provider key to source_module and destination_provider key to destination_module. These names are already in use in the logic, and no one objected, so is likely agreeable to all.

heddn’s picture

So, I've looked at this a while, trying to figure it out. A few things play into answering what is the "plugin, core or component". Is it the destination, which is a plugin. Is it the source, which is also a plugin, or is it the process, which is also a plugin. Or lastly, is it the entire thing which wraps everything together (migrate plugin). Because if it is the source or destination, those /are/ owned by the dblog and user modules of the world. If it is the process plugin, that's just a pass through and so let's just ignore that argument. Is it the whole thing "migrate container plugin"? Maybe. It isn't cut and dry. Over in #2711099: Categorize migrations according to their type, we are reaching into the destination to pull out the category. So the destination has some place in directing the identity of the migration. Should we argue the same here?

Who is the user of this provider data? The user is the UI's of the world. Primarily speaking, migrate_drupal_ui. If we don't use provider, we'd be stuck with coming up with another name for /arguably/ the same thing. So, given this isn't an easy problem to identify the provider, let's not bikeshed on what function should return the data for migrate_drupal_ui. If it needs to change, can we /please/ punt that to a follow-up? We're already at 175+ comments on an 80K+ patch that is a pain to re-roll. Let's just commit this thing. Please.

  /**
   * Gets the plugin provider.
   *
   * The provider is the name of the module that provides the plugin, or "core',
   * or "component".
   *
   * @return string
   *   The provider.
   */
  public function getProvider();
+++ b/core/modules/migrate/src/Plugin/migrate/destination/Config.php
@@ -149,4 +149,15 @@ public function rollback(array $destination_identifier) {
+  /**
+   * {@inheritdoc}
+   */
+  public function getProvider() {
+    $destination_module = parent::getProvider();
+    // The migrate modules are not valid destination modules.
+    if (in_array($destination_module, ['migrate', 'migrate_drupal', 'migrate_drupal_ui'], TRUE)) {
+      $destination_module = !empty($this->configuration['config_name']) ? explode('.', $this->configuration['config_name'], 2)[0] : NULL;
+    }
+    return $destination_module;
+  }
 }
phenaproxima’s picture

I +1 @quietone's approach in #173 so we can finally, finally, FINALLY slam this stupid bike shed shut forever. Please, let's just do this. source_module and destination_module are fine by me. They make sense.

A source plugin that retrieves theme settings from D6/7 will almost certainly be pulling data from the variables table. That comes from the System module. Therefore its source_module will be system, and that's that. Nice and simple.

For Cthulhu's sake, let's finish this. I believe I speak for my fellow maintainers when I say that as thoroughly sick of this issue as I am, I'm equally sick of the Array of Doom that only exists to provide information to the UI.

quietone’s picture

Assigned: Unassigned » quietone

Have mostly completed the conversion to source_module and destination_module. But have hit a snag with d6_field_instance and d7_field_instance. When I change those ProvidersExistTest reports. Anyone know what is causing this?

1) Drupal\Tests\migrate\Kernel\Plugin\ProvidersExistTest::testProvidersExist
Drupal\Core\Database\DatabaseExceptionWrapper: SQLSTATE[42S02]: Base table or view not found: 1146 Table 'test.test556954410field_config_instance' doesn't exist: SELECT fci.*, fc.type AS type, fc.data AS field_data
FROM
{field_config_instance} fci
INNER JOIN {field_config} fc ON fci.field_id = fc.id
WHERE (fci.deleted = :db_condition_placeholder_0) AND (fc.active = :db_condition_placeholder_1) AND (fc.deleted = :db_condition_placeholder_2) AND (fc.storage_active = :db_condition_placeholder_3); Array
(
joelpittet’s picture

@quietone, just a stab but field_config_instance table doesn't exist in D6 only in D7, could that be it?

quietone’s picture

Status: Needs work » Needs review
FileSize
114.75 KB
81.24 KB

This is not yet finished. I'm uploading a patch so see what testbot finds. Locally, MigrateUpgrate[6-7]Test doesn't complete and I've get a mysql has gone away message.

Status: Needs review » Needs work

The last submitted patch, 178: 2569805-178.patch, failed testing. View results

Jo Fitzgerald’s picture

Status: Needs work » Needs review
FileSize
135.42 KB

The patch in #178 did not apply so re-rolled.

quietone’s picture

@Jo Fitzgerald, thx. I wasn't working from HEAD.

Jo Fitzgerald’s picture

@quietone It seemed like you were making good progress so I wanted to help keep the momentum going (and I'm curious to see what the testbot makes of it).

Status: Needs review » Needs work

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

quietone’s picture

Assigned: quietone » Unassigned
Status: Needs work » Needs review
FileSize
110.65 KB
26.48 KB

Hopefully, this pass tests.

However, the method name for getting the source or destination module is still getProvider(). That should probably be changed to avoid confusion with the plugin provider. Any suggestions? I've not come up with any thing that seems remotely useful.

Status: Needs review » Needs work

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

quietone’s picture

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

MigrationProviderInterface was omitted from the patch. Restored so lets try again,

quietone’s picture

Changed getProviders to getMigrationProviders to help indicate that these providers are specific to migration and have a different meaning that the plugin provider.

Status: Needs review » Needs work

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

maxocub’s picture

+++ b/core/modules/migrate_drupal_ui/src/Form/MigrateUpgradeForm.php
@@ -1055,19 +397,34 @@ public function buildConfirmForm(array $form, FormStateInterface $form_state) {
+        $source_module = $source_plugin->getProvider();
...
+        $destination_module = $destination_plugin->getProvider();

You just forgot to change those two getProvider(), and this will get back to green.

Jo Fitzgerald’s picture

Status: Needs work » Needs review
FileSize
1.41 KB
114.91 KB

Corrections highlighted by @maxocub in #189.

quietone’s picture

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

Thx guys. A couple of things. The first was the patch didn't apply for me. I fixed that and tested by going to /upgrade. I notice that image, field, file, profile and shortcut are not found. That needs to be addressed. And I also wonder if there is a way we can check that a source or destination is not being overlooked?

And I'm thinking the logic for determining the migration provider should change to be more explicit. Like, If the migration provider exists in the migration configuration use that, then if it exists in the plugin definition use that, and if not in either of those places use the plugin provider. And keep the existing special case for the config destination.

Assigning to myself since I've got a working patch for that, I just need to run a few more tests tonight.

quietone’s picture

Status: Needs work » Needs review
FileSize
115.12 KB

This is just a reroll.
Testing at /upgrade results in a failure on d7_file_private. "Source module not found for d7_file_private." . I didn't correct that because I want to see if the testbot detects the errors in MigrateUpgrade7Test.php. A test which takes a long time to run, so I'm letting the testbot do it for me.

Status: Needs review » Needs work

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

quietone’s picture

Status: Needs work » Needs review
FileSize
115.21 KB
794 bytes

As expected it failed on the new tests MigrationProvidersExistTest. But I think it should have also failed in MigrateUpgrade7Test as well. So I have added a test there checking if the text 'module not found' is detected. Let's see if that fails this time.

Note I tried to run it locally this time but I just get the 'MySQL server has gone away' msg on both MigrateUpgrade6Test and MigrateUpgrade7Test.

Status: Needs review » Needs work

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

quietone’s picture

Status: Needs work » Needs review
FileSize
121.22 KB
7.42 KB

Turns out the test module were accidentally removed in patch 178. Yes, that was me and here I am cleaning up after myself.

Another factor is that modules/migrate_drupal_ui/src/Tests/MigrateUpgradeTestBase.php has been deprecated, replaced by modules/migrate_drupal_ui/tests/src/Functional/MigrateUpgradeTestBase.php and the test code wasn't moved there.

So, this patch should tidy all that up. A few assertions have been added to make sure the test module is actually installed and to check for any 'module not found' errors. Now, hopefully, the error on d7_private_file will be found in MigrationProvidersExistTest and MigrateUpgrade7Test.

Status: Needs review » Needs work

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

quietone’s picture

Status: Needs work » Needs review
FileSize
121.86 KB
515 bytes

Now add source_module to d7_private_file.yml.

quietone’s picture

Reviewed all usages of source_module and found it was not used consistently, sometimes is was in the migration yml and sometimes in the source plugin. Here, the preference is to assign source_module in the source plugin, not the migration. There are a couple of exceptions, such as the variable and embedded source plugin where the source_module must be declared in the migration.

Still need to do the same for the destination_module.

quietone’s picture

This has a few changes to the source_module declarations. An annotation for destination_module has been added. The destination provider 'core' has been removed in favor of obtaining the destination_module from, firstly, the plugin configuration, then the plugin definition. And if not found in either of those, from the migration provider. Config destinations are a bit different. For config destinations, if the destination_module is not in the plugin configuration or the plugin definition, it is then checked if this is a translation destination. If it is, the destination_module is set to 'config_translation'. And if not a translation, then the destination_module is taken from the config_name in the configuration.

Status: Needs review » Needs work

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

quietone’s picture

Assigned: quietone » Unassigned
Status: Needs work » Needs review
FileSize
115.5 KB
426 bytes

Fix the test.

neclimdul’s picture

This freaking awesome work. I want to dig into the part where we're hooking into provider still a little bit but seriously awesome work breaking out the explicit annotations.

quietone’s picture

Fixing the coding standard errors.

neclimdul’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/migrate/src/Plugin/migrate/destination/Config.php
    @@ -195,4 +195,20 @@ public function rollback(array $destination_identifier) {
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public function getMigrationProvider() {
    +    if (!empty($this->configuration['destination_module'])) {
    +      return $this->configuration['destination_module'];
    +    }
    +    if (!empty($this->pluginDefinition['destination_module'])) {
    +      return $this->pluginDefinition['destination_module'];
    +    }
    +    if (isset($this->configuration['translations'])) {
    +      return 'config_translation';
    +    }
    +    return !empty($this->configuration['config_name']) ? explode('.', $this->configuration['config_name'], 2)[0] : NULL;
    +  }
    +
    

    Could we document why Config has a different implementation. The code is different but I couldn't figure out why.

  2. +++ b/core/modules/migrate/tests/src/Kernel/Plugin/MigrationProvidersExistTest.php
    @@ -0,0 +1,73 @@
    +      $source_plugin = $migration->getSourcePlugin();
    +      if ($source_plugin instanceof MigrationProviderInterface) {
    +        $source_module = $source_plugin->getMigrationProvider();
    +      }
    +      $destination_plugin = $migration->getDestinationPlugin();
    +      if ($destination_plugin instanceof MigrationProviderInterface) {
    +        $destination_module = $destination_plugin->getMigrationProvider();
    +      }
    

    So, I'm not sure if the MigrationProviderInterface makes sense but maybe it does so lets talk it through. These aren't providers anymore or even really connected so the name doesn't help us. Could we just have "getSourceModule" and "getDestinationModule" and call it a day? What does the generalization and sharing it between the plugin types buy us?

quietone’s picture

Status: Needs work » Needs review
FileSize
115.88 KB
1.03 KB

One step at a time here. Adding comments in response to #1 above.

quietone’s picture

And now for #2. I probably did this a bit quickly but I've got some other stuff to attend to now.

Personally, this is clearer about what is going on. But I'd like the others to chime in on your questions in #2.

So, MigrationProviderInterface has been removed. And getMigrationProvider is split to getSourceModule, in MigrateSourceInterface.php, and getDestinationModule, in MigrateDestinationInterface.php.

neclimdul’s picture

+++ b/core/modules/migrate/tests/src/Kernel/Plugin/MigrationProvidersExistTest.php
@@ -41,16 +40,8 @@
+      $source_module = $migration->getSourcePlugin()->getSourceModule();
+      $destination_module = $migration->getDestinationPlugin()->getDestinationModule();

This reads so much better!

I'm happy with it but there's a lot of changes so going to let someone else take a look.

heddn’s picture

Status: Needs review » Needs work

Super nice work here. Hopefully my feedback is easy to address.

  1. +++ b/core/modules/migrate/src/Annotation/MigrateDestination.php
    @@ -43,4 +43,16 @@ class MigrateDestination extends Plugin {
    +   * This can be any type, and the destination plugin itself determines how the
    

    Type or value? The word 'Type' is overloaded. Maybe "object type" is better. I'll discuss this type more later in my review so hold on.

  2. +++ b/core/modules/migrate/src/Annotation/MigrateDestination.php
    @@ -43,4 +43,16 @@ class MigrateDestination extends Plugin {
    +   * @var mixed
    

    This can be anything? String, class, array?

  3. +++ b/core/modules/migrate/src/Annotation/MigrateSource.php
    @@ -44,15 +44,15 @@ class MigrateSource extends Plugin implements MultipleProviderAnnotationInterfac
        * This can be any type, and the source plugin itself determines how the value
    

    Same nit here. Any object type seems better.

  4. +++ b/core/modules/migrate/src/Annotation/MigrateSource.php
    @@ -44,15 +44,15 @@ class MigrateSource extends Plugin implements MultipleProviderAnnotationInterfac
    -   * source_provider to be the name of a module that must be installed and
    

    perhaps 'source_provider to be the string name of a module that...'.

  5. +++ b/core/modules/migrate/src/Annotation/MigrateSource.php
    @@ -60,7 +60,7 @@ class MigrateSource extends Plugin implements MultipleProviderAnnotationInterfac
        * This can be any type, and the source plugin itself determines how it is
    

    Again, any object type.

  6. +++ b/core/modules/migrate/src/Plugin/MigrateDestinationInterface.php
    @@ -133,4 +133,11 @@ public function supportsRollback();
    +   * @return string|null
    +   *   The destination module or NULL if not found.
    +   */
    +  public function getDestinationModule();
    
    +++ b/core/modules/migrate/src/Plugin/MigrateSourceInterface.php
    @@ -100,4 +100,12 @@ public function __toString();
    +   * @return string|null
    +   *   The source module or NULL if not found.
    +   */
    +  public function getSourceModule();
    
    +++ b/core/modules/migrate/src/Plugin/migrate/destination/Config.php
    @@ -195,4 +195,26 @@ public function rollback(array $destination_identifier) {
    +    if (!empty($this->configuration['destination_module'])) {
    ...
    +    if (!empty($this->pluginDefinition['destination_module'])) {
    
    +++ b/core/modules/migrate/src/Plugin/migrate/destination/DestinationBase.php
    @@ -110,4 +110,22 @@ protected function setRollbackAction(array $id_map, $update_action = MigrateIdMa
    +    if (!empty($this->configuration['destination_module'])) {
    
    +++ b/core/modules/migrate/src/Plugin/migrate/source/SourcePluginBase.php
    @@ -541,4 +541,17 @@ public function postRollback(MigrateRollbackEvent $event) {
    +    if (!empty($this->configuration['source_module'])) {
    +      return $this->configuration['source_module'];
    ...
    +    elseif (!empty($this->pluginDefinition['source_module'])) {
    +      return $this->pluginDefinition['source_module'];
    

    Here's my promised further notes on 'type'. In MigrateDestinationInterface and MigrateSource we state that this can be any object type. But if we say that the value in 'destination_module' can be any object type, but then assume that it is a string everywhere, that seems really strange. We either need to require it to be a string in the doxygen or comment somehow that we assume a string value.

  7. +++ b/core/modules/migrate/src/Plugin/migrate/destination/Config.php
    @@ -195,4 +195,26 @@ public function rollback(array $destination_identifier) {
    +    if (isset($this->configuration['translations'])) {
    +      return 'config_translation';
    +    }
    

    Hmm, ok. I don't like this one off logic here, but cannot think of a better option.

quietone’s picture

Status: Needs work » Needs review
FileSize
114.09 KB
1.91 KB

@neclimdul, Thanks for the timely response.

@heddn, changed source_module and destination_module to be string, and adjusted the docs.

However, having a single source and destination module is not quite right. There were times I wanted it to be an array. For example, translations require more than one module to be enabled on the source destination. But do we need the extra accuracy? We could open a followup to decide on that.

heddn’s picture

Status: Needs review » Reviewed & tested by the community

If we want to list an array of source modules, we can do that later, no? Migrate Drupal is still in experimental, right? Yes it is, but the annotations are in migrate, which is in beta. Can we punt to later or do we need to decide that now? I think we can punt because there wouldn't need to be an API changes for it to work.

LGTM.