Only migrate role permissions that exist on the destination

this adds a destination plugin for entity:user_role that will filter out permissions that do not exist on the destination.

Changing to Critical because #3055557: Role migration results in migration permissions that do not exist on the target site is critical.

Due to #3192893: [META] Serialization issues in Migration tests I had to use the debugger to get the exception messages. They all fail when getting all the permissions in the new EntityUserRole destination,

$this->permissionHandler->getPermissions()

The exception messages are:

d6/MigrateBlockContentTranslationTest.php
'Route "entity.filter_format.edit_form" does not exist.'

d7/MigrateBlockContentTranslationTest.php and d6/MigrateTermNodeTranslationTest.php
'You have requested a non-existent service "locale.config_manager".'

The errors are fixed by adding 'locale' to the list of modules installed in the test. I do not understand why a missing filter_format route is fixed by installing locale nor why the permission handler is requesting 'locale.config_manager' when it does not exist. This seems like a bug somewhere.

This affects d6 source as well.

+++ b/core/modules/user/src/Plugin/migrate/destination/EntityUserRole.php
@@ -0,0 +1,92 @@
+  /**
+   * Builds a user role entity destination.
+   *
+   * @param array $configuration
+   *   A configuration array containing information about the plugin instance.
+   * @param string $plugin_id
+   *   The plugin_id for the plugin instance.
+   * @param mixed $plugin_definition
+   *   The plugin implementation definition.
+   * @param \Drupal\migrate\plugin\MigrationInterface $migration
+   *   The migration.
+   * @param \Drupal\Core\Entity\EntityStorageInterface $storage
+   *   The storage for this entity type.
+   * @param array $bundles
+   *   The list of bundles this entity type has.
+   * @param \Drupal\Core\Language\LanguageManagerInterface $language_manager
+   *   The language manager.
+   * @param \Drupal\Core\Config\ConfigFactoryInterface $config_factory
+   *   The configuration factory.
+   * @param \Drupal\user\PermissionHandler $permission_handler
+   *   The module handler.
+   */
+  public function __construct(array $configuration, $plugin_id, $plugin_definition, MigrationInterface $migration, EntityStorageInterface $storage, array $bundles, LanguageManagerInterface $language_manager, ConfigFactoryInterface $config_factory, PermissionHandler $permission_handler) {
+    parent::__construct($configuration, $plugin_id, $plugin_definition, $migration, $storage, $bundles, $language_manager, $config_factory);
+    $this->permissionHandler = $permission_handler;
+  }
+
+  /**
+   * {@inheritdoc}
+   */
+  public static function create(ContainerInterface $container, array $configuration, $plugin_id, $plugin_definition, MigrationInterface $migration = NULL) {
+    $entity_type_id = static::getEntityTypeId($plugin_id);
+    return new static(
+      $configuration,
+      $plugin_id,
+      $plugin_definition,
+      $migration,
+      $container->get('entity_type.manager')->getStorage($entity_type_id),
+      array_keys($container->get('entity_type.bundle.info')->getBundleInfo($entity_type_id)),
+      $container->get('language_manager'),
+      $container->get('config.factory'),
+      $container->get('user.permissions')
+    );
+  }

it could use BC compatible patterns like https://git.drupalcode.org/project/drupal/-/blob/9.2.x/core/modules/work...

OK, changes made for #19.

I am starting to review this. If I forget to un-assign myself later today, feel free to reassign this issue.

The new destination class is very simple: kudos to @quietone and to everyone who helped design the migration framework! This class defines the entity:user_role destination plugin. Without this patch, that plugin is provided by the deriver from the EntityConfigBase class.

I looked into the user.permissions service, Drupal\user\PermissionHandler, to see how getPermissions() is implemented. At first, I was worried by method names like buildPermissionsYaml(), since I thought that would just find static permissions defined on YAML files. Looking a little closer, I see that permissions callbacks are declared in the YAML files, and those callbacks are invoked. Just to be sure, I tested \Drupal::service('user.permissions')->getPermissions() to make sure that dynamic permissions like view article revisions are included.

So far, so good!

My main concern is dependencies.

For the core migrations, we have to declare a dependency on any migration that (indirectly) creates new permissions. The obvious examples are the d6_node_type and d7_node_type migrations, which create things like the article content type and then the view article revisions permission, for example.

Is an optional dependency good enough? I think so.

We should review all the .permission.yml files in core modules to find other callbacks, in case there are less obvious examples. I hope we are not creating circular dependencies. Are there any config migrations that depend on d6_user_role or d7_user_role?

The other thing we have to do is warn anyone developing a custom migration to add similar dependencies. In practice, this should not be a big problem: after considering dependencies, migrations are normally run in alphabetical order (I think), so the user_role migrations should run near the end of the config migrations. (Do content migrations have some sort of implicit dependence on config migrations, or does that have to be declared explicitly?) But a custom migration might create a content type in a migration without the d6_ or d7_ prefix, and that could cause a problem.

We need at least a change record, and perhaps also a release note.

The change record should also mention that relevant modules should be enabled before running the migrations. For example, if the source site has the backup_migrate module enabled, as described in the issue summary, then until now there is no need to enable it on the destination site before migration. Now there is.

Could we add an option to the destination plugin and, if set, save a message when roles are filtered out?

There are 9 modules that define permission callbacks:

$ grep -A1 callback core/modules/*/*.permissions.yml
core/modules/content_moderation/content_moderation.permissions.yml:permission_callbacks:
core/modules/content_moderation/content_moderation.permissions.yml-  - \Drupal\content_moderation\Permissions::transitionPermissions
--
core/modules/content_translation/content_translation.permissions.yml:permission_callbacks:
core/modules/content_translation/content_translation.permissions.yml-  - \Drupal\content_translation\ContentTranslationPermissions::contentPermissions
--
core/modules/field_ui/field_ui.permissions.yml:permission_callbacks:
core/modules/field_ui/field_ui.permissions.yml-  - Drupal\field_ui\FieldUiPermissions::fieldPermissions
--
core/modules/filter/filter.permissions.yml:permission_callbacks:
core/modules/filter/filter.permissions.yml-  - Drupal\filter\FilterPermissions::permissions
--
core/modules/layout_builder/layout_builder.permissions.yml:permission_callbacks:
core/modules/layout_builder/layout_builder.permissions.yml-  - \Drupal\layout_builder\LayoutBuilderOverridesPermissions::permissions
--
core/modules/media/media.permissions.yml:permission_callbacks:
core/modules/media/media.permissions.yml-  - \Drupal\media\MediaPermissions::mediaTypePermissions
--
core/modules/node/node.permissions.yml:permission_callbacks:
core/modules/node/node.permissions.yml-  - \Drupal\node\NodePermissions::nodeTypePermissions
--
core/modules/rest/rest.permissions.yml:permission_callbacks:
core/modules/rest/rest.permissions.yml-  - Drupal\rest\RestPermissions::permissions
--
core/modules/taxonomy/taxonomy.permissions.yml:permission_callbacks:
core/modules/taxonomy/taxonomy.permissions.yml-  - Drupal\taxonomy\TaxonomyPermissions::permissions

I will have a look at the tests, but I am adding the tag for a change record and setting this issue to NW for the dependencies.

@benjifisher, thanks for reviewing this critical.

Ran out of steam just as I was about to leave a comment so this is very brief.

Are there any config migrations that depend on d6_user_role or d7_user_role?

Yes, d6_block and d7_block.

Added the logging of a message to the destination plugin and updated the tests. Also started the change record.

Looks like it is failing on ordering. This adds the source rid as the index to the array of expected messages. Maybe that will help.

Silly mistake.

Again? I don't understand how I keep making this mistake. I guess I should not be making a patch and just before or after an America's Cup race.

+1 for always logging the permissions that are dropped. My suggestion to add an option for that was over complicated.

It is moderately expensive to calculate the permissions: read a bunch of YAML files from disk, parse them, run 9 callback functions (more or less). Will it save time if we do this once in the constructor/create() method and save the result in a class variable? Plugins are only instantiated once, right?

I did not number the tasks in my previous comment. Instead, I will update the "Remaining tasks" section in the issue description. One item is additional test coverage. I see the test that "blog" permissions are logged, but we also need a test that these permissions will be migrated if the d?_node_type migration is run first.

28.
paragraph 2. I thought the same thing but didn't implement it, don't know why anymore. It has been added.
paragraph 3. Added a test that does the node_type migration first.

I have not added the dependency on node_type to the user role migration.

Yes, I did it again.

@quietone, thanks for the improvements!

The change to EntityUserRole looks good, saving the list of all permissions in the create() method.

There are a lot of changes to the tests. In general, the test coverage is great, but I have some suggestions below.

Back to NW for these changes and for the remaining items listed in the issue summary.

1. Can you put all the assert*() helper methods together? I do not feel strongly about whether they should go at the top of the file or the bottom. The existing helpers are at the top, so I would put the new ones there, unless you care more than I do.

2. Some of the new methods need type declarations and/or @param comments and/or the @param comments need type declarations. (But see the next point before fixing all of these.)

    +++ b/core/modules/user/tests/src/Kernel/Migrate/d6/MigrateUserRoleTest.php
    @@ -50,74 +106,24 @@ protected function assertRole($id, array $permissions, $lookupId, MigrateIdMapIn
        * @param \Drupal\migrate\Plugin\MigrateIdMapInterface $id_map
        *   The map table plugin.
        */
    -  protected function assertRoles(MigrateIdMapInterface $id_map) {
    +  protected function assertRoles($data, MigrateIdMapInterface $id_map) {
   
    @@ -157,11 +203,103 @@ public function testUserRole() {
    ...
    +  /**
    +   * Helper to test the logged migrate messages.
    +   *
    +   * @param array $expected_messages
    +   *   An array of log messages.
    +   * @param $id_map
    +   *   The migration ID map plugin.
    +   */
    +  public function assertMessages($expected_messages, $id_map) {
   
    +++ b/core/modules/user/tests/src/Kernel/Migrate/d7/MigrateUserRoleTest.php
    @@ -95,12 +105,297 @@ public function testUserRole() {
    ...
    +  /**
    +   * Helper to test the logged migrate messages.
    +   *
    +   * @param array $expected_messages
    +   *   An array of log messages.
    +   * @param $id_map
    +   *   The migration ID map plugin.
    +   */
    +  public function assertMessages($expected_messages, $id_map) {

3. In the D6 test, the assertRoles() method is always called with the first parameter set to $this->expectedRoleData. It would be simpler to let assertRoles() access the class variable directly, so it does not need the new $data parameter.

4. The D6 test sorts the permissions in assertRole(), so we could use array_merge() or array_push() here to add just the permissions that are added (indirectly) by the d6_node_type migration. That would emphasize that this is testing what I asked for in #28. On the other hand, this would make the tests less explicit, so maybe it is best to leave it as is.

    @@ -157,11 +203,103 @@ public function testUserRole() {
    ...
    +    // Assert the permissions per role.
    +    $id_map = $this->getMigration('d6_user_role')->getIdMap();
    +    $this->expectedRoleData['migrate_test_role_2']['permissions'] = [
    +      // From permission table.
    +      'access content overview',
    +      'administer nodes',
    +      'create forum content',
    +      'delete any forum content',
    +      'delete own forum content',
    +      'edit any forum content',
    +      'edit own forum content',
    +      'use text format php_code',
    +    ];
    +    $this->assertRoles($this->expectedRoleData, $id_map);

5. Same snippet: either way, adjust the comments. Make the first something like "After the d6_node_type migration, there are additional roles." I think we can remove the second.

6. The $roles variable does not change between the two definitions, so we could remove the second. If we do that, then we should probably give it a more descriptive name, maybe $duplicate_role_names.

    +++ b/core/modules/user/tests/src/Kernel/Migrate/d7/MigrateUserRoleTest.php
    @@ -95,12 +105,297 @@ public function testUserRole() {
    ...
    +  public function testUserRoleWithNodeType() {
    ...
    +    // Test there are no duplicated roles.
    +    $roles = [
    +      'anonymous1',
    +      'authenticated1',
    +      'administrator1',
    +    ];
    +    $this->assertEmpty(Role::loadMultiple($roles));
    ...
    +    // Test there are no duplicated roles.
    +    $roles = [
    +      'anonymous1',
    +      'authenticated1',
    +      'administrator1',
    +    ];
    +    $this->assertEmpty(Role::loadMultiple($roles));

7. Same snippet: consider changing the second comment to "Test there are still no …".

P.S. (8):

    // Tests the migration log contains an error message.

It is a warning message, not an error message.

@benjifished, thanks for the review. I was initially cautious about further changes to the tests but on reading through they are all sensible and stay within scope.

#31.
1. Moved the helpers to the top to minimize the changes.
2. Fixed
3. Fixed
4. OK, leaving as is
5. Fixed
6. Fixed
7, Fixed
8. Fixed

Now add an optional dependency on the node type migration.

This requires some changes to the D6 test. So far, this issue has added a test to d6/MigrateUserRole that added the running of d6_node_type, so there are two tests one without running d6_node_type (testUserRole) and one with running d6_node_type (testUserRoleWithNodeType). However, now that the optional dependency has been added it is no longer a simple task to run the test that does not run d6_node_type. That is because \Drupal\Tests\migrate_drupal\Kernel\d6\MigrateDrupal6TestBase installs 7 modules, one of which is 'node'. That means that the node migrations are discovered and d6_user_role now requires d6_node_type. Because of the recently added test (testUserRoleWithNodeType) has been removed and the existing test (testUserRole) modified to handle the running of d6_node_type. This is all fine, when a dependency is added the migration test is altered.

For the companion d7/MigrateUserRole this is not a problem, the base class does not install modules so it can have both a testUserRole and a testUserRolwWithNodeType.

I should add that I tried a quick solution of uninstalling node in the D6 test and that fails with

1) Drupal\Tests\user\Kernel\Migrate\d6\MigrateUserRoleTest::testUserRole
Drupal\Core\Database\DatabaseExceptionWrapper: SQLSTATE[42S02]: Base table or view not found: 1146 Table 'test.test80182599users_data' doesn't exist: DELETE FROM "test80182599users_data" 
WHERE "module" IN (:db_condition_placeholder_0); Array
(
    [:db_condition_placeholder_0] => node
)

The test could be done by adding a test module that alters d6_user_role but that isn't needed - we know the dependency system works as we can see with the d7 test.

Adding the dependency to d6_user_role broke a lot of tests because of the problem mentioned about the base class enabling modules.

Missed two tests.

@quietone:

While I was taking a walk before dinner, I thought about how we could make this change less disruptive. Perhaps we could disable the check for whether permissions exist unless the migration specifies validate: true.

Now it is after dinner, and I am reading your comments. I think if we made this validation optional, it would make updating the tests much simpler. I think the trouble you ran into is a sign that the code is too tightly coupled, and adding an option might be the right way to loosen things up a bit.

I am also looking at the existing code, and I see that the validate option is defined in EntityConfigBase, so it is not available in EntityUserRole. (If we were dealing with a content-entity destination, then I guess we could be changing the entity type’s validate() method.) But we can add an option. To avoid confusion, I think we should call it validate_permissions instead of just validate.

The disadvantage of this approach is that validation would be disabled by default. If we want to change that, then we can open a follow-up issue to make validate_permissions a required option, or change the default. I think that would be simpler than the current issue, but more disruptive, so we would want to target Drupal 10.

I am sorry I did not think of this earlier. I guess I should take a walk before dinner more regularly.

I will not do a complete review at this point, but here are a few questions and comments on the latest patch.

1. You added $this->migrateContentTypes() to several tests. I guess that is so that content types are created, which affects the permissions. But it seems as though you added it to a lot of tests. Do they all end up running the user-role migrations and testing the migrated roles? In any event, we should need fewer changes if we add the validate_permissions option.

2. You also enabled menu_ui in many of the tests. I guess this is required by migrateContentTypes()?

3. Why make the node-type migration required for D6 and optional for D7?

    +++ b/core/modules/user/migrations/d6_user_role.yml
    @@ -39,4 +39,5 @@ destination:
       plugin: entity:user_role
     migration_dependencies:
       required:
    +    - d6_node_type
         - d6_filter_format
    +++ b/core/modules/user/migrations/d7_user_role.yml
    @@ -37,4 +37,5 @@ destination:
       plugin: entity:user_role
     migration_dependencies:
       optional:
    +    - d7_node_type
         - d7_filter_format

4. The @param comment is good, but this function still needs an array declaration:

    +++ b/core/modules/user/tests/src/Kernel/Migrate/d6/MigrateUserRoleTest.php
    @@ -44,72 +50,37 @@ protected function assertRole($id, array $permissions, $lookupId, MigrateIdMapIn
    ...
    +  protected function assertRoles($expected_role_data, MigrateIdMapInterface $id_map) {

5. This function needs two type declarations:

    ...
    +  public function assertMessages($expected_messages, $id_map) {

6. And this one:

    +++ b/core/modules/user/tests/src/Kernel/Migrate/d7/MigrateUserRoleTest.php
    @@ -32,31 +31,62 @@ protected function setUp(): void {
    ...
    -  protected function assertEntity($id, $label, $original_rid) {
    +  protected function assertEntity($id, $label, $original_rid, $permissions) {

7. I remember working on an issue to add permissions for each custom block type. So I think we need to add a dependency on block-type migrations. If I have enough time and energy before bed, I will add another comment with a more comprehensive list.

@benjifisher, I agree that walks are great for problem solving.

I started reading the comment and stopped at

Why make the node-type migration required for D6 and optional for D7?

Oh my, that is a serious mistake and that needs to be fixed before anything else. And it proves my understanding of 'optional' above was wrong. It was correctly behaving as 'required' and I just plain missed it.

This patch is based on #32 and adds the optional dependency on the node_type migration.

Here are all the permissions callbacks declared in core/modules/*/*.permissions.yml:

1. \Drupal\content_moderation\Permissions::transitionPermissions Load all the workflows defined using the content_moderation module. Get the transitions for each workflow. Create a permission for each transition. Drupal\Workflows\Transition is not an entity, and I find nothing when I search for /transition/i in core/modules/migrations/*.

2. \Drupal\content_translation\ContentTranslationPermissions::contentPermissions

   // Create a translate permission for each enabled entity type and (optionally)
   // bundle.

   I think that means we need a dependency on any migration that creates an entity type or a bundle. I do not think there are any migrations that create entity types, but there are several that create bundles:

   • block_content_type.yml
   • d6_comment_type.yml
   • d7_comment_type.yml
   • d6_language_types.yml
   • d7_language_types.yml
   • d6_node_type.yml
   • d7_node_type.yml
   • d6_taxonomy_vocabulary.yml
   • d7_taxonomy_vocabulary.yml

   I found most of those by searching for "type", but that missed the vocabulary migrations.

   There are 25 config entities in Drupal core modules. (Search for "@ConfigEntityType" in core/modules/*/src/**.) I searched the migration YAML files for entity:(block|block_content_type|...) and found 75 matches. They are all the destination plugin in the migration file.

   I think that list has a lot of false positives. Not every config entity describes the bundles of some content entity type. If I only count the config entities that include the bundle_of = ... annotation, then there are just 7. These are the destination plugins in 11 migrations:

   • block_content_type.yml
   • d6_comment_type.yml
   • d7_comment_type.yml
   • d6_taxonomy_vocabulary_translation.yml
   • d7_taxonomy_vocabulary_translation.yml
   • contact_category.yml
   • d6_node_type.yml
   • d7_node_type.yml
   • d7_shortcut_set.yml
   • d6_taxonomy_vocabulary.yml
   • d7_taxonomy_vocabulary.yml

   That seems like a lot of work to find the 4 that I missed and rule out the two language_types migrations.

3. Drupal\field_ui\FieldUiPermissions::fieldPermissions

4. Drupal\filter\FilterPermissions::permissions

5. \Drupal\layout_builder\LayoutBuilderOverridesPermissions::permissions

6. \Drupal\media\MediaPermissions::mediaTypePermissions

7. \Drupal\node\NodePermissions::nodeTypePermissions

8. Drupal\rest\RestPermissions::permissions

9. Drupal\taxonomy\TaxonomyPermissions::permissions

That is all I have time for tonight.

We discussed this issue briefly during #3204288: [meeting] Migrate Meeting 2021-03-25. Using optional dependencies seems like the right thing to do. I still think we should consider adding the validate_permissions option as in #38. For now, I am going to finish what I started in #40.

3. Drupal\field_ui\FieldUiPermissions::fieldPermissions:

    // Create a permission for each fieldable entity to manage
    // the fields and the display.

   As I said, I do not think there are any migrations that create entity types.

4. Drupal\filter\FilterPermissions::permissions

    // Generate permissions for each text format. Warn the administrator that any
    // of them are potentially unsafe.

   I think we need dependencies on

   • d6_filter_format
   • d7_filter_format

5. \Drupal\layout_builder\LayoutBuilderOverridesPermissions::permissions: Load all entity_view_display config entities that are configured for per-entity LB settings. Add two permissions for each. The permission names include "layout overrides", so I do not think any Drupal 6/7 sites would have such permissions. I think we can skip this one.

6. \Drupal\media\MediaPermissions::mediaTypePermissions:

    // Generate media permissions for all media types.

   There are no core migrations using the destination plugin entity:media_type.

7. \Drupal\node\NodePermissions::nodeTypePermissions: These are the first permissions we thought of.

8. Drupal\rest\RestPermissions::permissions: For each RestResourceConfigInterface configuration entity, create permissions defined by its resource plugin.

   There are no migrations that use entity:rest_resource_config as the destination plugin, so I think we can skip this one.

9. Drupal\taxonomy\TaxonomyPermissions::permissions: For each vocabulary, add CRUD permissions for terms in that vocabulary. I think we already handled this under (2) in the previous comment.

Combining (and sorting) the lists from (2) and (4), I think these are the migrations we need to add as dependencies:

• block_content_type.yml
• contact_category.yml
• d6_comment_type.yml
• d6_filter_format
• d6_node_type.yml
• d6_taxonomy_vocabulary.yml
• d6_taxonomy_vocabulary_translation.yml
• d7_comment_type.yml
• d7_filter_format
• d7_node_type.yml
• d7_shortcut_set.yml
• d7_taxonomy_vocabulary.yml
• d7_taxonomy_vocabulary_translation.yml

Back to NW to add these dependencies.

Added all the optional dependencies identified by benjisher (thanks!), added the validate_permissions option to the destinations updated the tests. Both the d6 and d7 test now have a new test method testUserRoleWithPermission that runs all the optional and required migrations with validate_permissions set to TRUE. I reworked the test quite a bit with the aim of making it easier to see the changes in permissions. I hope I succeeded.

I think this issue is getting close to done! Of course, I still have a few small suggestions.

I need to get some sleep, so I did not yet review the D7 stest.

1. Now that we calculate the list of permissions in the create() method, the permission handler can be local to that method. We do not need to make it a class property.

    +++ b/core/modules/user/src/Plugin/migrate/destination/EntityUserRole.php
    @@ -0,0 +1,66 @@
    ...
    +class EntityUserRole extends EntityConfigBase {
    +
    +  /**
    +   * The permission handler.
    +   *
    +   * @var \Drupal\user\PermissionHandler
    +   */
    +  protected $permissionHandler;

2. I think that $destinationPermissions is a better name for this variable:

    +  /**
    +   * All permissions on the destination site.
    +   *
    +   * @var string[]
    +   */
    +  protected $allPermissions;

3. You discussed enabling the locale module in #15. Let’s try again to figure out why this makes a difference.

4. Let’s be consistent:

    +++ b/core/modules/user/migrations/d6_user_role.yml
    @@ -37,6 +37,14 @@ process:
         - plugin: filter_format_permission
     destination:
       plugin: entity:user_role
    +  validate_permissions: false
   
    +++ b/core/modules/user/migrations/d7_user_role.yml
    @@ -37,4 +37,11 @@ destination:
       plugin: entity:user_role
     migration_dependencies:
       optional:

   Since we might want to make validate_permissions true by default, some time in the future, let’s be consistent by setting it (to false) in both migrations. Besides, this makes it a little more discoverable.

5. Why call the key lookup_up?

    +++ b/core/modules/user/tests/src/Kernel/Migrate/d6/MigrateUserRoleTest.php
    @@ -14,12 +15,62 @@
    ...
    +    foreach ($data as $datum) {
    +      $this->sourcePermissions[$datum->name]['permissions'] = explode(', ', $datum->perm);
    +      $this->sourcePermissions[$datum->name]['lookup_up'] = $datum->rid;
    +    }

6. This comment seems out of place now:

    @@ -51,75 +102,38 @@ protected function assertRole($id, array $permissions, $lookupId, MigrateIdMapIn
    ...
       protected function assertRoles(MigrateIdMapInterface $id_map) {
    -
         // The permissions for each role are found in the two tables in the Drupal 6
         // source database. One is the permission table and the other is the
         // filter_format table.
    -    $permissions = [
    -      // From permission table.
    -      'access content',
    -      'migrate test anonymous permission',
    -      // From filter_format tables.
    -      'use text format filtered_html',
    -    ];

   Maybe just delete the comment. Maybe replace it with one in the setUp() function, explaining that permissions from the filter_format table are not migrated when validate_permissions is set and the filter_format migration has not been run.

7. I think this can be simplified:

    +  public function assertMessages($expected_messages, $id_map) {
    +    $messages = $id_map->getMessages();
    +    $count = 0;
    +    foreach ($messages as $message) {
    +      $src_rid = $message->src_rid;
    +      $this->assertSame($message->message, $expected_messages[$src_rid]);
    +      $this->assertSame(MigrationInterface::MESSAGE_WARNING, (int) $message->level);
    +      $count++;
    +    }
    +    $this->assertSame(count($expected_messages), $count);
    +  }

   Why not loop through the messages, testing the message level as you go but then $actual_messages[$message->src_rid] = $message->message;? Then, after the loop, assert that the two arrays are the same.

8. Same function: we can use string[] instead of array in the @param comment for $expected_messages.

9. Same function: the function signature has no type declarations.

10. This comment seems off point now that we only filter permissions if validate_permissions is set:

     @@ -129,6 +143,46 @@ public function testUserRole() {
     ...
     +  public function testUserRole() {
     ...
     +    // After the d6_filter_format migration, there are changes to the
     +    // permissions.

11. These long lines are hard to read. What do you think of helper functions to merge and diff with $this->expectedPermissions? The helper function could also handle translating role names. Or maybe it can just take source and destination roles as arguments.

     +    $this->expectedPermissions['anonymous']['permissions'] = array_merge($this->sourcePermissions['anonymous user']['permissions'], [
     +      'use text format filtered_html',
     +    ]);

12. Do we need shortcut in the D6 test? My list in #41 includes d7_shortcut_set but not d6_shortcut_set.

     @@ -162,6 +217,126 @@ public function testUserRole() {
     ...
     +  public function testUserRoleWithPermissions() {
     +    // Install modules that have migrations that may cause permissions
     +    // to be created.
     ...
     +      'shortcut',

13. What about d6_node_type?

     +    // Run all the migrations.
     +    $this->installEntitySchema('block_content');
     +    $this->installConfig(['block_content', 'comment']);
     +    $this->migrateContentTypes();
     +    $this->executeMigrations([
     +      'd6_comment_type',
     +      'block_content_type',
     +      'contact_category',
     +      'd6_filter_format',
     +      'd6_taxonomy_vocabulary',
     +      'd6_taxonomy_vocabulary_translation',
     +    ]);

14. Didn’t we deprecate $migration->set()? No, I guess I owe you a review on #2796755: [PP-1] Deprecate Migration::set().

     +    // Create the user role migration with validate_permissions set.
     +    $migration = $this->getMigration('d6_user_role');
     +    $destination = $migration->getDestinationConfiguration();
     +    $destination['validate_permissions'] = TRUE;
     +    $migration->set('destination', $destination);
     +    $this->executeMigration($migration);

@benjifisher, good to know we are getting close. I had intended to comment that I wanted to do a self review but obviously forgot. You picked up quite a few of the things wanted to improve.

1,2,4,8,9 Fixed
5. changed lookup_up to rid.
6. Instead of setup() I put it in the class docs. If I were new to this that is where I would like to find that information.
7. Changes applied to both d6 and d7. And removed $count, there isn't a need to test that anymore.
10. d6_user_role has an existing required dependency on d6_filter_format so this comment is currently true. I guess that should change as well. Adding this to the todo
11. Added a helper to set the expected permissions. I wasn't sure about the formatting just did something I like.
12. copy/paste error. Fixed.
13. d6_node_type is executed in the helper $this->migrateContentTypes();
14. Yes I was thinking this would be changing in the near future.

TODO 3, 10

#43.3 Further to needing 'locale' installed on three tests that previously didn't needed it. Poked around a bit and saw that each of those tests installs config_translation which has a dependency on 'locale'. locale also adds a permission. When getting the permission the permissionHandler is looking for the service 'locale.config_manager' which, of course, does not exist and results in a fatal error. Installing 'locale' fixes that.

There are other Kernel migration tests that install config_translation and not locale and they work fine because they are not doing anything that requires the locale.config_manager.

@quietone:

Nice work! I especially appreciate the helper setDestPermissions(). When I looked at the patch from #42, I did not even realize that the array_merge() and the array_diff() were adding to/removing from the same arrays. Long lines really are hard to understand.

I see you already fixed a lot of minor issues in the D7 test.

And thanks for tracking down the dependency on the locale module. That gives me an idea …

1. Let’s initialize $destinationPermissions = [] and then wrap the middle two lines in if (!empty($configuration['validate_permissions'])). Then we only generate the list of permissions when we need it. As I said before, it is moderately expensive. And we can remove the locale that we added to the existing tests. (I tried one of them.)

    +++ b/core/modules/user/src/Plugin/migrate/destination/EntityUserRole.php
    @@ -0,0 +1,59 @@
    +  public static function create(ContainerInterface $container, array $configuration, $plugin_id, $plugin_definition, MigrationInterface $migration = NULL) {
    +    $instance = parent::create($container, $configuration, $plugin_id, $plugin_definition, $migration);
    +    $permission_handler = $container->get('user.permissions');
    +    $instance->destinationPermissions = array_keys($permission_handler->getPermissions());
    +    return $instance;
    +  }

2. What do you think of renaming this helper function to expectPermissions()?

    +++ b/core/modules/user/tests/src/Kernel/Migrate/d6/MigrateUserRoleTest.php
    @@ -162,6 +232,156 @@ public function testUserRole() {
    ...
    +  public function setDestPermissions($role, array $additions, array $deletions) {

3. Same function: if we add the default value $deletions = [], then we can leave off that argument in 5 of the places it is called.

4. Same function: can we move it to the top of the file, with the other helpers?

5. I think we can move the sort() to the setUp() function. We mostly use assertEntity() with the permission arrays set there. I think there are just two exceptions, and they both start with those permissions and then remove some with array_diff(). There was another recent issue where we discussed this: array_diff() does not change the order.

    +++ b/core/modules/user/tests/src/Kernel/Migrate/d7/MigrateUserRoleTest.php
    @@ -29,34 +47,39 @@ protected function setUp(): void {
    ...
    -  protected function assertEntity($id, $label, $original_rid) {
    +  protected function assertEntity($id, $label, array $expected_permissions) {
         /** @var \Drupal\user\RoleInterface $entity */
         $entity = Role::load($id);
         $this->assertInstanceOf(RoleInterface::class, $entity);
         $this->assertSame($label, $entity->label());
    +    sort($expected_permissions);
    +    $this->assertSame($expected_permissions, $entity->getPermissions());
    +  }

I forgot to set the status to NW. While I am at it, let me see if the issue summary needs any attention ... also the change record.

Another installment...

#46
1. Yes, that should done. Fixed. Removed local from MigrateTermNodeTranslationTest, still needed in the block tests.
2. Name changed.
3. 3rd parameter defaults to [] now and 3rd parameter removed from calls as needed.
4. Yes, moved. Oops, it is there because I prefer helpers at the end of the file.
5. Moved the sort but still need one in UserRoleWithPermissions. Still, it is less sorting overall.

It helps to upload the patch.

I updated the change record, and I copied the first line of my draft as the release-notes snippet here.

I think we want !empty($configuration['validate_permissions']), not empty($instance->destinationPermissions):

    +++ b/core/modules/user/src/Plugin/migrate/destination/EntityUserRole.php
    @@ -0,0 +1,61 @@
    +  public static function create(ContainerInterface $container, array $configuration, $plugin_id, $plugin_definition, MigrationInterface $migration = NULL) {
    +    $instance = parent::create($container, $configuration, $plugin_id, $plugin_definition, $migration);
    +    if (empty($instance->destinationPermissions)) {
    +      $permission_handler = $container->get('user.permissions');
    +      $instance->destinationPermissions = array_keys($permission_handler->getPermissions());
    +    }
    +    return $instance;
    +  }

I have a feeling that the test in #48 is going to fail. If we get this straight, then I do not see why we need locale in the block tests.

Update: I take it back: the tests will not fail, but we will always calculate the permissions, whether we use them or not.

This is what I meant in #46.5: if $this->sourcePermissions[3] is already sorted (in setUp()), then $admin_permissions does not need to be sorted.

    +    // Remove permissions not on the destination site.
    +    $admin_permissions = array_diff($this->sourcePermissions[3], [
    +      'access all views',
    ...
    +      'view revisions',
    +    ]);
    +    sort($admin_permissions);

This will be very brief as I have an appointment.

#51 What a ditz I am. Fixed now and also the Block tests.
#52. The array_diff preserves the keys which means it will not match. The sorting process fixes that.

Edit: s/not/now

Instead of sort use array_values.

@quietone:

Do not be too hard on yourself. I often say that even smart people make dumb mistakes. I also usually blame multitasking.

I am glad we can remove the changes to unrelated tests. Such changes are always reason for skepticism.

Good call on using array_values()!

The patch in #55 fixes the last of my concerns: RTBC.

I just added #3207086: [HEAD BROKEN] Consistent failure in MonthDatePluginTest for the unrelated test failure. Let's see if there is any response on that issue before we ask the testbot to reconsider.

If the testbot just switched to DST, then a lot of tests will fail, and we do not want to queue them all up at once.

#3207086: [HEAD BROKEN] Consistent failure in MonthDatePluginTest was committed. Ordered retest and put this issue back to RTBC per #56.

Now a different unrelated test is failing!

I reported the failure at #3207125: [random test failure] SettingsTrayBlockFormTest:: testEditModeEnableDisable(), and I am queuing another re-test.

+++ b/core/modules/user/migrations/d6_user_role.yml
@@ -37,6 +37,14 @@ process:
+  validate_permissions: false

+++ b/core/modules/user/migrations/d7_user_role.yml
@@ -35,6 +35,14 @@ process:
+  validate_permissions: false

I think this should be true for new migrations. Existing migrations will not have this key and therefore will use the existing behaviour but new migrations should benefit from this change today without having to do additional steps.

Existing migrations will have already copied the template and not have the key so I don't think we need to be concerned about them. If someone is starting fresh and doing repeated core upgrades from D6 or D7 while also upgrading there codebase then they are doing that because they want the latest migration bug fixes of which this is one.

+++ b/core/modules/user/migrations/d6_user_role.yml
@@ -37,6 +37,14 @@ process:
+  optional:
+    - block_content_type
+    - contact_category
+    - d6_comment_type
+    - d6_node_type
+    - d6_taxonomy_vocabulary
+    - d6_taxonomy_vocabulary_translation

+++ b/core/modules/user/migrations/d7_user_role.yml
@@ -35,6 +35,14 @@ process:
   optional:
+    - block_content_type
+    - contact_category
+    - d7_comment_type
     - d7_filter_format
+    - d7_node_type
+    - d7_shortcut_set
+    - d7_taxonomy_vocabulary
+    - d7_taxonomy_vocabulary_translation

This bit kinda concerns me though. We're adding these because they create dynamic permissions and so the role migration needs to depend on that. But it's possible for contrib to have dynamic permissions like this to. How is it going to work for such a module if it provides a migration? For example, what's the impact of this change on commerce migrations? I'm pretty sure that set of modules has some dynamic permissions.

I think in an ideal world permissions would be fixed up after all the migrations have been run and the migration is finished. This would be kinda of what would happen if #2571235: [regression] Roles should depend on objects that are building the granted permissions lands and someone does a migration and then goes to the user permissions screen and presses save. But we don't have the ability to set post migration tasks do we?

@alexpott:

Thanks for taking a look!

Re #62:

I agree with changing the default from false to true, but I do not think we should do that until Drupal 10. This is potentially disruptive: not only may it affect current migrations, but an experienced migrator might start a new project and expect things to work as they always have. We have already made BC breaks in the migration system, like the one fixed by #3184650: ContentEntity migration source adds revision ID as source key, incompatible with Drupal 8.8 and earlier.

Existing migrations will have already copied the template and not have the key …

That applies to projects that use config entities to represent migrations, supported by the migrate_plus module. That is not the only way to run migrations. There is the core UI, the migrate_run module, and Drush 10.4.

If someone is … doing repeated core upgrades … then they are doing that because they want the latest migration bug fixes of which this is one.

When I start a new project, I like to target the version of Drupal that will be current when the project is ready to launch. If I were to start one today, then I would use 9.2.x. There are lots of reasons I might want to update that are unrelated to migrations.

Re #63:

I think we have done everything we can in core to handle the dependencies. A contrib module that provides migrations will probably need to add dependencies.

What can we do to make this easier for the maintainers? We should probably add some code samples to the change record. Anything else?

This seems like another good reason to keep the BC default until Drupal 10.

Luckily, one of the maintainers of the commerce_migrate module is already involved with this issue!

Re #64:

[W]e don’t have the ability to set post migration tasks do we?

It depends on what you mean by "post migration". We have MigrateEvents::POST_IMPORT: see the change record. I do not think that has any advantage over changing the destination plugin, which is what we do here.

If you mean when the migrations are "done done" and the site is ready to go live, then there is no way to tell. Incremental migrations can be done even after the site goes live.

To recap:

1. I agree with changing the default, but I recommend waiting until Drupal 10.
2. We should update the change record with sample code for adding a dependency to the user-role migrations.
3. Rider: also mention in the CR that we log the removed permissions!

If the current patch is committed, then it will be easy to change the default in a later issue. The hard part will be the documentation.

If I still have not convinced you, then I am willing to go along with changing the default now.

Another related issue: #2571235: [regression] Roles should depend on objects that are building the granted permissions.

It depends on what you mean by "post migration". We have MigrateEvents::POST_IMPORT: see the change record. I do not think that has any advantage over changing the destination plugin, which is what we do here.

Yeah that doesn't offer any advantage. We need is something that will run once a complete set of migrations is finished. What I think would work best is if there was some role cleanup migration that always depends on all the existing migrations - this step will remove any permissions which doesn't exist at this point. So I think we leave the existing role migration as it is but somehow introduce another migration whose sole purpose is to fix the permissions and run after all other migrations have finished.

However I don't think we can do this until we've landed #2571235: [regression] Roles should depend on objects that are building the granted permissions - since that'll mean all this step will need to do is to re-save the role without the migrating flag. While is #2571235 is outstanding and we're dealing with D6 and D7 data I think attempts to make this stricter are likely to run into hard to predict edge cases. If you look at user_role_change_permissions() works and \Drupal\user\Form\UserPermissionsForm::submitForm() it's like the system is designed to work with permissions that are not declared to the system.

From #67:

We need is something that will run once a complete set of migrations is finished.

That is what I was thinking of when I said (in #65), "... there is no way to tell. Incremental migrations can be done even after the site goes live."

I just read a description of an upcoming DrupalCon session. It talked about how bad content keeps getting migrated from one template to another, and no one ever cleans it up. The same thing happens with permissions. At least, this issue (in its current form) gives the developer working on the migration the option to break that cycle.

Perhaps we should

1. Demote this issue from Critical to Normal.
2. Update the title and summary.
3. Consider this issue on its own merit.
4. Reopen #3055557: Role migration results in migration permissions that do not exist on the target site with something like #67 as its proposed resolution.

As I suggested in #68, I am demoting this issue from Critical to Normal. I am also cleaning up the issue tags.

I updated the issue summary and the change record, mentioning that we log messages when removing permissions.

@alexpott, it seems to me that what you are asking for is a different, but related, feature. I do not see that as a reason to postpone this issue. We can reopen #3055557: Role migration results in migration permissions that do not exist on the target site or open a new issue to do what you want, postponed on #2571235: [regression] Roles should depend on objects that are building the granted permissions if that is appropriate.

... somehow introduce another migration whose sole purpose is to fix the permissions and run after all other migrations have finished.

I still do not see how that is supposed to work. We can discuss that on another issue, or at one of the weekly migration meetings. I just added a note to #3207879: [meeting] Migrate Meeting 2021-04-22.

... I think attempts to make this stricter are likely to run into hard to predict edge cases.

I am not especially worried about the edge cases. The new option is opt-in, and we log a message for any role that has permissions removed. Let's give developers this option and let them tell us what the edge cases are!

I finally noticed that #67 refers to "the migrating flag." What flag is that? I see that it is added as part of #2571235: [regression] Roles should depend on objects that are building the granted permissions. I think I will leave a comment on that issue.

As stated in #67 I don't think this is rtbc. The solution of adding dependencies to the roles migrations doesn't work because core doesn't yet enforce any sort of dependencies on permissions - they don't even have to exist in permissions.yml or a callback in order to be used! So it's perfectly possible for a role have a permissions and it to be checked in code and for the permission to not be declared. We have tests in HEAD that rely on this behaviour - see #2571235: [regression] Roles should depend on objects that are building the granted permissions. I think we need another approach. I think perhaps the way forward is to use a special configuration for migrate that stores permissions that do not exist at the time of role migration. These permissions are declared as real permissions by the user module under a special migrated permission section. As we go through migrations we have MigrateEvents::POST_IMPORT event that cleans up these permissions as they come to exist. If we combine that approach with #2571235: [regression] Roles should depend on objects that are building the granted permissions think I think we'll get to a place where we can deprecate support for permissions that don't exist and solve the critical bugs that exist because we don't tidy up after ourselves.

Now that I have looked at #2571235: [regression] Roles should depend on objects that are building the granted permissions, I think the right action is to postpone this issue until that one is fixed.

I am doing that and also restoring the Critical status of this issue. I will also make some updates to the issue summary.

Possibly related? #2969282: Error UserPermissionsForm

#2571235: [regression] Roles should depend on objects that are building the granted permissions has landed, so we can return to this issue.

Given the changes in #2571235, we will have to reconsider the proposed resolution, so I am adding the tag for an IS update.

Restarting by making a test only patch from the latest patch, including fixing a typo. No need to run tests right now.

Re #64:

[W]e don’t have the ability to set post migration tasks do we?

It depends on what you mean by "post migration". We have MigrateEvents::POST_IMPORT: see the change record. I do not think that has any advantage over changing the destination plugin, which is what we do here.

If you mean when the migrations are "done done" and the site is ready to go live, then there is no way to tell. Incremental migrations can be done even after the site goes live.

At the core level, we really only need to worry about this for the migrate_drupal_ui module. Such a cleanup can be run at the end of the batch process in that module. Running incremental migrations, or migrations one at time or with drush, or other developer-y things that developers do to migrate a site in the real world end up being out of our control, and there are infinite ways to use the migration system to do bad things if you have developer access.

There are also 'Follow-up Migrations'

I am not sure how to proceed on this.

For the Migrate UI, do we want to only migrate roles that exist on a row by row basis or migrate all the roles and clean the up at the end.

For those migrating with other tools. What support is needed? Will they need a way to remove migrated roles if they are migrates as per #71.

For those migrating with other tools. What support is needed? Will they need a way to remove migrated roles if they are migrates as per #71.

This is an interesting question! I was looking at the core issue that added dependencies to roles (#2571235: [regression] Roles should depend on objects that are building the granted permissions) to see how they handled cleaning up existing roles. They have a post_update function that does all the cleanup:

https://git.drupalcode.org/project/drupal/-/blob/9.4.x/core/modules/user...

To me it seems like from the migrate perspective, we just need to come up with a way to call that function (or a copy of it) after the migration is complete. Sounds like @mikelutz things we can put that at the end of the batch that migrate_drupal_ui uses.

From a contrib standpoint, maybe migrate-tools gets a new drush command to call the update?

I am not convinced of that. If the permission is not migrated in the first place then there is no need for cleanup. That is what the destination plugin does in #55, when validate_permissions is enabled on the destination.

Right, but the issue that @alexpott brought up is that #55 makes the D7_user_role migration dependent on any core migrations that may create dynamic permissions, but it can't do that for contrib modules that might have migrations that create dynamic permissions. So if we go with #55 and user_role runs before products for example, it will strip out per product permissions because they don't yet exists. (This is an off the top of my head example, I don't recall the permission structure of commerce, but the idea is the same in general.) Since we can't make D7_user_role depend on any random contrib migrations that would have to run first to create dynamic permissions, the only choice we have is to migrate all the permissions and then clean up when the whole upgrade process is done.

@mikelutz, thanks. that all makes sense.

Here is the start of a new patch, working to implement #67, where the permissions that do not exist at the time of the user_role migration are created as temporary permissions. When a migration is done via /upgrade these temporary permissions are removed and the roles updated when the migration is complete. The roles are updated using an exact copy of user_post_update_update_roles(). When a migration is run by other means the temporary permissions are not removed but since they are stored in config they can easily be accessed and removed.

So, is this now moving in the right direction?

No interdiff because of the new approach.

oops, I wasn't working from HEAD

Instead of adding more changes, this moves the abstract method declaration. However, it is inconsistent with the way the getEntityCounts() is defined but this should allow the tests to pass. It can be changed later to be consistent if that is what we want.

Needed a reroll and moving to D10

And update the new Upgrade tests in aggregator.

This is an interesting approach. I was not thinking that the Migrate module would actually define (or provide) permissions, but I guess that is what we have to do.

I think we should expand the "Proposed resolution" section in the issue summary. Be more explicit about how we add permissions and how we remove them, and how the user roles get updated. I can do that, but not at this hour. I see this issue already has the tag for an issue summary update. +1

1. It is a problem if these temporary permissions are never removed. That should not happen with migrate_drupal_ui, but it could happen if an error interrupts the process before the cleanup stage. At least, I think that could happen. And it is certainly a possibility for custom migrations. So let's implement hook_requirements() and add a warning to the site status report if any temporary permissions are defined in the new config object.
2. In #82 and #85, we discussed how to update the user roles. I did not comment here, but I think I may have endorsed this approach in Slack, possible during one of the weekly meetings. I am changing my mind.

   There is a simpler way. Make all of the temporary permissions depend on the new config object. See Permissions can define dependencies, one of the change records for #2571235: [regression] Roles should depend on objects that are building the granted permissions. We do not need the new trait mentioned there. Just add the dependencies in nonExistentPermissions(). Then, whenever the config object is updated, the config dependency system will take care of updating the roles. (I am pretty sure that is true. We need to confirm!)

   This also makes it really easy for a contrib or custom module to clean up the permissions: just delete the temporary permissions.

3. A natural extension of the first two points is that we should add a settings form in the admin UI, listing the temporary permissions and allowing the site admin to delete them. There is no need to provide a complete CRUD UI: just a button to remove them all. Then the warning on the status report can be actionable: provide a link to the new page.

   Actually, the permissions are already listed on /admin/people/permissions/module/migrate. Would it make sense to add a link there to a confirmation form?

4. If I am right about the config dependency system, then we do not need updateRoles(). We might still want it, in order to provide logging. If so, and if I am right about dependencies, then we have to call the function before updating the config. But if this method is really an exact copy of user_post_update_update_roles(), can we just call that function instead? Maybe not: I guess that update functions are eventually removed.
5. When we migrate a non-existent permission to a role, we add the temporary permission to the config item and we add the permission to the role. The role should get a dependency on the migrate module automatically. If you follow my earlier suggestion, then the role should also depend on the config item. If the migrate module is uninstalled, or if the permission is removed from the config object, then the role should be updated (removing the permission).

   I think all of that is pretty good DX.

   I would like to see test coverage of all of that. If we test removing a role from the config object, then maybe we do not also have to test uninstalling the module.

6. +  /**
   +   * Gets roles with non-existent permissions removed.
   +   *
   +   * @return string
   +   *   A list of roles that have had permissions removed.
   +   */
   +  protected function getRoles() {
   +    return '';
   +  }
   +
   +  /**
   +   * Gets roles with non-existent permissions removed post incremental.
   +   *
   +   * @return string
   +   *   A list of roles that have had permissions removed.
   +   */
   +  protected function getRolesIncremental() {
   +    return '';
   +  }
   

   Nit: Can we add "comma separated" to the @return comments?

7. Same request (nit) for some of the helper functions in the tests.

8. -  protected function assertUpgrade(array $entity_counts) {
   +  protected function assertUpgrade(array $entity_counts, $roles) {
        $session = $this->assertSession();
        $session->pageTextContains(t('Congratulations, you upgraded Drupal!'));
   +    if ($roles) {
   +      $regex = '/The role[s]? ' . $roles . ' have|has had non-existent permissions removed/';
   +      $session->pageTextMatches($regex);
   +    }
   

   The brackets are optional in [s]?. If you think they make the regexp easier to read, then it is worth keeping them. I think we need to group the alternation: (have|has). I am nervous about inserting a variable into a regular expression. I guess it is OK, since it is a comma-separated list of machine names. Can we just apply preg_quote() to be safe?

9. Is it worth adding a description to each permission? We would have to store it in the config item and retrieve it in the permissions callback. I was thinking that it might be useful to know which migration(s) added the permission, but maybe we do not have access to that in the destination plugin.

Naming things is always hard. I will not insist on any of these changes. They are just suggestions.

1. Use "temporary permissions" instead of "non-existent permissions": in the config schema, Permissions method name, doc blocks and other code comments, variable names, and log and Migrate messages.
2. Maybe use getTemporaryRoles() in the tests instead of getRoles().
3. Instead of the migrate.permission config item with the key permissions, I would use migrate.settings with the key temporary_permissions.
4. In #40 on this issue, I listed the modules that have permissions callbacks. The classes are not consistently named, but they usually follow the pattern ModuleNamePermissions. Maybe we should use MigratePermissions.
5. I think that removeTemporaryPermissions() is clearer than updateRoles().

#92.3: I guess the simplest thing to do is add links to the warning message (on the site status report) to the Migrate permissions page and to a confirmation form. Other than the confirmation form, we do not need a new page.

In the new destination plugin, it is a little confusing to use the same variable for a string and then for an array:

+      $non_existent_permissions = print_r($diff, TRUE);
+      $message = "Permission(s) '$non_existent_permissions' not found.";
+      $this->migration->getIdMap()
+        ->saveMessage($row->getSourceIdValues(), $message, MigrationInterface::MESSAGE_WARNING);
+
+      // Save the non-existent permissions in config so they can be added as
+      // permissions by the migrate module.
+      // @see \Drupal\migrate\Permissions
+      $config = $this->configFactory->getEditable('migrate.permission');
+      $non_existent_permissions = $config->get('permissions') ?? [];
+      $config->set('permissions', array_values(array_unique(array_merge($diff, $non_existent_permissions))));

The first two times I looked at this, I thought we were passing a string as the second argument of array_merge().

Maybe just eliminate the first variable:

$message = 'Permission(s) ' . print_r($diff, TRUE) . ' not found.';

or something similar with sprintf().

@benjifisher, thanks for the review and the renaming suggestions. I recall not liking the names at the time but I was focused on the functionality.

It is late and I only worked on the simpler points and the renaming.

#92. 6, 7 and 8 Fixed. Also all the renaming points in the final paragraph.

TODO. #92 1 -> 5 and 9. # 93

@quietone, thanks for keeping this moving!

By the way, having a patch to review is a huge help. In principle, I could have made those suggestions before you did any work, but having a patch to review gives me a lot more focus.

After sleeping on it, it occurs to me that we are not thinking about the iterative nature of migration development. We work on the migrations and the site building in parallel. For this issue, that means we have to consider what happens when we migrate some user roles, then enable one or two modules that define "missing" or "temporary" permissions, then repeat the process.

What happens when two modules define the same permission? I do not know. The permission machine names are the array keys of $permission_handler->getPermissions(), so that function will return only one of them.

I was thinking that the destination plugin should recalculate the temporary permissions from scratch, instead of just adding to them. But I was wrong: the destination plugin is looking at just one role at a time. It could check the existing temporary permissions, and remove the ones that are defined by another module. But maybe it does not have to: see the next point.

I think what we have to do is react whenever a module is enabled. There is a hook for that, isn't there? Or maybe an event. Check the permissions it defines, including its permissions callback. Because of the array-key issue I mentioned before, we cannot rely on $permission_handler->getPermissions(). If any of those permissions are defined by the Migrate module, then remove them.

it occurs to me that we are not thinking about the iterative nature of migration development.

That it true. mikelutz provides an answer in #78 which is that we only have to handle the case of /upgrade. And, right now, I that is correct for the scope of this issue. Exploring implications with iterative nature of migration can be in a separate issue? However, I just realized that there is nothing in the current patch that checks what happens to the temporary permissions when the user role migration is rolled back. Added that to the todo list at the end of this comment.

What happens when two modules define the same permission?

Ah, I stumbled on this while working on this issue. I reckon this needs an issue, if there isn't one already.

This patch adds information to status report page (#92.1), which will need work. It should at least link to somewhere where the user can learn how to remove the permissions. Also, it removes the check for skipping the missing permission deprecation in Entity/Role.php that was accidentally left in the previous patch.

TODO. #92 1 -> 5 and 9. # 93, test rollback

#92. 2 and 9. The destination plugin EntityUserRole now adds a title, description and dependency on 'migrate' to the stored permissions. This required a few tweaks in the tests and an expanded schema file. The description can and does include the migration id as can be seen in this screenshot of /admin/people/permissions/module/migrate. Since it is very likely that there is only one migration of user roles, it doesn't seem very useful to me. Viewing the permissions is not yet available with this patch. I did it by running a Drupal 6 upgrade with some code commented out so the permissions were not removed.

I did uninstall migrate and only some of the permissions were removed but I need to retest that.

When I uninstalled migrate I got the confirmation screen about what config was to be updated and when I uninstalled all the migrate permissions were removed. So, that is good. I also noted that on the module page that migrate had a permission link when there were permissions.

But I found another thing to look into, when there are no migrate permission navigating to /admin/people/permissions/module/migrate results in a redirect loop.

And thinking about a form to allow deleting the temporary permissions, right now I think that would be better in a followup.

Did some work on the IS so removing tag.

I forgot to update the status page test due to the changes to the config schema.

... when there are no migrate permission navigating to /admin/people/permissions/module/migrate results in a redirect loop.

What is supposed to happen is a 403 response, and the link should not appear on /admin/modules.

I tested this, and it seems to work as designed. I applied the patch from #101 and installed the Standard profile. The "Migrate permissions page" means /admin/people/permissions/module/migrate.

1. Log in as admin.
2. Visit the Migrate permissions page. Access denied (403 response).
3. Use drush config:import --partial --source=config to import the YAML file below, saved as config/migrate.settings.yml.
4. Reload the Migrate permissions page. It shows the permission as expected.
5. Use drush config:edit migrate.settings to set temporary_permissions: [].
6. Reload the Migrate permissions page. Access denied (403 response).

I was afraid that the access result was cached incorrectly, so I did not clear caches during this test.

Can you give steps to reproduce the redirect loop?

Here is my YAML file:

temporary_permissions:
  - title: Test permission
    description: Pretend that this permission was migrated from somewhere.
    dependencies:
      config:
        - migrate.settings

I did a quick review of the changes inside the migrate module. It looks good so far! I just have a few comments and really minor suggestions.

1. I guess that StringTranslationTrait provides both t() and formatPlural(), but procedural code only has the first. Oh, well.
2. Do we really need an inline template for the requirements? Can't we just use an existing TranslatableMarkup object as the replacement for a token in t()? Or, as I suggested in #93, we do not have to work so hard here. Since the permissions are already listed on the Migrate permissions page, we can give a link to that instead of generating the list here.
3. The permissions callback does not need an editable config object.
4. I was thinking of adding dependencies in the permissions callback, but adding them to the config object should work, too. It might even be handy to let developers modify the permissions with config import/export.
5. If you eliminate the variable, then the permissions callback becomes a one-liner.

I looked at the changes inside the user module. Again, just a few minor points.

1. The destination plugin extends EntityConfigBase, which extends Entity (abstract destination plugin). I am surprised that this base class has $storage and $bundles properties, and these have to be supplied by the constructor. I would expect those to be in EntityContentBase. Can config entity types have bundles?
2. Initialize $temporary_permissions = []; at the top of the function, outside the if() block, so that it is always defined when you get to the last block of the function.
3. In the d6 test, it looks as though $provided_by_migrate is defined and then never used. Am I missing something?

Edit: to answer my own question, yes, config entities can have bundles. So I guess it does make sense for the Entity class to have those properties.

I looked at the remaining changes: in the aggregator and migrate_drupal_ui modules. I think there are a couple of things that should be changed.

1. I know this is a direct copy of the post-update function, but I think we let a goof slip through:

    +++ b/core/modules/migrate_drupal_ui/src/Batch/MigrateUpgradeImportBatch.php
    @@ -367,4 +379,40 @@ public static function onIdMapMessage(MigrateIdMapMessageEvent $event) {
    +    \Drupal::classResolver(ConfigEntityUpdater::class)
    +      ->update($sandbox, 'user_role', function (Role $role) use ($existing_permissions, &$cleaned_roles) {
    +        $removed_permissions = array_diff($role->getPermissions(), $existing_permissions);
    +        if (!empty($removed_permissions)) {
    +          $cleaned_roles[] = $role->label();
    +          \Drupal::logger('update')->notice(
    +            'The role %role has had the following temporary permission(s) removed: %permissions.',
    +            [
    +              '%role' => $role->label(),
    +              '%permissions' => implode(', ', $removed_permissions),
    +            ]
    +          );
    +        }
    +        $permissions = array_intersect($role->getPermissions(), $existing_permissions);
    +        $role->set('permissions', $permissions);
    +        return TRUE;
    +      });

   The last three lines should be inside the if block. If there are no removed permissions, then there is no need to update the role.

2. You missed one doc block that can use "comma separated":

    +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/MigrateUpgradeExecuteTestBase.php
    @@ -22,6 +22,26 @@ protected function setUp(): void {
    ...
    +  /**
    +   * Gets roles with temporary permissions removed.
    +   *
    +   * @return string
    +   *   A list of roles that have had permissions removed.
    +   */
    +  protected function getTemporaryRoles() {

3. If the method returns '', then there is no need to
   override the base implementation.

    +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/d6/Upgrade6Test.php
    @@ -190,6 +190,20 @@ protected function getMissingPaths() {
    ...
    +  /**
    +   * {@inheritdoc}
    +   */
    +  protected function getTemporaryRolesIncremental() {
    +    return '';
    +  }
    +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/d7/Upgrade7Test.php
    @@ -217,6 +217,20 @@ protected function getMissingPaths() {
    ...
    +  /**
    +   * {@inheritdoc}
    +   */
    +  protected function getTemporaryRoles() {
    +    return '';
    +  }
    +
    +  /**
    +   * {@inheritdoc}
    +   */
    +  protected function getTemporaryRolesIncremental() {
    +    return '';
    +  }

There are still a couple of to-dos from #92, right? If you want to leave the cleanup form to a follow-up issue, then I think I can implement it.

Addressed point 1 and 3 of comment #105, still needs work for others.

@ravi.shankar:

Thanks for helping with those points. Are you planning to continue working on this issue?

I went through all the remaining points to determine what is still to do. I wanted to make a summary but I don't have the brain power for that now. I should be able to have another look at this in the next two days.

Updated patch with some fixes.

#92
1. Fixed, hook_requirements is in migrate.install
2. Fixed in #98
3. I agree that a UI is needed for deleting the temporary permissions. Creating such things is a weak point for me but adding a button to the migrate permission page seems wrong and inconsistent. I reckon a new form is needed.
4. Todo
5. Todo
9. Fixed in #98
93. Removed $temporay_permission variable that was used for the building of the log message.

#102. Todo Provide steps to reproduce the redirect loop

#103
1. Oh well indeed.
2. Todo
3. Fixed, no longer using editable config.
4. I did add the dependencies in the callback at first. But that split up writing to the config in two places and it seems
cleaner to do it in one place, the destination plugin.
5. Fixed permission callback is one liner.

#104
1. It does seem a bit odd, the bundle is the same name as the config entity.
2. No change needed due to #105.1.
3. Ah, that is dead code. The real test is at the end of the file, after the comment "Confirm that temporary permissions are stored.".

#105
1. Fixed in #106
2. Fixed.
3. Fixed in #106

todo: test rollback

Looks like test failures are unrelated.

@yogeshmpawar, thanks for confirming the failure was a random fail. If you restarted the tests, I think we can save resources and not rerun the tests on every failure. There is lots more work to be done here anyway.

This does #103.2, removes the inline template in hook_requirements. The message on the status report page could use improvement and now looks like this:

List of remaining items
#92. 4, 5
102. Provide steps to reproduce the redirect loop
test rollback
UI to remove temporary permission

Another installment.

test rollback - The destination simply removes all the temporary permission.
92. 5 - Changes StatusTest to PermissionTest and, at the end, uninstalled migrate and confirmed the temporary permissions are gone.

List of remaining items
#92. 4
102. Provide steps to reproduce the redirect loop
UI to remove temporary permission

Another random failure: Drupal\Tests\media_library\FunctionalJavascript\CKEditorIntegrationTest::testButton

And now for the remaining issues.
#92-4. MigrateUpgradeImportBatch::removeTemporaryPermissions. Yes, this is definitely needed for logging. I also think that this should
stay so that each role has the temporaryPermissions removed at the end of the upgrade. I fixed the method so it is now comparing with the temporary permission and removing said permission after the roles are updated.

102. Provide steps to reproduce the redirect loop. I am not able to reproduce this so perhaps just a wonky test site. This is not for this issue but when there are no permissions I get an access denied message instead of a some nice message that there are no permissions.

And the last remaining task is to provide a UI to remove the temporary permissions. I still think that is outside the scope of this issue. And, if the tests are correct, we know that the temporary permissions will be removed when migrate is uninstalled.

Hopefully, all the points have had a response and we are all caught up.

And ready for the next round!

@quietone:

Yay, progress!

This is not for this issue but when there are no permissions I get an access denied message instead of a some nice message that there are no permissions.

That is by design.

The access checker means that we do not generate links to empty permissions forms. For example, /admin/modules already checks access for the Configure links; as of Drupal 9.3, it does the same for the Permissions links.

If you can think of a better way, let's talk about it ... on another issue. (As you said, "... not for this issue".)

I am not able to reproduce this so perhaps just a wonky test site.

If that were a separate issue, then we would close it as "cannot reproduce".

And the last remaining task is to provide a UI to remove the temporary permissions. I still think that is outside the scope of this issue.

I am willing to implement that in a follow-up issue.

I will do a full review in a couple of days. I already looked at the interdiff in #108, and that much looked reasonable.

@benjifisher, thanks. Yes, progress indeed, I am beginning to think the end is in sight!

Thanks for the explanation about the access checker. I am content to live with it as is. And ehile I do enjoy closing issues, after experience with Bug Smash I won't be making a new issue. ;-)

Adding tag for the followup.

Now for this patch. The test failed because temporary_permissions was expected to be an array and it was NULL. So, this add a config/install so that the temporary_permissions are initialized to an empty array. And I got a bit defensive and ensured that $temporary_permissions is never NULL by using '??'. This patch also adds some comments.

What is to do is to improve the requirements message. I have updated the remaining tasks.

I added #3272352: Add a UI for deleting temporary permissions as a child issue, and assigned it to myself.

I think we should update the tile of this issue.

Oh, no! I must have forgotten to force-reload this page. I did not intend to make all those changes in the issue summary.

I am doing my best to restore the IS.