Show field type migrations correctly in Migrate Drupal UI

I think a lot of this could/would be fixed in #2905227: Migrate UI: Improve 'Review Upgrade' page UX. Shall we repurpose this to fix some of the errors where we are obviously migrating things like link and date and telephone? Now that the static mapping array is gone, we should fix this up to show that some of these obvious things *are* getting migrated.

Reviewed this in our weekly migrate call and are upgrading this to a migrate critical.

Coming from #2905741: *DRAFT* Proposed product goals for Drupal 8.5/8.6(+) core

- field_collection => paragraphs, paths exist with a custom migration
- addressfield => address, paths exist with a custom migration
- phone => in core and fixable with this issue
- name => ???
- entity =>in core and fixable with this issue
- book => in core and fixable with this issue
- entityreference => in core and fixable with this issue
- node_reference ( (=> entityreference) => in core and fixable with this issue
- nodequeue (=> entityqueue) => ???
- statistics => in core and fixable with this issue
- uuid => parts in core, what do we need to migrate???
- xbbcode => ???
- xmlsitemap => xmlsitemap, paths exist with a custom migration

heddn, thanks for the summary. New issue is #2906878: [Meta] Support for D7 -> D9 contrib migrate

- "name" is https://www.drupal.org/project/name and there is a D8 port, beta1, though from Jul 2016.
- xbbcode is an upgrade/replacement for the earlier bbcode filter module. Mostly the need is to port the config so that the site content is filtered properly.

could you elaborate on 'with a custom migration'?

custom migration: https://www.drupal.org/project/migrate_tools and https://www.drupal.org/project/migrate_upgrade where you export the config, add in your own custom process plugins and run the migration using drush.

I think we need to add to the field plugins an annotation that mentions what is the source module, then loop over the fields and add those field modules to the list of enabled modules

Interesting idea. I need to think about that a bit. There is still a need to resolve modules like 'ctools' and 'field_ui' and 'field_sql_storage'. Which can be done in a static array.

This is rough, it is exploring the idea of adding annotations to the field plugins in #11. In the screen shot, link is not enabled on the destination, so it is correctly listed as 'Missing'. So, it looks good. However, 'Options' and 'Numbers' should probably not be listed as missing. That requires more work.

I also changed the source module for d7_field to field_sql_storage.

Set to NR for the testbot.

Hopefully, fixing the errors. Just typos in two files.

A bit of cleanup. Inject the migrate field plugin manager interface and the module handler. Add the field migrations to the table_data array and remove unused code.

Some stray stuff seems to be in the last few patches.

The patch wouldn't apply with the stray setting tray and rest changes. Here's a re-roll without it.

Here's a different approach, since the previous one had issues with finding all the field types. I manually ran a d6 and d7 migration through the UI and caught some missing/incorrect mappings. Next we need to do some asserts on the review page to confirm we have all the right things mapped correctly. That should be a fairly good novice test task, so tagging. Hint, add the asserts to MigrateUpgradeTestBase around line ~150.

And here's the patches that were missing from #20.

+++ b/core/modules/migrate_drupal/src/Annotation/MigrateField.php
@@ -51,4 +51,26 @@ public function __construct($values) {
+  public $source_module;
...
+  public $destination_module;

+++ b/core/modules/migrate_drupal_ui/src/Form/MigrateUpgradeForm.php
@@ -459,6 +484,17 @@ public function buildConfirmForm(array $form, FormStateInterface $form_state) {
+      $source_module = $definition['source_module'];
+      $destination_module = $definition['destination_module'];

Any chance we can trigger an errror or throw an exception if these are missing from the field plugin? I'd like to throw an exception, but that could break BC. So trigger_warning is probably a better option.

@heddn, sorry for the extraneous stuff in the patch. It is 'interesting' getting time to work on an issue while on holiday.

Any chance we can trigger an errror or throw an exception if these are missing from the field plugin?

We can copy the existing method used for informing the user, which is a drupal_set_message.

      if (!$source_module) {
        drupal_set_message($this->t('Source module not found for @migration_id.', ['@migration_id' => $migration_id]), 'error');
      }
      $destination_module = $migration->getDestinationPlugin()->getDestinationModule();
      if (!$destination_module) {
        drupal_set_message($this->t('Destination module not found for @migration_id.', ['@migration_id' => $migration_id]), 'error');
      }

I kinda prefer trigger_error, personally. It seems more php-ish and a little louder and would get caught in contrib module testing.

This should fix the errors.

Still to do:

1) If we are adding 'providers' to the field plugins, then we need to test that they exist. That can be done in MigrationProvidersExistTest.php. This is a new item, unless someone thinks this should not be done.

2) Add assertions for the output on the review page to confirm the modules are correctly listed in either the available or the missing migrations. This is from #20.

3) Use trigger_error if the migration or the field plugin does not have a source_module and destination_module. This is from #22.

For the question about trigger_error vs throw exception, I've opened #2908282: Throw exception for source plugins without a source_module property. And moving back to NW for tests. And tagging for a CR to mention the new requirement for field plugins.

Totally agree with the new issue. Thanks

I've picked up 27.3 here. And it is an exception per #2908282: Throw exception for source plugins without a source_module property.

'id' is caught in the parent. No need to duplicate it in the child.

So, cckfield plugin manager was extending field plugin manager. I've split that out so it extends directly from the base migrate plugin manager and we don't require source/destination then any more for legacy cck field plugins. There's bound to be a few failures/missing source/destinations. Adding this should be novice.

Added to D6FileField the same source_module and destination_module as D6NoCoreVersionSpecified.

Back to NW for #27.

Can we "group them" and just say 27 missing modules, and then the user can opt into looking at them?

@Bojhan, the work to redesign the review page is being done in #2905227: Migrate UI: Improve 'Review Upgrade' page UX.

Per discussion with @catch, we're promoting the "Migrate critical" issues to critical given the importance of a stable migration path at this point in the release cycle. (The framework and release managers will confirm this in a later triage for individual issues.) Thanks @quietone and all!

I made basically the same remarks as #41 but @Gábor Hojtsy clarified that this issue will just be removing irrelevant/incorrect messages entirely while the redesign happens in

Missed actually promoting it in #44.

Rerolled the patch because ti did not apply.

