Make service for field discovery for use in migrate entity derivers

Throwing this here to see how many tests I broke. After building out the trait, I really felt like a base class was a better solution for both dependency injection and access to the class derivatives property. Anyway, this needs a bit of refactoring for coding standards among other things, so just a WIP.

@mikelutz, thanks, this is coming together nicely. I'm not doing a review, only asking a question. It looks like this will work fine for entities with bundles but what about the cases where there are no bundles, like user?

@quietone Good point, perhaps I do need to at least pull part of this into a trait that can be used by the user field migration. I may be working on two separate things actually. Shared code for bundlable entities that need a deriver, and shared code for fieldable entities that need to scan and load fields. The bundle stuff definitely makes more sense as a deriver base class, but the field stuff would better make a trait. Let me take another pass at this with that mindset, I bet I can do it in a way that makes more sense.

It was intentional, though again, I just threw this up here to see how many tests I broke. There's no consistency in the tags and revision and translation checks. All the tags are uppercase except for 'translation', d7 node checks if it's a translation migration with a tag, and if it's a revision migration with a hard-coded id check, d6 checks for translations with a hard coded id check and no tags, and it doesn't make any sense. It would be nice for a contrib deriver to be able to use the tags in a consistent way and get the translation check and the dependency addition by tagging their migrations.

I don't think there would be any issue with adding a Revisions tag to revision migrations. Changing the case on the translation tag or adding it to the d6_node_translation migration I suspected might be a little more problematic. I just wanted to see how problematic it would be.

So, let's try this again. I was trying to do too much in one issue before. This patch is limited to the issue scope of moving code related to fieldable entities into a trait. I do still think some of the bundle deriver code should be moved into a base class, but that is not what this issue is about.

1) I agree, that's why I put it in it's own trait. It really feels like it should be a method on Migration, but it only makes sense on Drupal migrations and we have no Drupal specific migration class. Then I thought about a static method somewhere, but nowhere made sense. I'm still not sure a trait is the right spot for what is effectively a static utility function, but it had to go somewhere.

2) User doesn't currently check against bundle, but non-bundlable entities get their entity type as a bundle, so I call from user using that. Another option might be to provide a default empty string for the bundle and only set it on the source if provided, but I chose to require the bundle to be passed in explicitly to simplify the logic and reduce the chance for errors.

3) MigrationDeriverTrait is not included in FieldMigrationTrait. That trait contains a method bundle derivers use to collect bundles, which is why user does not need it.

4) User has no properties to clean up. User extends FieldMigration which contains the dependency injection.

Here's an update. It cleans up some comments, and deals a bit more with bundles/no bundles.

For non-bundleable entities it no longer requires you to submit a bundle.
For bundleable entities, It gets and caches all fields on the first go, because it is being called inside a loop in a deriver. There's no need to create a source migration and database call for each bundle.

Meh. Too many names for bundles over the years..

Only time for a few comments.

1. +++ b/core/modules/migrate_drupal/src/DrupalMigrationTrait.php
   @@ -0,0 +1,31 @@
   +    if (in_array('Drupal 7', $migration->getMigrationTags())) {
   

   These should probably case insensitive?

2. +++ b/core/modules/migrate_drupal/src/FieldMigrationTrait.php
   @@ -0,0 +1,220 @@
   +   *   The Drupal Core Version to migrate from.
   

   I think this type of thing is usually documented to refer to the source not 'from'. Something like 'The core version of the source database.

   Similar with the entity_type comment.

3. +++ b/core/modules/migrate_drupal/src/FieldMigrationTrait.php
   @@ -0,0 +1,220 @@
   +    if ($core == '7') {
   ...
   +        $bundle_property = $core == 6 ? 'type_name' : 'bundle';
   

   The tests using $core are different. What is going on here?

This is on my list to test.

Applied the patch in #14 and tested it by converting the 2 derivers, ProductDeriver and ProductVariationDeriver, in commerce_migrate. It took a bit to figure out how to have two different source entity types involved for these derivers. Two are needed because the entity type names changed from Commerce1 to Commerce2. In d7 a product is a node entity but it is a commerce_product entity in d8 and a d7 commerce_product entity is a d8 commerce_product_variation entity.

This can be accomplished by just using getFields, for the case of a Commerce 1 product, to use 'node' for the source but 'commerce_product' for the destination, as shown here:
$this->fields[$core]['commerce_product'] = $this->getFields($core, 'node');

@quietone, That shouldn't be necessary. Commerce migrate should reduce with the attached patch (which passed the deriver tests for me locally, though I didn't run the full commerce_migrate suite)

FWIW the field_collections and paragraphs migrations work as well with this patch in a similar way.

Oh, oops. Sorry, mikelutz. You are correct. I fixed my errors and, like you, both deriver tests run successfully. Then I ran the migration tests for Product and ProductVariation. The ProductVariationTest passes but not ProductTest, which among other things is dependent on d7_user.

This is the error and I can't look further into it now.

1) Drupal\Tests\commerce_migrate_commerce\Kernel\Migrate\d7\ProductTest::testProduct
TypeError: Argument 2 passed to Drupal\migrate\Plugin\MigrateDestinationPluginManager::createInstance() must be of the type array, null given, called in /opt/sites/d8/core/modules/migrate/src/Plugin/Migration.php on line 416

