Derive metatag migrations by entity type (and bundle)

#3165112 has been committed.

Lovely, thank you @DamienMcKenna! When are you planning another release?

I don't have another release currently planned.

This is unfortunately nowhere not quite ready yet 😔

I started reviewing and improving #7 this morning. In the mean time, @tim.plunkett has posted #8.

Here's a partial review plus reroll with lots of fixes. It's still not fully finished though. In particular, the migration dependencies need a lot more attention. And d7_metatag_field_instance_widget_settings has not yet been updated at all yet …

1. +++ b/src/Migrate/MigrationPluginAlterer.php
   @@ -0,0 +1,193 @@
   +      $target_entity_type_id = $migration_plugin['source']['entity_type_id'];
   +      $target_entity_bundle = $migration_plugin['source']['bundle'] ?? NULL;
   +      $content_entity_migration_id = NULL;
   +      $required_dependencies = [];
   +
   +      $content_entity_migration_id = ($target_entity_type_id === 'node' && $complete_node_migration) ? 'd7_node_complete' : "d7_$content_entity_migration_id";
   +      if (!isset($migrations[$content_entity_migration_id])) {
   +        continue;
   +      }
   +
   +      // If the migration is derived by both entity type and bundle, we
   +      // filter on the potential derived entity migrations, and only add
   +      // what we need.
   +      if ($target_entity_bundle) {
   

   When $target_entity_type_id === 'node' and $target_entity_bundle === 'article' (for example), this will result in $content_entity_migration_id == 'd7_node_complete', which is just the base plugin ID.

   That next if-test does take into account the bundle. So this patch is compatible with Drupal core, but not with Drupal core plus #3097336: Improve the DX for migrating content entities: view modes, fields, formatters, widgets etc should be migrated per entity type + bundle applied.

   (The d7_node_complete migration "disappears" and only derived ones remain when that is applied.)

   Let's make this compatible with both an unpatched Drupal core and this core patch.

2. +++ b/src/Migrate/MigrationPluginAlterer.php
   @@ -0,0 +1,193 @@
   +      $migration_plugin['migration_dependencies']['required'] = array_unique(array_merge($migration_plugin['migration_dependencies']['required'], array_values($required_dependencies)));
   

   Precise dependencies being added here means we should remove this from d7_metatag_field_instance.yml

       # @todo Is this accurate? Does it really need the vocabulary migration, or
       # is it more precautionary, that it *might* be needed so it might as well be
       # executed first?
       - d7_node_type
       - d7_taxonomy_vocabulary
   
   

3. +++ b/src/Plugin/migrate/source/d7/MetatagFieldDeriver.php
   @@ -0,0 +1,98 @@
   +  /**
   +   * Required entity type DB tables, keyed by the entity type ID.
   +   *
   +   * @var string[][]
   +   */
   +  protected $supportedEntityTypesTables = [
   +    'node' => ['node', 'node_type'],
   +    'taxonomy_term' => ['taxonomy_term_data', 'taxonomy_vocabulary'],
   +    'user' => [],
   +  ];
   

   This is not used.

4. +++ b/src/Plugin/migrate/source/d7/MetatagFieldDeriver.php
   @@ -0,0 +1,98 @@
   +   * Constructs a PathRedirectDeriver instance.
   

   c/p leftover

5. +++ b/src/Plugin/migrate/source/d7/MetatagFieldDeriver.php
   @@ -0,0 +1,98 @@
   +    foreach ($entity_type_ids as $entity_type_id) {
   ...
   +        '@type' => $this->entityTypeManager->getDefinition($entity_type_id)->getPluralLabel(),
   

   This assumes that all source entity types also exist on the destination site — if they don't, this will cause the migration deriver to crash.

6. +++ b/src/Plugin/migrate/source/d7/MetatagFieldDeriver.php
   @@ -0,0 +1,98 @@
   +    $this->derivatives['other'] = $base_plugin_definition;
   

   This is blindly adding the other case. It should only be added if there indeed is an "other".

   See #3051251-14: Existing menu links show validation issues on migration (and ALL menu links pointing to node translations are invalid) or #3122649-34: [PP-2] Derive path alias migrations per entity type (and bundle) for an example.

   Furthermore, this is a migration definition which is not actually restricting the source plugin to those which are "other".

   EDIT: upon closer inspection, metatags can by definition only be applied to entities. Therefore the "other" derivative doesn't really make sense here, unlike for the path alias and path redirect migrations (the latter of which is the origin for much of this code). Because paths are independent of entities, a "other" category is necessary. No such restriction is needed here! 😊

   Yay for simplification! :D

7. +++ b/src/Plugin/migrate/source/d7/MetatagFieldInstanceDeriver.php
   @@ -0,0 +1,174 @@
   +   * Constructs a PathRedirectDeriver instance.
   

   c/p leftover

8. +++ b/src/Plugin/migrate/source/d7/MetatagFieldInstanceDeriver.php
   @@ -0,0 +1,174 @@
   +          // We want to get a per-node-type path redirect migration. So we
   

   c/p leftover

9. +++ b/src/Plugin/migrate/source/d7/MetatagFieldInstanceDeriver.php
   @@ -0,0 +1,174 @@
   +          $foo = '';
   

   Debug leftover 🤓

10. +++ b/src/Plugin/migrate/source/d7/MetatagFieldInstanceDeriver.php
    @@ -0,0 +1,174 @@
    +          $this->derivatives[$derivative_id]['source']['entity_type_id'] = $entity_type_id;
    +          $this->derivatives[$derivative_id]['source']['entity_type'] = $entity_type_id;
    +          $this->derivatives[$derivative_id]['source']['bundle'] = $bundle_id;
    

    ⚠️ These restrictions are not yet being respected by \Drupal\metatag\Plugin\migrate\source\d7\MetatagFieldInstance — and for that reason every derivative still returns all metatag field instances as rows to migrate.

#8:

1. +++ b/migrations/d7_metatag_field_instance.yml
   @@ -24,9 +24,6 @@ destination:
   -    # The base field migration is required before this migration can run.
   -    - d7_metatag_field
   

   We can't just remove this, that could break existing migrations. We just need to refine this in the deriver! Existing migrations won't inherit the deriver, so they'll be fine.

2. +++ b/src/Migrate/MigrationPluginAlterer.php
   @@ -82,9 +92,6 @@ public function alterMigrationPlugins(array &$migrations) {
   -      if (!isset($migrations[$content_entity_migration_id])) {
   -        continue;
   -      }
   

   I see you noticed this was wrong, as I pointed out in #9.1 🤓👍

3. +++ b/src/Migrate/MigrationPluginAlterer.php
   @@ -92,6 +99,7 @@ public function alterMigrationPlugins(array &$migrations) {
   +          $required_dependencies[] = sprintf('d7_%s:%s', $this->entityTypeManager->getDefinition($target_entity_type_id)->getBundleEntityType(), $target_entity_bundle);
   

   This assumes that #3097336: Improve the DX for migrating content entities: view modes, fields, formatters, widgets etc should be migrated per entity type + bundle is applied, which we cannot. (See #9.1 again.)

4. +++ b/src/Migrate/MigrationPluginAlterer.php
   @@ -105,6 +113,7 @@ public function alterMigrationPlugins(array &$migrations) {
   +      $migration_plugin['migration_dependencies']['required'] = array_diff($migration_plugin['migration_dependencies']['required'], ['d7_comment_type', 'd7_node_type', 'd7_taxonomy_vocabulary']);
   

   What's the purpose of this? 🤔

It's still not fully finished though. In particular, the migration dependencies need a lot more attention. And d7_metatag_field_instance_widget_settings has not yet been updated at all yet …

This solves all that.

Interdiff explained:

1. +++ b/src/Plugin/migrate/source/d7/MetatagFieldInstanceDeriver.php
   @@ -62,7 +62,7 @@ class MetatagFieldInstanceDeriver extends DeriverBase implements ContainerDerive
   -    $source = $this->getSourcePlugin('d7_metatag_field_instance');
   +    $source = $this->getSourcePlugin($base_plugin_definition['source']['plugin']);
   

   This was necessary to make it work for both d7_metatag_field_instance and d7_metatag_field_instance_widget_settings.

2. +++ b/src/Plugin/migrate/source/d7/MetatagFieldInstanceDeriver.php
   @@ -157,6 +157,31 @@ class MetatagFieldInstanceDeriver extends DeriverBase implements ContainerDerive
   +          // Add bundle dependency.
   +          switch ($entity_type_id) {
   +            case 'node':
   +              $this->derivatives[$derivative_id]['migration_dependencies']['required'][] = "d7_node_type:$bundle_id";
   +              break;
   +            case 'taxonomy_term':
   +              $this->derivatives[$derivative_id]['migration_dependencies']['required'][] = "d7_taxonomy_vocabulary:$bundle_id";
   +              break;
   +          }
   

   This should still be generalized to other entity types, but it it covers the same set of explicitly supported integrations as in HEAD.

And clean-up. The MigrationPluginAlterer class is obsolete as of #12.

Interdiff explained:

+++ /dev/null
@@ -1,193 +0,0 @@
-  public function alterMigrationPlugins(array &$migrations) {
-    $complete_node_migration = $this->isCompleteNodeMigration($migrations);
-    $metatag_migration_ids = array_keys(array_filter($migrations, function ($migration) {
-      return $migration['provider'] === 'metatag' && in_array('Drupal 7', $migration['migration_tags']) && !empty($migration['deriver']) && !empty($migration['source']['entity_type_id']);
-    }));
-
-    foreach ($metatag_migration_ids as $metatag_migration_id) {
-      $migration_plugin = &$migrations[$metatag_migration_id];
-      $migration_plugin += ['migration_dependencies' => []];
-      $migration_plugin['migration_dependencies'] += [
-        'required' => [],
-        'optional' => [],
-      ];
-
-      $target_entity_type_id = $migration_plugin['source']['entity_type_id'];
-      $target_entity_bundle = $migration_plugin['source']['bundle'] ?? NULL;
-      $content_entity_migration_id = NULL;
-      $required_dependencies = [];
-
-      $content_entity_migration_id = ($target_entity_type_id === 'node' && $complete_node_migration) ? 'd7_node_complete' : "d7_$content_entity_migration_id";
-      if (!isset($migrations[$content_entity_migration_id])) {
-        continue;
-      }
-
-      // If the migration is derived by both entity type and bundle, we
-      // filter on the potential derived entity migrations, and only add
-      // what we need.
-      if ($target_entity_bundle) {
-        if (isset($migrations["$content_entity_migration_id:$target_entity_bundle"])) {
-          $required_dependencies[] = "$content_entity_migration_id:$target_entity_bundle";
-        }
-        elseif (isset($migrations[$content_entity_migration_id])) {
-          $required_dependencies[] = $content_entity_migration_id;
-        }
-      }
-      // If this migration is derived only by entity type, we already know the
-      // dependencies.
-      else {
-        if (isset($migrations[$content_entity_migration_id])) {
-          $required_dependencies[] = $content_entity_migration_id;
-        }
-      }
-
-      $migration_plugin['migration_dependencies']['required'] = array_unique(array_merge($migration_plugin['migration_dependencies']['required'], array_values($required_dependencies)));
-    }
-  }

This was making the d7_metatag_field_instance:<entity type ID>:<bundle ID> migration definitions depend on the corresponding <entity type ID>:<bundle ID> migration.

But … that does not actually make sense.

It makes sense for depending content entities like the Redirect content entity that the d7_path_redirect migration does (which served as the inspiration for much of this code).

But in this case, the metatag information is part of the <entity type ID>:<bundle ID> entities it is for, so these dependencies were wrong.

See

+++ b/metatag.module
@@ -830,12 +830,38 @@ function metatag_migration_plugins_alter(array &$definitions) {
+        // When there are no bundle derivatives, make e.g. the d7_node_complete
+        // migration depend on:
+        // - d7_metatag_field:node
+        // - d7_metatag_field_instance:node
+        // - d7_field_instance_widget:node
+        // but if there is a bundle derivative such as d7_node_complete:article,
+        // then instead make it depend on:
+        // - d7_metatag_field:node
+        // - d7_metatag_field_instance:node:article
+        // - d7_field_instance_widget:node:article
+        // Either way, this matches the dependencies used by for example
+        // d7_node_complete, which has dependencies on d7_field_instance and
+        // d7_comment_field_instance to ensure correct migration order.
+        if ($bundle_id && isset($definitions["d7_metatag_field_instance:$entity_type_id:$bundle_id"])) {
+          $definition['migration_dependencies']['required'][] = "d7_metatag_field:$entity_type_id";
+          $definition['migration_dependencies']['required'][] = "d7_metatag_field_instance:$entity_type_id:$bundle_id";
+          $definition['migration_dependencies']['required'][] = "d7_metatag_field_instance_widget_settings:$entity_type_id:$bundle_id";
+        }
+        elseif (isset($definitions["d7_metatag_field_instance:$entity_type_id"])) {
+          $definition['migration_dependencies']['required'][] = "d7_metatag_field:$entity_type_id";
+          $definition['migration_dependencies']['required'][] = "d7_metatag_field_instance:$entity_type_id";
+          $definition['migration_dependencies']['required'][] = "d7_metatag_field_instance_widget_settings:$entity_type_id";
+        }

which #12 added.

I cannot reproduce this failure locally 😬

That's an unusual error, it might be dependent upon the version of MySQL being used.

I've started another test run using MySQL 5.5.

Ok, the tests are passing again, thank you Wim.

This is excellent work, thank you both.

What's next for this?

What's next for this?

Nothing AFAIK 😊

Excellent, thank you both!

Exciting! 🚀

Committed. Thank you both!

I originally posted complaining that this broke my migrations with no documentation I could find. I see this has changed migration quite a bit, it is better, and necessary, but potentially adds many migrations needed to migrate the site.