Improve variable names in MigrationLookup for DX

Here is a patch.

Hi @mahesh thanks for the patch I can confirm this applies cleanly on branch 8.6.x-dev.Just one small thing for joachim may be....

Hi @joachim do you think using $migration_lookups instead of $lookup_migrations and $migration_lookup instead of $lookup_migration makes more sense while naming this variable, the name of file being ../MigrationLookup.php

@msankhala, thanks for the patch. Clarifying code is always welcome.

On review I have one question and one change.

The transform method begins with this block where $migration_ids refers to the migrations to be looked up, which the suggestion is to call them $lookup_migration. So, shouldn't this be changed to $lookup_migrations_id and $lookup_migrations_ids?

    $migration_ids = $this->configuration['migration'];
    if (!is_array($migration_ids)) {
      $migration_ids = [$migration_ids];
    }

+++ b/core/modules/migrate/src/Plugin/migrate/process/MigrationLookup.php
@@ -155,11 +155,11 @@ public function transform($value, MigrateExecutableInterface $migrate_executable
+    /** @var \Drupal\migrate\Plugin\MigrationInterface[] $lookup_migrations */

This should be moved to the line before $lookup_migrations is first used.

Hi @quietone, Thanks for the review. Yes I agree with your suggestion about moving /** @var \Drupal\migrate\Plugin\MigrationInterface[] $lookup_migrations */ right above $lookup_migrations is first used.

Actually, it was already there so I left it unmodified. But I am happy to modify it.

The transform method begins with this block where $migration_ids refers to the migrations to be looked up, which the suggestion is to call them $lookup_migration. So, shouldn't this be changed to $lookup_migrations_id and $lookup_migrations_ids?

Yes, calling them $lookup_migrations_ids and $lookup_migration_id make more sense. It was not explicitly mentioned in scope of the issue so i left it.

I would like to know @joachim's opinion on this.

removed duplicate comment.

@msankhala, thanks for continuing to work on this.

I think it is ok to go ahead and make the changes. Why? Because when I read the issue it states that the meaning of $migrations is deduced from reading the foreach block not that the change is limited to foreach block.

For the inline @var I mean that that should be zero lines between it and where $lookup_migrations is first used. When I apply the patch there are 2 intervening lines.

Here is updated patch.

assigning for review

Installed the patch and read the code. Much better and easier to follow. I was troubled for a while with the double plural of 'lookup_migrations_ids' but since there could be multiple migrations to lookup and they could have multiple ids, it make some sense. So, yes let's move this along.

And the inline doc comment was moved as well.

Looks like testbot hickup. Rescheduling test.

Back to RTBC.

This is kind of weird. Why test boat is only showing PHP 7 & MySQL 5.5 Build Successful instead of showing the number of tests being passed. Any idea?

I think the testbot failures have been resolved, back to RTBC.

Patch applied cleanly not sure why it failed by testbot.

I'll check if patch needs reroll. Here is failure reason.

Patch Failed to Apply - View results on dispatcher

error: patch failed: core/modules/migrate/src/Plugin/migrate/process/MigrationLookup.php:150
error: core/modules/migrate/src/Plugin/migrate/process/MigrationLookup.php: patch does not apply

Yes this needs reroll.

MigrationLookup file recently changed in https://cgit.drupalcode.org/drupal/commit/core/modules/migrate/src/Plugi...

Here is the rerolled patch.

Change in MigrationLookup is reverted now https://cgit.drupalcode.org/drupal/commit/core/modules/migrate/src/Plugi...

See https://www.drupal.org/project/drupal/issues/2865247#comment-12625399

Adding reroll tag

I'll work on the reroll of this patch.

Hi guys,

I have rerolled the patch following Rerolling patches and Rerolling patches: Failed Rebase. Unfortunately, there was a conflict when I did the reroll specifically in what @msankhala shared from https://cgit.drupalcode.org/drupal/commit/core/modules/migrate/src/Plugi.... Basically the change conflicts fixes were to:

1. Transfer the var comment above `$destination_ids = NULL;` instead of directly above the `$lookup_migrations` declaration.
2. Remove the "If no migrations are loaded it means..." comment section based on the latest changes from the provided issue ticket.
3. Remove the MigrateException `if` block based also on the latest changes from the provided issue ticket.

Hope everything is in order.

Thanks!

This re-name will definitely make it easier to read the code. Great fixes.

Looks like an issue with test bot. Putting it to Need review again. Once passed it is ready for RTBC.

Tests are green, back to RTBC.

Add tag for DX and change title

This is definitely an improvement but I think if we're addressing all the $migration(s) confusion we have one more thing to address:

+++ b/core/modules/migrate/src/Plugin/migrate/process/MigrationLookup.php
@@ -184,17 +186,17 @@ public function transform($value, MigrateExecutableInterface $migrate_executable
       // If the lookup didn't succeed, figure out which migration will do the
       // stubbing.
       if ($self) {
         $migration = $this->migration;
       }
       elseif (isset($this->configuration['stub_id'])) {
-        $migration = $migrations[$this->configuration['stub_id']];
+        $migration = $lookup_migrations[$this->configuration['stub_id']];
       }
       else {
-        $migration = reset($migrations);
+        $migration = reset($lookup_migrations);
       }

I think we should consider renaming $migration here too. Because it's not always the $migration in progress. Maybe we should go for something like $stub_migration?

Here is the updated patch as per @alexpott suggestion in #38.

Looks good to me. Feedback in #38 is addressed.

Committed 37f6cce and pushed to 8.7.x. Thanks!