@quietone I saw that too, let me check into it more in the morning. I got the failure without applying the commerce_migrate patch, but it's green on d.o. as of a few days ago, so let me make sure that the core patch isn't causing the failure. I don't see why it would, it looks like unmet requirements on the migrate test at first glance, but I definitely want to check it out and know where it is coming from.

A quick response to #17,

1) I think the tag is set with that capitalization and checked against that capitilization in other places, but let me know if I'm wrong.
2) I'll update the comments and post a new patch tomorrow.
3) I've always fallen in the pro-ternary camp, the if statements above set values in the array, and I personally like the ternary for the scaler later, but I can refactor and set that identifier value in the core check blocks above. In this case it may be more readable, I can put everything I need to differentiate between the d6 and d7 migrations in the same spot.

#17.1 I'm not sure either. I don't think at most it is a convention. What do others think?
#17.3 It is about the type used not the ternary. I should have said, "Why are some testing string and not strict, and others testing integer"?

Ah! good call, that '6' should have been a string. I'll fix that in tomorrow's update too.

I found the issue with the commerce_migrate test, I didn't realize the user getProcess() reused a stub migration definition later in the function to test for the profile module, and I had inadvertently removed it.

commerce_migrate must have profile enabled in their fixture, while the core fixture doesn't. Because of this, core tests went to the requirements check, threw an exception on the missing profile module, which is caught and life went on. Commerce migrate passed the source check, which moved to the destination check, which threw the uncaught exception due to the incomplete definition.

I enabled the profile module for the core user test so that this bug will have a failing test in the future.

I also updated the comments, moved the bundle identifier setting into the other core version checks and removed the integer check.

Finally, WRT 17.1 Drupal\migrate_drupal\MigrationConfigurationTrait::getMigrations() uses

$version_tag = 'Drupal ' . $drupal_version;

This tag feeds into MigrationPluginManager::createInstancesByTag() which uses the same

in_array($tag, $migration['migration_tags']) 

check to generate the migration plugins, so I think the capitalization is fine.

Typo fix...

Thanks for tracking down the commerce_migrate failure. I haven't retested yet. And I realized this morning that there is an issue to document the yml file properties,, and when that gets done it should include that tags are capitalized. #2911781: Document migration yml properties

Looked at the comments and found some things. There is inconsistent capitalization on Field, Plugin and Migration in the comments. I haven't identified them all but they need to be corrected. And the summary lines beginning with a verb "should be in third person singular present tense, such as "Handles file uploads." "

1. +++ b/core/modules/migrate_drupal/src/DrupalMigrationTrait.php
   @@ -0,0 +1,31 @@
   + * A trait for drupal migrations.
   

   Needs to be more descriptive. 'Provides common functionality for migrations' or some such.

2. +++ b/core/modules/migrate_drupal/src/DrupalMigrationTrait.php
   @@ -0,0 +1,31 @@
   +   *   A string representation of the Drupal Version, or false.
   

   s/false/FALSE/

3. +++ b/core/modules/migrate_drupal/src/FieldMigrationTrait.php
   @@ -0,0 +1,222 @@
   + * A trait to provide field management functions for migrations.
   

   s/A trait to provide/Provides/

4. +++ b/core/modules/migrate_drupal/src/FieldMigrationTrait.php
   @@ -0,0 +1,222 @@
   +   * The Migration Plugin Manager.
   

   The migration plugin manager.

5. +++ b/core/modules/migrate_drupal/src/FieldMigrationTrait.php
   @@ -0,0 +1,222 @@
   +   * Get the CCK Plugin manager.
   

   s/Get/Gets/

6. +++ b/core/modules/migrate_drupal/src/FieldMigrationTrait.php
   @@ -0,0 +1,222 @@
   +   * Get the Migratable Fields.
   ...
   +   *   The entity type of the source.
   

   When I read Migratable I think this method is doing some sort of checking and will only return a subset of all possible fields. Should be 'Gets the fields for a given entity type'.

7. +++ b/core/modules/migrate_drupal/src/FieldMigrationTrait.php
   @@ -0,0 +1,222 @@
   +   *   An array of source fields to migrate.
   

   Can we expand on what is actually in this array. And remove 'to migrate'.

8. +++ b/core/modules/migrate_drupal/src/FieldMigrationTrait.php
   @@ -0,0 +1,222 @@
   +      // below we'll return an empty array.  The migration will proceed without
   

   Extra space before The

9. +++ b/core/modules/migrate_drupal/src/FieldMigrationTrait.php
   @@ -0,0 +1,222 @@
   +   *   The entity type to add fields from.
   

   I prefer "The source entity type".

