Stubs should only be created if the referenced source row actually exists

1) Drupal\Tests\migrate_drupal_ui\Functional\d7\Upgrade7Test::testMigrateUpgradeExecute
Found 16 menu_link_content entities, expected 12.

This means there's probably 4 entities pointing to non-existing menu links

All other failures are only occurring for Drupal 6 migrations; apparently there's something different about how Drupal 6 migration plugins/definitions are processed.

#7: superb comment! Crystal clear. I obviously made grossly simplistic assumptions. Thanks for describing in detail why I was wrong, it makes it very easy to see that your proposed patch is the appropriate solution!

1. 🥳🥳🥳 Tested in detail, works great!

2. +++ b/core/modules/migrate/src/MigrateStub.php
   @@ -70,13 +76,59 @@ public function createStub($migration_id, array $source_ids, array $default_valu
   +        $sql_succeed = TRUE;
   

   🤓 Nit: I'd have preferred to start with $sql_succeed = TRUE and then set it to FALSE if an exception is caught.

   I think that'd be easier to understand.

   Right now on the first reading it would seem that if the SQL query doesn't find the requested source row, that then we fall back to the slower behavior anyway! That's not the case, but restructuring the logic that slight way would make it more obvious I think 😊

3. +++ b/core/modules/migrate/src/MigrateStub.php
   @@ -70,13 +76,59 @@ public function createStub($migration_id, array $source_ids, array $default_valu
   +    // Cannot use the source plugin's query and condition, let's do the slower
   +    // discovery.
   +    if (!$exists_in_source && !$sql_succeed) {
   +      try {
   +        foreach ($source_plugin as $row) {
   +          assert($row instanceof Row);
   +          $id_values = [];
   +          foreach (array_keys($source_ids) as $source_id_key) {
   +            $id_values[$source_id_key] = $row->getSourceProperty($source_id_key);
   +          }
   +          $id_values = array_filter($id_values);
   +          if ($exists_in_source = $id_values === $source_ids) {
   +            break 1;
   +          }
   +        }
   +      }
   +      catch (\Exception $e) {
   +      }
   +    }
   +
   

   👍 This will be slow but there's nothing we can do about it…

4. +++ b/core/modules/migrate/src/Plugin/migrate/process/MigrationLookup.php
   @@ -289,4 +271,85 @@ protected function isValid($value) {
   +        catch (PluginNotFoundException $e) {
   

   🤓 Shouldn't this be $exception instead of $e? Otherwise it will not be rethrown…

5. +++ b/core/modules/migrate/src/Plugin/migrate/process/MigrationLookup.php
   @@ -289,4 +271,85 @@ protected function isValid($value) {
   +        catch (MigrateException $exception) {
   +        }
   +        catch (MigrateSkipRowException $exception) {
   +        }
   +        catch (\Exception $exception) {
   +        }
   

   🙏 Let's not lose the helpful comments that used to be there before this refactor?

6. +++ b/core/modules/migrate/tests/src/Unit/MigrateStubTest.php
   @@ -43,27 +44,54 @@ protected function setUp(): void {
   +    $source_plugin = new EmbeddedDataSource([
   +      'data_rows' => [
   +        $row_source_2->getSource(),
   +      ],
   +      'ids' => $ids,
   +    ], 'embedded_data', [], $migration->reveal());
   

   Clever 😃

7. +++ b/core/modules/migrate/tests/src/Unit/MigrateStubTest.php
   @@ -43,27 +44,54 @@ protected function setUp(): void {
   +    // If MigrateStub is asked to create only valid stubs, then with the
   +    // incoming "['id' => 1]" source IDs array shouldn't create a stub.
   +    $this->assertSame(FALSE, $stub->createStub('test_migration', $row_source_1_missing->getSource(), [], NULL, TRUE));
   +    $this->assertSame($destination_2, $stub->createStub('test_migration', $row_source_2->getSource(), [], NULL, TRUE));
   

   🥳

8. +++ b/core/modules/taxonomy/tests/modules/taxonomy_term_stub_test/migrations/taxonomy_term_stub_test_valid.yml
   @@ -0,0 +1,31 @@
   +label: Taxonomy term migration which stubs a valid source id
   

   🤔 What makes this have a valid source ID and taxomy_term_stub_test not? See next point.

9. +++ b/core/modules/taxonomy/tests/src/Kernel/Migrate/MigrateTaxonomyTermStubTest.php
   @@ -67,20 +68,24 @@ public function testStubWithWeightMapping() {
   -    // We have a term referencing an unmigrated parent, forcing a stub to be
   -    // created.
   -    $migration = $this->getMigration('taxonomy_term_stub_test');
   -    $term_executable = new MigrateExecutable($migration, $this);
   +    // We have a term referencing a not migrated parent. Check whether a valid
   +    // stub is created.
   +    $migration_valid = $this->getMigration('taxonomy_term_stub_test_valid');
   +    $migration_missing_stub = $this->getMigration('taxonomy_term_stub_test');
   +    $term_executable = new MigrateExecutable($migration_valid, $this);
        $term_executable->import();
   -    $this->assertNotEmpty($migration->getIdMap()->getRowBySource(['2']), 'Stub row exists in the ID map table');
   +    $this->assertNotEmpty($migration_missing_stub->getIdMap()->getRowBySource(['3']), 'Stub row exists in the ID map table');
    
        // Load the referenced term, which should exist as a stub.
   -    /** @var \Drupal\Core\Entity\ContentEntityBase $stub_entity */
   -    $stub_entity = Term::load(2);
   -    $this->assertNotEmpty($stub_entity, 'Stub successfully created');
   -    if ($stub_entity) {
   -      $this->assertCount(0, $stub_entity->validate(), 'Stub is a valid entity');
   -    }
   +    $stub_entity = Term::load(3);
   +    $this->assertTrue($stub_entity instanceof TermInterface, 'Stub successfully created');
   +    $this->assertCount(0, $stub_entity->validate(), 'Stub is a valid entity');
   +
   +    // We have a term referencing a missing parent: check whether no stub is
   +    // created.
   +    $term_executable = new MigrateExecutable($migration_missing_stub, $this);
   +    $term_executable->import();
   

   🤔🙏 It took me a while to understand that the new taxonomy_term_stub_test_valid is to be used in tandem with taxonomy_term_stub_test, specifically for this test. I think this would've clarified it a lot:

    .../taxonomy/tests/src/Kernel/Migrate/MigrateTaxonomyTermStubTest.php   | 2 ++
    1 file changed, 2 insertions(+)
   
   diff --git a/core/modules/taxonomy/tests/src/Kernel/Migrate/MigrateTaxonomyTermStubTest.php b/core/modules/taxonomy/tests/src/Kernel/Migrate/MigrateTaxonomyTermStubTest.php
   index c007426796..cc3c286f76 100644
   --- a/core/modules/taxonomy/tests/src/Kernel/Migrate/MigrateTaxonomyTermStubTest.php
   +++ b/core/modules/taxonomy/tests/src/Kernel/Migrate/MigrateTaxonomyTermStubTest.php
   @@ -80,11 +80,13 @@ public function testStubWithParentStub() {
        $stub_entity = Term::load(3);
        $this->assertTrue($stub_entity instanceof TermInterface, 'Stub successfully created');
        $this->assertCount(0, $stub_entity->validate(), 'Stub is a valid entity');
   +    $this->assertNotSame('cat', $stub_entity->label());
    
        // We have a term referencing a missing parent: check whether no stub is
        // created.
        $term_executable = new MigrateExecutable($migration_missing_stub, $this);
        $term_executable->import();
   +    $this->assertSame('cat', Term::load(3)->label());
        $this->assertEmpty($migration_missing_stub->getIdMap()->getRowBySource(['2']), 'Stub row not exists in the ID map table, since "2" is missing from the source rows');
      }
    
   

10. +++ b/core/modules/taxonomy/tests/src/Kernel/Migrate/MigrateTaxonomyTermStubTest.php
    @@ -67,20 +68,24 @@ public function testStubWithWeightMapping() {
    +    $this->assertEmpty($migration_missing_stub->getIdMap()->getRowBySource(['2']), 'Stub row not exists in the ID map table, since "2" is missing from the source rows');
    

    🤓 s/not exists/does not exist/

+++ b/core/modules/migrate/src/Plugin/migrate/process/MigrationLookup.php
@@ -289,4 +271,85 @@ protected function isValid($value) {
+      throw new MigrateException(sprintf('A(n) %s was thrown while attempting to stub.', gettype($exception)), $exception->getCode(), $exception);

This is resulting in hard-to-debug messages such as:

A(n) object was thrown while attempting to stub.

It'd be better to include the exception class + message, like so:

throw new MigrateException(sprintf('A(n) %s was thrown while attempting to stub, with the following message: %s.', get_class($exception), $exception->getMessage()), $exception->getCode(), $exception);

1. +++ b/core/modules/migrate/src/Plugin/migrate/process/MigrationLookup.php
   @@ -321,7 +321,17 @@ protected function getDestinationIds(array $source_id_values): ?array {
   +        catch (\LogicException $e) {
   

   Ah yes, InvalidArgumentException extends LogicException and therefore this catch would've prevented the problem we saw in #21. 👍

2. +++ b/core/modules/taxonomy/tests/src/Kernel/Migrate/MigrateTaxonomyTermStubTest.php
   @@ -68,24 +69,29 @@ public function testStubWithParentStub() {
   +    $this->assertEquals(MigrateIdMapInterface::STATUS_NEEDS_UPDATE, $stub_row_3['source_row_status']);
   ...
   +    // The previously stubbed parent term should be fully migrated.
   +    $this->assertEquals(MigrateIdMapInterface::STATUS_IMPORTED, $migration_missing_stub->getIdMap()->getRowBySource(['3'])['source_row_status']);
   

   WFM! 👍

+++ b/core/modules/migrate/src/Plugin/migrate/process/MigrationLookup.php
@@ -144,13 +153,16 @@ class MigrationLookup extends ProcessPluginBase implements ContainerFactoryPlugi
-  public function __construct(array $configuration, $plugin_id, $plugin_definition, MigrationInterface $migration, MigrateLookupInterface $migrate_lookup, MigrateStubInterface $migrate_stub) {
+  public function __construct(array $configuration, $plugin_id, $plugin_definition, MigrationInterface $migration, MigrateLookupInterface $migrate_lookup, MigrateStubInterface $migrate_stub, MigrationPluginManagerInterface $migration_plugin_manager) {
...
+    $this->migrationPluginManager = $migration_plugin_manager;

@@ -163,7 +175,8 @@ public static function create(ContainerInterface $container, array $configuratio
+      $container->get('migrate.stub'),
+      $container->get('plugin.manager.migration')

This is a BC break.

AFAICT this BC break can easily be avoided? 🤔

#3146646-2: Clean up ParagraphsLookup could've been avoided too then?

Ahhh, that looks much better indeed! And much simpler 💪

For #35.

#3207968: Replace @codingStandards comments with phpcs: comments broke this between 9.1.7 and 9.2.0-rc1.

This does not address #34 yet, so keeping NW.

Thank you, @floydm! 😊 I'm glad Acquia isn't the only organization using this Migration system improvement! 👍

I will not have capacity to address the maintainer's feedback from #34.

Conflicted with #3312733: SQL migrations cannot be instantiated if database is not available and Node, Migrate Drupal modules are enabled. Rerolled #53.