Drupal 7 to 9 acl_user migration error on incremental or update

I wonder whether removing the unset() fixes the Update but breaks the Insert...

@salvis, as I read the issue description, I had the same instinct as you in #4. Does the import() API provides the action type (update v.s. insert)? It doesn't look like it to me. Or maybe one of the internal properties has the action type. I will have to take a closer look tomorrow.

In any case, I don't have a D7 environment or a setup that allows me to test this easily.

Hey @salvis. I am not familiar with the migration API. The best I can think of is the following:

  /**
   * {@inheritdoc}
   */
  public function import(Row $row, array $old_destination_id_values = []) {
    $destination = $row->getDestination();
    $needsUpdate = $row->needsUpdate();

    $keys = [];
    foreach (array_keys($this->ids) as $id) {
      $keys[$id] = $destination[$id];
      if (!$needsUpdate) {
        unset($destination[$id]);
      }
    }

    \Drupal::database()->merge($this->tableName)
      ->keys($keys)
      ->fields($destination)
      ->execute();

    return array_map(function ($key) use ($row) {
      return $row->getDestinationProperty($key);
    }, $keys);
  }

I'm not entirely sure that $row->needsUpdate() actually does what It sounds like here (identify whether $row represents and update or an insert).

On an unrelated note, the return statement seems odd (its cleaned up above).

But, the more I look at this, the more I am also curious what the heck the unset() is all about. The database merge() function automatically handles whether to update or insert ( example → ). I don't really see why we would need to be removing key/value pairs from $destination. That said, I'm not very familiar with this API, so I could be missing something.

The has me wondering... is this an issue with updates in general, or only into the context of migrating to D9. I'm guessing the former, as there doesn't appear to be any ACL migration code specifically relevant to D9.

I see we have migration tests, but I cannot easily determine if the use case described in this issue is covered by the test suite. If not, do you have a system for testing this type of D7->D9 workflow?

rerolling for testrunner

Thank you for working on this, @xeM8VfDh!

Unfortunately, I know close to nothing about the migration process and its test coverage.

We have have not enough evidence for or against the change, so let's leave it as it is right now.

The patch is here, if anyone finds that it's needed, please provide feedback here!

Thanks @salvis, fine leaving this for now if it expedites D10 release