10. +++ b/core/modules/migrate_drupal/src/FieldMigrationTrait.php
    @@ -0,0 +1,222 @@
    +   *   The entity bundle to add fields from.
    

    Same as above

11. +++ b/core/modules/migrate_drupal/src/FieldMigrationTrait.php
    @@ -0,0 +1,222 @@
    +    // Loop through the fields, and find add the appropiate process.
    

    s/find add/ the plugin and add/ ?

12. +++ b/core/modules/node/src/Plugin/migrate/D6NodeDeriver.php
    @@ -16,7 +16,7 @@
    +  use FieldMigrationTrait, MigrationDeriverTrait;
    

    Probably one per line though can't find a standard for that. There isn't another in core with two traits on one line.

With the latest patch, the commerce_migration tests ProductDeriverTest, ProductVariationDeriverTest, ProductVariationTest and ProductTest pass locally.

Feedback addressed. I appreciate the comments on the comments, It should help me be more consistent in the future.

There ya go.

Temporary testbot hiccup.

I really like what this patch is doing but I think doing it as a trait is risky for BC processes and kinda unnecessary. Could we add a service to migrate_drupal that does this for migrations instead. That way everything would be nicely encapsulated and we less risk of any BC breaks.

Converted Trait into Service.

Assigning for review.

A definite DX improvement, and nice-looking patch. I think we can streamline it some, though...

1. +++ b/core/modules/migrate_drupal/migrate_drupal.services.yml
   @@ -16,3 +16,5 @@ services:
   +  manager.migrate.field:
   +    class: Drupal\migrate_drupal\MigrationFieldManager
   

   Let's rename the service to migrate_drupal.field_discovery. The word "manager" is a bit overloaded in Drupal, and field_discovery better explains what the service does.

2. +++ b/core/modules/migrate_drupal/src/MigrationFieldManager.php
   @@ -0,0 +1,226 @@
   +  /**
   +   * A cache of fields.
   +   *
   +   * @var array
   +   */
   

   This doc comment needs more detail. What kind of data is contained in the cache? What does "fields" mean in this context?

3. +++ b/core/modules/migrate_drupal/src/MigrationFieldManager.php
   @@ -0,0 +1,226 @@
   +  /**
   +   * Gets the CCK plugin manager.
   +   *
   +   * @return \Drupal\migrate_drupal\Plugin\MigrateCckFieldPluginManagerInterface
   +   *   The CCK plugin manager.
   +   */
   +  public function getCckPluginManager() {
   +    if (!$this->cckPluginManager) {
   +      $this->cckPluginManager = \Drupal::service('plugin.manager.migrate.cckfield');
   +    }
   +    return $this->cckPluginManager;
   +  }
   +
   +  /**
   +   * Gets the field plugin manager.
   +   *
   +   * @return \Drupal\migrate_drupal\Plugin\MigrateFieldPluginManagerInterface
   +   *   The field plugin manager.
   +   */
   +  public function getFieldPluginManager() {
   +    if (!$this->fieldPluginManager) {
   +      $this->fieldPluginManager = \Drupal::service('plugin.manager.migrate.field');
   +    }
   +    return $this->fieldPluginManager;
   +  }
   

   We don't need these methods; services can and should have their dependencies injected directly in the constructor.

4. +++ b/core/modules/migrate_drupal/src/MigrationFieldManager.php
   @@ -0,0 +1,226 @@
   +  /**
   +   * Gets the migration plugin manager.
   +   *
   +   * @return \Drupal\migrate\Plugin\MigrationPluginManagerInterface
   +   *   The migration plugin manager.
   +   */
   +  public function getMigrationPluginManager() {
   +    if (!$this->migrationPluginManager) {
   +      $this->migrationPluginManager = \Drupal::service('plugin.manager.migration');
   +    }
   +    return $this->migrationPluginManager;
   +  }
   

   Same here.

5. +++ b/core/modules/migrate_drupal/src/MigrationFieldManager.php
   @@ -0,0 +1,226 @@
   +   * @param string $core
   +   *   The core version of the source database.
   

   We should mention that '6' and '7' are the only allowed values.

6. +++ b/core/modules/migrate_drupal/src/MigrationFieldManager.php
   @@ -0,0 +1,226 @@
   +   *   The entity type of the source.
   

   What does this mean?

