Handle entity_references related to Drupal 6 and 7 node translations with different IDs

Tagging.

Reviewed in weekly maintainers meeting and Adam is going to look at doing this with deriver.

Here's how I think we could approach this.

1. We'll need a new migration. It will probably live in Migrate Drupal. Let's call it entity_references. This migration will have a deriver.
2. The deriver's job is to create many variants of the migration -- one for each bundle of every content entity type that has at least one entity reference field attached to it. Whew!
3. Each of these migrations will implement at least one process pipeline -- one for each entity reference field attached to that bundle of that entity type.
4. The process pipeline for each field will be pretty much identical. It will iterate over every value of the entity reference field and run target_id through the migration_lookup process plugin.

Obviously there will be a lot more to this, but that's the basic idea. I'll try to write this and see how far I get.

Subscribing so that I can help testing this when the patch arrives.

I'll have some time in the next few days to take another shot at this.

@phenaproxima: Things are going pretty well here, I have a working deriver and I think I'm on the right track, except for on key point from #4:

4. The process pipeline for each field will be pretty much identical. It will iterate over every value of the entity reference field and run target_id through the migration_lookup process plugin.

My million dollar question: How can we know which migration to lookup?

My million dollar question: How can we know which migration to lookup?

I ran into this as well. My solution (for now): just hard-code it.

This patch is still a work in progress, I just wanted to upload it so we can discuss the approach.

What I understood was that this migration was kind of a D8 to D8 migration, in the sense that we would loop through all migrated entity reference fields and update the ones that are pointing to a translation.

The problem is that when the deriver is executed, the migrations have not run yet so there's no entity reference field on the D8 site yet, and then no derived migration is created.

I guess that the approach is wrong and we should try to derive the migrations based on the fields of the source site, if that's possible?
Or is there a way to make the deriver run after the migrations? I don't think so.

The problem is that when the deriver is executed, the migrations have not run yet so there's no entity reference field on the D8 site yet

This is a misunderstanding. The entity_references migrations *are* only supposed to be discovered, and executed, after the D6/7 -> 8 migration process has completed. So the derivers will discover the fields; we just have to execute these migrations as a bunch, separately, after the main D6/7 ones are completed.

...we just have to execute these migrations as a bunch, separately, after the main D6/7 ones are completed.

And how can we do that? With migration dependencies?

Will it be doable both from the UI and with drush migrate-upgrade --configure-only?

EDIT: Nevermind, I just read the Migration.php file and I see that it can be done using the requirements (or dependencies?) I think. I'll try that.

SECOND EDIT: Hmm, doesn't seem to work.

And how can we do that? With migration dependencies?

How about an event subscriber in Migrate Drupal? I'm not 100% sure what event we'd be reacting to, although I can see some ways to do it.

For example:

1. Migrate Drupal could implement hook_migration_info_alter(), or whatever it is, and scan for all migrations tagged with 'migrate_drupal_post_process' or something like that. It would collect the IDs of all migrations thus tagged and save them in state, where the event subscriber could get to them.
2. The event subscriber would react to whichever event is fired when a migration is completed. Each time a migration is done, it removes it from the list of tagged migrations stored in state.
3. When all tagged migrations are completed, it executes the entity_references derivatives.

It's a bit clumsy, but it could work. I'm open to other ideas...?

OK, I have to admit that I'm stuck. I hit several walls trying to solve this thing. Here's two things I tried:

Version 1:
This is what have bee suggested in the previous comments of this issue.
The new migration is being derived based on the migrated fields on the D8 site. So it have to run and be derived after some required migration have been run. To do that, we put that in state in a hook_migration_plugins_alter() and we check if the dependencies have been run in an eventsubscriber.
This seems like a weird thing to do and I haven't been able to make it work.

Version 2:
I also tried to derive the new migration based on the values in the source database, this way we don't have to try to make it derived after all the others. I didn't had anymore success with this approach.

I would appreciate if someone could look over those two version to see if I'm doing something wrong, and if one approach is more promising thatn the other.

Damn this is a hard problem.

I decided to document the problem statement mainly for myself to understand the challenge @maxocub, @phenaproxima and @heddn were discussing in the maintainers' meeting this week.

Drupal 7 source data model

• Enabled modules: Content translation (D7 core), Locale (D7 core), Entityreference (D7 contrib) along with the dependencies
• Languages: English (default), Finnish
• Content type Article has 'Publishing options / Multilingual support' defined as 'Enabled, with translation'
• Content type Article has an entity reference field 'Related Article'

The data relations look like this in Drupal 7:

• When we create a Finnish translation for 'Article 1', the translated content gets its own node id.
• In this example the original English version 'Article 1' has nid 1 and the Finnish translation 'Artikkeli 1' has nid 2.
• When we have 'Artikkeli 2' and we add a 'related article' to 'Artikkeli 1', the relation points to nid 2.

