Create Migration Lookup and Stub services

Triaging issue queue.

1. +++ b/core/modules/file/src/Plugin/migrate/process/d6/FieldFile.php
   @@ -35,32 +43,49 @@ class FieldFile extends ProcessPluginBase implements ContainerFactoryPluginInter
   +  public function __construct(array $configuration, $plugin_id, $plugin_definition, MigrationInterface $migration, $migrate_lookup) {
   

   The constructors of plugins are explicitly not part of the BC contract. What is the reason we are doing this complicated BC layer here? Do we have a lot of folks that are extending file field?

2. +++ b/core/modules/migrate/src/MigrateLookup.php
   @@ -0,0 +1,66 @@
   +  public function lookup($migration, array $source_ids) {
   +    if (!is_array($source_ids)) {
   

   I'm pretty sure this requires $source_ids to be an array based on the type hint.

3. +++ b/core/modules/migrate/src/MigrateStub.php
   @@ -0,0 +1,108 @@
   +  public function stub($migration, $source_ids, array $default_values = []) {
   ...
   +    if (!is_array($source_ids)) {
   

   Can we type hint an array here?

I suggest stubMigration() as method name

+++ b/core/modules/migrate/src/MigrateStub.php
@@ -0,0 +1,136 @@
+  public function stub($migration_id, array $source_ids, array $default_values = [], $derivative_id = NULL) {
+    try {
+      $migrations = $this->migrationPluginManager->createInstances($migration_id);
...
+    return $this->doStub($migration, $source_ids, $default_values);
...
+    try {
+      $destination_ids = $destination->import($row);
...
+      return $destination_ids;

+++ b/core/modules/migrate/src/MigrateStubInterface.php
@@ -0,0 +1,27 @@
+   * Creates a stub.
...
+   * @return array
+   *   An array of destination ids for the new stub.
+   */
+  public function stub($migration_id, array $source_ids, array $default_values = [], $derivative = NULL);

Something wrong with naming here
- it creates stub just internally to return destination IDs
- also under the hood it importing row and that's what it returns

+1 on general approach. Some minor feedback below.

1. +++ b/core/modules/migrate/src/MigrateLookup.php
   @@ -0,0 +1,92 @@
   + * Provides a migration lookup and stubbing service.
   ...
   +   * @var \Drupal\Core\Logger\LoggerChannelInterface
   

   This doesn't stub any more.

2. +++ b/core/modules/migrate/src/MigrateLookup.php
   @@ -0,0 +1,92 @@
   +      $this->logger->error("Could not create an instance of the migration plugin {migration_id} for lookup. The error was: {message}", ['migration_id' => $migration_id, 'message' => $e->getMessage()]);
   

   Nit, we've been typically using double quotes, '"', around variables in error strings like this.

3. +++ b/core/modules/migrate/src/MigrateStub.php
   @@ -0,0 +1,127 @@
   +  public function stub(string $migration_id, array $source_ids, array $default_values = []) {
   

   I agree, we need something like "create" as part of this method name.

1. +++ b/core/modules/migrate/migrate.services.yml
   @@ -30,3 +30,12 @@ services:
   +  migrate.lookup:
   ...
   +  migrate.stub:
   

   I like these as 2 different services.

2. +++ b/core/modules/migrate/src/MigrateStub.php
   @@ -0,0 +1,180 @@
   +  public function createStubRaw(string $migration_id, array $source_ids) {
   

   If we plan to mark this as deprecated, let's mark it as @internal. Would that help in any way?

3. +++ b/core/modules/migrate/src/MigrateStub.php
   @@ -0,0 +1,180 @@
   +      $stub = array_combine(array_keys($migration->getDestinationPlugin()->getIds()), $stub);
   

   A description and example of what we are doing here would help.

I like it. Let's just bounce over to RTBC and see how that fares.

This looks great, nice cleanup and solid test-coverage.

1. +++ b/core/modules/migrate/src/MigrateLookup.php
   @@ -0,0 +1,96 @@
   +  public function lookup(string $migration_id, array $source_ids) {
   
   +++ b/core/modules/migrate/src/MigrateLookupInterface.php
   @@ -0,0 +1,26 @@
   +  public function lookup(string $migration_id, array $source_ids);
   

   note to self: we're using a scalar type-hint here so this is solid 8.8 only territory.

   but we don't use a return type-hint...but we have no cases of that in core (yet) so we shouldn't start doing that here in my opinion

2. +++ b/core/modules/migrate/src/MigrateLookup.php
   @@ -0,0 +1,96 @@
   +        $this->logger->error('Could not create an instance of the migration plugin "{migration_id}" for lookup. The plugin was not found.', ['migration_id' => $migration_id]);
   ...
   +      $this->logger->error('Could not create an instance of the migration plugin "{migration_id}" for lookup. The error was: "{message}"', ['migration_id' => $migration_id, 'message' => $e->getMessage()]);
   

   These are fairly silent errors. Should we return a value object instead that has a ::lookupSucceeded method and a ::getResults so calling code can work out if/what went wrong? Or throw exceptions?

3. +++ b/core/modules/migrate/src/MigrateLookup.php
   @@ -0,0 +1,96 @@
   +      $this->logger->error('Could not retrieve destination ids for provided source ids.  The error was: "{message}"', ['message' => $e->getMessage()]);
   

   Same here

4. +++ b/core/modules/migrate/src/MigrateStub.php
   @@ -0,0 +1,182 @@
   +    if (array_keys($stub) === range(0, count($stub) - 1)) {
   +      // If the return from ::import is numerically indexed, index it
   +      // associatively using the destination id keys.
   +      $stub = array_combine(array_keys($migration->getDestinationPlugin()->getIds()), $stub);
   +    }
   +    return $stub;
   

   It seems that the only difference between createStubRaw and createStub is this hunk - should we add a fourth parameter to createStub something like bool $keyByDestinationIds or similar and then just have the one method? Or does that paint us into a corner if/when we remove the issues in the various plugins where the return is not consistent?

5. +++ b/core/modules/migrate/src/MigrateStub.php
   @@ -0,0 +1,182 @@
   +      $this->logger->error('Cannot stub derivable migration "{migration_id}".  You must specify the id of a specific derivative to stub.', ['migration_id' => $migration_id]);
   +      return FALSE;
   ...
   +      $this->logger->error('Cannot create stub for migration "{migration_id}". Expected and provided source id counts do not match.', ['migration_id' => $migration->id()]);
   +      return FALSE;
   

   Again, these are very silent failures. It is very reminiscent of a lot of the stuff we used to have in file.inc that now (thankfully) throws actual exceptions so you don't need to go trawling through logs to work out why your image styles aren't getting created :)

   Is there are reason why we don't throw exceptions for these things that are clearly wrong. e.g. the 'you must specify the id of a specific derivative' is basically a bad method call or logic exception - shouldn't we convey that? Its not really something an admin/site owner (who logs are generally for) can fix, its something the calling code (developer) should fix - so an exception might be more appropriate?

6. +++ b/core/modules/migrate/src/MigrateStubInterface.php
   @@ -0,0 +1,51 @@
   +   * @internal
   ...
   +  public function createStubRaw(string $migration_id, array $source_ids);
   

   If this is internal, we shouldn't put it on the interface - as that conveys a mixed message - and once its there, its forever.

7. +++ b/core/modules/migrate/src/Plugin/migrate/process/MigrationLookup.php
   @@ -158,18 +186,15 @@ public static function create(ContainerInterface $container, array $configuratio
   +    if (!is_array($lookup_migration_ids)) {
   +      $lookup_migration_ids = [$lookup_migration_ids];
        }
   

   can't we just do a hard cast here?

   $lookup_migration_ids = (array) $lookup_migration_ids;
   

   And avoid the if?

Not seeing much to comment upon.

1. +++ b/core/modules/migrate/src/MigrateException.php
   @@ -46,7 +46,7 @@ class MigrateException extends \Exception {
   +    parent::__construct($message, $code, $previous);
   

   This seems out of scope.

2. +++ b/core/modules/migrate/src/MigrateLookupResult.php
   @@ -0,0 +1,124 @@
   +  public function lookupSucceeded(): bool {
   ...
   +  public function setLookupSucceeded(bool $lookupSucceeded) {
   ...
   +  public function getMessage() {
   ...
   +  public function setMessage(string $message) {
   ...
   +  public function getResults(): array {
   ...
   +  public function setResults(array $results) {
   

   Let's be consistent w/ our type and return type hinting.

3. +++ b/core/modules/migrate/src/MigrateStub.php
   @@ -0,0 +1,120 @@
   +  public function createStub(string $migration_id, array $source_ids, array $default_values = [], $key_by_destination_ids = NULL) {
   

   Do we need to trigger error (yet) on this as deprecated for keying by destination id?

Some feedback on coding standards and comments.

1. +++ b/core/modules/migrate/src/MigrateStub.php
   @@ -0,0 +1,120 @@
   +      throw new \LogicException(sprintf('Cannot stub derivable migration "%s".  You must specify the id of a specific derivative to stub.', $migration_id));
   
   +++ b/core/modules/migrate/src/Plugin/migrate/process/MigrationLookup.php
   @@ -130,13 +140,26 @@ class MigrationLookup extends ProcessPluginBase implements ContainerFactoryPlugi
   +      @trigger_error("Not passing the migrate lookup service as the fifth parameter to " . __METHOD__ . " is deprecated in drupal:8.8.0 and will throw a type error in drupal:9.0.0. Pass an instance of \Drupal\migrate\MigrateLookupInterface. See https://www.drupal.org/node/3047268", E_USER_DEPRECATED);
   ...
   +      @trigger_error("Not passing the migrate stub service as the sixth parameter to " . __METHOD__ . " is deprecated in drupal:8.8.0 and will throw a type error in drupal:9.0.0. Pass an instance of \Drupal\migrate\MigrateStubInterface. See https://www.drupal.org/node/3047268", E_USER_DEPRECATED);
   
   @@ -181,10 +200,24 @@ public function transform($value, MigrateExecutableInterface $migrate_executable
   +        throw new MigrateException(sprintf("A %s was thrown while processing this migration lookup", gettype($e)), 0, $e);
   
   @@ -193,45 +226,35 @@ public function transform($value, MigrateExecutableInterface $migrate_executable
   +        throw new MigrateException(sprintf("A(n) %s was thrown while attempting to stub.", 0, $e));
   

   Values should be surrounded by single quotes, not double quotes.

2. +++ b/core/modules/migrate/src/MigrateLookup.php
   @@ -0,0 +1,74 @@
   +   * Perform a direct lookup.
   

   My first though was 'Is there an indirect lookup?'. Can we explain what direct means here.

3. +++ b/core/modules/migrate/src/MigrateLookup.php
   @@ -0,0 +1,74 @@
   +   *   Thrown when $source_id_values contains unknown keys, or is the wrong
   +   *   length.
   

   Can we have something clearer than 'wrong length' as it sounds like a problem with string length whereas it is about finding extra ids.

4. +++ b/core/modules/migrate/src/Plugin/migrate/process/MigrationLookup.php
   @@ -156,20 +180,15 @@ public static function create(ContainerInterface $container, array $configuratio
        /** @var \Drupal\migrate\Plugin\MigrationInterface[] $lookup_migrations */
   

   No longer used.

5. +++ b/core/modules/migrate/src/Plugin/migrate/process/MigrationLookup.php
   @@ -193,45 +226,35 @@ public function transform($value, MigrateExecutableInterface $migrate_executable
   +        throw new MigrateException(sprintf("A(n) %s was thrown while attempting to stub.", 0, $e));
   

   Seem to have an extra argument, '0'. Can be removed.

6. +++ b/core/modules/migrate/tests/src/Kernel/MigrateStubTest.php
   @@ -0,0 +1,130 @@
   +   * The migrate stub service..
   

   s/../.

1. +++ b/core/modules/migrate/src/Plugin/migrate/process/MigrationLookup.php
   @@ -193,45 +226,35 @@ public function transform($value, MigrateExecutableInterface $migrate_executable
   -      $stub_row = $this->createStubRow($values + $stub_migration->getSourceConfiguration(), $source_ids);
   

   With this removed the createStubRow method is no longer used. Remove it?

2. +++ b/core/modules/migrate/src/MigrateStub.php
   @@ -0,0 +1,120 @@
   +    $migrations = $this->migrationPluginManager->createInstances($migration_id);
   

   createInstances expects the parameter to be an array but a string is being passed.

3. +++ b/core/modules/migrate/tests/src/Unit/MigrateLookupTest.php
   @@ -0,0 +1,63 @@
   +    $lookup = new MigrateLookup($plugin_manager->reveal(), $logger->reveal());
   

   Why pass the logger?

4. +++ b/core/modules/migrate/tests/src/Unit/MigrateStubTest.php
   @@ -0,0 +1,102 @@
   +    $stub = new MigrateStub($this->migrationPluginManager->reveal(), $this->logger->reveal());
   ...
   +    $stub = new MigrateStub($this->migrationPluginManager->reveal(), $this->logger->reveal());
   ...
   +    $stub = new MigrateStub($this->migrationPluginManager->reveal(), $this->logger->reveal());
   

   Why pass the logger?

5. +++ b/core/modules/migrate/src/MigrateStub.php
   @@ -0,0 +1,120 @@
   +   * @param bool $key_by_destination_ids
   

   Regarding this parameter, In the comment in #28 paragraph 2 mikelutz states

   Thankfully all the destination plugins do return this array, but they are inconsistently keyed. The core destination plugins return a numerically indexed array of destination identifiers, but some rather important contrib destinations (I'm looking at you, entity_reference_revisions....) return an array of identifiers indexed by the destination keys.

   I feel like this info should be in the comment, that is, that core returns one type of array and that contrib may be different.

And finally,

+++ b/core/modules/migrate/tests/modules/migrate_lookup_test/migrate_lookup_test.info.yml
@@ -0,0 +1,8 @@
+name: 'Migration Lookup Test'
...
+description: 'Provides test migrations to test migration lookup service.'

+++ b/core/modules/migrate/tests/modules/migrate_lookup_test/migrations/sample_lookup_migration.yml
@@ -0,0 +1,20 @@
+label: "Sample Lookup Migration"
...
+      title: "Node 1"

Quotes are not needed. They all can be removed. Of course, this applies to the other test module as well.

+++ b/core/modules/migrate/tests/src/Kernel/MigrateLookupTest.php
@@ -0,0 +1,162 @@
+class MigrateLookupTest extends MigrateTestBase {

As soon I pressed 'enter' I remembered this one as well. The test coverage in all the tests looks good to me. However, I'd like to see MigrateLookupTest also test with an id that is a string. A common example is a language code.

Came back to look at something and noticed a typo.

+++ b/core/modules/migrate/tests/src/Unit/MigrateStubTest.php
@@ -0,0 +1,102 @@
+   * {@inheritDoc}

s/inheritDoc/inheritdoc/

@mikelutz, thanks for the fixes.

There are two issue about quotes in yml files, though specfic to info.yml.
#2116897: Adopt identical YAML formatting for info.yml files
#2994928: Quote consistency in .info.yml files for Core modules

Why don't we make an issue to fix the doc on createInstances? I can do that later today if noone else gets to it.

And this can go back to RTBC. It was back in #32 and later larowlan reviewed it noting that it " looks great, nice cleanup and solid test-coverage". Since then it has been cleanup up some more and another test added.

Just a couple of minor things, but otherwise, this is looking good

1. +++ b/core/modules/migrate/src/MigrateLookup.php
   @@ -0,0 +1,74 @@
   +      if ($result = $this->doLookup($migration, $source_id_values)) {
   +        $results = array_merge($results, $result);
   +      }
   

   ubernit: array_merge with an empty array is a nil-op, so we probably don't need the if here i.e. we could write this as

   $results = array_merge($results, $this->doLookup(...));
   

2. +++ b/core/modules/migrate/src/MigrateStub.php
   @@ -0,0 +1,130 @@
   +   * @throws \Drupal\Component\Plugin\Exception\PluginException
   +   * @throws \Drupal\Component\Plugin\Exception\PluginNotFoundException
   +   * @throws \Drupal\migrate\MigrateException
   

   These don't meet PHPCS, we should be detailing the scenarios in which this can happen or similar.

@mikelutz pointed me at https://www.drupal.org/node/1354#throws

Fixed on commit

diff --git a/core/modules/migrate/tests/src/Unit/MigrateLookupTest.php b/core/modules/migrate/tests/src/Unit/MigrateLookupTest.php
index f6c7dfe3d7..417b08a864 100644
--- a/core/modules/migrate/tests/src/Unit/MigrateLookupTest.php
+++ b/core/modules/migrate/tests/src/Unit/MigrateLookupTest.php
@@ -3,7 +3,6 @@
 namespace Drupal\Tests\migrate\Unit;

 use Drupal\Component\Plugin\Exception\PluginNotFoundException;
-use Drupal\Core\Logger\LoggerChannelInterface;
 use Drupal\migrate\MigrateLookup;
 use Drupal\migrate\Plugin\MigrateDestinationInterface;
 use Drupal\migrate\Plugin\MigrateIdMapInterface;
diff --git a/core/modules/migrate/tests/src/Unit/MigrateStubTest.php b/core/modules/migrate/tests/src/Unit/MigrateStubTest.php
index 8f3816c5e0..0a026ffc52 100644
--- a/core/modules/migrate/tests/src/Unit/MigrateStubTest.php
+++ b/core/modules/migrate/tests/src/Unit/MigrateStubTest.php
@@ -3,7 +3,6 @@
 namespace Drupal\Tests\migrate\Unit;

 use Drupal\Component\Plugin\Exception\PluginNotFoundException;
-use Drupal\Core\Logger\LoggerChannelInterface;
 use Drupal\migrate\MigrateStub;
 use Drupal\migrate\Plugin\MigrateDestinationInterface;
 use Drupal\migrate\Plugin\MigrateIdMapInterface;
diff --git a/core/modules/migrate/tests/src/Unit/process/LegacyMigrationLookupTest.php b/core/modules/migrate/tests/src/Unit/process/LegacyMigrationLookupTest.php
index a28c9d8a31..96a0685356 100644
--- a/core/modules/migrate/tests/src/Unit/process/LegacyMigrationLookupTest.php
+++ b/core/modules/migrate/tests/src/Unit/process/LegacyMigrationLookupTest.php
@@ -4,7 +4,6 @@

 use Drupal\migrate\Plugin\migrate\process\MigrationLookup;
 use Drupal\migrate\Plugin\MigrationInterface;
-use Drupal\Tests\migrate\Unit\MigrateTestCase;

 /**
  * Tests deprecated features of the migration lookup plugin.
@@ -31,7 +30,7 @@ public function testCreateStubRow() {
     /** @var \Drupal\migrate\Row $row */
     $row = $method->invoke($lookup, [
       'id' => 1,
-      'value' => 'test'
+      'value' => 'test',
     ], ['id' => ['type' => 'integer']]);
     $this->assertTrue($row->isStub());
     $this->assertSame('test', $row->get('value'));

Committed 63ecb2d and pushed to 8.8.x. Thanks!

Published change record

I'm having trouble with a test in commerce migrate and have tracked it to this issue. I create a new test, MigrateUrlAliasNoTranslationTest.php that does not run d7_node_translation. This test fails on HEAD but passes when the commit in this issue is reverted. This patch has just that new test.

I have learned that when a migration does not exist, in this case d6_node_translation, that migrate lookup with throw a pluginnotfound exception which is caught when processing the pipeline and then import is never run. Before this patch, when a migration was not found the pipeline would store an empty value in the destination property ($row->setEmptyDestinationProperty($destination)) and the pipeline would continue to be processed.

Oops the test class is incorrectly named. Just a reflection of how well my day is going. I am really looking forward to the long walk into town soon for dinner and the symphony.

But I see that mikelutz has fixed that in the followup. Thanks.

Where can I find an example of the usage of the Lookup service?

Francisco