Migration lookup process plugin doesn't call setMessage on the migration idMap

Good catch, I saw this happen recently and didn't have a chance to track it down.

At this points bug fixes should go against 8.3.x - I don't believe there will be any more 8.2.x releases, barring critical security fixes (which won't include "normal" bug fixes).

Assigning to myself to do a review this week.

This needs a re-roll after #2845486: Rename Migration process plugin and add documentation. Can we also see a tests only patch to demonstrate the problem?

Re roll.

Correct the omissions in the re-roll.

Re-setting to Needs Work for adding a tests-only patch to demonstrate the problem (as requested by @heddn in #16).

Added a test to make sure setMessage() is called.

Assigning to myself to review this week.

I'm comfortable calling this one done. The test looks pretty heafty, not sure if that could be refactored but it does the job. @heddn, feel free to jump in.

+++ b/core/modules/migrate/src/Plugin/migrate/process/MigrationLookup.php
@@ -222,15 +223,17 @@ public function transform($value, MigrateExecutableInterface $migrate_executable
+      $id_map->setMessage(new MigrateMessage());

This is tying understanding of how the id map works in the migration lookup class.

Are we sure we want to do that?

Shouldn't the id map do this itself? It would make sense for it to have knowledge of how to create a new MigrateMessage class if it needs one.

It doesn't make sense for this knowledge to be embedded in the MigrationLookup class. It also means anyone else using that needs to know that knowledge too.

This is known as 'Inappropriate intimacy'.

+++ b/core/modules/migrate/src/Plugin/migrate/process/MigrationLookup.php
@@ -222,15 +223,17 @@ public function transform($value, MigrateExecutableInterface $migrate_executable
+        $id_map->saveMessage($stub_row->getSourceIdValues(), $e->getMessage());

Perhaps saveMessage could new it up or inject it somehow if the object doesn't yet exist. I think doing that would address the problem in the IS. If we do that, do we want to deprecate setMessage then? I haven't looked at the code closely enough to make a call on that.

this is what I was trying to articulate earlier

1. +++ b/core/modules/migrate/src/Plugin/MigrateIdMapInterface.php
   @@ -268,10 +268,10 @@ public function destroy();
      public function setMessage(MigrateMessageInterface $message);
   

   Should we mark setMessage as deprecated then too?

2. +++ b/core/modules/migrate/src/Plugin/migrate/id_map/Sql.php
   @@ -156,6 +157,7 @@ public function __construct(array $configuration, $plugin_id, $plugin_definition
   +    $this->message = new MigrateMessage();
   

   This looks great.

3. +++ b/core/modules/migrate/tests/src/Unit/process/MigrationLookupTest.php
   @@ -62,6 +62,7 @@ public function testTransformWithStubbing() {
   +    $destination_id_map->setMessage(Argument::any())->willReturn(NULL);
   
   +++ b/core/modules/migrate/tests/src/Unit/process/MigrationTest.php
   @@ -108,6 +108,7 @@ public function testTransformWithStubbing() {
   +    $id_map->setMessage(Argument::any())->willReturn(NULL);
   

   Is this necessary any more? Or is that a follow-up?

There is one other call to setMessage so I think keep it, removing it is out of scope here.

w.r.t test changes - yep they can go.

Having waited 21 days for a Novice, I've gone ahead and made the minor changes recommended by @heddn and @larowlan in #29/#30.

This is probably ready for RTBC, but I can't do that having contributed a few patches to this issue.

Back to RTBC as items have been addressed from #29 and #30

Fixed this on commit to 8.5.x/cherry-pick to 8.4.x, thanks!

FILE: ...e/modules/migrate/src/Plugin/migrate/process/MigrationLookup.php
----------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
----------------------------------------------------------------------
 6 | WARNING | [x] Unused use statement
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------