MigrationLookup doesn't create stub when there's multiple migrations.

Add related issues

I believe it's worse than that, if you look at how $self is set:

    foreach ($migrations as $migration_id => $migration) {
      if ($migration_id == $this->migration->id()) {
        $self = TRUE;
      }
      ...
    }

This means that if one of the migrations is the current one, stub_id is ignored, even if it is set explicitly. Which seems like a bug.

The stub_id configuration should take precedence on the fact that the current migration is in the list of migrations.

In fact I doubt if the current migration being in the list has any meaning wrt the stubbing so perhaps we should delete it and take the first.

OK, I think this really needs to start with some tests. I am pretty familiar with the issue we are facing here, and to figure out all the scenarios and make sure we don't break backwards compatibility, let's start with some tests.

Note that as JvE mentioned, the last else can never be reached, no need for tests to prove that but it would indeed be useful to have this covered by tests.

I'm going to throw my hat into this one, starting with this test. This tests against the documented logic, asserting that in a single migration lookup, the single migration is used, and in a multiple migration lookup, the first migration in the list is selected unless 'stub_id' is set, in which case that migration is selected. If also asserts that the self migration is not a factor in this logic.

I'm going to follow it up with a patch that actually implements that logic just to see what regressions we have. The whole $self thing confuses me. As I was going through it, I thought for a while that it was put in as a way to select from a set of derivative migrations. since MigrationPluginManager::CreateInstances() will split a migration into its derivatives, If you lookup, say d7_node, and then have to go create a stub, you would probably need to pick a specific node migration to stub, to properly configure a destination plugin.

The problem is 'self' is a really bad way to try to select a derivative. It really has no bearing on which derivative you actually want to run, and if you don't configure the derivative specifically, 'self' doesn't seem like a much better choice than 'first'

It may have just come along since a self reference would be a typical use case for stubs, along with circular references, but again, not in the documentation, and no good reason. So, all I can think of to do is fix the 'bug' and see what breaks, and then decide from there if it needs some BC, or if anything it breaks is buggy in and of itself.

And here's a fix to match the documentation. It passes the migrate and migrate_drupal tests, but I haven't run it through the whole gambit, so no idea what might break.

6 failures is better than I expected. 4 are in the path module, they do a lookup against d6/7_node_translations with no stub id. Prior to the patch, these would not have created stubs because node_translations would be split into derived migrations, and fail to select one to stub. I think now that it is creating new translation node stubs, the expected node ids path's test's are checking for have all changed. adding 'no_stub:true' to the migration silences the errors, but I think it should actually be stubbing them, and the tests should be updated. I'm not certain where in the order that migration usually fires, so I don't know if it even matters outside the scope of the tests.

I'm looking into the 2 Functional tests as well to see if I can figure out what is going on with those.

Added reroll of patch #11 on Drupal 9.4.x.

This problem is also about that for stubbing, the plugin should be able to identify which migration contains the source entry(entries) with the matching ID(s) before it actually tries to create the stub(s?).

We (@Wim Leers and I) tried to add this in #3156730: Stubs should only be created if the referenced source row actually exists, but because of the concerns core maintainers raised there we moved all of our work into Migrate Magician module. The migmag_lookup process plugin over there does exactly what @JvE needs imho.

Rerolled #11, essentially deprecating $self and making the stub_id more deterministic rather than relying on the first entry.

It should only create a stub if:

• The stub_id is specified,
• Or performing a single migration lookup

This reduces the amount of breaking changes as much as possible, and seems like a reasonable compromise rather than picking the first migration from the list to use as a stub.
We may choose to throw an exception if an appropriate stub could not be determined if no_stub is FALSE, causing the developer to fix their code.

@huzooka, thanks for the update. That module certainly looks promising, I'll be sure to look into it!