Postponing on #2908282: Throw exception for source plugins without a source_module property because both are changing the destination_module annotations.

They are changing the destination and source modules, but they are doing it on different things. This is doing it on field plugins and the other issue is working on source, destination and process plugins. Un-postponing.

Tagging for sprinting.

I've lost track - what further work does this need? What still needs tests (or is that tag out of date)?

Working on what is needed. Patch coming.

@Jo Fitzgerald, good question.

From #27 till to do:

1) If we are adding 'providers' to the field plugins, then we need to test that they exist. That can be done in MigrationProvidersExistTest.php. This is a new item, unless someone thinks this should not be done.

2) Add assertions for the output on the review page to confirm the modules are correctly listed in either the available or the missing migrations. This is from #20.

3) Use trigger_error if the migration or the field plugin does not have a source_module and destination_module. This is from #22.

heddn, worked on #3 but I am not yet sure if it has been fixed. Just wanted to answer your question.

But it looks like heddn is on it

Remaining task:

Add assertions for the output on the review page to confirm the modules are correctly listed in either the available or the missing migrations. This is from #20.

This picks up test coverage, mainly.

+++ b/core/modules/file/src/Plugin/migrate/field/d6/FileField.php
--- a/core/modules/file/src/Plugin/migrate/field/d6/FileField.php
+++ b/core/modules/file/src/Plugin/migrate/field/d6/ImageField.php

