Port roleassign to D8

Just a question from an interested bystander who was comparing modules...

It seems to me that the D7 role_delegation module

• does more than roleassign
• does not add a lot of complexity (which would make a case for roleassign to exist)

...in other words, I don't want to dissuade you if you want to - but I'm wondering if it's necessary to port this module?

@tkuldeep17: Please excuse me for taking so long to reply.

I've created a D8 branch and a snapshot release for you and I'm very happy to see you take this on.

@roderik: The front page gives my perspective on the Role Delegation module. I think both modules have their place.

Well that was silly, I somehow failed to read the front page carefully...

1. It seems that hook_user_operations() is gone:
https://www.drupal.org/node/2084727
#1851086-94: Replace admin/people with a View
Don't worry about this for now.

2. Please make a best guess/proposal and post a patch here, so that we can start gathering feedback.

@tkuldeep17: Are you still working on this or should I remove your assignment?

Ok, thank you.

Hi,

I've been adding some improvements to the sandbox of tkuldeep17, to get a working version of the module for D8.
Most notably I solved the issue 1 described by tkuldeep17, to filter out unassigned roles from the actions dropdown on the users admin View.

Could I get maintainer access to this module, to merge in the changes of tkuldeep17 as well as my own, so we can create a first (dev) D8 release of this module?
Alternatively I could also work in my own sandbox, but I think it would be better to work in here, to gather feedback through the issue queue etc...
Let me know what you think.

I'll be happy to have you on board and I've added you to the maintainer list.

As a long-time contributor you surely know the Best practices for co-maintaining projects, so please post a patch in the D8 branch.

How do you want to ago about attributing the porting work done by tkuldeep17?
I prefer using the --author git commit parameter, but that will be a bit difficult if I post one huge patch here with both of our changes.
Let me know what you prefer.

In any case, here is the patch file with the D8 port by tkuldeep17 and improved by me.
If the functionality gets tested and is found to be OK, we can create separate followup issues for further cleanup and improvements, where necessary.

How do you want to ago about attributing the porting work done by tkuldeep17?

I asked him to post a patch here in #6, but he didn't, so unfortunately I have nothing to commit from him and the --author will go to you, unless you specifically request otherwise. I'll put both of your names into the comment, of course.

(A sandbox is a branch, not a contribution, and it's not the maintainer's job to reconcile branches.)

Here's some initial feedback:

1. +++ b/roleassign.install
   @@ -10,5 +10,6 @@
    function roleassign_uninstall() {
   -  variable_del('roleassign_roles');
   +  // Clear book data out of the cache.
   +  \Drupal::cache('data')->deleteAll();
    }
   

   This looks like it doesn't belong here.

2. +++ b/roleassign.module
   @@ -6,142 +6,144 @@
   +    $sticky_roles_str = '';
   

   Unnecessary assignment.

3. +++ b/roleassign.module
   @@ -6,142 +6,144 @@
   +      $assigned_roles = array_combine($assigned_roles, $assigned_roles);
   

   What are we doing here?

4. +++ b/roleassign.module
   @@ -6,142 +6,144 @@
   +    // Make a string of all sticky roles.
   +    $sticky_roles[RoleInterface::AUTHENTICATED_ID] = $roles[RoleInterface::AUTHENTICATED_ID];
   +
   +    // Store sticky roles used in updation of user.
   +    $form['sticky_roles'] = array('#type' => 'value', '#value' => $sticky_roles);
   +    $sticky_roles_str = implode(', ', $sticky_roles);
   

   The comments got mixed up.

5. +++ b/roleassign.module
   @@ -6,142 +6,144 @@
   +  $sticky_roles = $form_state->getValue('sticky_roles');
   

   I think this opens up the form for tampering...

6. +++ b/src/Plugin/views/field/RoleAssignUserBulkForm.php
   @@ -0,0 +1,51 @@
   +    // @TODO: refactor following 2 if statements - also in roleassign_form_alter().
   

   Yes, definitely. Duplicating code is evil.

This is surely not final, but I'd like to get an updated patch and hope that the testbot will kick in.

Updated patch taking your comments into account, except for 5.
This patch also does extra code cleanup, fixes some things in the code of tkuldeep17 that were unused, and replaces the extra check on the module uninstall form with a custom UninstallValidator service.

Still TODO:
* As per your comment 5, there is too much dependency on the Form API. In D7 the roles are stored in hook_user_presave(). Since this is no longer possible in D8, tkuldeep17 replaced this with a custom submit form handler, which is suboptimal at best. We should find a way to do this properly, that also works if the user gets created programmatically or through a REST web service.
Still thinking about what the logic should be, since I'm a bit confused with the sticky roles.
Basically as follows:
--> upon saving an account, only assign this account roles that are assignable by the current user or that were already assigned in the original state of the account.
That should be doable in hook_user_presave(), and avoids using the sticky roles or get the assignable roles from the form (get them from config instead), but I'd like for someone else to think this through as well, before I implement.
* Add tests

Perhaps add a validation Constraint to the User entity that validates the roles added to the user entity.
That would add a sanity check when the user gets assigned a role through the Form API, as well as programmatically.
But I'm not sure if it's good practice to add Constraints that are dependent on the context (permissions) of the user trying to manipulate the entity that is being validated.

Attached is an updated version of the Drupal 8 port. I think this is pretty much feature complete.

