Improve the DX for migrating content entities: view modes, fields, formatters, widgets etc should be migrated per entity type + bundle

I've looked at this over the past days (while on holiday) and every time I am not convinced this is needed. I am happy to be wrong about that and maybe I still have blinders on because of #2208401: [META] Remaining multilingual migration paths.

Speaking from a user/developer experience POV, I like it because it allows me to divide my migration pain into more manageable chunks.

If I were to migrate Drupal.org to D8, for example, we have, let's see...

- ~85,500,000 nodes
- ~8,000,000 comments (and that's just the published ones :P)
- ~2,000,000 users

...and so on.

That's an extreme example. :) But even in my crappy D7 blog, this would allow me to focus on one or two smaller areas of the migration (e.g. custom blocks, or pages, for which I only have a few) prior to getting into the stuff with more hard-core dependencies that need thought/work (e.g. a couple blog posts have the PHP filter, oh noes), and still experience early "hey things are happening!" success.

Refined the patch in #2 so that it will be easily used for other entity types and bundles. I think I lost some of the work to establish proper dependencies that was in #2. I'll restore that tomorrow.

Okay, this now breaks the field configuration migration out into separate migrations per entity type and sets up the appropriate dependencies for nodes, terms and field instances.

Next up: views modes, field formatter and field widget configurations.

Alright this patch completes the derivation of all migrations listed in the issue summary, it also adds the d7_view_modes, d7_field_formatter_settings, and d7_field_instance_widget_settings migrations as optional dependencies of the d7_node and d7_comment migrations as this information was previously missing.

This should clean up some undefined index errors and a copy pasta problem with an assert that was mucking up the watchdog logs.

I think there are still some weird dependency things going one where certain derived node migrations are depending on all of the derived view mode migrations instead of just depending on the ones that it "cares" about.

Fixes the view mode thing.

I'm not sure that replicating like rabbits the various derivers helps anything in core. If someone wanted to add this in contrib (migrate_upgrade???) and incubate it there for a while, there might be some benefit. But doing this in core for core's sake wouldn't really help much at this point.

Yes, that is what has been bothering me about this work, that is, core does not need this. So let's move it to contrib and keep this valuable work. I have no suggestion for where to put it and mgrate_upgrade is definitely a possibility. I would like to mention that there is an issue to move migrate_upgrade to drush itself #2709537: Move Drush commands into Drush itself. Maybe that will influence the decision?

So, migrate_plus?

All things related to upgrading from a previous version of drupal, without regard to if that means drush or web ui or whatever should really land in migrate upgrade. So while drush is the only mechanism in migrate upgrade, that doesn't mean someone could add a web ui.

Yes.

Implementation details... we'd want to have an additional flag to the drush command to use this option. Or maybe an option to turn it off. Some way to flag it.

It would work however we wanted it to work. There could be a state flag or something. It could be developed initially for drush but later made available for a web ui. 🤷‍♂️

Do this and the similar issues need change notices?

I would like to close this, but I'm setting to nw because it needs a follow up issue filed in migrate upgrade. Once that is done, this should be closed won't fix and the new issue linked.

Alternatively, you can just move tickets from one project queue to another without closing them.

I prefer the alternative and will take the action to move this to Migrate Upgrade.

Patch #30 still relative to Drupal core.

To be able to apply it on core, you have to apply:

• #3096951-8: d7_node migration should have dependency on d7_node_title_label migration, and
• #3097314-2: d7_comment migration should have dependency on d7_comment_entity_display, same for d7_custom_block + block_content_entity_display

Taxonomy term (and user) derivatives should be fine.

Still @TODO: node_type, comment_type.

Adding the missing WIP patch (still on top of core + the patches from #33).

This patch fixes the taxonomy vocabulary migrate source plugin issue we have in #35, and a regression of the comment migrations.

Drupal 7 node type migrations are derivered migrations.

1. +++ b/core/modules/field/src/Plugin/migrate/D7FieldConfigurationMigrationDeriver.php
   @@ -19,13 +21,31 @@ final class D7FieldConfigurationMigrationDeriver extends DeriverBase {
   +
   +    try {
   +      $source->checkRequirements();
   +    }
   +    catch (RequirementsException $e) {
   +      // If the source plugin requirements failed, that means we do not have a
   +      // Drupal source database configured - there is nothing to generate.
   +      return $this->derivatives;
   +    }
   +
   
   +++ b/core/modules/field/src/Plugin/migrate/D7FieldInstanceMigrationDeriver.php
   @@ -19,15 +21,33 @@ class D7FieldInstanceMigrationDeriver extends DeriverBase {
   +
   +    try {
   +      $source->checkRequirements();
   +    }
   +    catch (RequirementsException $e) {
   +      // If the source plugin requirements failed, that means we do not have a
   +      // Drupal source database configured - there is nothing to generate.
   +      return $this->derivatives;
   +    }
   +
   

   These try / catch blocks are really important: without these, if we don't have any fields to migrate, we would see exceptions instead of our Drupal site 😶.

2. +++ b/core/modules/node/migrations/d7_node_type.yml
   @@ -3,6 +3,7 @@ label: Node type configuration
   +deriver: Drupal\node\Plugin\migrate\D7NodeDeriver
   
   +++ b/core/modules/node/src/Plugin/migrate/D7NodeDeriver.php
   @@ -74,24 +74,31 @@ public function getDerivativeDefinitions($base_plugin_definition) {
    
   -    $node_types = static::getSourcePlugin('d7_node_type');
   +    $source_plugin = $this->getSourcePlugin($base_plugin_definition['source']['plugin']);
        try {
   -      $node_types->checkRequirements();
   +      $source_plugin->checkRequirements();
        }
   

   Since D7NodeDeriver almost fulfilled my needs (explode the d7_node_type migration to derived migrations), I modified this class instead of adding a new one.

3. +++ b/core/modules/node/src/Plugin/migrate/D7NodeDeriver.php
   @@ -115,26 +122,40 @@ public function getDerivativeDefinitions($base_plugin_definition) {
   -          $values['migration_dependencies']['required'][$node_title_dep_index] .= ":$node_type";
   ...
   +            if ($dependency_index !== FALSE) {
   +              $values['migration_dependencies']['required'][$dependency_index] .= ":$node_type";
   +            }
   ...
   -          foreach ($base_migration_ids as $base_migration_id) {
   -            $dependency_index = array_search($base_migration_id, $values['migration_dependencies']['required']);
   -            $values['migration_dependencies']['required'][$dependency_index] .= ":node:$node_type";
   +          foreach ($type_and_bundle_migration_derivers as $migration_id) {
   +            $dependency_index = array_search($migration_id, $values['migration_dependencies']['required']);
   +            if ($dependency_index !== FALSE) {
   +              $values['migration_dependencies']['required'][$dependency_index] .= ":node:$node_type";
   +            }
   ...
   -          foreach ($base_migration_ids as $base_migration_id) {
   -            $dependency_index = array_search($base_migration_id, $values['migration_dependencies']['required']);
   -            $values['migration_dependencies']['required'][$dependency_index] .= ":node";
   +          foreach ($type_migration_derivers as $migration_id) {
   +            $dependency_index = array_search($migration_id, $values['migration_dependencies']['required']);
   +            if ($dependency_index !== FALSE) {
   +              $values['migration_dependencies']['required'][$dependency_index] .= ":node";
   +            }
   

   These $dependecy_index vars weren't checked before, these are now fixed as well 🙂

+++ b/core/modules/field/src/Plugin/migrate/D7FieldSettingsMigrationDeriver.php
@@ -18,6 +18,7 @@ public function getDerivativeDefinitions($base_plugin_definition) {
       $entity_type_suffix = ':' . $derivative['source']['entity_type'];
       $entity_type_and_bundle_suffix = $entity_type_suffix;

@@ -25,10 +26,9 @@ public function getDerivativeDefinitions($base_plugin_definition) {
       if (isset($derivative['migration_dependencies']['required'])) {
-        $required_migration_ids = [
-          $entity_type_suffix => ['d7_view_modes', 'd7_field'],
-          $entity_type_and_bundle_suffix => ['d7_field_instance'],
-        ];
+        $required_migration_ids[$entity_type_suffix][] = 'd7_view_modes';
+        $required_migration_ids[$entity_type_suffix][] = 'd7_field';
+        $required_migration_ids[$entity_type_and_bundle_suffix][] = 'd7_field_instance';
 

This is where I had faulty logic:

For comment derivatives, both $entity_type_suffix and $entity_type_and_bundle_suffix was :comment, and so this class checked only d7_field_instance for comment:

$required_migration_ids was

[
  ':comment' => [
    'd7_field_instance',
  ],
]

instead of

[
  ':comment' => [
    'd7_view_modes',
    'd7_field',
    'd7_field_instance',
  ],
]

I'm torn w/ this issue. I can see some (small?) benefit for it for custom migrations. There's obviously interest in seeing it move along too. But from my personal experience, I don't quite get the large DX that others seem to find in splitting these things up. I'm willing to take and commit anything that the community wants if things like tests, etc are present. But right now I'm struggling w/ understanding why this is needed.

I think that's also why this is in contrib land at the moment too. Which means that we are stuck w/ nasty migration_plugin_alters, which I really don't like as they are hard to maintain for the long term. What are the names of the resulting migration yml files that get spit out when this patch is applied? I'm mainly interested in the field config migrations, rather then the other more content related migrations.

FWIW the dependency mapping and data integrity use cases of this seem really valuable to me, although I totally understand the concerns about maintenance burden.

re #48: I'm hopefully not playing dumb, that isn't my intention. What data integrity issues? I don't see it mentioned anywhere in this issue. Data dependency between related migrations is usually handled successfully by the migrate_lookup plugin. If there are specific cases where we have issues, we typically fix those as we find them.

re #49: There's already 150+ migration templates. If we add more derivers, we'd be looking at many more. There's nothing in the existing API that stops a site from handling these scenarios (via custom migrations) and adding more per bundle or entity type migration templates. But for the smaller sites, less can be more. I am worried that adding more derivatives, we'd be making it more complicated.

Additionally, in real life migrations the config set of migrations get run pretty rarely. A site might use the config generated migrations to build out the new site with the basic config structure. But then site builders start swapping out field collections for custom field types (or paragraphs) and combining content types, etc. Basically, lots of config changes. And at that point, you can't run the config migrations any more. You can only run the content migrations.

From the IS, we're looking at almost (maybe exclusively?) config migrations. So, I state again, I'm torn. I'm happy to go with a larger majority here, but I'm still not fully convinced in the benefit.

I'm torn w/ this issue.

Yes, I am torn too. I can see why it makes sense from one point of view. One thing about this that may be a problem from some is the increase in the number of tables. I recall reports of sites not being about to upgrade because of a limit on the number of tables they could have.

All up I completely agree with heddn comments and questions.

This patch refines comment-related migrations.

The migration lookup process plugins will check the corresponding d7_comment:node_type and d7_comment_type:node_type</code migrations instead of all the <code>d7_comment and d7_comment_type migrations.

This change adds d7_filter_format as a required migration dependency to every derived d7_field_instance migration that might need it.

Addressing feedbacks in #56.

@huzooka

It's good to un-assign the issue.

@shaktik, thanks for reminding me, you're right!

Re #60:

I deeply agree, I think this is the right approach!

Fixed bugs:

1. +++ b/core/modules/comment/src/Plugin/migrate/D7CommentDeriver.php
   @@ -42,7 +42,6 @@ class D7CommentDeriver extends DeriverBase {
          $node_comment_fields_statement = $db->select('field_config_instance', 'fci')
            ->fields('fci', ['bundle'])
            ->condition('fci.entity_type', 'comment')
   -        ->orderBy('fci.bundle')
            ->execute();
          $node_comment_fields_sorted = array_reduce($node_comment_fields_statement->fetchAllAssoc('bundle'), function ($ctypes, $item) {
            $node_type_from_ctype = preg_replace('/^comment_node_(.+)/', '${1}', $item->bundle);
   @@ -50,6 +49,7 @@ class D7CommentDeriver extends DeriverBase {
   
   @@ -50,6 +49,7 @@ class D7CommentDeriver extends DeriverBase {
            return $ctypes;
          }, []);
    
   +      sort($node_comment_fields_sorted);
          $node_types_sorted = array_keys($node_types);
          sort($node_types_sorted);
          if (array_values($node_types_sorted) !== array_values($node_comment_fields_sorted)) {
   

   We should sort both of the query results before a strict comparison.

2. +++ b/core/modules/node/src/Plugin/migrate/D7NodeDeriver.php
   @@ -94,9 +94,9 @@ class D7NodeDeriver extends DeriverBase implements ContainerDeriverInterface {
          foreach ($node_types as $node_type => $label) {
   -        $is_node_migration = in_array($base_plugin_definition['id'], ['d7_node', 'd7_node_complete', 'd7_node_revision', 'd7_node_translation']);
   -        $is_comment_config_migration = strpos($base_plugin_definition['id'], 'd7_comment_') === 0;
            $values = $base_plugin_definition;
   +        $is_node_migration = $this->getDestinationEntityTypeId($values) === 'node';
   +        $is_comment_config_migration = strpos($values['id'], 'd7_comment_') === 0;
            $values += ['migration_dependencies' => []];
   
   @@ -177,7 +177,7 @@ class D7NodeDeriver extends DeriverBase implements ContainerDeriverInterface {
   -        if (!$is_comment_config_migration) {
   +        if ($is_node_migration) {
              /** @var \Drupal\migrate\Plugin\MigrationInterface $migration */
              $migration = \Drupal::service('plugin.manager.migration')->createStubMigration($values);
              $this->fieldDiscovery->addBundleFieldProcesses($migration, 'node', $node_type);
   

   This change makes node bundle field processes to be added only to node entity migrations.

3. Every other change reverts a previous "new" (but bad) behavior: the patch in #00 or #60 skipped the creation of field and view mode migrations when the source entity type ID wasn't available on the destination site.

   Previously I thought this was a neat solution, but since the source entity type ID does not necessarily match the new one (e.g. field_collection_item is mapped to paragraph in #2911244: Field collections deriver and base migration), we cannot make such a decision until all migrations have been collected and altered.

This issue happens even without this patch, because the d7_comment_type migration plugin uses the d7_node_type migration source plugin.

The solution is to create a separate source plugin for comment bundle migration that extends d7_node_type, but with a source_module annotation set to comment.

Re-rolled #62:

This new patch still applies on drupal core and it depends on:

• #3096951-16: d7_node migration should have dependency on d7_node_title_label migration,
• #3097314-2: d7_comment migration should have dependency on d7_comment_entity_display, same for d7_custom_block + block_content_entity_display, and
• #3176394: Do not migrate comment related configurations if "comment" wasn't enabled on the source site

This new patch contains a lot of cleanup.
Short update:

• node and comment migration deriver classes are now separated – lot of logic is removed from D7NodeDeriver and merged into D7CommentDeriver
• Further config migrations (d7_field_option_translation, d7_field_instance_option_translation.yml and d7_field_instance_label_description_translation.yml) are derived per entity type and bundle
• Content entity translation are derived per bundle

#66 is unaddressed, but imho it merit to get a standalone issue...

1. +++ b/core/modules/content_translation/migrations/d7_node_entity_translation.yml
   @@ -5,7 +5,7 @@ migration_tags:
      - translation
      - Content
      - Multilingual
   -deriver: Drupal\node\Plugin\migrate\D7NodeDeriver
   ...
    source:
      plugin: d7_node_entity_translation
    process:
   

   Drupal\content_translation\Plugin\migrate\D7NodeDeriver this this deriver does not exists – this should be Drupal\language\Plugin\migrate\D7NodeDeriver (language is the dependency of content_translation).

2. --- a/core/modules/content_translation/migrations/d7_taxonomy_term_entity_translation.yml
   +++ b/core/modules/content_translation/migrations/d7_taxonomy_term_entity_translation.yml
      - translation
      - Content
      - Multilingual
   -deriver: Drupal\taxonomy\Plugin\migrate\D7TaxonomyTermDeriver
   +deriver: Drupal\content_translation\Plugin\migrate\D7TaxonomyTermDeriver
    source:
   

   This should be moved into a core patch (with the Drupal\content_translation\Plugin\migrate\D7TaxonomyTermDeriver class)

3. +++ b/core/modules/content_translation/src/Plugin/migrate/source/d7/EntityTranslationSettings.php
   @@ -14,6 +17,17 @@ use Drupal\migrate_drupal\Plugin\migrate\source\DrupalSqlBase;
   @@ -35,6 +49,7 @@ class EntityTranslationSettings extends DrupalSqlBase {
   
   @@ -35,6 +49,7 @@ class EntityTranslationSettings extends DrupalSqlBase {
          // which comment type uses entity translation.
          ->condition('name', 'language_content_type_%', 'LIKE');
        $query->condition($condition);
   +
        return $query;
      }
   

   Unnecessary linebreak.

4. +++ b/core/modules/field/src/Plugin/migrate/source/d7/FieldInstance.php
   @@ -15,6 +18,21 @@ use Drupal\migrate_drupal\Plugin\migrate\source\DrupalSqlBase;
   @@ -26,6 +44,7 @@ class FieldInstance extends DrupalSqlBase {
   
   @@ -26,6 +44,7 @@ class FieldInstance extends DrupalSqlBase {
          ->condition('fc.storage_active', 1)
          ->condition('fc.deleted', 0)
          ->condition('fci.deleted', 0);
   +
        $query->join('field_config', 'fc', 'fci.field_id = fc.id');
   

   Unnecessary linebreak.

5. +++ b/core/modules/field/src/Plugin/migrate/source/d7/FieldLabelDescriptionTranslation.php
   @@ -14,10 +17,25 @@ use Drupal\migrate_drupal\Plugin\migrate\source\DrupalSqlBase;
   @@ -45,7 +63,16 @@ class FieldLabelDescriptionTranslation extends DrupalSqlBase {
   
   @@ -45,7 +63,16 @@ class FieldLabelDescriptionTranslation extends DrupalSqlBase {
        $query->condition($condition);
        $query->innerJoin('locales_target', 'lt', 'lt.lid = i18n.lid');
    
   -    $query->leftjoin('field_config_instance', 'fci', 'fci.bundle = i18n.objectid AND fci.field_name = i18n.type');
   +    // This was a left outer join before, even though "entity_type" and "bundle"
   +    // are required properties. Since this was fixed here, the original
   +    // migration plugin definition could be simplified a lot.
   +    $query->join('field_config_instance', 'fci', 'fci.bundle = i18n.objectid AND fci.field_name = i18n.type');
   +
   +    if ($entity_type && $bundle) {
   +      $query->condition('fci.entity_type', $entity_type)
   +        ->condition('fci.bundle', $bundle);
   +    }
   +
   

   This should be moved into a core patch.

6. +++ b/core/modules/language/src/Plugin/migrate/source/d7/LanguageContentSettings.php
   @@ -65,4 +85,18 @@ class LanguageContentSettings extends DrupalSqlBase {
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function checkRequirements() {
   +    parent::checkRequirements();
   +    if (!$this->moduleExists('node')) {
   +      // The source plugin requires the "node_type" database table of the node
   +      // module.
   +      throw new RequirementsException('The node module is not enabled in the source site.', [
   +        'source_module_additional' => 'node',
   +      ]);
   +    }
   +  }
   

   This should be moved into a core patch.

7. +++ b/core/modules/language/src/Plugin/migrate/source/d7/LanguageContentSettingsTaxonomyVocabulary.php
   @@ -25,6 +25,8 @@ class LanguageContentSettingsTaxonomyVocabulary extends Vocabulary {
   +
   +    $qcr = (clone $query)->execute()->fetchAll();
   

   This should be removed.

New task(s) discovered after 9.1 core update:

• https://dgo.to/3110669
• https://www.drupal.org/node/3008028

I don't really see how these could be derived as well since the major part of the required (code) infrastructure is in a core issue #3051251: Existing menu links show validation issues on migration (and ALL menu links pointing to node translations are invalid).

Addressed #70.1, #70.3, #70.4 and #70.7

Re #66:

It seems that the book_allowed_types setting does not restrict anything neither in Drupal 7 nor in Drupal 8 or Drupal 9.

Even so, we can refine the migration dependencies of the d7_book migration.

This addresses #76.

Now I'll address #73.

This patch addresses the review posted in #73 and also adds the comment from #74.

Re #73:

1. 😀
2. Removed.
3. 😀 Yepp, exactly!
4. Fixed.
5. 👍
6. 👍
7. 😀
8. I don't want to override the constructor just for setting a single configuration key used only once.
9. 👍
10. Removed.
11. 👍
12. Removed.
13. Added a very simple comment.
14. 👍
15. Whenever a deriver class instantiates this source plugin, it doesn't want to set entity_type (or bundle) restrictions. So ideally, a source plugin should handle these configs as optional configs.
16. 👍
17. 👍
18. Removed.
19. And this was also removed.
20. 👍

This patch is identical to #82 – but it can be applied after #3202462-6: [PP-1] Provide option for contrib modules to map their D6 / D7 field formatter and widget plugin IDs to the equivalent D9 plugin ID.

If comment module is not enabled on soure site then we should not migrate d7_field: comment,
Interdiff denotes the actual change and other changes are caused because of the re-roll.

follow up for #87. Modified the second operand so that if comment is not present it won't show a php error.