+++ b/core/modules/file/src/Plugin/migrate/field/d6/ImageField.php
@@ -8,11 +8,13 @@
- *   id = "filefield",
- *   core = {6}
+ *   id = "imagefield",
...
-class FileField extends FieldPluginBase {
+class ImageField extends FieldPluginBase {

This seems out of place. How did this get into the patch?

Also, after looking at what we are doing in #2908282-41: Throw exception for source plugins without a source_module property, do we need to do similarly for field plugins and assume the provider is the destination module if no annotation is provided?

@heddn, The change to ImageField was made in #21. Your comment about it in the previous comment you "manually ran a d6 and d7 migration through the UI and caught some missing/incorrect mappings".

The assertion are covering the fields. Should we have another issue for covering the others instances of source_module and destination_module in the same way, that is, checking the values, not existance?

Found a spacing issue.

+++ b/core/modules/migrate/tests/src/Kernel/Plugin/MigrationProvidersExistTest.php
@@ -61,4 +63,180 @@ public function testProvidersExist() {
+          'source_module' => 'text',
+          'destination_module' => 'text',

Out of alignment.

I tried to test the review page output but wasn't able to. We can't just assertText (which I now) because the output is a big table and we need to know what column it is in. Can someone provide some direction?

I have a D7 site which I tried to upgrade on a current 8.4.x D8 with patch #52 installed. The attached text file indicates the "errors" I got as a result.

I have passed a copy of the D7 SQL to @quietone as requested so that she is able to check further but this DB is not publishable on the net.

D8 site prepared with:

$ drush si -y
$ drush en -y responsive_image migrate migrate_drupal migrate_drupal_ui telephone aggregator book tracker
$ composer install
$ composer require "drupal/address ~1.0"
$ drush dl name ctools paragraphs filefield_sources mollom chosen number 
$ drush en -y address name ctools paragraphs filefield_sources xbbcode xmlsitemap pathauto

Ran a test with the database given to me in #58 using the following steps.

1. Worked from 8.4.x HEAD
2. Made a clean install of D8
3. Installed patch in #52
4. Installed modules listed in #58 with composer, enabled modules.
5. Navigated to /upgrade

The results:

• A screen full of 'source module not found' error messages
• 4 undefined index errors
• No available paths.

Not sure what is causing the errors yet, but seems like a suitable problem for the 30 hour journey home. Therefore assigning to myself.

Note that with the D7 test fixture, the results are correct.

@quietone thanks :)

The next time I booted up (after several hours of traveling) and navigated to /upgrade, the errors did not appear. I recall that I did a 'drush cr' and had navigated to /upgrade and entered the details for the D7 test fixture, which didn't produce errors, before shutting down. Admittedly it is all a bit fuzzy due to the demands of travel. So, right now, it seems that a cache clear is needed. But I have not retested that.

But I did update the patch.

Changed how source_module and destination_module are obtained in the migrate_drupal_ui form. Before, the destination site was searched for all available field types, then the field plugin was obtained by the type. This is wrong because the input parameter to getPluginIdFromFieldType is the field_type from the source database, not the destination.

Added two new methods MigrateFieldPluginManager::getSourceModule() and MigrateFieldPluginManager::getDestinationModule().

There are 3 todo items in #52. If those #1 is done. I think #2, testing the output of the review page itself, isn't necessary. The data to be displayed is tested, there hasn't been a response to how to test that output table, and that page is being redesigned in #2905227: Migrate UI: Improve 'Review Upgrade' page UX.

That leaves #52.3 todo.

3) Use trigger_error if the migration or the field plugin does not have a source_module and destination_module. This is from #22.

Need to add source_module and destination_module on the cck field plugins.

Missed a few cck field plugins.

Assigning for review.

Did some code cleanup and added a draft change record.

#52.3 is done. We throw an exception during plugin discovery if any plugin is missing the annotation.

I like the idea of removing those confusing missing field migration paths.

1. +++ b/core/modules/field/src/Plugin/migrate/source/d7/Field.php
   @@ -15,7 +15,7 @@
     * @MigrateSource(
     *   id = "d7_field",
   - *   source_module = "field"
   + *   source_module = "field_sql_storage"
     * )
   

   Why is there no destination_module here? Are the tests suficient if this didn't make them fail? Edited: Oh, this is not a field plugin, but a source plugin.

2. +++ b/core/modules/file/src/Plugin/migrate/cckfield/d7/FileField.php
   @@ -11,7 +11,11 @@
   + *   source_module = "file",
   + *   destination_module = "file",
   + *   source_module = "text",
   + *   destination_module = "text",
   

   Duplicate source_module & destination_module

3. +++ b/core/modules/file/src/Plugin/migrate/field/d6/FileField.php
   similarity index 88%
   copy from core/modules/file/src/Plugin/migrate/field/d6/FileField.php
   
   copy from core/modules/file/src/Plugin/migrate/field/d6/FileField.php
   copy to core/modules/file/src/Plugin/migrate/field/d6/ImageField.php
   

   Why was this file copied? The two files looks almost identical. Can one of them extends the other?

4. +++ b/core/modules/link/src/Plugin/migrate/field/d6/LinkField.php
   @@ -10,8 +10,10 @@
   - *     "link_field" = "link"
   ...
   + *     "link_field" = "link",
   

   Nit: unecessary comma addition

5. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/field/NodeReference.php
   @@ -11,6 +11,8 @@
   +*    source_module = "node_reference",
   + *   destination_module = "entity_reference",
   

   White space typo

6. +++ b/core/modules/migrate_drupal/tests/modules/migrate_field_plugin_manager_test/src/Plugin/migrate/field/D6NoCoreVersionSpecified.php
   @@ -6,10 +6,9 @@
   +class D6NoCoreVersionSpecified extends FieldPluginBase { }
   

   Nit: unecessay space between curly brackets

7. +++ b/core/modules/migrate_drupal_ui/src/Form/MigrateUpgradeForm.php
   @@ -49,6 +53,20 @@ class MigrateUpgradeForm extends ConfirmFormBase {
   +   * The field migrate plugin manager.
   
   @@ -59,12 +77,18 @@ class MigrateUpgradeForm extends ConfirmFormBase {
   +   *   The field plugin manager.
   

   Could we make those two descriptions match?

8. +++ b/core/modules/migrate_drupal_ui/src/Form/MigrateUpgradeForm.php
   @@ -59,12 +77,18 @@ class MigrateUpgradeForm extends ConfirmFormBase {
   +   * @param \Drupal\Core\Extension\ModuleHandlerInterface $module_handler
   +   *   The ID map migration plugin manager.
   

   Copy/paste error

9. +++ b/core/modules/migrate_drupal_ui/src/Form/MigrateUpgradeForm.php
   @@ -474,6 +500,54 @@ public function buildConfirmForm(array $form, FormStateInterface $form_state) {
   +        'plugin' => 'd' . "$version" . '_field',
   

   unecessary quotes around $version

10. +++ b/core/modules/migrate_drupal/src/Annotation/MigrateField.php
    @@ -51,4 +51,26 @@ public function __construct($values) {
       public $core = [];
    
    +++ b/core/modules/migrate_drupal/src/Plugin/MigrateFieldPluginManager.php
    @@ -53,4 +54,31 @@ public function getPluginIdFromFieldType($field_type, array $configuration = [],
    +    foreach (['core', 'source_module', 'destination_module'] as $required_property) {
    

    If the 'core' property is required, should we remove the empty array as the default value of this property?

11. +++ b/core/modules/migrate_drupal/src/Plugin/MigrateFieldPluginManager.php
    @@ -53,4 +54,31 @@ public function getPluginIdFromFieldType($field_type, array $configuration = [],
    + /**
    

    Indent typo

12. +++ b/core/modules/migrate_drupal/src/Plugin/MigrateCckFieldPluginManager.php
    @@ -14,4 +17,42 @@
    +class MigrateCckFieldPluginManager extends MigrateFieldPluginManager implements MigrateCckFieldPluginManagerInterface {
    +
    +  /**
    +   * The default version of core to use for cck field plugins.
    +   *
    +   * These plugins were initially only built and used for Drupal 6 cck fields.
    +   * Having been extended for Drupal 7 with a "core" annotation, we fall back to
    +   * Drupal 6 where none exists.
    +   */
    +  const DEFAULT_CORE_VERSION = 6;
    +
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public function getPluginIdFromFieldType($field_type, array $configuration = [], MigrationInterface $migration = NULL) {
    +    $core = static::DEFAULT_CORE_VERSION;
    +    if (!empty($configuration['core'])) {
    +      $core = $configuration['core'];
    +    }
    +    elseif (!empty($migration->getPluginDefinition()['migration_tags'])) {
    +      foreach ($migration->getPluginDefinition()['migration_tags'] as $tag) {
    +        if ($tag == 'Drupal 7') {
    +          $core = 7;
    +        }
    +      }
    +    }
    +
    +    $definitions = $this->getDefinitions();
    +    foreach ($definitions as $plugin_id => $definition) {
    +      if (in_array($core, $definition['core'])) {
    +        if (array_key_exists($field_type, $definition['type_map']) || $field_type === $plugin_id) {
    +          return $plugin_id;
    +        }
    +      }
    +    }
    +    throw new PluginNotFoundException($field_type);
    +  }
    +
    +}
    

    I don't understand why this is added, it's identical to the parent class.

This should fix all the items above. About #2, the file was copied way back in #21, it now extends from FileField as you suggested.

Edit: This only addresses #1-9 above.

+++ b/core/modules/migrate_drupal/src/Plugin/migrate/field/NodeReference.php
@@ -4,13 +4,15 @@
+/*3*

Stray character caused the test failure.

10. Reasonable. The default value has been removed. But the MigrateFieldPluginManager set a default of 6. Maybe the default should be 6 in the Annotation?
11. Fixed
12. Fixed

I tested this both with the Drupal 7 fixtures and with fresh empty Drupal 7 install.

1. With the fixtures: Options still shows up in the missing paths when it's clearly enabled on both the source and the destination. Any idea why?

2. With a fresh install: Options, Number and List still show up in the missing paths. Those modules are enabled on both the source and the destination.

3. Should this issue also deals with missing paths that will never have a path? Like Help, Overlay, Toolbar, etc.

Oups, removed the tag by mistake. I will try to see if and how it would be possible to assert text on the missing paths table on the review page.

@maxocub, thanks for the review and the testing

1. Don't know yet.
2. When I tested with a fresh install I only had 'options' as a missing path.
3. I'd rather have a follow up for that and keep this strictly or fields.

Setting to NW.

Since this issue is for the UI, I think we should add assertions about the missing/available paths in MigrateUpgradeTestBase. I found a way to do that with xpath:

Examples

• To check that 'options' is not in the missing paths:
  $this->assertSession()->elementNotExists('xpath', '//table[@id="edit-missing-module-list"]//td[text()="options"]');
• To check that 'node' is in the available paths:
  $this->assertSession()->elementExists('xpath', '//table[@id="edit-available-module-list-module-list"]//td[text()="node"]');

Cross post, back to NW.

Re #75: I agree with 3.

73.1.Options wasn't found because only those field plugins that matched a type in the source were looked at. And since options isn't a field type it wasn't found. So, ditched the code getting the field plugin source and destination module information and started over. This version gets the information from the plugin in much the same way as is done for the migration and adds it to the big array. Much simpler too. This fixes the problem with options still showing up, it no longer does.
73.2. Haven't retested on a fresh install.

@maxocub, awesome that you found a way to test with xpath. I never even thought of that. I might get time to try later tonight when the little one is aslepp.

Todo:
Make a followup for missing paths that will never have a path? Like Help, Overlay, Toolbar, etc.

Implemented the suggestion in #76 for testing the output. The arrays for available and missing need to be more thoroughly checked. And the tests for a missing path fails, and I don't know why.

This test fails:
$this->assertSession()->elementExists('xpath', '//table[@id="edit-missing-module-list"]//td[text()="' . $missing . '"]');

And these are probably false positives:
$this->assertSession()->elementNotExists('xpath', '//table[@id="edit-missing-module-list"]//td[text()="' . $available . '"]');

Made the follow up, #2914974: Migrate UI - handle sources that do not need an upgrade.

I like that we can now test all the missing/available paths!

I think the tests are failing at the first wrong assertion, so there were no false positive, only paths in the wrong array.

Take a look at this interdiff, I think it will make everything green.

Also, there's a few coding standard messages to be corrected.

@maxocub, thanks for showing me the error of my ways. MigrateUpgrade6Test passed locally so, hopefully, the edits to the D7 test are OK too. Not sure I got all the reported coding standard errors fixed though, what appears to be a link on the test result page isn't really a link.

Remove unused use statement.

Wrong patch. Try this one.

Update the IS.

1. +++ b/core/modules/file/src/Plugin/migrate/field/d6/ImageField.php
   @@ -0,0 +1,13 @@
   +<?php
   +
   +namespace Drupal\file\Plugin\migrate\field\d6;
   +
   +/**
   + * @MigrateField(
   + *   id = "imagefield",
   + *   core = {6},
   + *   source_module = "imagefield",
   + *   destination_module = "file"
   + * )
   + */
   +class ImageField extends FileField {}
   

   I don't know if this is necessary. Couldn't we simply implement an alter hook to create this plugin definition dynamically, rather than define an empty class whose only purpose is to carry an annotation?

2. +++ b/core/modules/migrate/tests/src/Kernel/Plugin/MigrationProvidersExistTest.php
   @@ -61,4 +63,188 @@ public function testProvidersExist() {
   +      unset($definitions[$key]);
   +    }
   +    // Make sure we test all the fields.
   +    $this->assertEmpty($definitions);
   

   I don't see why this is necessary. The foreach is looping over $definitions, which means every item in there will be dealt with.

3. +++ b/core/modules/migrate/tests/src/Kernel/Plugin/MigrationProvidersExistTest.php
   @@ -61,4 +63,188 @@ public function testProvidersExist() {
   +  public function testFieldProviderMissingRequiredProperty($definitions, $missing_property) {
   

   $definitions should be type hinted.

4. +++ b/core/modules/migrate/tests/src/Kernel/Plugin/MigrationProvidersExistTest.php
   @@ -61,4 +63,188 @@ public function testProvidersExist() {
   +    $this->setExpectedException(InvalidPluginDefinitionException::class, "The missing_{$missing_property} plugin should define the $missing_property property.");
   

   Rather than pass in a $missing_property value, can the data provider simply provide the full, expected exception message, and the test can assert that? It will be easier to understand.

5. +++ b/core/modules/migrate/tests/src/Kernel/Plugin/MigrationProvidersExistTest.php
   @@ -61,4 +63,188 @@ public function testProvidersExist() {
   +            'core' => [
   +              0 => 6,
   +              1 => 7,
   +            ],
   

   Nit: This should be 'core' => [6, 7]. The explicit numeric indices make it hard to read.

6. +++ b/core/modules/migrate/tests/src/Kernel/Plugin/MigrationProvidersExistTest.php
   @@ -61,4 +63,188 @@ public function testProvidersExist() {
   +            'core' => [
   +              0 => 6,
   +              1 => 7,
   +            ],
   

   Should be 'core' => [6, 7] for readability.

7. +++ b/core/modules/migrate_drupal/src/Annotation/MigrateField.php
   @@ -49,6 +49,28 @@ public function __construct($values) {
   -  public $core = [];
   +  public $core;
   

   Why is this no longer defaulted to an array?

8. +++ b/core/modules/migrate_drupal/src/Annotation/MigrateField.php
   @@ -49,6 +49,28 @@ public function __construct($values) {
   +   * The field plugin itself determines how the value is used. For example,
   +   * Migrate Drupal's field plugins expect source_module to be the name of a
   +   * module that must be installed and enabled in the source database.
   

   This doesn't seem accurate. The field plugin is not determining how the value is used. This annotation is a Migrate Drupal-specific annotation, and $source_module (and $destination_module) should always refer to Drupal modules. So can we fix both doc comments to reflect that?

9. +++ b/core/modules/migrate_drupal/src/Plugin/MigrateFieldPluginManager.php
   @@ -53,4 +54,31 @@ public function getPluginIdFromFieldType($field_type, array $configuration = [],
   +  public function processDefinition(&$definition, $plugin_id) {
   +    parent::processDefinition($definition, $plugin_id);
   +
   +    foreach (['core', 'source_module', 'destination_module'] as $required_property) {
   +      if (empty($definition[$required_property])) {
   +        throw new InvalidPluginDefinitionException($plugin_id, sprintf('The %s plugin should define the %s property.', $definition['id'], $required_property));
   +      }
   +    }
   +  }
   

   Doesn't this step on the toes of #2908282: Throw exception for source plugins without a source_module property?

10. +++ b/core/modules/migrate_drupal/src/Plugin/MigrateFieldPluginManager.php
    @@ -53,4 +54,31 @@ public function getPluginIdFromFieldType($field_type, array $configuration = [],
    +    return isset($definition['source_module']) ? $definition['source_module'] : '';
    

    processDefinition() throws an exception if source_module and destination_module are not defined in the plugin definition. Therefore, we can assume that $definition['source_module'] and $definition['destination_module'] are always set. So I'm not sure this method, or getDestinationModule(), is really needed.

11. +++ b/core/modules/migrate_drupal_ui/src/Form/MigrateUpgradeForm.php
    @@ -474,6 +504,23 @@ public function buildConfirmForm(array $form, FormStateInterface $form_state) {
    +      if (!$source_module) {
    

    The exception thrown in processDefinition() guarantees that we will never hit this condition.

12. +++ b/core/modules/migrate_drupal_ui/src/Form/MigrateUpgradeForm.php
    @@ -474,6 +504,23 @@ public function buildConfirmForm(array $form, FormStateInterface $form_state) {
    +      if (!$destination_module) {
    

    The exception thrown by processDefinition() guarantees that we will never hit this condition either.

13. +++ b/core/modules/migrate_drupal_ui/src/Form/MigrateUpgradeForm.php
    @@ -474,6 +504,23 @@ public function buildConfirmForm(array $form, FormStateInterface $form_state) {
    +      if ($source_module && $destination_module) {
    

    Therefore, this if check is not necessary.

14. +++ b/core/modules/migrate_drupal_ui/src/Form/MigrateUpgradeForm.php
    @@ -485,6 +532,11 @@ public function buildConfirmForm(array $form, FormStateInterface $form_state) {
    +    // Remove core profiles from the list.
    +    foreach (['standard', 'minimal'] as $profile) {
    +      unset($unmigrated_source_modules[$profile]);
    +    }
    

    Can this be explained a little more?

15. +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/MigrateUpgradeTestBase.php
    @@ -259,4 +274,20 @@ protected function translatePostValues(array $values) {
    +   * @return int[]
    +   *   An array of expected counts keyed by entity type ID.
    +   */
    +  abstract protected function getAvailablePaths();
    +
    +  /**
    +   * Gets the missing upgrade paths.
    +   *
    +   * @return int[]
    +   *   An array of expected counts keyed by entity type ID.
    +   */
    +  abstract protected function getMissingPaths();
    

    @return docs are inaccurate.

16. +++ b/core/modules/options/src/Plugin/migrate/field/d6/OptionWidgetsField.php
    @@ -0,0 +1,15 @@
    +<?php
    +
    +namespace Drupal\options\Plugin\migrate\field\d7;
    +
    +use Drupal\migrate_drupal\Plugin\migrate\field\FieldPluginBase;
    +
    +/**
    + * @MigrateField(
    + *   id = "optionwidgets",
    + *   core = {6},
    + *   source_module = "optionwidgets",
    + *   destination_module = "options"
    + * )
    + */
    +class OptionWidgetsField extends FieldPluginBase {}
    

    If possible, let's define this dynamically in an alter hook in the Options module, rather than create an empty class just to support an annotation.

17. +++ b/core/modules/options/src/Plugin/migrate/field/d7/OptionsField.php
    @@ -0,0 +1,15 @@
    +<?php
    +
    +namespace Drupal\options\Plugin\migrate\field\d7;
    +
    +use Drupal\migrate_drupal\Plugin\migrate\field\FieldPluginBase;
    +
    +/**
    + * @MigrateField(
    + *   id = "options",
    + *   core = {7},
    + *   source_module = "options",
    + *   destination_module = "options"
    + * )
    + */
    +class OptionsField extends FieldPluginBase {}
    

    Same here.

This issue is making it possible for modules providing fields to be accurately shown in the available or missing line. So changing the title

89.1, 16, 17. Sure we could use an alter hook but, I think, it makes it harder to find the annotation and the code to alter the hook is a bit harder to maintain than the explicit declaration in the annotation. Now, one way to find the field plugins is to just 'grep '@migratefield'. If this change were added one would also have to grep for all alter hooks and read them.

And there is precedence, maxocub pointed out on IRC that in #2901851: Replace the static map in the d7_field migration by field plugins classes are created that only provide the annotation for @MigrateField.

So, all together, doing this seems like extra work to find the field plugins. And I'd rather not.
2. Fixed.
3. Fixed.
4. Sure but, I think it would be harder to read. Why? Because I have to scan the whole string each time to see what the missing property is. As it is now, the data provider tells me exactly what property is being tested. And why copy/past the string 3 times when 1 version of it will do?
5. Fixed
6. Fixed
7. No longer default to array becuase it is now a required property (#69.10) But questioned if it should be set to 6. (#72.10).
8. Comments updated.
89.9. Possibly steps on the toes of #2908282: Throw exception for source plugins without a source_module property but that issue is at NW and the last comment states that it needs a rethink. Because of that let's keep this and the other issue can tidy up.
10. Removed the if statements. Kept getSourceModule and getDestinationModule, although I'm not convinced it is all that helpful.
11. Fixed
12. Fixed
13. Fixed
14. Not sure about this one. It was added by heddn in #20. Maybe he can comment.
15. Fixed
16. See #1 above
17. See #1 above

And some coding standard fixes.

FWIW, I 100% agree with @quietone in "89.1, 16, 17. Sure we could use..." as it seems far easier and less likely to break to have one single pattern: define a class deriving from X with an annotation -- than patterns using hooks as well.

Employ the "principle of least surprise"...

Reviewed in weekly migrate maintainers. Get rid of the getter on source/destination module. Adam will add some other comments.

Discussed in the Migrate maintainer call this week, and we agreed on a few things:

• getSourceModule() and getDestinationModule() are pointless, and we will remove them
• #89.7 is addressed. Because 'core' is now a required property that will result in an exception if it's not set, we do not need to set a default value.
• #89.14 is there for a legitimate reason. It would be preferable, though, to dynamically check for profiles. We can do that with system_get_module_data(), and checking the 'type' key from the parsed extension info.
• I concede that we can use annotations on empty classes. There is precedent for it already, and it is better to stick with one pattern. So we don't need to change any of that in the patch.

@quietone agreed to fix these issues, so I'm assigning this issue to her. Once these are done, I'm pretty sure we are either at RTBC, or close to it.

There are two things left to do
1) Remove getSourceModule() and getDestinationModule() from FiedPluginManager and Interface. Done.
2) Don't think this does what was asked. $system_data has already been manipulated such that the type, module and theme, are keys. I just moved the foreach loop to just after getting the system_data from the form_state.

Forgot to modify the ProvidersExist test file.

I'll review this in the next few days.

+++ b/core/modules/migrate_drupal_ui/src/Form/MigrateUpgradeForm.php
@@ -483,8 +522,12 @@ public function buildConfirmForm(array $form, FormStateInterface $form_state) {
+    // Remove core profiles from the system data.
+    foreach (['standard', 'minimal'] as $profile) {
+      unset($system_data[$profile]);
+    }

This doesn't seem to be working, I still see "standard" in the missing upgrade paths. I think there's a "module" sub-array in the $system_data array.

Also, we should try to do what @phenaproxima suggested in #94, that is to dynamicaly check if it's a profile by using system_get_info() (I have not found any function called system_get_module_data()) and check if the type is "profile".

'Standard' no longer appears.

As for the other point. I don't see the point of using system_get_info. Besides, how does one use that method on the source db? Anyway, please enlighten me.

Oh, this looks good. Definitely very close.

1. +++ b/core/modules/migrate/tests/src/Kernel/Plugin/MigrationProvidersExistTest.php
   @@ -55,10 +57,186 @@ public function testProvidersExist() {
   +      'taxonomy_term_reference' => [
   +        'source_module' => 'taxonomy',
   +        'destination_module' => 'entity_reference',
   

   The entity_reference module, as I understand it, is a shell module that will be gone in Drupal 9. Entity reference fields and functionality are completely merged into core. So I think the destination module could be 'core', no?

2. +++ b/core/modules/migrate/tests/src/Kernel/Plugin/MigrationProvidersExistTest.php
   @@ -55,10 +57,186 @@ public function testProvidersExist() {
   +    foreach ($definitions as $key => $definition) {
   +      $this->assertEquals($expected_mappings[$key]['source_module'], $definition['source_module']);
   +      $this->assertEquals($expected_mappings[$key]['destination_module'], $definition['destination_module']);
   +    }
   

   For easier debugging, maybe we should also add $this->assertArrayHasKey($key, $expected_mappings); in the foreach loop?

3. +++ b/core/modules/migrate/tests/src/Kernel/Plugin/MigrationProvidersExistTest.php
   @@ -55,10 +57,186 @@ public function testProvidersExist() {
   +    $mock_discovery = $this->getMockBuilder(MigrateFieldPluginManager::class)
   +      ->disableOriginalConstructor()
   +      ->setMethods(['getDefinitions'])
   +      ->getMock();
   +    $mock_discovery->method('getDefinitions')
   +      ->willReturn($definitions);
   +    $mock_plugin_manager = $this->getMockBuilder(MigrateFieldPluginManager::class)
   +      ->disableOriginalConstructor()
   +      ->setMethods(['getDiscovery'])
   +      ->getMock();
   +    $mock_plugin_manager->method('getDiscovery')
   +      ->willReturn($mock_discovery);
   

   Why do we need to mock both getDiscovery() and getDefinitions()? Surely all we need to do is mock getDefinitions()??

   If there is a reason we need to mock both, can we have a comment explaining why?

4. +++ b/core/modules/migrate/tests/src/Kernel/Plugin/MigrationProvidersExistTest.php
   @@ -55,10 +57,186 @@ public function testProvidersExist() {
   +  /**
   +   * Data provider for field plugin definitions.
   +   *
   +   * @return array
   +   */
   

   Nit: @return is missing a description.

5. +++ b/core/modules/migrate_drupal/src/Annotation/MigrateField.php
   @@ -49,6 +49,26 @@ public function __construct($values) {
   +   * The source_module is expected to be the name of a Drupal module that must
   +   * must be installed and enabled in the source database.
   

   Nit: "installed and enabled" is redundant. Can we just say "installed"?

6. +++ b/core/modules/migrate_drupal/src/Annotation/MigrateField.php
   @@ -49,6 +49,26 @@ public function __construct($values) {
   +   * The destination_module is expected to be the name of a Drupal module on the
   +   * destination site that must must be installed and enabled.
   

   Same here.

7. +++ b/core/modules/migrate_drupal/src/Plugin/MigrateFieldPluginManager.php
   @@ -53,4 +54,17 @@ public function getPluginIdFromFieldType($field_type, array $configuration = [],
   +        throw new InvalidPluginDefinitionException($plugin_id, sprintf('The %s plugin should define the %s property.', $definition['id'], $required_property));
   

   If we want to get fancy, we could create a new type of exception, extending InvalidPluginDefinitionException, which builds the message string in its constructor. That way we can throw these exceptions more easily: throw new BadPluginDefinitionException($plugin_id, $missing_property)

   That's just a suggestion, though. Definitely not a big deal!

8. +++ b/core/modules/migrate_drupal_ui/src/Form/MigrateUpgradeForm.php
   @@ -483,8 +522,12 @@ public function buildConfirmForm(array $form, FormStateInterface $form_state) {
   +    // Remove core profiles from the system data.
   +    foreach (['standard', 'minimal'] as $profile) {
   +      unset($system_data['module'][$profile]);
   +    }
   

   We should really open a follow-up (major, not critical) to allow dynamic discovery of profiles. Because as soon as a distro or custom profile enters the picture, this won't work.

9. +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/MigrateUpgradeTestBase.php
   @@ -178,6 +178,21 @@ public function testMigrateUpgrade() {
   +      $this->assertSession()->elementExists('xpath', '//table[@id="edit-available-module-list-module-list"]//td[text()="' . $available . '"]');
   +      $this->assertSession()->elementNotExists('xpath', '//table[@id="edit-missing-module-list"]//td[text()="' . $available . '"]');
   

   Let's call $this->assertSession() once and re-use the return value.

10. +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/MigrateUpgradeTestBase.php
    @@ -259,4 +274,20 @@ protected function translatePostValues(array $values) {
    +  /**
    +   * Gets the available upgrade paths.
    +   *
    +   * @return string[]
    +   *   And array of available upgrade paths.
    +   */
    +  abstract protected function getAvailablePaths();
    

    @return should be "An array...", not "And".

11. +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/MigrateUpgradeTestBase.php
    @@ -259,4 +274,20 @@ protected function translatePostValues(array $values) {
    +  /**
    +   * Gets the missing upgrade paths.
    +   *
    +   * @return string[]
    +   *   And array of missing upgrade paths.
    +   */
    +  abstract protected function getMissingPaths();
    

    Same here.

12. +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/d6/MigrateUpgrade6Test.php
    @@ -80,6 +80,72 @@ protected function getEntityCounts() {
    +   * {@inheritdoc}
    +   */
    +  public function getAvailablePaths() {
    

    The abstract method is protected, so this should be as well.

13. +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/d6/MigrateUpgrade6Test.php
    @@ -80,6 +80,72 @@ protected function getEntityCounts() {
    +
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public function getMissingPaths() {
    

    Same here.

14. +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/d7/MigrateUpgrade7Test.php
    @@ -84,6 +84,74 @@ protected function getEntityCounts() {
    +   * {@inheritdoc}
    +   */
    +  public function getAvailablePaths() {
    

    And here.

15. +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/d7/MigrateUpgrade7Test.php
    @@ -84,6 +84,74 @@ protected function getEntityCounts() {
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public function getMissingPaths() {
    

    And here too.

This should fix 101-2, 4, 5, 6, 9, 10, 11, 12, 13, 14, 15.

1. There are more references to entity_reference, just don't have the time right now.
3. Can someone else do this one? Still not confident with all the mocking ...
7. Conceptually like the idea of making it easier to send useful error messages.
8. 'Because as soon as a distro or custom profile enters the picture, this won't work. ' Now I get it. Sometimes I don't see the forest for the trees. Agree with a follow up.

Haven't had time to test, fingers crossed.

Back to needs work for #101 - 1,3,7, and 8

Tagging to we remember to create a follow-up for distro/custom profile support.

I have addressed #101-3 by renaming the variable names of the 2 mocks and adding a couple of line breaks to distinguish them. I couldn't find a way to phrase a comment or two that could explain things in code, but here are my ramblings in case someone can do better:

The $plugin_manager has a single mocked method, getDiscovery(), that, when called within findDefinitions() (which is called within getDefinitions()) returns the $discovery. The $discovery has a single mocked method, getDefinitions() which is also called within findDefinitions() and returns the data-provider-specified $definitions. $definitions is looped over and processDefinition() is called which throws the expected exception.

We can't simply mock getDefinitions() on $plugin_manager because we are testing whether one of the methods it contains is functioning correctly.

I hope that helps...

Back to Needs Work for #101 - 1 ,7, and 8

I have addressed the remaining points from #101:
1. Removed the 3 references to entity_reference as a destination_module (userreference, nodereference & taxonomy_term_reference).
7. Added BadPluginDefinitionException.
8. Created a follow-up ticket - #2918185: Don't display install profiles in the Migrate UI.

Oops, sorry! Just couple of concerns about the new exception. Other than that, this patch is music to my eyeballs.

1. +++ b/core/lib/Drupal/Component/Plugin/Exception/BadPluginDefinitionException.php
   @@ -0,0 +1,29 @@
   +namespace Drupal\Component\Plugin\Exception;
   

   I think we should not put this in the plugin component, because it will be more controversial that way. Let's move this into Migrate's namespace as a Migrate-specific variant of InvalidPluginDefinitionException. So we can do something like this:

   namespace Drupal\migrate\Plugin\Exception;
   
   use Drupal\Component\Plugin\InvalidPluginDefinitionException as BaseException;
   
   class InvalidPluginDefinitionException extends BaseException { /* our fancy constructor here */ }
   

2. +++ b/core/lib/Drupal/Component/Plugin/Exception/BadPluginDefinitionException.php
   @@ -0,0 +1,29 @@
   +   * @param string $plugin
   +   *   The ID of the plugin that has a missing property.
   

   We don't need this as a parameter. We can just reuse $plugin_id.

Removing the "needs followup" tag, now that #2918185: Don't display install profiles in the Migrate UI is filed.

Made the corrections to the new Exception.

I think this looks good.

There is a critical issue already open, blocked on this one, to handle the core modules that have no upgrade path: #2914974: Migrate UI - handle sources that do not need an upgrade. Once that lands, a separate one, quite possibly critical as well, will be opened for contrib.

*sigh*

OK, Drupal CI, you're the boss.

Reroll.

Oh yea, NR for the testbot

Sorry, my bad, the structure of the "review upgrade" page was changed in #2905227: Migrate UI: Improve 'Review Upgrade' page UX so the xpath assertions didn't pass anymore.

re #120: that will be handled in #2914974: Migrate UI - handle sources that do not need an upgrade.

However, there is no upgrade path for views. See https://www.drupal.org/docs/8/upgrade/known-issues-when-upgrading-from-d... & https://www.drupal.org/docs/8/upgrade/known-issues-when-upgrading-from-d... for more details.

Date will also get addressed in that follow-up.

Date will also get addressed in that follow-up.

Date is handled in this patch. Date API, on the other hand, is not. And rightfully so: date fields are already properly migrated and have been for some time; Date API, in and of itself, will have no upgrade path.

It's counterproductive to look at this issue separately from #2914974: Migrate UI - handle sources that do not need an upgrade. They are really two sides of the same coin; we are dealing with it in multiple issues so that this one doesn't become an overlong, overcomplicated patchzilla. The scope of this issue is for field migrations. Once #2914974: Migrate UI - handle sources that do not need an upgrade is done, the UI will be a great deal clearer about which upgrade paths are actually missing.

Full steam ahead.

I'm copying @jhodgdon's list of modules from #120 into #2914974: Migrate UI - handle sources that do not need an upgrade because I think it will be helpful to determine an initial list of modules which should be marked as having no upgrade path.

It is unfortunately impossible to tell with the issue summary how does this improve the UI which would be important on a UX issue :) Maybe steps to reproduce if they are relatively easy if a static screenshot would not do.

Also the change notice draft says "Migrate field plugins now require source_module and destination_module" -- how are we going about the backwards compatibility of this on the API? (And if there is backwards compatibility, why is it required -- is it strongly suggested instead?)

Added steps to reproduce.

s/step/steps/

The previous comment should have that I added an bit more clarification that this issue is solving the field migration part of the problem.

And fixed formatting

IS is updated.

re #125: it is an intentional BC break for field plugins. We discussed part of our decision over in #2908282: Throw exception for source plugins without a source_module property in the very early comments. This change also in migrate_drupal, which is still in alpha so BC *is* possible, although we have tried to avoid it as much as we can.

Discussed at migrate meeting and we agree this is ready to review.

I'm gonna go ahead and put this back to RTBC since both questions in #125 have been answered.

I'm still really missing a "before/after" here. I don't see it in the issue summary, only the description of the original problem.

Here's some before/after screenshots!

Before

After

Edited: new screenshots.

My previous screenshots were not clearly showing the improvments, here are new ones. (I edited my previous comment to display these new screenshots)

@maxocub, many thanks for clarifying the screenshots.

I agree that there are some really good steps forward here, but I would also like to point out that from the 'after' image, I would expect that these modules:

- entity
- book
- blog

.. could have content, not just configuration, and so their 'missing' state should be explained in the UI.

I also believe there is a need for some architectural developer documentation in this area - as I noted in an earlier post I found the current migrate difficult to understand. Would it be appropriate to add "Needs documentation"?

For a possible new Issue, I have been wondering whether there is a need for a part of Migrate to:

- provide suggestions of which D8 modules should be downloaded & enabled so that they can migrate the existing content;
- optionally, use a 'drush pm-download' type tool to get them;

- for modules that are known to be config only, this could be explained in the UI, and others could be reported as 'No information' rather than the rather inscrutable 'Missing'

- list configuration associated with the original D6/7 modules to make a sort of 'report' which could be saved for later reference.

I was pondering adding something to a new migrate-related module -- migrate_suggestions? -- that could be extended with this metadata for those modules known to support D2D migrations. Should I create such an issue?

@rivimey,thanks!

I will try to address the item in #136 in order.

1. Currently there is no way to know if a migration is for content or configuration. So we can only inform that isn't a migration path for the module. See #2711099: Categorize migrations according to their type.

2. If you want more documentation about this form to appear in the, MigrateUpgradeForm api documentation then, I think, Needs Documentation tag is the one to use. Otherwise is this something for the Upgrade to Drupal 8 section? If so, then a new issue for that would be helpful, tagged 'd.o documentation'.

3. We don't have the information to provide suggestions for what contrib module to download. We will know that the source db installed some module 'XYZZY' but we do not know what the D8 equivalent is.

4. As with #1, migrate doesn't know if a migration is for content of configuration. Yes,'Missing' is accurate and a long way to say 'no migration found'. Since this issue is about fields and I don't see an issue addressing that wording will you please make one.

5. This would be a Feature Request. What is the purpose of such a report?

6. Making an issue would get more eyes on the ideas and provide more feedback.

@quietone,

Regarding the docs issue, I suspect there are two aspects; the more detailed API-level aspects that will enable a module maintainer to implement a suitable migration path, and the site-builder level which will assist in identifying how to effectively use the migration ecosystem, diagnose and address issues found.

Consequently I have marked up Needs doc here (for the module maintainer who wishes to update or implement a migration path) here. I will add the other as well.

I will also add an issue around UI wording et al.

Regarding the report, I was thinking of it as a way for site-builder level devs to gain a summary of the config of the existing site, to assist in setting up D8 appropriately. For example, it might well contain api keys, URLs and more for external services which still need to be included. In real terms, it is likely to be a listing of the variables table, but it would be nice to be able to put some structure around it too.

For the report and for the config/content split, I know that migrate cannot by itself know whether a module is one or both at present, and in any case if the module isn't installed on the D8 site it can't be asked. So the 'suggestions' module would essentially have to be a store of metadata about those modules people care enough about to record in it.

... I will make an issue for that, too :-)

Just seen there is a recently-completed issue that should cover the document migrate-for-module-maintainers issue:

https://www.drupal.org/node/2610062#comment-12307159

with the docs here:

https://www.drupal.org/docs/8/api/migrate-api/writing-migrations-for-con...

Is this (still) correct and complete?

I have created #2919565: Migrate: add "suggestions" module to assist D2D migrations to discuss the metadata/suggestions module idea.

Regarding the docs issue

Thanks for the clarification. Your points are valid and the documentation is being worked on and they are also beyond the scope of this issue. There is an existing issue for improving documentation for module maintainers, #2914500: Add documentation for writing custom/contrib modules. And there are quite a few api documentation issues still being worked on. Because of the scope issue and, I think, there are existing documentation issue covering this I am removing the Needs Documentation tag.

Oh, the Meta for the documentation issues is #2561243: [meta] Migration system documentation

Is this (still) correct and complete?

This is the latest version. I can't confirm it is complete. But the issue for that page is #2610062: Document implementing migration paths for contrib modules

I think that covers everything.

Now I am really updating the tags. Removing needs documentation and adding blocker.

This is blocking a Critical #2914974: Migrate UI - handle sources that do not need an upgrade.

This is also blocking a second Critical #2908282: Throw exception for source plugins without a source_module property.

So +1 for the blocker tag.

After reviewing the changes carefully, it is apparent that any BC breaking changes are made to alpha modules within the migrate modules. While we try to avoid BC breaks even though they are technically alpha and that would allow BC breaks, this does not seem to be possible otherwise as we need the metadata about the plugins as well as the management of that data to happen.

While this does not solve all the module listing issues, this is a sizable subset and is well tested. Thanks all for contributing to this!

Published change record at https://www.drupal.org/node/2914530

A bit confused here. The version of this issue is 8.4.x and it was committed to 8.5.x. Is this getting committed to 8.4.x?

Re-opening for 8.4.x cherry-pick consideration.

On a quick scan it looks like this patch has a bunch of API additions to stable core subsystems as well as to migrate which is in beta and so has the same BC policy according to stable code. According to #145 also adds BC internal breaks. What's the impact vs. risk of backporting it?

We discussed this in the migrate maintainers meeting today and decided not to push for a cherry-pick. Too many potential BC breaks. See #2920118: Update cck and field migration plugin as an example where we are trying to get ahead of things for 8.5.x

Tagging for 8.5 release notes.

The 8.5.0 release notes need to mention the need for updates to modules due to the API change in the core experimental modules. Changes like #2920118: Update cck and field migration plugin needed in possibly other modules to be 8.5 compatible.