Expected result in Drupal 8

• Enabled modules: Content translation (D8 core), Language (D8 core)
• Languages: English (default), Finnish

The data relations look like this in Drupal 8:

• The Finnish translation 'Artikkeli 1' has the same node id (1) as the original 'Article 1'.
• When we migrate 'Artikkeli 2' the source entity reference points to nid 2 which does not exist in Drupal 8.

Challenge

The migrations we're trying to write here MUST be derived *after* all other relevant migrations are run, because they derived based on that data and at the time they are derived, there's no migrated fields on the D8 site.

Two phase migration as a possible solution

The concept that was discussed in the maintainer meeting was something as follows:

• The first phase would migrate the nodes.
• After this step we will know that the D7 'Artikkeli 2' (D7 nid 2) was migrated to D8 as nid 1
• The second migration phase would then take care of the entity references.
• In this second step, we need to identify that the relation that used to point to nid 2 in D7 actually needs to point to nid 1 in D8.

Next steps

Did this accurately cover the concept that was discussed this Thrusday?

Re #14: Thanks @masipila for this great detailed summary of the problem! I might update the IS with this since it's so damn clear.

Meanwhile, here's yet another WIP/POC. This just works from the UI, but the migration are now derived at the right moment.

The problem now is that my nodes are not updated, I get this error when the entity reference migrations are run:

A translation already exists for the specified language (fr). (/home/maxocub/drupal8/core/lib/Drupal/Core/Entity/ContentEntityBase.php:726)

I'll take that as progress and as a small hope that we might find a solution.

What about something like the following?

1. As the main migration is running, add every node with a reference to a queue if content translation is enabled in the source.
2. In the queue runner, check the reference fields to see if items are in the reference map and re-add items to the queue if they aren't
3. We'd then need to run through the queues. Drush migrations could add a drush queue-run at the end. UI is trickier, but there are cron runs if nothing else.

Since this is something that can happen without any access to the source database, it might be OK if the post-process step isn't technically a migration.

I think another part of the issue here is that we don't have a good reliable way to know when all migrations are completed. When can we trigger this second set of migrations? If you run drush, you can trigger migrations individually. If you run the UI, then we know its a one shot migration. But what about rollbacks? Do we then need to clean out the queue? What about partial rollbacks. Do we only clean out part of the queue?

Also, we don't have a way to rollback the d8->d8 migration if we don't use the migrate ecosystem.

So, to address these concerns... what about if we have a service that can be called from migrate_drupal_ui or drush? This service generates any new migrations needed. I'd envision this being part of the upgrade form/wizard in Core. And in Drush, I'd see it as a drupal_set_message that gets triggered whenever someone runs a migration that has i18n data in it. Then the runner of the drush migration knows they have to run a new command. Which this new command will generate a bunch of new migrations. And when the user desires, they can run these new migrations.

The d8=>d8 migration path is already solved in contrib with https://www.drupal.org/project/migrate_drupal_d8. We'd need to maybe clean things up a little, but that project has pretty decent test coverage and could be brought into core.

If you run drush, you can trigger migrations individually. If you run the UI, then we know its a one shot migration. But what about rollbacks? Do we then need to clean out the queue? What about partial rollbacks. Do we only clean out part of the queue?

You can't really clear out a queue as such since items can't be introspected unless they're claimed. When processing the queue, we'd need to check the id_map and return early for anything that's not there, then the queue item goes away via that early return. Possibly an additional check that the uuid in the queue item and the uuid of the queue entity matches, in case a migration has been run, rolled back and run again in-between.

Generally any queue that's doing something after node save needs to do something like this (at least checking that entities still load), since you never know if the node is going to be deleted before the queue item is processed.

So, to address these concerns... what about if we have a service that can be called from migrate_drupal_ui or drush? This service generates any new migrations needed.

If that's simpler than doing the queue it sounds like it could work, but also feels like adding an explicit manual step in the middle of quite a long process. For a very long migration that takes hours, it's going to take some extra bash scripting to generate and call those additional migrations at the end (as opposed to running a queue via drush which is just an extra line or two).

The last patch here looks pretty close to what I would have expected. However, I think we should build a service to trigger the derriver and such as I mentioned in my last comment.

1. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/EntityReferenceTranslationDeriver.php
   @@ -0,0 +1,134 @@
   +              $this->derivatives[$derivative]['process'][$field_name] = [
   +                'plugin' => 'migration_lookup',
   +                'source' => $field_column,
   +                'migration' => 'd7_node_translation',
   +              ];
   

   I think 'd7_node_translation' is returning the nid of the most recent revision. Hmm, no. It is returning nid + langcode. Or it should be. I wonder if the source plugin isn't pulling the correct language code and nid combination?

2. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/source/d8/EntityReferenceTranslation.php
   @@ -0,0 +1,55 @@
   +class EntityReferenceTranslation extends SqlBase {
   

   This seems like almost an exact copy of the functionality in http://cgit.drupalcode.org/migrate_drupal_d8/tree/src/Plugin/migrate/sou.... Perhaps we should adopt it into core instead? I had some test coverage. Were this new source plugin has none.

Re #20:

1. Good question, I'll check that.
2. That would be a good idea. It would certainly be super useful for these post process migration issues.

I also wonder if we've also stumbled on a failing test case for #2921661: Add support to migrate multilingual revisions. Because we'd want to key the data in the mapping table by nid and langcode. Even on the revisions.

I have not given up on this one, I'll be working on it this week, especially during the Migrate Sprint on January 11 & 12.

@heddn: How can we get migrate_drupal_d8 into core? It would be super helpful here. Open a new issue and postpone this issue? Or add it right here?

re #23: If we are pretty sure it will help, then open a new issue and postpone this is probably a good idea.

While #2935951: Copy migrate source plugin from migrate_drupal_d8 into migrate_drupal is not yet finished, I copied the ContentEntity source plugin and it's deriver from there to show how it will be useful here.

The new service is just a start, I'm not quite sure what it should do exactly and how it should do it. For now it just returns the post process migrations.

The new form step is also not ideal. It would be nice to have a separate form for those post process migrations, for if someone doesn't want to run them right away after the upgrade and would like to review the migrated content and come back to run them later.

I added a short test for D7 to show that it actually works, I don't have a D6 site right now but I'll add a test for it another time.

There's a major lack of comments in the code to explain what's going on, I'll also fix that later.

In summary, this is still just a work in progress, but it show that it IS progressing!

(No interdiff because to much changed)

I'm a fan of breaking out the form step into a new form with a new route that can be run separately. See #2918761: Break up MigrateUpgradeForm into smaller forms for where we might start doing something like that for the rest of the form.

Here's a new problem that I stumbled upon. Imagine a simple scenario with Drupal 7:

• The core translation module and the contrib entityreference module are enabled.
• You have one content type, which is translatable, and you have an entity reference field on it.
• You have 4 nodes:
  • node/1 (English) with title 'Page 1 EN'
  • node/2 (French translation of node/1) with title 'Page 1 FR'
  • node/3 (English) with title 'Page 2 EN'
  • node/4 (French translation of node/3) with title 'Page 2 FR'
• If you edit any one of those node and start typing 'Page' in the autocomplete widget of the entity reference field, you'll see the 4 nodes being suggested, in English and in French. So on D7 an English node can refer to a French node, and vice versa.

But on D8 this is different, the default autocomplete widget of the entity reference field (or the select field and the checkboxes for that matter) will only show entities in the same language as of the current entity. (See #2144377: Entity reference autocomplete lists entity labels only in current content language)

So what will we do when we want to migrate a D7 site with entities that refer to entities in other languages?

One solution that comes to mind would be to try to detect those cases, warn the users about it, and point them to a documentation page where they can learn how to create an entity reference autocomplete view that shows entities in all languages.

@maxocub I think that's worth a separate issue, but for me it shouldn't block migrate being stable.

Discussed in the weekly migrate meeting. Since D8 entity reference field only refer to IDs and no langcode, theres no way to solve #29 with migrate. We should at least document it.

Added the documentation tag. The correct place to document this is the 'known issues' page in the upgrade handbook. https://www.drupal.org/docs/8/upgrade/known-issues-when-upgrading-from-d...

Markus

This is no longer blocked.

Just a re-roll, for starters.

Fix one of the test failures.
Correct the coding standards infractions.

This should fix the tests. But why is there one additional field_config and field_storage_config migrated, that I don't understand yet.

Tests are green, back to work.

It would be nice if #2918761: Break up MigrateUpgradeForm into smaller forms could land before this one, but I don't think we should postponed it.
For now, I just moved the post process form in its own class with a new route so that it can be accessed easily after a migration.
It still needs a lot of work, like checking if post process migrations are ready to be run and displaying the post process migrations that will be run. I'm on it.

Still a work in progress but I would appreciate reviews to make sure it's in the right direction and to know your opinion on the approach.

Just quick notes during the migrate meeting.

1. +++ b/core/modules/migrate_drupal/migrations/entity_reference_translation.yml
   @@ -0,0 +1,21 @@
   +migration_tags:
   +  - migrate_drupal_post_process
   

   This should be 'I18n Content' or something. And adjust the recent category/tagging tests.

2. +++ b/core/modules/migrate_drupal/src/PostProcessMigrationManager.php
   @@ -0,0 +1,57 @@
   +class PostProcessMigrationManager {
   

   Bikeshed name here. Could pick a better name.

We discussed this issue at length during the Migrate maintainer call this morning. Here's what we're thinking:

Migrations already fire a post-import event when they are complete. Migrate Drupal UI and Migrate Tools/Migrate Upgrade should subscribe to this event and handle it in different ways. For migrations which will require additional "clean-up" work (we decided to get rid of the "post-process" verbiage, since that sounds like it has something to do with the process pipeline)...

• Migrate Drupal UI will catch the event and add the new clean-up migration(s )to its master list of migrations which still need to be run. Then, once the main migration set finishes, it will trigger a new batch job to run the migrations which were added to the list. This way, the user doesn't have to do anything at all, and every migration that has to run, is run.
• Migrate Tools/Upgrade will catch the event and set a Drush message which explains that the migration(s) generated additional migrations which must be run, and inform the user how to run those migrations (maybe by echoing the command to run).

This dual approach is the best of all possible worlds. For users who want a one-click, "just works" solution, the Migrate Drupal UI will do all the work. For power users who want to use Drush and heavily customize things, Migrate Tools will tell them what they need to do, and how they need to do it, but leave the actual work to them.

For the second point about drush in #44, I think the subscribed event would let us know that we just ran a migration that could necessitate running a migrate:generate-post-migrations (or some such) command. That way if someone isn't done with all the migration rebuilding yet in their custom world, they have time to futz with things before they then run the generate command. After running the generate command, then folks can simply run drush mim --tags="I18n Content" or drush mim --all

* "I18n Content" is perhaps a little premature naming , but replace this with whatever we land on for tagging.
* "migrate:generate-post-migrations" is perhaps a little premature naming , but replace this with whatever we land on for the drush command.

Here's another patch trying the post migration event idea, and I think it's in fact way better.

The migrate upgrade batch is now listening for POST_IMPORT events and if the migration that just finished implements MigrationFollowUpInterface, then it generates it's follow-up migrations and adds them to the current batch process.

I will now try to add tests for D6 and functional tests that shows that this event approach really works.

No interdiff because so much has changed.

Here's tests for Drupal 6.

I had to modify the NodeReference field plugin because it was creating stubs of translated nodes that were never converted to real nodes on the D8 site.

Still needs some functional tests.

Fixing the test.

This patch looks great and makes sense. Amazing work, @maxocub!

I have more points of review, but these are just the initial set of minor things I found.

1. +++ b/core/modules/migrate_drupal/migrations/d6_entity_reference_translation.yml
   @@ -0,0 +1,21 @@
   +# The destination plugin will be set by the deriver.
   +destination: []
   

   I suggest we set the default destination to the 'null' destination here, and have the deriver adjust it later.

2. +++ b/core/modules/migrate_drupal/src/MigrationConfigurationTrait.php
   @@ -97,6 +97,11 @@ protected function getMigrations($database_state_key, $drupal_version) {
   +        if (in_array('i18n Content', $migration->getMigrationTags())) {
   

   For type safety, let's pass TRUE as the third argument to in_array(). Also, we should move this if check outside of the enclosing try block.

3. +++ b/core/modules/migrate_drupal/src/Plugin/MigrationFollowUpInterface.php
   @@ -0,0 +1,18 @@
   +/**
   + * Interface for follow-up migrations.
   + */
   +interface MigrationFollowUpInterface {
   

   We should probably expand this doc comment, but that doesn't need to happen just yet. Let's sic @masipila on it :)

4. +++ b/core/modules/migrate_drupal/src/Plugin/MigrationFollowUpInterface.php
   @@ -0,0 +1,18 @@
   +  /**
   +   * Gets follow-up migrations.
   +   *
   +   * @return \Drupal\migrate\Plugin\MigrationInterface[]
   +   *   The follow-up migrations.
   +   */
   +  public function getFollowUpMigrations();
   

   Same here. I also think we should rename this method to defineFollowUpMigrations() or generateFollowUpMigrations(), because I think 'get' is too vague.

5. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/EntityReferenceTranslationDeriver.php
   @@ -0,0 +1,176 @@
   + * migration_tags:
   + *   - Drupal 7
   + *   - migrate_drupal_post_process
   

   I think the migrate_drupal_post_process tag is not applicable anymore :)

6. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/EntityReferenceTranslationDeriver.php
   @@ -0,0 +1,176 @@
   + * In this example, the only supported target type is 'node' and the associated
   + * mapping table is 'd7_node_translation'.
   

   d7_node_translation is the name of the migration, not the mapping table.

7. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/EntityReferenceTranslationDeriver.php
   @@ -0,0 +1,176 @@
   +   *   The base plugin ID for the plugin ID.
   

   Is "for the plugin ID" necessary?

8. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/EntityReferenceTranslationDeriver.php
   @@ -0,0 +1,176 @@
   +          $target_type = $field_definitions[$field_name]->getSettings()['target_type'];
   

   I think (but could be wrong) that we can use getSetting('target_type'), rather than dereferencing from getSettings().

9. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/EntityReferenceTranslationDeriver.php
   @@ -0,0 +1,176 @@
   +          if (!in_array($target_type, array_keys($base_plugin_definition['target_types']))) {
   

   Let's pass TRUE as the third argument to in_array().

10. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/EntityReferenceTranslationDeriver.php
    @@ -0,0 +1,176 @@
    +            // Set the process pipeline.
    +            $base_field_definitions = $this->entityFieldManager->getBaseFieldDefinitions($entity_type);
    +            foreach (array_keys($base_field_definitions) as $base_field) {
    +              $this->derivatives[$derivative]['process'][$base_field] = $base_field;
    +            }
    

    I'd rather we just mapped the ID, revision ID, and langcode keys than every base field.

All points in #50 are addressed, except for the interface docs.

Here's an attempt to test that the follow-up migrations are successfully run in the migrate_drupal_ui functional tests.

Fix for the failing tests and the coding standards message.

+++ b/core/modules/migrate_drupal/src/Plugin/migrate/EntityReferenceTranslationDeriver.php
@@ -0,0 +1,177 @@
+            // Set the source plugin.
+            $this->derivatives[$derivative]['source']['plugin'] = 'content_entity:' . $entity_type;
+            $this->derivatives[$derivative]['source']['bundle'] = $bundle;
+
+            // Set the process pipeline.
+            $this->derivatives[$derivative]['process'] = [
+              'nid' => 'nid',
+              'vid' => 'vid',
+              'langcode' => 'langcode',
+            ];

@phenaproxima: About #50.10, I just realized that the idea behind getting all the base fields dinamically like I did before was because this migration can have any entity types as its source & destination, so the NID, VID & langcode won't always be the required base fields.

Here's a new patch with two improvements:

1. Changed the hardcoded nid, vid & langcode with the $entity_type->getKey() method.
2. Improve the addition of the follow-up migrations to the batch job so they don't get added multiple times when they are triggered by a derived migration.

Didn't do a detailed review yet but the approach is really encouraging here and I couldn't see any obvious problems at all.

I love it. I think we are on the right track and getting close to home stretch. This work is heroic.

1. +++ b/core/modules/migrate_drupal/migrations/d6_entity_reference_translation.yml
   @@ -0,0 +1,22 @@
   +target_types:
   +  node:
   +    - d6_node_translation
   

   This is really clever!

2. +++ b/core/modules/migrate_drupal/src/MigrationConfigurationTrait.php
   @@ -96,6 +96,11 @@ protected function getMigrations($database_state_key, $drupal_version) {
   +      // Skip migrations tagged with 'i18n Content', they will be run after all
   +      // other migrations.
   +      if (in_array('i18n Content', $migration->getMigrationTags(), TRUE)) {
   +        continue;
   +      }
   

   I'm not a huge fan of hard-coding magic tags; maybe we can make this a config setting somewhere, or find another way to make it a little less concrete?

3. +++ b/core/modules/migrate_drupal/src/Plugin/MigrationFollowUpInterface.php
   @@ -0,0 +1,18 @@
   +/**
   + * Interface for follow-up migrations.
   + */
   +interface MigrationFollowUpInterface {
   

   Doc comment needs substantial expansion, and I think we should rename this to FollowUpMigrationInterface.

4. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/EntityReferenceTranslationDeriver.php
   @@ -0,0 +1,181 @@
   + * configured with one of the supported target types. The migrations will update
   

   Can "with" be changed to "...to point to one of the supported target entity types"?

5. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/EntityReferenceTranslationDeriver.php
   @@ -0,0 +1,181 @@
   +          if (!isset($this->derivatives[$derivative])) {
   

   I don't think we need this isset() check.

6. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/EntityReferenceTranslationDeriver.php
   @@ -0,0 +1,181 @@
   +            $this->derivatives[$derivative] = $base_plugin_definition;
   

   For readability, can we set up the $derivative in its own variable before adding it to $this->derivatives? It'll just make the lines below easier to read.

7. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/EntityReferenceTranslationDeriver.php
   @@ -0,0 +1,181 @@
   +            if ($entity_type_definition->isRevisionable() && $revision_key = $entity_type_definition->getKey('revision')) {
   

   The entity type cannot be revisionable wihtout a revision key, so I don't think we need the implicit check of $revision_key.

8. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/EntityReferenceTranslationDeriver.php
   @@ -0,0 +1,181 @@
   +            if ($entity_type_definition->isTranslatable() && $langcode_key = $entity_type_definition->getKey('langcode')) {
   

   Same here.

9. +++ b/core/modules/migrate_drupal_ui/src/Batch/MigrateUpgradeImportBatch.php
   @@ -138,6 +148,15 @@ public static function run($initial_ids, $config, &$context) {
   +              if (!in_array($migration_id, $context['sandbox']['migration_ids'])) {
   

   Should probably have TRUE as the third argument here.

10. +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/MigrateUpgradeTestBase.php
    @@ -260,4 +268,19 @@ protected function assertMigrationResults(array $expected_counts, $version) {
    +  /**
    +   * Checks that follow-up migrations have been performed successfully.
    +   *
    +   * @param array $expected_results
    +   *   The follow-up migrations expected results.
    +   */
    +  protected function assertFollowUpMigrationResults(array $expected_results) {
    +    $plugin_manager = $this->container->get('plugin.manager.migration');
    +    foreach ($expected_results as $migration_id => $expected_result) {
    +      /** @var \Drupal\migrate\Plugin\MigrationInterface $migration */
    +      $migration = $plugin_manager->createInstance($migration_id);
    +      $this->assertSame($migration->getIdMap()->importedCount(), $expected_result);
    +    }
    +  }
    

    I'm not entirely clear on the value that this code adds. Wouldn't it be enough for the test to assert that the follow-up migrations made the appropriate changes to the nodes they affected? What do we gain by checking the imported count?

@maxocub, absolutely superb work with this issue!

I can do some wordsmithing for the FollowUpMigrationInterface docblock but I would need some input from you. Could you try to summarize this on a conceptual level with a couple of bullet points and I'll then catch up from there?

Edit: no need to focus on the conceptual background / problem statement. That one is clear (#14 summarizes that if anyone else is interested) but it would be great if you could summarize the solution.

Cheers,
Markus

@phenaproxima: Thanks for the review, I'm on it.

@masipila: Thanks, I'll summarize it for you.

1. :)
2. OK, I'm not sure my idea is good, but what about having two new interfaces, MigrationWithFollowUpInterface & FollowUpMigrationInterface? The first one should be implemented by migrations needing follow-up migrations, and the second one by the follow-up migrations themselves. This way we could get rid of the tag and do an instanceof?
3. I'll work on the documentation with @masipila.
4. Done.
5. The isset check is necessary here because if an entity type has more than one entity reference field, we want to update the derivative and not override any entity reference fields previously added to the process pipeline. Another way to do it would be to key derivatives by entity types, bundle & field name, but I don't think it would be better.
6. Same reasons as in 5.
7. Done.
8. Done.
9. Done.
10. I tried something different, I hope it's better

First, this is looking really nice. Thanks for all the hard work on this.

#58.2/#61.2: we got around that for other magic tags by adding a config option to migrate_drupal. These tags only have special significance in that context any way. Perhaps we can write an update hook and set some config that way? That gets around hard-coded, per se. But interface checks are another fine way to go about this.

1. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/EntityReferenceTranslationDeriver.php
   @@ -0,0 +1,184 @@
   + * A migration will be created for every entity reference field that is
   + * configured to point to one of the supported target entity types. The
   

   Is this true if multiple ER fields exist on a single entity type and bundle?

2. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/EntityReferenceTranslationDeriver.php
   @@ -0,0 +1,184 @@
   +            $this->derivatives[$derivative]['source']['plugin'] = 'content_entity:' . $entity_type;
   

   There is a derivative separator const in PluginBase, but I'm not sure we can use it here. To bad.

3. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/EntityReferenceTranslationDeriver.php
   @@ -0,0 +1,184 @@
   +          // Allow overwriting the entity reference field.
   +          $this->derivatives[$derivative]['destination']['overwrite_properties'][] = $field_name;
   

   A comment explaining why we are doing this could help.

Thanks for the review!

1. You're right, it will generate one migration by bundle, even if a bundle has multiple ER fields.
2. I guess we can use the constant.
3. Expanded the comment.

This has gone through a couple rounds of feedback. All of which is now addressed. Let's try RTBC on for size.

Back to NW because of missing documentation and change record.

Discussed in the weekly Migrate meeting. Things left to do:

1. Replace the empty interface for a tag and make it a config option, as suggested in #58.2
2. Refactor the deriver to use a variable for the derivatives and add them to $this->derivatives at the end, as suggested in #58.6
3. Write the documentation on the new Interface.
4. Write the change record.

I'll work on these tomorrow.

Re #66:
Points 1 & 2 have been addressed, let's see if the tests pass.

Re #66:
Point 3 is now done, at least a first draft.

IS update.

Typo in the image path.

Change record draft created.

I'm leaving the "Needs Documentation" tag added in #33 because we need to document the problem mentioned in #29 in the Known issues handbook page.

A re-roll was needed.

Self-assigning for review, today.

Discussed this issue in the Migrate call this morning and we decided to open a follow-up issue to talk about #29 and how we might go about fixing it.

Needed a re-roll.

And follow-up created: #2959839: Migrating entity reference fields pointing to nodes in different language now point to current language

This patch is magnificent. It's a really elegant solution to an extraordinarily complex problem and it makes me glow with pride that I had a hand in conceiving and reviewing it. Truly, it's a privilege to be a part of this team.

I have no major complaints to lodge against the patch. I am itching to RTBC it. I do have some questions -- nothing blocking -- and very minor changes/typos. Let's land this.

1. +++ b/core/modules/migrate_drupal/src/MigrationConfigurationTrait.php
   @@ -120,6 +134,20 @@ protected function getMigrations($database_state_key, $drupal_version) {
   +      $this->followUpMigrationTags = \Drupal::configFactory()
   

   Can we inject this? If not, or not easily, I have no problem deferring this to a minor follow-up.

2. +++ b/core/modules/migrate_drupal/src/Plugin/MigrationWithFollowUpInterface.php
   @@ -0,0 +1,40 @@
   + * system is not enough since all migration would still be derived before any
   

   s/migration/migrations

3. +++ b/core/modules/migrate_drupal/src/Plugin/MigrationWithFollowUpInterface.php
   @@ -0,0 +1,40 @@
   + * Those "follow-up" migrations need to be taged with the "Follow-up migration"
   

   s/taged/tagged

4. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/EntityReferenceTranslationDeriver.php
   @@ -0,0 +1,192 @@
   +    $this->basePluginId = $base_plugin_id;
   

   I'm not seeing this used anywhere else in the deriver, so we can probably remove it.

5. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/EntityReferenceTranslationDeriver.php
   @@ -0,0 +1,192 @@
   +          if (!in_array($target_type, array_keys($base_plugin_definition['target_types']), TRUE)) {
   

   array_key_exists() will be easier to read here.

6. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/EntityReferenceTranslationDeriver.php
   @@ -0,0 +1,192 @@
   +          if (isset($this->derivatives[$derivative_key])) {
   +            $this->derivatives[$derivative_key] += $derivative;
   +          }
   

   I'm not sure we want to use the += operator here, because it only merges the top level of the array. I could be totally wrong here, but don't we want to merge recursively? NestedArray::mergeDeep() might be better here.

7. +++ b/core/modules/migrate_drupal/tests/src/Kernel/d6/FollowUpMigrationsTest.php
   @@ -0,0 +1,70 @@
   +    // Run the follow-ups migrations.
   

   s/follow-ups/follow-up

8. +++ b/core/modules/migrate_drupal/tests/src/Kernel/d6/FollowUpMigrationsTest.php
   @@ -0,0 +1,70 @@
   +    $this->executeMigrations(array_keys($follow_up_migrations));
   

   I think executeMigrations() accepts MigrationInterface objects, so we can just pass $follow_up_migrations directly.

9. +++ b/core/modules/migrate_drupal/tests/src/Kernel/d7/FollowUpMigrationsTest.php
   @@ -0,0 +1,97 @@
   +    // Run the follow-ups migrations.
   

   s/follow-ups/follow-up

10. +++ b/core/modules/migrate_drupal/tests/src/Kernel/d7/FollowUpMigrationsTest.php
    @@ -0,0 +1,97 @@
    +    $this->executeMigrations(array_keys($follow_up_migrations));
    

    I could be wrong, but I don't think we need array_keys() here.

11. +++ b/core/modules/migrate_drupal_ui/src/Batch/MigrateUpgradeImportBatch.php
    @@ -56,6 +58,13 @@ class MigrateUpgradeImportBatch {
    +  protected static $followUpMigrations;
    

    The event subscriber is a service, so is there any reason this needs to be static?

12. +++ b/core/modules/migrate_drupal_ui/src/Batch/MigrateUpgradeImportBatch.php
    @@ -138,6 +148,15 @@ public static function run($initial_ids, $config, &$context) {
    +          if (!empty(static::$followUpMigrations)) {
    +            foreach (static::$followUpMigrations as $migration_id => $migration) {
    +              if (!in_array($migration_id, $context['sandbox']['migration_ids'], TRUE)) {
    +                $context['sandbox']['migration_ids'][] = $migration_id;
    +                $context['sandbox']['max']++;
    +                unset(static::$followUpMigrations[$migration_id]);
    +              }
    +            }
    +          }
    

    This bit could use a few comments.

13. +++ b/core/modules/migrate_drupal_ui/src/Batch/MigrateUpgradeImportBatch.php
    @@ -263,6 +282,19 @@ public static function onPostRowSave(MigratePostRowSaveEvent $event) {
    +      static::$followUpMigrations = $migration->generateFollowUpMigrations();
    

    Do we really want to overwrite $followUpMigrations here? Don't we want to merge the generated follow-up migrations into it instead, so that we can execute multiple follow-up migrations in one request?

14. +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/d6/MigrateUpgrade6Test.php
    @@ -63,8 +64,8 @@ protected function getEntityCounts() {
    +      'field_config' => 87,
    +      'field_storage_config' => 61,
    

    Why is there a new field?

1. I'm not sure how to do dependency injection with a trait, and there's also the state and migration plugin manager services that could be injected, so it would become out of scope. If it is possible to inject services in a trait, we should do it in a follow-up and do all three services together.
2. Done.
3. Done.
4. Done.
5. Done.
6. You're right, the += does not work here, nor does the NestedArray::mergeDeep(). To make sure the deriver supports multiple entity reference fileds on the same bundle, I added a second field in the fixtures and corresponding tests.
7. Done.
8. No, executeMigrations() expects migration IDs.
9. Done.
10. No, executeMigrations() expects migration IDs.
11. This is not an event subscriber, this is a batch class. All of it's properties are static, and if we don't make it static, we get Error: Using $this when not in object context.
12. Comments added.
13. Yes it's OK to overwrite the $followUpMigration property. The follow-up migrations are added to the batch immediately (and removed from the property) after the migration on which they depend has been executed. Otherwise they were added to the batch multiple times. I added a comment.
14. There's a new field (now 2 new fields) because we needed a new entity reference field on a translatable content type for the tests.

Follow-up created for #77.1: #2961226: Use dependency injection in MigrationConfigurationTrait.

+++ b/core/modules/migrate_drupal/migrate_drupal.install
@@ -14,3 +14,13 @@ function migrate_drupal_update_8501() {
+/**
+ * Sets the follow-up migration tags.
+ */
+function migrate_drupal_update_8502() {
+  \Drupal::configFactory()
+    ->getEditable('migrate_drupal.settings')
+    ->set('follow_up_migration_tags', ['Follow-up migration'])
+    ->save();
+}

Only question for me is why does this need to be configurable. I've used drush migrate-run before and it's nice to tag custom migrations to isolate them from everything else, if that's the use-case then great.

Otherwise I think this is ready.

Only questions is why does this need to be configurable. Otherwise I think this is ready. I've used drush migrate-run before and it's nice to tag custom migrations to isolate them from everything else, if that's the use-case then great.

We're following an established pattern in Migrate Drupal, where "magic" tags are configurable. So it's mostly just to keep things consistent.

And the established pattern was because of drush migrate-run being able to create custom tags.

I have only one complaint:

+++ b/core/modules/migrate_drupal/tests/src/Kernel/d6/FollowUpMigrationsTest.php
@@ -0,0 +1,78 @@
+    $this->assertSame([0 => ['target_id' => '13']], $node->get('field_reference')->getValue());
+    $this->assertSame([0 => ['target_id' => '13']], $node->get('field_reference_2')->getValue());
+    $translation = $node->getTranslation('fr');
+    $this->assertSame([0 => ['target_id' => '20']], $translation->get('field_reference')->getValue());
+    $this->assertSame([0 => ['target_id' => '20']], $translation->get('field_reference_2')->getValue());

Can we make these assertions easier to read by converting them to the format: $this->assertSame('13', $node->get('field_reference')->target_id)?

And really, that's a nitpick that need not block RTBC. So...

(To the tune of "YMCA", by the Village People): R! T! BC!

I made the changes asked in #83.

I put it back in NR because I had to change the visibility of the $followUpMigrationTags property from protected to private in MigrationConfigurationTrait. Otherwise, and I never got this before today, I get this error when I run the migrate_drupal_ui functional tests:

Drupal\Tests\migrate_drupal_ui\Functional\MigrateUpgradeTestBase and Drupal\migrate_drupal\MigrationConfigurationTrait define the same property ($followUpMigrationTags) in the composition of Drupal\Tests\migrate_drupal_ui\Functional\MigrateUpgradeExecuteTestBase. This might be incompatible, to improve maintainability consider using accessor methods in traits instead. Class was composed

Re the trait error and the private property. It is happening because MigrateUpgradeExecuteTestBase and MigrateUpgradeTestBase both use MigrationConfigurationTrait. There were no properties on the trait till this patch so no error. But now there is a property - there is a problem. We can remove the "use MigrationConfigurationTrait" from MigrateUpgradeExecuteTestBase and make the property protected again and everything will be fine.

@alexpott: Thanks for explaining that, I was baffled.

I Forgot the Trait was also used in MigrateUpgradeReviewPageTestBase. This should fix the failures on PHP 5.6.

Ace. I'm so glad we have the wisdom of @alexpott at our disposal. Back to RTBC!

Committed/pushed to 8.6.x and cherry-picked to 8.5.x. Thanks!

Yay, thanks all!

Yeah finally, thanks everyone. can't believe it finally landed :O

The change record at https://www.drupal.org/node/2955658 never got published 😨😅

So … just published it! Thanks to @huzooka for discovering this 🤓