Migrate system broken if content_translation enabled

The patch seems OK to me with this small API break (given it's a critical regression due to last minute clean-up in order to mark the module as stable), but could do with a second or third opinion.

We still want to leave the old copy in MD and trigger_error on it and point to the new one here. We discussed this at great length in the migrate maintainers meeting today (phenaproxima, heddn, masipila, maxocub) and thought that would work. It isn't the cleanest, but perhaps its a way forward.

Back to NW for the attempt at leaving a copy in MD and trigger_error on the BC nature of things.

+++ b/core/modules/migrate_drupal/src/Plugin/migrate/EntityReferenceTranslationDeriver.php
@@ -2,6 +2,8 @@
+@trigger_error(__NAMESPACE__ . '\EntityReferenceTranslationDeriver is deprecated in Drupal 8.6.0 and will be removed before Drupal 9.0.0. Instead, use \Drupal\content_translation\Plugin\migrate\EntityReferenceTranslationDeriver, see https://www.drupal.org/node/2991710.', E_USER_DEPRECATED);

@@ -13,35 +15,9 @@
+ * See https://www.drupal.org/node/2991710.

This should @see a change record, not a previous issue. Let's add a new one for this. https://www.drupal.org/list-changes/drupal

Thanks, @jcnventura! I've unpublished the change record, though -- committers publish them when they land the patch :)

I think all is in order here now.

+++ b/core/modules/migrate_drupal/src/Plugin/migrate/EntityReferenceTranslationDeriver.php
@@ -13,35 +15,9 @@
+ * @deprecated in Drupal 8.6.0 and will be removed before Drupal 9.0.0.
+ * Use \Drupal\content_translation\Plugin\migrate\EntityReferenceTranslationDeriver.
+ * See https://www.drupal.org/node/2991774.

Real nitty, but it should be '@see' with an empty line above. See https://www.drupal.org/core/deprecation#how-class

Another possible approach, which might solve the problem in a more holistic way: why don't we just change the discovery mechanism of the migration plugin manager?

Normally, the plugin system will throw an exception if the deriver doesn't exist -- see \Drupal\Component\Plugin\Discovery\DerivativeDiscoveryDecorator::getDeriverClass() -- but we could easily make the migration plugin manager use a derivative decorator which, if it can't find the deriver class, simply logs an error somewhere and skips that plugin definition (and all of its derivatives).

Framework managers should probably confirm my understanding here, but I don't think this would constitute an API break because the actual deriver discovery mechanisms we use are internal to the plugin manager. As long as we provided some way for developers to know *why* their migration was not discovered (i.e., by logging the error), this might be a very elegant solution indeed, one which works the way the plugin system wants to work, and doesn't require us to duplicate a single line of code.

MigrationPluginManager::getDiscovery() already invokes ProviderFilterDecorator. Doesn't that mean that we could solve this by simply adding:

provider:
  - migrate_drupal

to the *_entity_reference_translation.yml files?

Or if the module that it's in needs to be explicitly added too, then:

provider:
  - content_translation
  - migrate_drupal

?

+++ b/core/modules/content_translation/migrations/d6_entity_reference_translation.yml
@@ -5,6 +5,8 @@ migration_tags:
+provider:
+  - migrate_drupal

+++ b/core/modules/content_translation/migrations/d7_entity_reference_translation.yml
@@ -5,6 +5,8 @@ migration_tags:
+provider:
+  - migrate_drupal

I think that @effulgentsia is right and we should list both content_translation and migrate_drupal as both need to be enabled.

That said, do we need to log an error for this specific scenario or just silently discard these?

Lastly, still needs tests. Going to try working on this for the next few minutes.

I'm obviously doing something wrong here. This will fail, but it doesn't seem to fail at the right point. At least on local. Going to upload for now and hopefully someone else can move this further along.

Your $definitions array is always empty because, apparently, all migration plugins provided by core should depend on migrate_drupal. I found this by reading this test Drupal\Tests\migrate\Kernel\Plugin\MigrationPluginListTest:

  /**
   * @covers ::getDefinitions
   */
  public function testGetDefinitions() {
    // Make sure retrieving all the core migration plugins does not throw any
    // errors.
    $migration_plugins = $this->container->get('plugin.manager.migration')->getDefinitions();

    // All the plugins provided by core depend on migrate_drupal.
    $this->assertEmpty($migration_plugins);

    [...]

    // Enable migrate_drupal to test that the plugins can now be discovered.
    $this->enableModules(['migrate_drupal']);

So why does d6_entity_reference_translation do not already depends on migrate_drupal?

Also, the EntityReferenceTranslationDeriver will not derive any migrations unless there is some entity reference fields targeting nodes on the Drupal 8 (destination) site.

Some special about the empty source plugin?

grep -ri 'plugin: empty' core/modules/*/migrations/*
core/modules/content_translation/migrations/d6_entity_reference_translation.yml:  plugin: empty
core/modules/content_translation/migrations/d7_entity_reference_translation.yml:  plugin: empty

@quietone: Thanks! That's the reason why those migrations do not depend on migrate_drupal. Do you think adding the provider key to the yaml is a good solution or should we avoid using the empty source plugin?

@heddn: Sorry, I dropped your test and used Drupal\Tests\migrate\Kernel\Plugin\MigrationPluginListTest instead.

Here's a failing patch. No interdiff since this is a small patch and the interdiff would be bigger than the patch.

@maxocub, your welcome. I'm very glad it helped. I had been thinking about the problem and reading your comment made me think about requirements. Good team effort.

I think the provider idea is the way to go. However, what we discovered here needs to be documented. I'm thinking in the migrations yml files and in MigationWithFollowUpInterface just after the explanation of the tags. Right now, I even think there could a sub section on the design on the migration yml but I don't know enough of the doc standards.

I'm anxious to see the test results.

+++ b/core/modules/migrate/tests/src/Kernel/Plugin/MigrationPluginListTest.php
@@ -132,6 +141,11 @@ public function testGetDefinitions() {
+    $this->assertArrayHasKey('d6_entity_reference_translation:user__user', $migration_plugins);
...
   }

Let's do an assertArrayNotHasKey before migrate_drupal is enabled. Nevermind, there is an assertion later to confirm that getDefinitions returns an empty array.

The test file has this comment at line 78. I think this would be a good time to add why that migration does not depend on migrate_drupal, especially in light of #23.

    // Enable a module that provides migrations that do not depend on
    // migrate_drupal.

1. +++ b/core/modules/migrate/tests/src/Kernel/Plugin/MigrationPluginListTest.php
   @@ -132,6 +141,11 @@ public function testGetDefinitions() {
        // All the plugins provided by core depend on migrate_drupal.
   

   This is not a true statement.

2. +++ b/core/modules/migrate/tests/src/Kernel/Plugin/MigrationPluginListTest.php
   @@ -132,6 +141,11 @@ public function testGetDefinitions() {
        $this->assertNotEmpty($migration_plugins);
   ...
   +    $this->assertArrayHasKey('d6_entity_reference_translation:user__user', $migration_plugins);
   +    $this->assertArrayHasKey('d7_entity_reference_translation:user__user', $migration_plugins);
   

   Can we explicitly test the array does not contain these when MD is disabled?

1. If we just add the word 'migration' does it becomes true: 'All the migration plugins provided by core depend on migrate_drupal.' ?
2. Yes we can, but it seem silly to test that an array is empty and then test that some keys do not exists in that empty array.

I take back my comments in #32. That is a true statement and we do have test coverage. Onward to RTBC?

BTW, I re-added a CR to discuss the very specific details of the changes in the parent issue. It probably isn't strictly needed, but we do have a couple pre releases of 8.6 and it would be good to document these things for the poor souls that hit this issue.

Committed and pushed 9273076d01 to 8.7.x and 596caa3e51 to 8.6.x. Thanks!

Publish change record.