Problem/Motivation

The MigrateCckFieldPluginManager class overrides the createInstance method to create an instance using a field_type instead of the usual plugin_id. However, when loading definitions in CckMigration, it uses hasDefinition method which uses plugin_id. While debugging, I noticed that some of the fields were being skipped because of this but they are covered by the default map present in d7_field.yml and they went through correctly. With this, and the default process map generated in the derived node migration, the migration works.

But the problem remains for more complex field types (like address field). I created a cckfield plugin with id address and the field type in D7 is addressfield. Because of this, the plugin is never loaded because of the outer hasDefinition check.

      foreach ($source_plugin as $row) {
        $field_type = $row->getSourceProperty('type');
        if (!isset($this->processedFieldTypes[$field_type]) && $this->cckPluginManager->hasDefinition($field_type)) {
          $this->processedFieldTypes[$field_type] = TRUE;
          // Allow the cckfield plugin to alter the migration as necessary so
          // that it knows how to handle fields of this type.
          if (!isset($this->cckPluginCache[$field_type])) {
            $this->cckPluginCache[$field_type] = $this->cckPluginManager->createInstance($field_type, [], $this);
          }
          call_user_func([$this->cckPluginCache[$field_type], $this->pluginDefinition['cck_plugin_method']], $this);
        }
      }

Given the narrow set of errors that this produces which are difficult to debug, I am marking this as major. IMHO, this should also be a migrate critical.

Proposed resolution

There are two ways to do this.

  • Like the createInstance method, override hasDefinition and getDefinition to load by field_type instead of plugin_id.
  • Create new methods with clear naming that indicates field_type should be used instead of plugin_id. If we do this, we should ideally rename createInstance too.
  • Add a new method to get the plugin id from the field type and restore createInstance to its original definition.

Remaining tasks

User interface changes

None

API changes

None

Data model changes

None

CommentFileSizeAuthor
#8 2787639-8.patch11.72 KBhussainweb
#8 interdiff-5-8.txt3.61 KBhussainweb
#5 2787639-5.patch12.15 KBhussainweb
#5 interdiff-2-5.txt3.78 KBhussainweb
#2 2787639-2.patch8.37 KBhussainweb
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

hussainweb created an issue. See original summary.

hussainweb’s picture

hussainweb
Primary language English
CreditAttribution: hussainweb at Axelerant commented
Issue summary: View changes
Status: Active » Needs review
FileSize
2787639-2.patch8.37 KB

I am trying out a different approach altogether. I am not overriding hasDefinition or other methods but restoring createInstance to its original meaning. Instead, I am adding a new method to get the plugin id from the field type.

Status: Needs review » Needs work

The last submitted patch, 2: 2787639-2.patch, failed testing.

hussainweb’s picture

hussainweb
Primary language English
CreditAttribution: hussainweb at Axelerant commented

The tests fail because the test relies on the behaviour of createInstance we are trying to change. I will work on that. However, I just wanted to update that this worked nicely in manual tests with the process map getting updated correctly and data being migrated with nodes.

hussainweb’s picture

hussainweb
Primary language English
CreditAttribution: hussainweb at Axelerant commented
Status: Needs work » Needs review
FileSize
interdiff-2-5.txt3.78 KB
2787639-5.patch12.15 KB

Fixing the failures.

hussainweb’s picture

hussainweb
Primary language English
CreditAttribution: hussainweb at Axelerant commented
Related issues: +#2726803: Field formatters with names different than their field type can not be migrated, +#2787619: CckMigration does not type hint CckPluginManager correctly, +#2594631: Migration integration
phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
CreditAttribution: phenaproxima at Acquia commented
Status: Needs review » Needs work

Overall I think this looks good.

  1. +++ b/core/modules/migrate_drupal/src/Plugin/MigrateCckFieldPluginManager.php
