MigrateExecutable::rollback incorrectly assumes MigrateMapInterface::getRowByDestination() returns an array with 'rollback_action' key

+++ b/core/modules/migrate/src/MigrateExecutable.php
@@ -313,7 +313,7 @@ public function rollback() {
         $map_row = $id_map->getRowByDestination($destination_key);
-        if ($map_row['rollback_action'] == MigrateIdMapInterface::ROLLBACK_DELETE) {
+        if ($map_row && array_key_exists('rollback_action', $map_row) && $map_row['rollback_action'] == MigrateIdMapInterface::ROLLBACK_DELETE) {

Per \Drupal\migrate\Plugin\MigrateIdMapInterface::getRowByDestination()'s docs, $map_row must indeed ben an array:

   * @return array
   *   The row(s) of data.
   */
  public function getRowByDestination(array $destination_id_values);

And indeed, until #3227549: Sql id map plugin's getRowByDestination shouldn't return FALSE lands, it actually could be FALSE instead of an array.

👍 But even after that lands, it's indeed unguaranteed that a rollback_action key actually exists.

1. +++ b/core/modules/migrate/tests/src/Unit/MigrateExecutableTest.php
   @@ -498,4 +515,176 @@ protected function getMockSource() {
   +      'Rolling back with ID map having records with duplicated destination ID' => [
   +        'ID map records' => [
   +          [
   +            'source_1' => '1',
   +            'source_2' => '1',
   +            'destination' => '1',
   +            'rollback_action' => '0',
   +          ],
   +          [
   +            'source_1' => '2',
   +            'source_2' => '2',
   +            'destination' => '2',
   +            'rollback_action' => '1',
   +          ],
   +          [
   +            'source_1' => '3',
   +            'source_2' => '3',
   +            'destination' => '1',
   +            'rollback_action' => '0',
   +          ],
   +        ],
   

   This is the one that's failing, the second time we call

   $id_map->getRowByDestination($destination_key)
   

   with
   $destination_key = ['destination' => '1'];

   … but 100% of these ID map record have rollback_action specified. So … not sure I 100% understand what's going on here yet …

2. +++ b/core/modules/migrate/tests/src/Unit/MigrateExecutableTest.php
   @@ -498,4 +515,176 @@ protected function getMockSource() {
   +    $id_map->getRowByDestination(Argument::type('array'))->will(function () {
   +      $destination_ids = func_get_args()[0][0];
   +      $return = array_reduce(self::$idMapRecords, function (array $carry, array $record) use ($destination_ids) {
   +        if (array_merge($record, $destination_ids) === $record) {
   +          $carry = $record;
   +        }
   +        return $carry;
   +      }, []);
   +
   +      return $return;
   +    });
   

   Aha! So this is mocking the behavior of \Drupal\migrate\Plugin\MigrateIdMapInterface::getRowByDestination() and really of \Drupal\migrate\Plugin\migrate\id_map\Sql::getRowByDestination().

   I had to step through this with a debugger to understand.

   What this is doing is reducing the original records in the data provider to just the ones that matched. Which means that a repeat call for the same destination will yield a different result: it'll have been reduced away…

   🤔 This does not seem to really match the real-world behavior?

So … +1 for the hardening, but I still have doubts about the test coverage. @huzooka, could you clarify? 😊🙏

What this is doing is reducing the original records in the data provider to just the ones that matched. Which means that a repeat call for the same destination will yield a different result: it'll have been reduced away…

🤔 This does not seem to really match the real-world behavior?

@huzooka just walked me through this!~ Even though this seems weird and wrong, it actually is accurately mocking the behavior of \Drupal\migrate\MigrateExecutable::rollback(), where it does:

        $map_row = $id_map->getRowByDestination($destination_key);
        if ($map_row['rollback_action'] == MigrateIdMapInterface::ROLLBACK_DELETE) {
          $this->getEventDispatcher()
            ->dispatch(new MigrateRowDeleteEvent($this->migration, $destination_key), MigrateEvents::PRE_ROW_DELETE);
          $destination->rollback($destination_key);
          $this->getEventDispatcher()
            ->dispatch(new MigrateRowDeleteEvent($this->migration, $destination_key), MigrateEvents::POST_ROW_DELETE);
        }
        // We're now done with this row, so remove it from the map.
        $id_map->deleteDestination($destination_key);

That last line is deleting every row from the mapping table that contains the given destination key (which btw is an array of destination IDs, not a single key — so the name is pretty misleading), which is equivalent to the mocked behavior that I quoted in my previous comment! 👍👍👍

So … while this is perhaps a bit complicated/weird mock, it really is due to the weirdness in:

1. \Drupal\migrate\Plugin\migrate\id_map\Sql::getRowByDestination() which is not aware of the fact that multiple record can have the same destination ID — which particularly in d7_field_formatter_settings is not a correct assumption
2. \Drupal\migrate\Plugin\migrate\id_map\Sql::deleteDestination()
3. \Drupal\migrate\MigrateExecutable::rollback()

and the way these three interact.

Looks good, just a micro-optimisation suggestion

+++ b/core/modules/migrate/src/MigrateExecutable.php
@@ -313,7 +313,7 @@ public function rollback() {
-        if ($map_row['rollback_action'] == MigrateIdMapInterface::ROLLBACK_DELETE) {
+        if ($map_row && array_key_exists('rollback_action', $map_row) && $map_row['rollback_action'] == MigrateIdMapInterface::ROLLBACK_DELETE) {

Now that we have null coalesce, we could write this as

if (($map_row['rollback_action'] ?? NULL) === MigrateIdMapInterface::ROLLBACK_DELETE)

See https://3v4l.org/C6dJp

#10.3, Can anyone provide an example where the map for d7_field_formatter_settings can can multiple records with the same destination IDs?

I was re reading this change and think that using the rollback constants defined in MigrateIdMapInterface make the test a bit more readable, even though the comments are excellent. So I made that change.

This was to be marked a duplicate in [#3239298#comment-36]. Doing that now.

This was completely absorbed by #3239298: Fix \Drupal\migrate\Plugin\migrate\destination\EntityConfigBase::updateEntity() so that config translation migrations can be rolled back, which has been committed! 👍🥳