7. +++ b/core/modules/migrate_drupal/src/MigrationFieldManager.php
   @@ -0,0 +1,226 @@
   +  protected function getFields($core, $entity_type) {
   +
   +    if (!in_array($core, ['6', '7'])) {
   

   Nit -- empty blank line. Also, let's rename $entity_type to $entity_type_id, for clarity and consistency with other parts of core.

8. +++ b/core/modules/migrate_drupal/src/MigrationFieldManager.php
   @@ -0,0 +1,226 @@
   +      // ($core == '7')
   

   This should probably go...

9. +++ b/core/modules/migrate_drupal/src/MigrationFieldManager.php
   @@ -0,0 +1,226 @@
   +    catch (RequirementsException $e) {
   +      // If checkRequirements() failed then the content module did not exist and
   +      // we do not have any fields. Therefore, $fields will be empty and
   +      // below we'll return an empty array. The migration will proceed without
   +      // adding fields.
   +    }
   

   We should inject a logger service and log debugging info here.

Looking great! I see nothing seriously wrong here.

1. +++ b/core/modules/migrate_drupal/src/FieldDiscovery.php
   @@ -3,13 +3,18 @@
   + * Provides field discovery for Drupal migrations.
   

   Let's get a little more specific -- this is only for migrations from D6 and D7.

2. +++ b/core/modules/migrate_drupal/src/FieldDiscovery.php
   @@ -49,49 +54,33 @@ class MigrationFieldManager {
   +   * FieldDiscovery constructor.
   +   * @param \Drupal\migrate_drupal\Plugin\MigrateFieldPluginManagerInterface $field_plugin_manager
   

   Nit: We need a blank line after "FieldDiscovery constructor."

3. +++ b/core/modules/migrate_drupal/src/FieldDiscovery.php
   @@ -49,49 +54,33 @@ class MigrationFieldManager {
   +   *   The cck field plugin manager.
   

   Supernit: "cck" should be "CCK".

4. +++ b/core/modules/migrate_drupal/src/FieldDiscovery.php
   @@ -102,15 +91,14 @@ public function getMigrationPluginManager() {
   +   *   The entity type id of the source entity.
   

   Let's rephrase this -- "The entity type ID to which the fields are attached."

5. +++ b/core/modules/migrate_drupal/src/FieldDiscovery.php
   @@ -157,6 +144,10 @@ protected function getFields($core, $entity_type) {
   +      $this->logger->error('Field discovery failed for @entity_type_id. Error: @message', [
   +        '@entity_type_id' => $entity_type_id,
   +        '@message' => $e->getMessage(),
   +      ]);
   

   I love it!

6. +++ b/core/modules/migrate_drupal/src/DrupalMigrationTrait.php
   @@ -0,0 +1,32 @@
   +   * @return mixed
   +   *   A string representation of the Drupal version, or FALSE.
   

   Return type should be string|bool.

7. +++ b/core/modules/migrate_drupal/src/DrupalMigrationTrait.php
   @@ -0,0 +1,32 @@
   +    if (in_array('Drupal 6', $tags, TRUE)) {
   

   Should be elseif.

8. +++ b/core/modules/migrate_drupal/src/FieldDiscovery.php
   @@ -0,0 +1,217 @@
   +   * appropiate field instance source plugin. The array is keyed by
   

   Typo in 'appropriate'.

9. +++ b/core/modules/migrate_drupal/src/FieldDiscovery.php
   @@ -0,0 +1,217 @@
   +    if (!in_array($core, ['6', '7'])) {
   +      // Migration is supported from D6 or D7 databases only. Return no fields.
   +      return [];
   +    }
   

   Because this is a protected method, I'm not sure we'll need this. If we do, it should throw \InvalidArgumentException instead of returning an empty array.

10. +++ b/core/modules/migrate_drupal/src/FieldDiscovery.php
    @@ -0,0 +1,217 @@
    +    if ($core == '6') {
    

    ===

11. +++ b/core/modules/migrate_drupal/src/FieldDiscovery.php
    @@ -0,0 +1,217 @@
    +      // If checkRequirements() failed then the content module did not exist and
    

    content module is only a thing in D6. Maybe we should rephrase this as "If checkRequirements() failed, the source database did not support fields (i.e., CCK is not installed in D6 or Field is not installed in D7)."

12. +++ b/core/modules/migrate_drupal/src/FieldDiscovery.php
    @@ -0,0 +1,217 @@
    +   * @param string $bundle
    +   *   The source bundle type.
    

    Description should be prefixed by (optional).

13. +++ b/core/modules/migrate_drupal/src/FieldDiscovery.php
    @@ -0,0 +1,217 @@
    +    $core = $this->getCoreVersion($migration);
    

    What if getCoreVersion() returns FALSE?

14. +++ b/core/modules/migrate_drupal/src/FieldDiscovery.php
    @@ -0,0 +1,217 @@
    +        try {
    +          $plugin_id = $this->cckPluginManager->getPluginIdFromFieldType($field_type, ['core' => $core], $migration);
    +          if (!isset($this->cckPluginCache[$field_type])) {
    +            $this->cckPluginCache[$field_type] = $this->cckPluginManager->createInstance($plugin_id, ['core' => $core], $migration);
    +          }
    +          $this->cckPluginCache[$field_type]
    +            ->processCckFieldValues($migration, $field_name, $info);
    +        }
    

    Boy, I can't wait until we can rip the cckfield stuff out of Migrate Drupal...I swear, core will instantly shed 30 pounds.

15. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/FieldMigration.php
    @@ -73,6 +74,13 @@ class FieldMigration extends Migration implements ContainerFactoryPluginInterfac
    +   * The migration field discovery service
    

    Nit: Should end with a period.

16. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/FieldMigration.php
    @@ -95,11 +103,14 @@ class FieldMigration extends Migration implements ContainerFactoryPluginInterfac
    +    $this->fieldDiscovery = $field_discovery;
    

    What is this being used for?

Made corrections suggested by @phenaproxima in #44, highlights include:
9. & 13. Tweaked the condition.
16. fieldDiscovery is only used in User.php so moved code to there.

Thanks for fixing my complaints, @Jo Fitzgerald. I see nothing else to block this from landing. Explicit test coverage might be good, but we already have plenty of implicit tests. So I'm not worried about it.

1. +++ b/core/modules/migrate_drupal/src/DrupalMigrationTrait.php
   @@ -0,0 +1,32 @@
   +<?php
   +
   +namespace Drupal\migrate_drupal;
   +
   +use Drupal\migrate\Plugin\MigrationInterface;
   +
   +/**
   + * Provides common functionality for Drupal migrations.
   + */
   +trait DrupalMigrationTrait {
   +
   +  /**
   +   * Find the core version of a Drupal migration.
   +   *
   +   * @param \Drupal\migrate\Plugin\MigrationInterface $migration
   +   *   The migration.
   +   *
   +   * @return string|bool
   +   *   A string representation of the Drupal version, or FALSE.
   +   */
   +  protected function getCoreVersion(MigrationInterface $migration) {
   +    $tags = $migration->getMigrationTags();
   +    if (in_array('Drupal 7', $tags , TRUE)) {
   +      return '7';
   +    }
   +    elseif (in_array('Drupal 6', $tags, TRUE)) {
   +      return '6';
   +    }
   +    return FALSE;
   +  }
   +
   +}
   
   +++ b/core/modules/migrate_drupal/src/FieldDiscovery.php
   @@ -0,0 +1,216 @@
   +  use DrupalMigrationTrait;
   

   Let's not add a trait with one usage in this issue. If we want we can add a follow-up to add the trait and use it in multiple places.

2. +++ b/core/modules/node/src/Plugin/migrate/D6NodeDeriver.php
   @@ -26,57 +26,33 @@ class D6NodeDeriver extends DeriverBase implements ContainerDeriverInterface {
   -   * @var bool
   
   +++ b/core/modules/user/src/Plugin/migrate/User.php
   @@ -11,51 +18,83 @@
   +   * Constructs a FieldMigration.
   

   Constructs a User migration.

   Looking at FieldMigration - can we use this new service there and inject into into the constructor there so we don't have to override all this in the User migration.

I believe this will meet @alexpott's requirements from #47, but I would be interested to know if anyone could suggest a more elegant way to integrate the FieldDiscovery service into FieldMigration.

Fixed the test failures.
Corrected the coding standards errors.

Attempt at a more elegant version, based on the code removed from FieldMigration.

I'll do a review.

Re-rolled.

Re-rolled patch from #51.

Fix the failures and a couple of coding standards errors.

Sweet, this looks nice. All items from #47 are fixed, so really close.

I just found a few small things.

1. +++ b/core/modules/migrate_drupal/src/FieldDiscovery.php
   @@ -0,0 +1,240 @@
   +   * It is an array of arrays. If the entity type is bundlable, a third level of
   

s/bundlable/bundleable/

2. +++ b/core/modules/migrate_drupal/src/FieldDiscovery.php
   @@ -0,0 +1,240 @@
   +   * appropiate field instance source plugin. The array is keyed by
   

s/appropiate/appropriate/

3. +++ b/core/modules/migrate_drupal/src/FieldDiscovery.php
   @@ -0,0 +1,240 @@
   +    $this->logger = $logger;
   

   Probably should be a property instead of declaring dynamically.

4. +++ b/core/modules/migrate_drupal/src/FieldDiscovery.php
   @@ -0,0 +1,240 @@
   +  public function addFields(MigrationInterface $migration, $entity_type = '', $bundle = '') {
   

   Needs an @return.

1. Corrected spelling.
2. Corrected spelling.
3. Declared property.
4. Edited return statement (and comment) because this function is not required to return anything.

1. +++ b/core/modules/migrate_drupal/src/FieldDiscovery.php
   @@ -0,0 +1,247 @@
   +use Drupal\migrate_drupal\Plugin\MigrateCckFieldPluginManagerInterface;
   ...
   +  /**
   +   * The CCK plugin manager.
   +   *
   +   * @var \Drupal\migrate_drupal\Plugin\MigrateCckFieldPluginManagerInterface
   +   */
   +  protected $cckPluginManager;
   ...
   +  public function __construct(MigrateFieldPluginManagerInterface $field_plugin_manager, MigrateCckFieldPluginManagerInterface $cck_field_plugin_manager, MigrationPluginManagerInterface $migration_plugin_manager, LoggerChannelInterface $logger) {
   +    $this->fieldPluginManager = $field_plugin_manager;
   +    $this->cckPluginManager = $cck_field_plugin_manager;
   ...
   +      catch (PluginNotFoundException $ex) {
   +        try {
   +          $plugin_id = $this->cckPluginManager->getPluginIdFromFieldType($field_type, ['core' => $core], $migration);
   +          $manager = $this->cckPluginManager;
   +          $method = isset($pluginDefinition['cck_plugin_method']) ? $pluginDefinition['cck_plugin_method'] : 'processCckFieldValues';
   +        }
   

   This is deprecated - should we trigger a deprecation is this code is used? Also having the cck field plugin manager as the second argument makes it more awkward to remove. I think we need to deprecate the property and argument.

2. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/FieldMigration.php
   @@ -95,11 +100,14 @@ class FieldMigration extends Migration implements ContainerFactoryPluginInterfac
   -  public function __construct(array $configuration, $plugin_id, $plugin_definition, MigrateCckFieldPluginManagerInterface $cck_manager, MigrateFieldPluginManagerInterface $field_manager, MigrationPluginManagerInterface $migration_plugin_manager, MigratePluginManager $source_plugin_manager, MigratePluginManager $process_plugin_manager, MigrateDestinationPluginManager $destination_plugin_manager, MigratePluginManager $idmap_plugin_manager) {
   +  public function __construct(array $configuration, $plugin_id, $plugin_definition, MigrateCckFieldPluginManagerInterface $cck_manager, MigrateFieldPluginManagerInterface $field_manager, MigrationPluginManagerInterface $migration_plugin_manager, MigratePluginManager $source_plugin_manager, MigratePluginManager $process_plugin_manager, MigrateDestinationPluginManager $destination_plugin_manager, MigratePluginManager $idmap_plugin_manager, FieldDiscovery $field_discovery) {
        parent::__construct($configuration, $plugin_id, $plugin_definition, $migration_plugin_manager, $source_plugin_manager, $process_plugin_manager, $destination_plugin_manager, $idmap_plugin_manager);
        $this->cckPluginManager = $cck_manager;
   

   The cckPluginManager is no longer used. Should we remove or deprecate it?

3. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/FieldMigration.php
   @@ -126,44 +135,7 @@ public static function create(ContainerInterface $container, array $configuratio
   -        $method = $this->pluginDefinition[static::PLUGIN_METHOD];
   

   This removal makes \Drupal\migrate_drupal\Plugin\migrate\CckMigration completely obsolete. And it also means that \Drupal\migrate_drupal\Plugin\migrate\FieldMigration::PLUGIN_METHOD should be deprecated because it is not used.

4. As far as I can see the new service is only tests via inference - ie. by all the existing tests of the things that used to have similar code internally but we have no explicit tests of the service on it's own.

5. +++ b/core/modules/migrate_drupal/src/FieldDiscovery.php
   @@ -0,0 +1,247 @@
   +/**
   + * Provides field discovery for Drupal 6 & 7 migrations.
   + */
   +class FieldDiscovery {
   

   As a service it should have an interface so it can be decorated etc... and typehints will continue to work.

This blocks #2970108: Fix "MigrateCckField is deprecated in Drupal 8.3.x and will be removed before Drupal 9.0.x. Use \Drupal\migrate_drupal\Annotation\MigrateField instead." deprecation error as it refactors and consolidates the code that calls the deprecated CckPlugins.

Regarding 70.1, I'm of the opinion that the CCKPluginManager should not be injected at all. This plugin manager should only be instantiated as a BC shim to support modules that may have created a CCKPlugin or an old migration that was created with one. We have a pending subtask in #2959269: [meta] Core should not trigger deprecated code except in tests and during updates to remove the 'MigrateCckFieldPluginManager is deprecated in Drupal 8.3.x and will be removed before Drupal 9.0.x. Use \Drupal\migrate_drupal\Annotation\MigrateFieldPluginManager instead.' deprecation message from the global ignore list, and in order for that to happen, the deprecated service shouldn't be injected.

It makes more sense to me to call this service directly, and only if it's needed (which, after #2970108: Fix "MigrateCckField is deprecated in Drupal 8.3.x and will be removed before Drupal 9.0.x. Use \Drupal\migrate_drupal\Annotation\MigrateField instead." deprecation error won't happen anywhere in the core test suite outside of legacy tests. This patch adds this in.

for 70.2, Both CCKPluginManager and FieldPluginManager are no longer needed in FieldMigration. The D7Comment Migration extends FieldMigration and accesses the FieldPluginManager, but it should be using this service instead. The problem is the D7 Comment Migration does not currently respect bundles (see #3005718: D7 comment migration does not properly migrate fields by comment bundle.) and can't use this service as it sits. This patch takes both FieldPluginManager and CCKPluginManager out of FieldMigration, and adds FieldPluginManager directly into D7 Comment, with the intention that that migration is fixed up to respect bundles and use this service in #3005718: D7 comment migration does not properly migrate fields by comment bundle., making this issue a blocker for that as well.

I have not dug into the ramifications of 70.3 yet.

I have not created service tests for 70.4 yet.

I added an interface for 70.5

I offer: Test Coverage.

A few typehints changed from FieldDiscovery to FieldDiscoveryInterface, and I added a deprecated annotation to the constant referenced in 70.3

And removing two helper functions I added to the test base that I didn't end up using.

Completely uninteresting doc and formatting fixes.

And a couple more..

Testbot fodder, please ignore.

More testbot food. Getting closer. Needs more tests.

Adding some more work here. Addressed some feedback and made some more changes. I still need more tests to be added.

I used the new deprecated service injection trait for cck field manager, but I was forced to create the d6 phone and number plugins to avoid triggering its error message. I will repurpose the related issue for a cleanup of the d6 field migration processes.

I addressed some of the feedback, but I need to go over it again. Removed the checkcore method, it was redundant. I still need to go through all the documentation though.

Need to enable modules providing field plugins so they are discovered and we don't trigger the deprecation errors.

I've pretty much decided to not use the deprecated service injector here, since it is clearly require more cleanup than I want to inject into this issue. Going back to a custom private getter for the cck field plugin manager will kick the can down the road, but we still need to deal with the problem with the field plugin dependencies.

Not enabling the field modules in the migration tests is a problem. Nearly all of the normal tests around the migration system start with the field/field instance migrations and if you run those migrations without the modules holding the field plugins enabled, then you are not running them correctly, and who knows how it will affect tests. The whole thing may speak to a bigger requirements problem around the field plugins in that those migrations can be run at all without the field plugins against a database that has those fields, but either way, if we are going to run those migrations in a core test, then the field modules that support them should be enabled, even more so because we have to avoid falling back to the deprecated cck stuff.

Other potential solutions I can branstorm in no particular order and without much regard for their consequences:

Move all field plugins into the field module - Possible, but would require additional requirement checks somewhere, as the field plugins would be enabled but the module containing the field type might not be. The advantage being that we could detect that and decide on an explicit action, a warning, or something saying that you can't run this migration without the 'link' module enabled, or an option to skip all link fields, or something..

Create a private means of discovering a list CCK field plugins without triggering a deprecation error, and only create the manager if the plugin is on this list. If feels odd writing this much code to support a deprecated system, but if it was private and isolated, it would ensure that in core only tests that we don't trigger the deprecated code, since we won't find a cck plugin. still a little sketchy, since the remaining deprecated cck plugins trigger the error in the file and not the constructor, but those could maybe be fixed.

A hard coded list of field plugins provided by core modules, and we don't check the cck manager for plugins on that list. Potential bc break, and just stinks of a bad idea.

Some broader requirements improvements, but again, BC becomes an issue. The migrations shouldn't necessarily fail because the core field module isn't enabled.

Assigning to myself to work on this week.

This patch is a reset patch. I removed the new field plugins and deprecated service injector, and I'll revisit them in a separate issue. I kept the additional tests, rerolled, adjusted some tests around some recent fixture changes. Hopefully it's back to green, and I can add the last couple tests I want and get a real review.

Well, who deprecated a perfectly good test trait while I wasn't looking??

Back to passing tests, keeping assigned to myself for another go-over before submitting for review.

Assigning myself for Reviewing on migrate meeting

Looks good, Moving to RTBC

@rakesh.gectcr, did you also review both Change Records as well?

@quietone Sorry, I am not reviewed the CR.

OK, thanks for making that clear.
Then let's set this to NR for a review of the CRs.

I agree with rakesh.gectcr, this is looking so good. I'm eager to get this in. I started looking at the CR's, which I think are fine but could use another set of eye on them as well, and realized I really need to look at the patch as it has been a long while since I last looked at it. Maybe it is the late hour but I am finding it difficult to track that fixes were made for each issue raised. I'll try again on the weekend, I hope.

I'd like to see an IS update. It seems to me there is valuable history in the comments that should be in the IS. Like why are there changes to the comment and user tests? And are there any followups or todos. So, I'm adding that tag.

And I found a few things.

1. +++ b/core/modules/migrate_drupal/src/FieldDiscovery.php
   @@ -0,0 +1,384 @@
   +   * It is an array of arrays. If the entity type is bundleable, a third level of
   

   Line greater than 80 characters.

2. +++ b/core/modules/migrate_drupal/src/FieldDiscovery.php
   @@ -0,0 +1,384 @@
   +        // @todo Replace below try/catch block with $plugin = FALSE for Drupal 9.
   

   line > 80 characters

3. +++ b/core/modules/migrate_drupal/src/FieldDiscoveryInterface.php
   @@ -0,0 +1,68 @@
   +  const DRUPAL_6 = '6';
   

   #85 suggests using these values from which "are stored as config keys in migrate_drupal.settings.enforce_source_module_tags." Is there a followup for that?

4. +++ b/core/modules/migrate_drupal/src/FieldDiscoveryInterface.php
   @@ -0,0 +1,68 @@
   +   * for all field plugins applicable to this Drupal -> Drupal migration. This
   ...
   +   * for all field plugins applicable to this Drupal -> Drupal migration. This
   

   s/->/to/

5. +++ b/core/modules/user/tests/src/Kernel/Migrate/d7/MigrateUserTest.php
   @@ -39,7 +39,15 @@ class MigrateUserTest extends MigrateDrupal7TestBase {
   +    // Enable profile module for testing.
   

   Can you MigrateDumpAlterInterface for this. An example is in d6\MigrateFileTest

The same doc bloc is used for addAllFieldProcesses and addEntityFieldProcesses and it is hard to know the difference. And thenaddBundleFieldProcesses is spartan with no explanation. They could use some improved comments, maybe even including an example migration where each is used.

Sorry, back to NW

Wanted to keep this moving along and decided to fix items 1-5 in #102. It also include a doc block for assertSourcePlugin.

See comment #25 about why #104.3 was added. It increases the amount of code covered by MigrateUserTest, and without coverage over that section, I was able to introduce a bug that wasn't caught by tests.

Maybe, instead of altering MigrateUserTest that should be done here, #3030931: User profile migration needs tests , which hopefully will expose the problem.

#104.1. Issue made #3032317: Remove use of MigrateCckFieldInterface in FieldDiscovery service

104.2 - Issue made #3033733: Prevent discovery of CckPlugins in Drupal 9

What is left is to sort out MigrateUserTest. There is something bothering me about it but I'm not seeing it right now.

Actually, I don't see that the IS was updated. @heddn, looks like your changes to the IS in #104 weren't saved. :-( Correct me if I am wrong.

Remove the changes to the user test.

Fix the tests and the coding standard errors.

@heddn, thanks for the prompt review.

Yes, lets get this in!

Adding credit to @phenaproxima and myself for code review.

Committed and pushed a916e4d4fa to 8.8.x and fa94a4df4f to 8.7.x. Thanks!

Woohoo! Thanks everyone!!

WooHoo!! Very glad to see this one make it in.

It needs follow-up to fix broken pgsql tests https://www.drupal.org/node/3060/qa

1) Drupal\Tests\migrate_drupal\Kernel\d6\FieldDiscoveryTest::testGetAllFields
Failed asserting that Array &0 (
    0 => 'employee'
    1 => 'test_planet'
    2 => 'page'
    3 => 'test_page'
    4 => 'story'
) is identical to Array &0 (
    0 => 'employee'
    1 => 'test_planet'
    2 => 'page'
    3 => 'story'
    4 => 'test_page'
).

+++ b/core/modules/migrate_drupal/tests/src/Kernel/d6/FieldDiscoveryTest.php
@@ -0,0 +1,304 @@
+    $this->assertSame(['employee', 'test_planet', 'page', 'story', 'test_page'], array_keys($actual_fields['node']));

+++ b/core/modules/migrate_drupal/tests/src/Kernel/d7/FieldDiscoveryTest.php
@@ -0,0 +1,364 @@
+    $this->assertSame(['comment', 'node', 'user', 'taxonomy_term'], array_keys($actual_fields));

not clear why 6 & 7 tests are so different

Filed follow-up #3038899: Fix postgresql tests after 2951550

not clear why 6 & 7 tests are so different

+++ b/core/modules/migrate_drupal/tests/src/Kernel/d6/FieldDiscoveryTest.php
@@ -0,0 +1,304 @@
+    $this->assertSame(['node'], array_keys($actual_fields));
+    $this->assertSame(['employee', 'test_planet', 'page', 'story', 'test_page'], array_keys($actual_fields['node']));
+    $this->assertSame(21, count($actual_fields['node']['story']));

+++ b/core/modules/migrate_drupal/tests/src/Kernel/d7/FieldDiscoveryTest.php
@@ -0,0 +1,364 @@
+    $this->assertSame(['comment', 'node', 'user', 'taxonomy_term'], array_keys($actual_fields));
+    $this->assertArrayHasKey('test_vocabulary', $actual_fields['taxonomy_term']);
+    $this->assertArrayHasKey('user', $actual_fields['user']);
+    $this->assertArrayHasKey('test_content_type', $actual_fields['node']);
+    $this->assertSame(6, count($actual_fields['node']));
+    $this->assertSame(6, count($actual_fields['comment']));
+    $this->assertSame(22, count($actual_fields['node']['test_content_type']));

The tests aren't really different. The first line tests that all fieldable entities are found for the version, which for d6 are only nodes, and for d7 includes users, taxonomy, and comments, then we check that expected bundles show up then check some field counts under some bundle. The real difference is that since there are only node bundles in d6, we just checked that all the expected ones were there in a single array check, while we did a couple spot checks in d7 because there were more entity types and bundles to manage.