Stubbing goes wrong in processes with several migrations

Can I get an update of the proposed solution? And some tests?

+++ b/core/modules/migrate/src/MigrateExecutableInterface.php
@@ -7,6 +7,11 @@
+  public function setMigration(MigrationInterface $migration);

I haven't looked closely at changes to the migration process plugin itself, but this is a BC break to the migrate module (which is currently in experimental beta status).

#2835586: Allow customization of stub rows from Migration process plugin is making more progress as it just went RTBC. If/when it gets committed, this will need a re-roll.

Re-roll of #13 for 8.4.x including improved PHPdocs.

@pritish.kumar Thanks for your corrections and especially for including an interdiff. Next time it would also be useful if you could add a short comment describing the change(s) to save us having to investigate the patch and/or interdiff.

@Jo Fitzgerald this patch may need a re-roll once #2864563: Migration lookup process plugin doesn't call setMessage on the migration idMap They are conflicting with these lines currently:

patching file core/modules/migrate/src/Plugin/migrate/process/MigrationLookup.php
Hunk #1 FAILED at 3.
Hunk #2 succeeded at 167 (offset 1 line).
Hunk #3 FAILED at 190.
Hunk #4 succeeded at 310 (offset 3 lines).

Adding a public method isn't a "BC Break" @mikeryan, it's an API addition, or am I missing something?

I'll take a look at reviewing this week.

Re-rolled.

Going to review again.

Can we get a test only patch demonstrating the problem? The problem with the migrate_lookup plugin, is it is so complicated, touching it with a 10 foot pole is dangerous.

Adding something to the executable interface is a break. But I think we can do this in a non-breaking method by adding a new interface that is implemented by Core's executable. Then do instance_of check. Not clean, but it keeps BC.

+++ b/core/modules/migrate/src/MigrateExecutable.php
@@ -103,10 +103,9 @@ class MigrateExecutable implements MigrateExecutableInterface {
   public function __construct(MigrationInterface $migration, MigrateMessageInterface $message = NULL, EventDispatcherInterface $event_dispatcher = NULL) {
...
+    $this->setMigration($migration);

Alternatively, we could just new up a new executable, no?

I had hoped to address @heddn's comments in #28 & #29, but with the code in front of me, I realise that I don't really understand what is required.

...so here is a re-roll because the patch in #25 no longer applies.

@heddn Would you mind expanding on your suggestions, please.

#28 suggests that we add a new interface and check if the executable implements this interface before calling setMigration. Let's call this option #1. Alternatively, in #29 I suggest we could just new up a new executable. That would sorta work because that object isn't a service. Its the runner. To determine that, we can run get_class() against the executable, then new up a new one passing into it the migration. Do this where ever we want to do use setMigration. So, 2 options.

Which is better? Why don't we do both? And use the second (more hacky) solution as a fallback. Then we can sunset that approach in 9.x when we can do things more properly.

This looks like the problem I am currently seeing, though it's not entirely clear from the summary.

Here's what I have:

1. The main migration is running. One of its process mappings uses the migration_lookup plugin, configured to use stubs.

2. MigrationLookup::transform() wants to create a stub row, and then run it through the normal migration process:

      $process = $migration->getProcess();
...
      $migrate_executable->processRow($stub_row, $process);

(That's the lookup $migration, not the main one! See #2963811: Improve variable names in MigrationLookup for DX!)

3. MigrateExecutable::processRow() is ok to receive a $process pipeline as its parameter:

  public function processRow(Row $row, array $process = NULL, $value = NULL) {

So far so good!

4. Here is where problems start. MigrateExecutable thinks it knows the migration it is working with -- the main one. It's not told any different.

  public function processRow(Row $row, array $process = NULL, $value = NULL) {
    foreach ($this->migration->getProcessPlugins($process) as $destination => $plugins) {

Uh-oh. It's calling getProcessPlugins() on the *main* migration, with the $process pipeline configuration from the lookup migration!

5. The Migration plugin class is ok to receive a $process for getProcessPlugins(). That's to allow for the SubProcess process plugin, where an inner process pipeline is used. In this case though, it's a WTF! Migration A should not be asked to do anything with Migration B's process pipeline!

  public function getProcessPlugins(array $process = NULL) {

6. And now the real problem. In Migration::getProcessPlugins(), every process plugin gets the migration it is working with passed in as a constructor parameter. That's the $this in these two lines:

            $this->processPlugins[$index][$property][] = $this->processPluginManager->createInstance('get', $configuration, $this);
...
            $this->processPlugins[$index][$property][] = $this->processPluginManager->createInstance($configuration['plugin'], $configuration, $this);

Of course, this is the WRONG migration now!

+++ b/core/modules/migrate/src/MigrateExecutable.php
@@ -118,6 +117,14 @@ public function __construct(MigrationInterface $migration, MigrateMessageInterfa
+   * {@inheritdoc}
+   */
+  public function setMigration(MigrationInterface $migration) {
+    $this->migration = $migration;
+    $this->migration->getIdMap()->setMessage($this->message);
+  }
+

I think this is too convoluted. It's going to mean that whenever we jump out of an inner migration, such as a lookup, we have to ensure we restore the previous migration. I see bugs arising from times when we forget to do that.

I was thinking we could add an optional $migration parameter to MigrateExecutable::processRow(), and then this call uses that if it's provided:

    foreach ($this->migration->getProcessPlugins($process) as $destination => $plugins) {

That would fix things pretty simply, with the addition of an optional parameter to the interface, which is allowed.

> Alternatively, we could just new up a new executable, no?

Do you mean have MigrationLookup do:

  $my_inner_executable = new MigrateExecutable($migration, $log, $options);
  $my_inner_executable->processRow($stub_row, $process);

That looks like a good approach too, and cleaner overall. I'm a bit concerned that there may be side-effects we don't know about to having two MigrateExecutable objects in play, since it's been designed to run as a singleton.

BTW:

> Adding something to the executable interface is a break.

As it's not tagged as @api, then this part of the policy at https://www.drupal.org/core/d8-bc-policy applies:

Interfaces within non-experimental, non-test modules not tagged with either @api or @internal
Interfaces that are not tagged with either @api or @internal can be safely used as type hints. No methods will be changed or removed from these interface in a breaking way.
However, we reserve the ability to add methods to these interfaces in minor releases to support new features. When implementing the interface, module authors are encouraged to either:

Related #3156730: Stubs should only be created if the referenced source row actually exists

We have written a replacement lookup plugin and released it in Migrate Magician. I think it does what you expect.

See https://www.drupal.org/project/drupal/issues/2891073#comment-14452582