Migration lookup process plugin doesn't check validity of provided migration(s)

Let's add a test for this. Adding a phpunit test for this seems really fairly trivial, so tagging.

We need to do a reroll for this

rerolled with 8.6.x

Is it the righ direction?

Still needs tests.

1. +++ b/core/modules/migrate/src/Plugin/migrate/process/MigrationLookup.php
   @@ -155,10 +156,17 @@ public function transform($value, MigrateExecutableInterface $migrate_executable
   +  ¶
   ...
   +  ¶
   

   Nit: whitespace.

2. +++ b/core/modules/migrate/src/Plugin/migrate/process/MigrationLookup.php
   @@ -155,10 +156,17 @@ public function transform($value, MigrateExecutableInterface $migrate_executable
   +      throw new MigrateException('Some migrations were not matched. Ensure they are migrated first and/or check their spelling. ' . implode(', ', $migration_ids));
   

   sprintf('The (%s) plugin failed because no migrations from (%s) were available.', $this->getPluginId(), implode(', ', $migration_ids))

Addressed corrections and added tests.

+++ b/core/modules/migrate/src/Plugin/migrate/process/MigrationLookup.php
@@ -155,10 +156,17 @@ public function transform($value, MigrateExecutableInterface $migrate_executable
+      throw new MigrateException(sprintf('The (%s) plugin failed because no migrations from (%s) were available.', $this->getPluginId(), implode(', ', $migration_ids)));

The wording looks funny now that I see it concretely. The following seems to work better in my book:

The (migration_lookup) plugin failed because no valid migrations were found. Invalid migrations include: (invalid_migration).

The (migration_lookup) plugin failed because no valid migrations were found. Invalid migrations include: (d7nodepage, d7nodeblog).

Re-worded the exception as suggested.
Added a couple more test scenarios.

Looking good.

1. +++ b/core/modules/migrate/src/Plugin/migrate/process/MigrationLookup.php
   @@ -155,10 +156,17 @@ public function transform($value, MigrateExecutableInterface $migrate_executable
   +    if (!$migrations) {
   

   if (empty($migrations)) { but also maybe this is not the right condition.

2. +++ b/core/modules/migrate/src/Plugin/migrate/process/MigrationLookup.php
   @@ -155,10 +156,17 @@ public function transform($value, MigrateExecutableInterface $migrate_executable
   +      throw new MigrateException(sprintf('The (%s) plugin failed because no valid migrations were found. Invalid migrations include: (%s).', $this->getPluginId(), implode(', ', $migration_ids)));
   

   Like the helpful exception but the wording is odd and the use of brackets. How about:
   The '%s' plugin failed because no migrations with the following ID(s) exist: %s.
   Also do we always expect all the $migration_ids to load.
   Ie. should we be testing count($migration_ids) === count($migrations)?

   Or maybe...

   $missing_migrations = array_diff($migration_ids, array_keys($migrations));
   if (!empty($missing_migrations)) {
   

1. Changed condition.
2. Reworded the exception.

Also added another test for when some, but not all migrations are valid.

+++ b/core/modules/migrate/src/Plugin/migrate/process/MigrationLookup.php
@@ -162,10 +162,12 @@ public function transform($value, MigrateExecutableInterface $migrate_executable
+    if (!empty($missing_migrations)) {
+      throw new MigrateException(sprintf("The '%s' plugin failed because at least one of the migrations with the following ID(s) does not exist: %s.", $this->getPluginId(), implode(', ', $migration_ids)));

We now know the missing migration(s) - I think we should take the opportunity to use that info in the error since it's helpful.

@alexpott It seems the assumption that count($migration_ids) === count($migrations) is not correct - it does not take into account derivatives (I'm not sure whether that's the technical term), i.e. d7_node returns d7_node:article, d7_node:blog, etc.

I can see no better solution than empty($migrations) (without adding a lot of complexity).

I've gone back to the patch in #18 and worked off that, ignoring #21.

+++ b/core/modules/migrate/tests/src/Unit/process/MigrationLookupTest.php
@@ -298,4 +299,42 @@ public function testMultipleSourceIds() {
+  /**
+   * @covers ::transform
+   *
+   * @dataProvider transformWithInvalidMigrationsDataProvider
+   */
+  public function testTransformWithInvalidMigrations($migration_ids, $invalid_migrations) {

It would be nice to expand this docblock with a one line summary and details about the function's parameters.

Expanded the docblock.

@Jo Fitzgerald: Thanks!

Committed and pushed b208b07712 to 8.6.x and 3c812149b7 to 8.5.x. Thanks!

This change breaks the d6_url_alias and d7_url_alias mgrations when content_translation is not enabled. I found this because it broke HEAD on commerce_migrate.

Here is the relevant snippet form d6_url_alias.yml

  node_translation:
    -
      plugin: explode
      source: src
      delimiter: /
    -
      # If the source path has no slashes return a dummy default value.
      plugin: extract
      default: 'INVALID_NID'
      index:
        - 1
    -
      plugin: migration_lookup
      migration: d6_node_translation

So, when d6_url_alias or d7_url_alias (they have the same process in the pipeline) runs and d6_node_translation (or d7_node_translation) isn't found an exception is thrown and the row is not saved. But it should be saved. Before this change migration_lookup would return NULL and the row would save resulting in a successful migration. As it is now, content_translation must be enabled for the URL alias migrations to be successful.

If there is agreement on that can this be reverted?

Changing priority since this breaks migrations without content_translation enabled. Tempted to mark critical because of the data loss in the migration process.

Why does the path migration depend on a content translation migration? That seems wrong, just shown by the fact MigrationLookup now throws an error. Shouldn't node or content_translation edit the migration dynamically to add the "node_translation" parts to the url alias migrations?

So neat we discovered two things here. We seem not to have a test for the url alias migrtations without content_translation in core and that there seems to be a valid case when you have no valid migrations to look up.

I've reverted the issue so we can address these things.

@alexpott, thanks.

It look like the addition of d6_node_translation to the migration was added in #2827644: Fix path alias migration of translated nodes [D6].

I implemented @benji's suggestion from #33 and I moved all translation related code over to Content Translation.

I also modified the URL alias tests so that the first test is run without Content Translation enabled. I included a fail patch to show the bug.

Now, having all the files ready to upload, I'm wondering if I should have done this in a different issue. I'll upload them anyway so we can see if the test are green. If anyone think it should be done in a new issue, I'll be happy to do so.

1. +++ b/core/modules/content_translation/content_translation.module
   @@ -627,3 +627,49 @@ function content_translation_page_attachments(&$page) {
   +  if (isset($definitions['d7_url_alias'])) {
   +    $definitions['d7_url_alias']['process']['node_translation'] = [
   ...
   +  if (isset($definitions['d6_url_alias'])) {
   +    $definitions['d6_url_alias']['process']['node_translation'] = [
   

   We cannot be sure we know the ID and it won't change. We'll need to use the source plugin's class to check this. Can we add this in a deriver? I would much rather see this there than in a plugin alter. Much cleaner code.

2. +++ b/core/modules/migrate/src/Plugin/migrate/process/MigrationLookup.php
   @@ -155,10 +156,17 @@ public function transform($value, MigrateExecutableInterface $migrate_executable
   +    // If no migrations are loaded it means either the migration is unavailable
   +    // (perhaps in a disabled module) or doesn't exist (probably mis-typed).
   +    if (empty($migrations)) {
   

   Can we provide a config option to migration_lookup that will let us disable the exception throwing? Defaults to throw.

Thanks for the review @heddn!

1. I added the UrlAliasDeriver. I hesitated between adding two derivers, one for D6 and one for D7, or only one deriver with an if statement. I chose the later.
2. I added the validate_migration configuration to the migration_lookup plugin, defaulting to TRUE.

1. +++ b/core/modules/migrate/src/Plugin/migrate/process/MigrationLookup.php
   @@ -93,6 +96,17 @@
   + * it's unavailable or it doesn't exists, set the 'validate_migration' to FALSE.
   

   For readability, let's state this as, "set validate_migration to FALSE."

2. +++ b/core/modules/path/src/Plugin/migrate/UrlAliasDeriver.php
   @@ -0,0 +1,94 @@
   +            'plugin' => 'migration_lookup',
   +            'migration' => 'd6_node_translation',
   ...
   +            'plugin' => 'migration_lookup',
   +            'migration' => 'd7_node_translation',
   

   I think this is the only thing different, so can we move that part into the version conditional logic and move the rest outside so as to reduce duplicate code?

I'm also a little torn about defaulting to always throwing an exception. I understand it will help, but it could easily be a distraction. And in a way, if we want to stretch things a little, we are breaking BC. Because previously migrations that were built wouldn't throw that exception. What if we add validate_migration => TRUE to all migration_lookups in core yamls. Then all new migrations will get this feature. But ones that have been running for quite some time will not suddenly start failing. We still want to default to TRUE on any new generated migrations, but not on things have already been generated and someone is currently using.

1. Done.
2. Done.

I agree that this kinda break BC because an Exception might be thrown where it wasn't before. But I don't understand how we can default to TRUE for new migrations but not for existing (contrib or custom) migrations.

We can sorta do that for new if we assume that folks are copy/pasting the yaml files or using some type of config system with migrate_plus config entities. In those cases, the yml files they use are copies. If we just change all of core's templates and all uses of derivers, etc, to set validate_migration = TRUE, then we sorta get this for new migrations and not for existing. If we assume that existing copies wouldn't have the validate_migration in their yaml files because they copied them before we added it everywhere.

Discussed in the Migrate maintainer meeting. First, we should move the url_alias stuff to a new issue. Second, let's add this functionality with a default to FALSE, so we don't break BC, and open a follow-up to figure out how to implement it (adding the config to all migration_lookup plugins?).

This was committed to Drupal 8.5.x and re-opened because it the url_alias migrations were failing if content_translation was not enabled. That was a problem with a change of behavior introduced here and was fixed in #3086952: Migration Lookup should catch PluginNotFoundException exceptions and ignore them. and a test without content_translation was added. The following work here was fixing the problem by adding a lot more code by using a deriver.

Therefor, restoring the issue meta data to the time this was committed.

Thanks.