Add a weighting option to MigrateField plugin's definition and lower weight of deprecated 'date' plugin so it isn't used

I like this. I think we should also open a follow-up to add the deprecated flag to other deprecated filed plugins so they are not use by accident.

1. +++ b/core/modules/comment/src/Plugin/migrate/D7Comment.php
   @@ -32,7 +32,8 @@ public function getProcess() {
   -          $this->fieldPluginCache[$field_type] = $this->fieldPluginManager->createInstance($field_type, [], $this);
   +          $plugin_id = $this->fieldPluginManager->getPluginIdFromFieldType($field_type, [], $this);
   +          $this->fieldPluginCache[$field_type] = $this->fieldPluginManager->createInstance($plugin_id, [], $this);
   

   After a quick grep, I found that this should also be done in the Drupal\user\Plugin\migrate\User class.

2. +++ b/core/modules/migrate_drupal/src/Plugin/MigrateFieldPluginManager.php
   @@ -44,8 +44,9 @@ public function getPluginIdFromFieldType($field_type, array $configuration = [],
        $definitions = $this->getDefinitions();
   +
        foreach ($definitions as $plugin_id => $definition) {
   

   Super-nit: this extra line is not useful and only add noise to the patch.

@mikelutz: Thanks for the explanation. I agree D7Comment and User should be refactored to be more like the node and taxonomy term derivers.
RTBC provided the tests are green.

This is not how we deprecate plugins. We've learnt the hard way that plugins should be deprecated via the constructor and not in the main part of the class. See https://www.drupal.org/core/deprecation#how-plugin

Then there'll be no random keys in the annotation and no unintended API breaks such as:

+++ b/core/modules/migrate_drupal/src/Plugin/MigrateFieldPluginManager.php
@@ -45,7 +45,7 @@ public function getPluginIdFromFieldType($field_type, array $configuration = [],
     foreach ($definitions as $plugin_id => $definition) {
-      if (in_array($core, $definition['core'])) {
+      if (in_array($core, $definition['core']) && empty($definition['deprecated'])) {
         if (array_key_exists($field_type, $definition['type_map']) || $field_type === $plugin_id) {
           return $plugin_id;
         }

1. +++ b/core/modules/datetime/src/Plugin/migrate/field/d6/DateField.php
   @@ -18,7 +16,8 @@
   + *   weight = 9999999
   

   This looks like a good flexible solution. +1 - this is why skipping deprecations is a bad idea means we don't solve these things at the right time.

2. +++ b/core/modules/migrate_drupal/src/Plugin/MigrateFieldPluginManager.php
   @@ -44,8 +44,12 @@ public function getPluginIdFromFieldType($field_type, array $configuration = [],
   +    uasort($definitions, function ($a, $b) {
   +      return $a['weight'] - $b['weight'];
   +    });
   

   Let's do uasort(definitions, ['Drupal\Component\Utility\SortArray', 'sortByWeightElement']);

   But I also wonder if we should do a stable sort here - ie. sort by weight and plugin_id.

3. +++ b/core/modules/migrate_drupal/src/Plugin/MigrateFieldPluginManager.php
   @@ -44,8 +44,12 @@ public function getPluginIdFromFieldType($field_type, array $configuration = [],
   +      if (in_array($core, $definition['core']) && empty($definition['deprecated'])) {
   

   Can remove this change

+++ b/core/modules/migrate_drupal/tests/src/Unit/MigrateFieldPluginManagerTest.php
@@ -0,0 +1,182 @@
+    /* @var CacheBackendInterface $cache */
...
+    /* @var ModuleHandlerInterface $module_handler */

Nit: need full path to class.

+++ b/core/modules/migrate_drupal/tests/src/Unit/MigrateFieldPluginManagerTest.php
@@ -0,0 +1,182 @@
+        'weight' => 99999999,
...
+        'weight' => 0,
...
+        'weight' => 0,
...
+        'weight' => 0,
...
+        'weight' => 0,
...
+        'weight' => 0,

Except for the 99999 deprecated, can we remove the rest of these or at least some of these to test what happens when we don't explicitly define the weight?

Valid thoughts. Here's a few more to consider.

1. +++ b/core/modules/migrate_drupal/src/Plugin/MigrateFieldPluginManager.php
   @@ -28,7 +28,25 @@ class MigrateFieldPluginManager extends MigratePluginManager implements MigrateF
   -   * {@inheritdoc}
   +   * Get the plugin ID from the field type.
   ...
      public function getPluginIdFromFieldType($field_type, array $configuration = [], MigrationInterface $migration = NULL) {
   

   Why did this inheritdoc go away?

2. +++ b/core/modules/migrate_drupal/tests/src/Unit/MigrateFieldPluginManagerTest.php
   @@ -0,0 +1,227 @@
   +class MigrateFieldPluginManagerTestClass extends MigrateFieldPluginManager {
   

   Is this needed any more?

So, is there any way to test that the weight annotation wasn't there before the cache clear? It would seem the answer is no.

Lookin' really good. I think this is very logical, and it's nice to see the deprecation moved into the constructor and out of the "global" deprecations. My feedback is mostly minor.

1. +++ b/core/modules/datetime/src/Plugin/migrate/field/d6/DateField.php
   @@ -18,7 +16,8 @@
   + *   weight = 9999999
   

   Nit: This should have a trailing comma.

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

   Same here.

3. +++ b/core/modules/migrate_drupal/migrate_drupal.install
   @@ -31,3 +31,10 @@ function migrate_drupal_update_8502() {
   +/**
   + * Clear the Field Plugin Cache after adding weight annotation.
   + */
   +function migrate_drupal_update_8701() {
   +  \Drupal::service('plugin.manager.migrate.field')->clearCachedDefinitions();
   +}
   

   Let's replace this with an empty post-update hook, which will force a general cache clear and does not need a test.

4. +++ b/core/modules/migrate_drupal/src/Plugin/MigrateFieldPluginManager.php
   @@ -28,7 +28,25 @@ class MigrateFieldPluginManager extends MigratePluginManager implements MigrateF
   +   * type and core Drupal version, returning the lowest weighted plugin
   

   Can we say "Drupal core" instead of "core Drupal"?

5. +++ b/core/modules/migrate_drupal/src/Plugin/MigrateFieldPluginManager.php
   @@ -28,7 +28,25 @@ class MigrateFieldPluginManager extends MigratePluginManager implements MigrateF
   +   * supporting the provided core version, and who matches the field type either
   

   s/who/which

6. +++ b/core/modules/migrate_drupal/src/Plugin/MigrateFieldPluginManager.php
   @@ -28,7 +28,25 @@ class MigrateFieldPluginManager extends MigratePluginManager implements MigrateF
   +   * by plugin id, or in the type_map annotation keys.
   

   'id' should be ID. And can we add a @see annotation to this method, referring to the annotation class?

7. +++ b/core/modules/migrate_drupal/src/Plugin/MigrateFieldPluginManager.php
   @@ -28,7 +28,25 @@ class MigrateFieldPluginManager extends MigratePluginManager implements MigrateF
   +   * @param \Drupal\migrate\Plugin\MigrationInterface|null $migration
   

   We can remove the |null from here. "(optional)" covers it.

8. +++ b/core/modules/migrate_drupal/src/Plugin/MigrateFieldPluginManager.php
   @@ -28,7 +28,25 @@ class MigrateFieldPluginManager extends MigratePluginManager implements MigrateF
   +   *   The ID of the plugin for the field_type if available.
   

   field_type should be "field type".

9. +++ b/core/modules/migrate_drupal/src/Plugin/MigrateFieldPluginManager.php
   @@ -67,4 +85,27 @@ public function processDefinition(&$definition, $plugin_id) {
   +   * the plugin id, ensuring a stable, deterministic, and testable ordering of
   

   'id' --> ID

10. +++ b/core/modules/migrate_drupal/tests/src/Kernel/MigrateFieldPluginManagerTest.php
    @@ -3,54 +3,144 @@
    +    $this->assertEquals('link', $this->pluginManager->getPluginIdFromFieldType('link', ['core' => 6]));
    +    $this->assertEquals('link_field', $this->pluginManager->getPluginIdFromFieldType('link_field', ['core' => 7]));
    +    $this->assertEquals('image', $this->pluginManager->getPluginIdFromFieldType('image', ['core' => 7]));
    +    $this->assertEquals('file', $this->pluginManager->getPluginIdFromFieldType('file', ['core' => 7]));
    +    $this->assertEquals('d6_file', $this->pluginManager->getPluginIdFromFieldType('file', ['core' => 6]));
    +    $this->assertEquals('d6_text', $this->pluginManager->getPluginIdFromFieldType('text', ['core' => 6]));
    +    $this->assertEquals('d7_text', $this->pluginManager->getPluginIdFromFieldType('text', ['core' => 7]));
    

    Can we use assertSame() when comparing strings?

11. +++ b/core/modules/migrate_drupal/tests/src/Kernel/MigrateFieldPluginManagerTest.php
    @@ -3,54 +3,144 @@
    +    $nonexistent_plugins = [
    +      ['core' => 7, 'field_type' => 'filefield'],
    +      ['core' => 6, 'field_type' => 'link_field'],
    +      ['core' => 7, 'field_type' => 'link'],
    +      ['core' => 7, 'field_type' => 'd6_no_core_version_specified'],
    +    ];
    +
    +    foreach ($nonexistent_plugins as $plugin_info) {
    +      try {
    +        $this->pluginManager->getPluginIdFromFieldType($plugin_info['field_type'], ['core' => $plugin_info['core']]);
    +        $this->fail(sprintf("Looking up non-existent field plugin for field type '%s' in Drupal %d failed to throw expected '%s' exception", $plugin_info['field_type'], $plugin_info['core'], PluginNotFoundException::class));
    +      }
    +      catch (PluginNotFoundException $e) {
    +        $this->assertEquals($e->getMessage(), sprintf("Plugin ID '%s' was not found.", $plugin_info['field_type']));
    +      }
    

    We should probably replace this with a dataProvider pattern and $this->setExpectedException(), since it's more explicit and clear than $this->fail() in a try-catch.

12. +++ b/core/modules/migrate_drupal/tests/src/Kernel/MigrateFieldPluginManagerTest.php
    @@ -3,54 +3,144 @@
    +    $this->assertNotNull($plugin);
    

    This assertion is not needed (and therefore, neither is the $plugin variable).

13. +++ b/core/modules/migrate_drupal/tests/src/Kernel/MigrateFieldPluginManagerTest.php
    @@ -3,54 +3,144 @@
    +      if (in_array($id, $deprecated_plugins)) {
    

    in_array() should be called with TRUE as a third argument.

14. +++ b/core/modules/migrate_drupal/tests/src/Kernel/MigrateFieldPluginManagerTest.php
    @@ -3,54 +3,144 @@
    +        $this->assertEquals(9999999, $definition['weight']);
    +      }
    +      else {
    +        $this->assertEquals(0, $definition['weight']);
    

    Should be assertSame().

15. +++ b/core/modules/migrate_drupal/tests/src/Kernel/MigrateFieldPluginManagerTest.php
    @@ -3,54 +3,144 @@
    +  /**
    +   * Tests that HOOK_update_8701 clears the field plugin cache.
    +   */
    +  public function testUpdate8701() {
    +    /** @var \Drupal\Core\Cache\CacheBackendInterface $cache */
    +    $cache = $this->container->get('cache.discovery');
    +    $cache->set('migrate_plugins_field', [], Cache::PERMANENT);
    +    $definitions = $this->pluginManager->getDefinitions();
    +    $this->assertEquals([], $definitions);
    +
    +    module_load_include('install', 'migrate_drupal');
    +    migrate_drupal_update_8701();
    +
    +    $definitions = $this->pluginManager->getDefinitions();
    +    $this->assertEquals(20, count($definitions));
    +    foreach ($definitions as $id => $definition) {
    +      $this->assertArrayHasKey('weight', $definition);
    

    We can remove this once we replace the update function with an empty post-update.

16. +++ b/core/modules/migrate_drupal/tests/src/Unit/MigrateFieldPluginManagerTest.php
    @@ -0,0 +1,227 @@
    +    $discovery = $this->prophesize(AnnotatedClassDiscovery::class);
    +    $discovery->getDefinitions()->willReturn($this->pluginFixtureData());
    +    $manager = new MigrateFieldPluginManagerTestClass('field', new \ArrayObject(), $cache, $module_handler, MigrateField::class, $discovery->reveal());
    

    Nice :)

17. +++ b/core/modules/migrate_drupal/tests/src/Unit/MigrateFieldPluginManagerTest.php
    @@ -0,0 +1,227 @@
    +    try {
    +      $actual_plugin_id = $manager->getPluginIdFromFieldType($field_type, ['core' => $core]);
    +      $this->assertEquals($expected_plugin_id, $actual_plugin_id);
    +    }
    +    catch (PluginNotFoundException $e) {
    +      // We did not find a plugin, so assert that we didn't expect to.
    +      $this->assertFalse($expected_plugin_id, sprintf('Expected field plugin %s was not found by the field plugin manager', $expected_plugin_id));
    +    }
    

    This should use $this->setExpectedException() (the data provider can pass the expected exception class and message, if an exception is expected).

Looks great. Two tiny nits:

1. +++ b/core/modules/migrate_drupal/tests/src/Kernel/MigrateFieldPluginManagerTest.php
   @@ -51,58 +50,72 @@ public function setUp() {
   +  public function nonExistantPluginExceptionsData() {
   

   Minor spelling error: this should be nonExistent.

2. +++ b/core/modules/migrate_drupal/tests/src/Unit/MigrateFieldPluginManagerTest.php
   @@ -34,15 +34,11 @@ public function testWeights($field_type, $core, $expected_plugin_id) {
   +    $this->assertEquals($expected_plugin_id, $actual_plugin_id);
   

   This should be assertSame().

RTBC on green for all backends. Thanks!

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

   How come this change is necessary? And what is the impact on existing migrations due to this?

2. +++ b/core/modules/migrate_drupal/tests/src/Kernel/MigrateFieldPluginManagerTest.php
   @@ -8,49 +8,132 @@
   +        'core' => 7
   +        , 'field_type' => 'link',
   

   code style...

Looks like it needs a reroll...

All feedback seems to be addressed.

A question,

+++ b/core/modules/migrate_drupal/src/Plugin/MigrateFieldPluginManager.php
@@ -67,4 +87,27 @@ public function processDefinition(&$definition, $plugin_id) {
diff --git a/core/modules/migrate_drupal/tests/src/Kernel/MigrateFieldPluginManagerTest.php b/core/modules/migrate_drupal/tests/src/Kernel/MigrateFieldPluginManagerTest.php

+++ b/core/modules/migrate_drupal/tests/src/Kernel/MigrateFieldPluginManagerTest.php
@@ -8,49 +8,132 @@
+    $this->assertSame('link', $this->pluginManager->getPluginIdFromFieldType('link', ['core' => 6]));
+    $this->assertSame('link_field', $this->pluginManager->getPluginIdFromFieldType('link_field', ['core' => 7]));
+    $this->assertSame('image', $this->pluginManager->getPluginIdFromFieldType('image', ['core' => 7]));
+    $this->assertSame('file', $this->pluginManager->getPluginIdFromFieldType('file', ['core' => 7]));
+    $this->assertSame('d6_file', $this->pluginManager->getPluginIdFromFieldType('file', ['core' => 6]));
+    $this->assertSame('d6_text', $this->pluginManager->getPluginIdFromFieldType('text', ['core' => 6]));
+    $this->assertSame('d7_text', $this->pluginManager->getPluginIdFromFieldType('text', ['core' => 7]));
...
+    $this->assertSame('datetime', $this->pluginManager->getPluginIdFromFieldType('date', ['core' => 6]));

Why is a subset of the field plugins tested? Why these particular ones and not others? Notably all the reference fields are absent. Should all of them be tested? If this warrants a comment in code please, please sort the $modules list in the test. Otherwise, no worries

Thanks, just wanted to be sure I wasn't missing something. I spotted that the reference ones weren't they and wondered, so I asked without giving it much thought. So, no change needed. 'Tis great as it is.

Nice approach. I don't think we can backport this to 8.6.x, but if you strongly disagree please re-open.

Committed 6b14845 and pushed to 8.7.x. Thanks!