@@ -27,9 +27,22 @@ class MigrateCckFieldPluginManager extends MigratePluginManager {
+   * Get the plugin id from the field type.

    s/id/ID 

  2. +++ b/core/modules/migrate_drupal/src/Plugin/MigrateCckFieldPluginManager.php
@@ -27,9 +27,22 @@ class MigrateCckFieldPluginManager extends MigratePluginManager {
+   *   The id of the field type being migrated.

    Can this just be "The field type being migrated" or "The legacy field type"? 

  3. +++ b/core/modules/migrate_drupal/src/Plugin/MigrateCckFieldPluginManager.php
@@ -27,9 +27,22 @@ class MigrateCckFieldPluginManager extends MigratePluginManager {
+   *   The current migration instance.

    Missing (optional). 

  4. +++ b/core/modules/migrate_drupal/src/Plugin/MigrateCckFieldPluginManager.php
@@ -27,9 +27,22 @@ class MigrateCckFieldPluginManager extends MigratePluginManager {
+   *   The plugin_id of the plugin for the field_type if available.

    s/plugin_id/plugin ID 

  5. +++ b/core/modules/migrate_drupal/src/Plugin/MigrateCckFieldPluginManager.php
@@ -27,9 +27,22 @@ class MigrateCckFieldPluginManager extends MigratePluginManager {
+   *   If the instance cannot be created, such as if the field type is invalid.

    Can we change this to "If the plugin ID cannot be determined"? 

  6. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/CckMigration.php
@@ -106,14 +107,20 @@ public function getProcess() {
+          if (!isset($this->processedFieldTypes[$field_type])) {
+            $this->processedFieldTypes[$field_type] = TRUE;
+            // Allow the cckfield plugin to alter the migration as necessary so
+            // that it knows how to handle fields of this type.
+            if (!isset($this->cckPluginCache[$field_type])) {
+              $this->cckPluginCache[$field_type] = $this->cckPluginManager->createInstance($plugin_id, [], $this);
+            }
+            call_user_func([$this->cckPluginCache[$field_type], $this->pluginDefinition['cck_plugin_method']], $this);
           }

    For readability, can all of this be moved out of the try block, and just put a return in the catch?

hussainweb’s picture

hussainweb
Primary language English
CreditAttribution: hussainweb at Axelerant commented
Status: Needs work » Needs review
FileSize
interdiff-5-8.txt3.61 KB
2787639-8.patch11.72 KB

Thanks for the review. All fixed. We can't put a return in the catch because the loop has to continue. I tried putting a continue; instead. Please review to see if it is more readable than before.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
CreditAttribution: phenaproxima at Acquia commented 
+++ b/core/modules/migrate_drupal/src/Plugin/MigrateCckFieldPluginManager.php
@@ -27,22 +27,22 @@ class MigrateCckFieldPluginManager extends MigratePluginManager {
+   *   (optioanl) An array of configuration relevant to the plugin instance.

Misspelled optional, but this is fixable on commit.

Otherwise, I like it! If nobody else has objections, I'd say this is RTBC once Drupal CI passes it.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
CreditAttribution: phenaproxima at Acquia commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +Migrate critical

This is a pretty big WTF, so I'm marking it Migrate-critical. Luckily, it is also my opinion that this patch is RTBC :)

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 8: 2787639-8.patch, failed testing.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
CreditAttribution: phenaproxima at Acquia commented
Status: Needs work » Reviewed & tested by the community

Wrong!

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed cea3af0 to 8.3.x and 4607ba0 to 8.2.x and 3876957 to 8.1.x. Thanks!

+++ b/core/modules/migrate_drupal/src/Plugin/MigrateCckFieldPluginManager.php
@@ -27,9 +27,22 @@ class MigrateCckFieldPluginManager extends MigratePluginManager {
+  public function getPluginIdFromFieldType($field_type, array $configuration = [], MigrationInterface $migration = NULL) {

Ideally this should be on an interface... but let's add that in #2787619: CckMigration does not type hint CckPluginManager correctly

diff --git a/core/modules/migrate_drupal/src/Plugin/MigrateCckFieldPluginManager.php b/core/modules/migrate_drupal/src/Plugin/MigrateCckFieldPluginManager.php
index 3af95fb..7920040 100644
--- a/core/modules/migrate_drupal/src/Plugin/MigrateCckFieldPluginManager.php
+++ b/core/modules/migrate_drupal/src/Plugin/MigrateCckFieldPluginManager.php
@@ -33,7 +33,7 @@ class MigrateCckFieldPluginManager extends MigratePluginManager {
    *   The field type being migrated.
    * @param array $configuration
    *   (optioanl) An array of configuration relevant to the plugin instance.
-   * @param \Drupal\migrate\Plugin\MigrationInterface|NULL $migration
+   * @param \Drupal\migrate\Plugin\MigrationInterface|null $migration
    *   (optional) The current migration instance.
    *
    * @return string

Fixed on commit.

  • alexpott committed cea3af0 on 8.3.x 
    Issue #2787639 by hussainweb: MigrateCckFieldPluginManager mixes up its...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.