Issue #2716133 by quietone, kevinquillen, phenaproxima, mikeryan, Charlotte17: Roles are duplicated instead of updated existing roles

core/modules/migrate_drupal/tests/fixtures/drupal6.php

@@ -42136,13 +42136,13 @@ $connection->insert('permission')
 ->values(array(
   'pid' => '1',
   'rid' => '1',
-  'perm' => 'migrate test anonymous permission',
+  'perm' => 'access content, migrate test anonymous permission',
   'tid' => '0',
 ))
 ->values(array(
   'pid' => '2',
   'rid' => '2',
-  'perm' => 'migrate test authenticated permission',
+  'perm' => 'access comments, access content, post comments, post comments without approval, migrate test authenticated permission',
   'tid' => '0',
 ))
 ->values(array(
@@ -42157,18 +42157,6 @@ $connection->insert('permission')
   'perm' => 'migrate test role 2 test permission, use PHP for settings, administer contact forms, skip comment approval, edit own blog content, edit any blog content, delete own blog content, delete any blog content, create forum content, delete any forum content, delete own forum content, edit any forum content, edit own forum content, administer nodes',
   'tid' => '0',
 ))
-->values(array(
-  'pid' => '5',
-  'rid' => '1',
-  'perm' => 'access content',
-  'tid' => '0',
-))
-->values(array(
-  'pid' => '6',
-  'rid' => '2',
-  'perm' => 'access comments, access content, post comments, post comments without approval',
-  'tid' => '0',
-))
 ->execute();
 
 $connection->schema()->createTable('profile_fields', array(

core/modules/migrate_drupal_ui/src/Tests/d7/MigrateUpgrade7Test.php

@@ -56,13 +56,13 @@ class MigrateUpgrade7Test extends MigrateUpgradeTestBase {
       'search_page' => 2,
       'shortcut' => 6,
       'shortcut_set' => 2,
-      'action' => 18,
+      'action' => 16,
       'menu' => 10,
       'taxonomy_term' => 18,
       'taxonomy_vocabulary' => 3,
       'tour' => 4,
       'user' => 4,
-      'user_role' => 4,
+      'user_role' => 3,
       'menu_link_content' => 9,
       'view' => 12,
       'date_format' => 11,

core/modules/user/migration_templates/d6_user_role.yml

@@ -9,11 +9,6 @@ process:
     -
       plugin: machine_name
       source: name
-    -
-      plugin: dedupe_entity
-      entity_type: user_role
-      field: id
-      length: 32
     -
       plugin: user_update_8002
   label: name

core/modules/user/migration_templates/d7_user_role.yml

@@ -9,11 +9,6 @@ process:
     -
       plugin: machine_name
       source: name
-    -
-      plugin: dedupe_entity
-      entity_type: user_role
-      field: id
-      length: 32
     -
       plugin: user_update_8002
   label: name

core/modules/user/src/Plugin/migrate/source/d6/Role.php

@@ -69,7 +69,15 @@ class Role extends DrupalSqlBase {
       ->condition('rid', $rid)
       ->execute()
       ->fetchField();
+
+    // If a role has no permissions then set to an empty array. The role will
+    // be migrated and given the default D8 permissions.
+    if ($permissions) {
       $row->setSourceProperty('permissions', explode(', ', $permissions));
+    }
+    else {
+      $row->setSourceProperty('permissions', []);
+    }
     if (isset($this->filterPermissions[$rid])) {
       $row->setSourceProperty("filter_permissions:$rid", $this->filterPermissions[$rid]);
     }

core/modules/user/tests/src/Kernel/Migrate/d6/MigrateUserRoleTest.php

@@ -3,7 +3,9 @@
 namespace Drupal\Tests\user\Kernel\Migrate\d6;
 
 use Drupal\user\Entity\Role;
+use Drupal\user\RoleInterface;
 use Drupal\Tests\migrate_drupal\Kernel\d6\MigrateDrupal6TestBase;
+use Drupal\migrate\Plugin\MigrateIdMapInterface;
 
 /**
  * Upgrade user roles to user.role.*.yml.
@@ -21,29 +23,69 @@ class MigrateUserRoleTest extends MigrateDrupal6TestBase {
   }
 
   /**
-   * Tests user role migration.
+   * Helper function to perform assertions on a user role.
+   *
+   * @param string $id
+   *   The role ID.
+   * @param string[] $permissions
+   *   An array of user permissions.
+   * @param int $lookupId
+   *   The original numeric ID of the role in the source database.
+   * @param \Drupal\migrate\Plugin\MigrateIdMapInterface $id_map
+   *   The map table plugin.
    */
-  public function testUserRole() {
+  protected function assertRole($id, array $permissions, $lookupId, MigrateIdMapInterface $id_map) {
-    /** @var \Drupal\migrate\Plugin\MigrationInterface $migration */
+    /** @var \Drupal\user\RoleInterface $role */
-    $id_map = $this->getMigration('d6_user_role')->getIdMap();
+    $role = Role::load($id);
-    $rid = 'anonymous';
+    $this->assertInstanceOf(RoleInterface::class, $role);
-    $anonymous = Role::load($rid);
+    $this->assertSame($permissions, $role->getPermissions());
-    $this->assertSame($rid, $anonymous->id());
+    $this->assertSame([[$id]], $id_map->lookupDestinationIds(['rid' => $lookupId]));
-    $this->assertSame(array('migrate test anonymous permission', 'use text format filtered_html'), $anonymous->getPermissions());
+  }
-    $this->assertSame(array($rid), $id_map->lookupDestinationId(array(1)));
+
-    $rid = 'authenticated';
+  /**
-    $authenticated = Role::load($rid);
+   * Helper function to test the migration of the user roles. The user roles
-    $this->assertSame($rid, $authenticated->id());
+   * will be re-imported and the tests here will be repeated.
-    $this->assertSame(array('migrate test authenticated permission', 'use text format filtered_html'), $authenticated->getPermissions());
+   *
-    $this->assertSame(array($rid), $id_map->lookupDestinationId(array(2)));
+   * @param \Drupal\migrate\Plugin\MigrateIdMapInterface $id_map
-    $rid = 'migrate_test_role_1';
+   *   The map table plugin.
-    $migrate_test_role_1 = Role::load($rid);
+   */
-    $this->assertSame($rid, $migrate_test_role_1->id());
+  protected function assertRoles(MigrateIdMapInterface $id_map) {
-    $this->assertSame(array('migrate test role 1 test permission', 'use text format full_html', 'use text format php_code'), $migrate_test_role_1->getPermissions());
+
-    $this->assertSame(array($rid), $id_map->lookupDestinationId(array(3)));
+    // The permissions for each role are found in the two tables in the Drupal 6
-    $rid = 'migrate_test_role_2';
+    // source database. One is the permission table and the other is the
-    $migrate_test_role_2 = Role::load($rid);
+    // filter_format table.
-    $this->assertSame(array(
+    $permissions = [
+      // From permission table.
+      'access content',
+      'migrate test anonymous permission',
+      // From filter_format tables.
+      'use text format filtered_html'
+    ];
+    $this->assertRole('anonymous', $permissions, 1, $id_map);
+
+    $permissions = [
+      // From permission table.
+      'access comments',
+      'access content',
+      'post comments',
+      'skip comment approval',
+      'migrate test authenticated permission',
+      // From filter_format.
+      'use text format filtered_html',
+    ];
+    $this->assertRole('authenticated', $permissions, 2, $id_map);
+
+    $permissions = [
+      // From permission table.
+      'migrate test role 1 test permission',
+      // From filter format.
+      'use text format full_html',
+      'use text format php_code'
+    ];
+    $this->assertRole('migrate_test_role_1', $permissions, 3, $id_map);
+
+    $permissions = [
+      // From permission table.
       'migrate test role 2 test permission',
       'use PHP for settings',
       'administer contact forms',
@@ -59,14 +101,66 @@ class MigrateUserRoleTest extends MigrateDrupal6TestBase {
       'edit own forum content',
       'administer nodes',
       'access content overview',
+      // From filter format.
       'use text format php_code',
-      ), $migrate_test_role_2->getPermissions());
+    ];
-    $this->assertSame($rid, $migrate_test_role_2->id());
+    $this->assertRole('migrate_test_role_2', $permissions, 4, $id_map);
-    $this->assertSame(array($rid), $id_map->lookupDestinationId(array(4)));
+
-    $rid = 'migrate_test_role_3_that_is_long';
+    // The only permission for this role is a filter format.
-    $migrate_test_role_3 = Role::load($rid);
+    $permissions = ['use text format php_code'];
-    $this->assertSame($rid, $migrate_test_role_3->id());
+    $this->assertRole('migrate_test_role_3_that_is_longer_than_thirty_two_characters', $permissions, 5, $id_map);
-    $this->assertSame(array($rid), $id_map->lookupDestinationId(array(5)));
+  }
+
+  /**
+   * Tests user role migration.
+   */
+  public function testUserRole() {
+    $id_map = $this->getMigration('d6_user_role')->getIdMap();
+    $this->assertRoles($id_map);
+
+    // Test there are no duplicated roles.
+    $roles = [
+      'anonymous1',
+      'authenticated1',
+      'administrator1',
+      'migrate_test_role_11',
+      'migrate_test_role_21',
+      'migrate_test_role_3_that_is_longer_than_thirty_two_characters1'
+    ];
+    $this->assertEmpty(Role::loadMultiple($roles));
+
+    // Remove the map row for the migrate_test_role_1 role and rerun the
+    // migration. This will re-import the migrate_test_role_1 role migration
+    // again.
+    $this->sourceDatabase->insert('role')
+      ->fields([
+        'rid' => 6,
+        'name' => 'migrate test role 4',
+      ])
+      ->execute();
+    $this->sourceDatabase->insert('permission')
+      ->fields([
+        'pid' => 7,
+        'rid' => 6,
+        'perm' => 'access content',
+        'tid' => 0,
+      ])
+      ->execute();
+
+    $id_map->delete(['rid' => 3]);
+
+    $this->executeMigration('d6_user_role');
+
+    // Test there are no duplicated roles.
+    $roles[] = 'migrate_test_role_41';
+    $this->assertEmpty(Role::loadMultiple($roles));
+
+    // Test that the existing roles have not changed.
+    $this->assertRoles($id_map);
+
+    // Test the migration of the new role, migrate_test_role_4.
+    $permissions = ['access content'];
+    $this->assertRole('migrate_test_role_4', $permissions, 6, $id_map);
   }
 
 }

core/modules/user/tests/src/Kernel/Migrate/d7/MigrateUserRoleTest.php

@@ -56,6 +56,50 @@ class MigrateUserRoleTest extends MigrateDrupal7TestBase {
     $this->assertEntity('anonymous', 'anonymous user', 1);
     $this->assertEntity('authenticated', 'authenticated user', 2);
     $this->assertEntity('administrator', 'administrator', 3);
+    // Test there are no duplicated roles.
+    $roles = [
+      'anonymous1',
+      'authenticated1',
+      'administrator1',
+    ];
+    $this->assertEmpty(Role::loadMultiple($roles));
+
+    // Remove the map row for the administrator role and rerun the migration.
+    // This will re-import the administrator role again.
+    $id_map = $this->getMigration('d7_user_role')->getIdMap();
+    $id_map->delete(['rid' => 3]);
+
+    $this->sourceDatabase->insert('role')
+      ->fields([
+        'rid' => 4,
+        'name' => 'test role',
+        'weight' => 10,
+      ])
+      ->execute();
+    $this->sourceDatabase->insert('role_permission')
+      ->fields([
+        'rid' => 4,
+        'permission' => 'access content',
+        'module' => 'node',
+      ])
+      ->execute();
+    $this->executeMigration('d7_user_role');
+
+    // Test there are no duplicated roles.
+    $roles = [
+      'anonymous1',
+      'authenticated1',
+      'administrator1',
+    ];
+    $this->assertEmpty(Role::loadMultiple($roles));
+
+    // Test that the existing roles have not changed.
+    $this->assertEntity('administrator', 'administrator', 3);
+    $this->assertEntity('anonymous', 'anonymous user', 1);
+    $this->assertEntity('authenticated', 'authenticated user', 2);
+
+    // Test the migration of the new role, test role.
+    $this->assertEntity('test_role', 'test role', 4);
   }
 
 }