What has changed since the last patch:
* Logic to (un)assign roles is no longer added through a form submit handler, but taken care of in hook_user_presave(). This makes sure the validation logic is no longer tied to the form. Thus if a user account is created programmatically (e.g. through a REST API) by an administrative user with permissions "assign roles" but not "administer permissions" (i.e. the use case for RoleAssign), permissions that are allowed to be assigned by this administrative user, don't get assigned.
* Added automated tests for the role (un)assigning scenario's, both through the UI and programmatically.
* Added Migrate API templates so settings get updated from Drupal 6 or Drupal 7 if a migration is performed from any of those older Drupal versions.

Thanks for reviewing this patch, so this could get committed into the 8.x branch.

Moved the Migrate config files to the optional folder, where they should have been in the first place.
This no longer makes the testbot angry.

Updated patch with new location for Migration files.
As per a conversation with Mike Ryan, they should be placed in migration_templates for general-purpose D6/D7 upgrade migration, just like core.

Thanks, looks like a great step forward for a D8 release!

I did a functional test for the following features, all of them passed:

- Select roles that are allowed to be assigned
- Give the assign role permission to a role
- Only a user with that role can assign the selected roles
- Export/ import the role assign configuration

There are some very minor remarks that could be made regarding docblocks but for the sake of releasing a function D8 versioning I'm RTBC'ing this.

Thank you for the updated patch, svendecabooter, and for testing, klaasvw!

1. +++ b/CHANGELOG.txt
   @@ -1,5 +1,5 @@
   -CHANGELOG for RoleAssign for Drupal 7
   +CHANGELOG for RoleAssign for Drupal
   

   This is for Drupal 8 only. No records for D7 or D9 will ever be added to this file.

   However, I'll want to edit CHANGELOG.txt anyway, so never mind.

I would have liked to commit the patch, but unfortunately, I cannot apply the roleassign.module part (using msysgit).

Here's my output:

$ git apply -v port_roleassign_to_d8-2405341-26.patch
Checking patch CHANGELOG.txt...
Checking patch config/schema/roleassign.schema.yml...
Checking patch migration_templates/d6_roleassign_settings.yml...
Checking patch migration_templates/d7_roleassign_settings.yml...
Checking patch roleassign.admin.inc...
Checking patch roleassign.info...
Checking patch roleassign.info.yml...
Checking patch roleassign.install...
warning: roleassign.install has type 100644, expected 100755
Checking patch roleassign.links.menu.yml...
Checking patch roleassign.module...
warning: roleassign.module has type 100644, expected 100755
error: while searching for:
 * roles.
 */

/**
 * Implements hook_permission().
 *
=============8<==========================================
function _roleassign_module_load_include($type) {
  static $loaded = array();

  if (!isset($loaded[$type])) {
    $loaded[$type] = (bool) module_load_include($type, 'roleassign');
  }
  return $loade
error: patch failed: roleassign.module:6
error: roleassign.module: patch does not apply
Checking patch roleassign.permissions.yml...
Checking patch roleassign.routing.yml...
Checking patch roleassign.services.yml...
Checking patch roleassign.views.inc...
Checking patch src/Form/RoleAssignAdminForm.php...
Checking patch src/Plugin/views/field/RoleAssignUserBulkForm.php...
Checking patch src/ProxyClass/RoleAssignUninstallValidator.php...
Checking patch src/RoleAssignUninstallValidator.php...
Checking patch src/Tests/RoleAssignPermissionTest.php...

This is pretty weird, because the testbot apparently was able to apply it, and I did a fresh clone from d.o...

Any ideas?

There is one clearly defined problem though: please set the file mode to 644 for all files as is standard for Drupal.

File permissions changed... Dunno where those changes came from.
Tested with a fresh checkout of 8.x-1.x branch, and could apply the patch without problems.
It got applied on commit 08212ded5454dc4ec0708323b2e3fbf7d0bcc8f0 FYI.

Can you verify you where in the 8.x-1.x branch? Can't think of another reason why the patch wouldn't apply...

Setting status back since the last changes (file permissions) were trivial

Yes, I'm on

08212de - (HEAD, origin/HEAD, origin/8.x-1.x, 8.x-1.x) #2405341: Branch to port to D8.

too.

The new patch behaves the same as the old one. It applies to all files, except for the roleassign.module file.

If it were a file with minor changes, I'd apply it by hand, but this one is a bear. Please post your patched roleassign.module file.

Attached my patched version of roleassign.module.
Hope this helps.

Yes, this is helping.

We have two kinds of weirdnesses in your #32 patch:

All files in my msysgit bash show as 644, yet in http://cgit.drupalcode.org/roleassign/tree/ three of them show as 755. Your mode changes listed below give me warnings, but they wouldn't cause a failure.

However, your file is missing a 'd' in front of 'elete'. Apparently the testbot's patcher completely ignores that line, but msysgit doesn't. Why it complains about roleassign.module rather than roleassign.install is beyond me, but adding the missing 'd' lets the patch go through with two warnings:

warning: README.txt has type 100644, expected 100755
warning: roleassign.module has type 100644, expected 100755

1. diff --git a/README.txt b/README.txt
   old mode 100755
   new mode 100644
   

   No mode change needed, already is 100644 (on Windows/msysgit/bash, whatever that means).

2. diff --git a/roleassign.install b/roleassign.install
   eleted file mode 100644
   

   Your file is broken here.

3. diff --git a/roleassign.module b/roleassign.module
   old mode 100755
   new mode 100644
   

   No mode change needed, already is 100644 (on Windows/msysgit/bash, whatever that means).

Great job! Thank you very much, tkuldeep17 and svendecabooter!

Let's move on to #2622542: RoleAssign for D8 — ALPHA2.

Thanks for committing the patch!
Let's hope followup patches show a bit less Git weirdness ;)