Provide a mechanism to deprecate permissions

1. +++ b/core/modules/user/src/Form/UserPermissionsForm.php
   @@ -123,7 +123,7 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   -    $permissions = $this->permissionHandler->getPermissions();
   +    $permissions = $this->permissionHandler->getUndeprecatedPermissions();
   

   Couldn't this cause security issues by people not seeing that a certain role has a certain permission. Maybe we should show it, but not make it possible to grant a role the permission?

2. +++ b/core/modules/user/src/PermissionHandler.php
   @@ -110,6 +110,18 @@ public function getPermissions() {
   +      if (!empty($permission['deprecated'])) {
   +        return FALSE;
   +      }
   +      return TRUE;
   

   🙃 These 4 lines could be collapsed to return empty($permission['deprecated'])

Based on #1977498: Add version tracking for configuration schema and data, permissions can't be renamed, even with an upgrade path right? Any exported role that contained such a permission in a module or elsewhere would be invalid.

I wonder if a follow-up for this could be a "replaced_by" key on deprecated permissions, so contrib modules which check the legacy permission could have the access check replaced by a lookup on the new permission automagically.

Currently the runtime permission system is fully decoupled from hook_permissions, for example because of performance reasons. Maybe one thing we could do to keep this constraint alive is to update permissions automatically on role save.

Patch looks good to me -- nice idea! Sending back to NW for tests, though :)

1. +++ b/core/modules/user/src/Form/UserPermissionsForm.php
   @@ -124,13 +124,29 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   +          if (in_array($permission_name, $role_permission)) {
   
   @@ -183,10 +201,18 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   +          if (in_array($perm, $role_permissions[$rid])) {
   

   let's use strict=TRUE , as otherwise we run into https://3v4l.org/7qgqe potentially.

2. +++ b/core/modules/user/src/Form/UserPermissionsForm.php
   @@ -183,10 +201,18 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   +            $form['permissions'][$perm][$rid]['#default_value'] = 0;
   +            if (!empty($permissions[$perm]['deprecated'])) {
   +              $form['permissions'][$perm][$rid]['#disabled'] = TRUE;
   +            }
   

   Nice!

3. +++ b/core/modules/user/tests/modules/user_permission_form_test/user_permission_form_test.permissions.yml
   @@ -0,0 +1,15 @@
   +  title: 'A normal permission'
   ...
   +  title: 'Deprecated permission 1'
   +  deprecated: true
   +deprecated permission 2:
   +  title: 'Deprecated permission 2'
   +  deprecated: true
   
   +++ b/core/modules/user/tests/src/Functional/PermissionFormTest.php
   @@ -0,0 +1,162 @@
   +    $this->grantPermissions($role1, ['deprecated permission 1']);
   

   I'm wondering whether it makes sense to name the permissions in a way that as a reader you already know which permission is used an which isn't. So 'deprecated used permission' and 'deprecated unused permission'.

4. +++ b/core/modules/user/tests/src/Functional/PermissionFormTest.php
   @@ -0,0 +1,162 @@
   +  protected static function rowSelector($name) {
   

   Nice helper methods ...

I really agree with the approach:

• Hide the permission if nothing is using it right now
• Provide a warning otherwise. I would have personally added another drupal_set_message(, 'warning') to make people aware,
  even if they just save the long form. What do you think about this?

Docblocks had a bit of copypasta.

For me its all about good method names, because this is what people actually most likely see.

We could instead add a status report message or such so it's in the context of reviewing their site's status rather than updating something else.

Now that you think about it, the existing test deprecation messages are sort of the same order than status messages. Given that sending the messages not always make sense.
+1 for adding a status report message as well?

Looks pretty great to me. I found nothing major that needs work here -- mostly just suggestions, nitpicks, etc.

1. +++ b/core/modules/user/src/Form/UserPermissionsForm.php
   @@ -124,13 +124,29 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   +        foreach ($role_permissions as $role_name => $role_permission) {
   

   $role_permission is a confusing variable name. It sounds like a singular thing, but it's an array of permissions in the role. Can it be renamed to $permissions_in_role or something like that?

2. +++ b/core/modules/user/src/Form/UserPermissionsForm.php
   @@ -124,13 +124,29 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   +    if ($this->moduleHandler->moduleExists('node') && !empty($permissions['access content'])) {
   

   I don't quite understand how this change is in scope...?

3. +++ b/core/modules/user/src/Form/UserPermissionsForm.php
   @@ -162,10 +178,11 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   +          'deprecated_warning' => !empty($perm_item['deprecated']) ? $this->t('This permission should no longer be used.') : '',
   

   It might be prudent to only add this key if the permission is deprecated, so that it can properly pass isset() checks (for example, in hook_form_alter implementations).

4. +++ b/core/modules/user/src/Form/UserPermissionsForm.php
   @@ -162,10 +178,11 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   +          '#template' => '<div class="permission"><span class="title">{{ title }}</span>{% if description or warning or deprecated_warning %}<div class="description">{% if warning %}<em class="permission-warning">{{ warning }}</em> {% endif %}{% if deprecated_warning %}<em class="permission-warning">{{ deprecated_warning }} </em> {% endif %}{{ description }}</div>{% endif %}</div>',
   

   Superdupernit: There is an extra space after {{ deprecated_warning }}, before the </em>. Not sure if that is intentional?

5. +++ b/core/modules/user/src/Form/UserPermissionsForm.php
   @@ -174,6 +191,7 @@ public function buildForm(array $form, FormStateInterface $form_state) {
            if (!$hide_descriptions) {
              $form['permissions'][$perm]['description']['#context']['description'] = $perm_item['description'];
              $form['permissions'][$perm]['description']['#context']['warning'] = $perm_item['warning'];
   +          $form['permissions'][$perm]['description']['#context']['deprecated_warning'] = $perm_item['deprecated_warning'];
   

   Because the deprecated_warning key is hidden behind the $hide_descriptions check, this will hide the deprecation warning if descriptions are turned off. Seems like we might want to show the deprecation warning unconditionally instead...

6. +++ b/core/modules/user/src/Form/UserPermissionsForm.php
   @@ -183,10 +201,20 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   +          if (in_array($perm, $role_permissions[$rid])) {
   

   This should be strict-checked (TRUE as the third and final argument).

7. +++ b/core/modules/user/src/Form/UserPermissionsForm.php
   @@ -183,10 +201,20 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   +            $form['permissions'][$perm][$rid]['#default_value'] = 0;
   +            // If the role is not already assigned the deprecated permission,
   +            // don't allow it to be added.
   +            if (!empty($permissions[$perm]['deprecated'])) {
   +              $form['permissions'][$perm][$rid]['#disabled'] = TRUE;
   +            }
   

   If the checkbox has or can have a title attribute, maybe we should add a little text to that attribute to explain why the checkbox is disabled ("This permission cannot be used because it is deprecated" or something).

8. +++ b/core/modules/user/src/PermissionHandlerInterface.php
   @@ -35,6 +35,11 @@
   +   *     and informing the developer. Be sure to also provide an ugprade path
   

   s/ugprade/upgrade :)

9. +++ b/core/modules/user/tests/src/Functional/PermissionFormTest.php
   @@ -0,0 +1,165 @@
   +  protected function rowElement($name) {
   +    $element = $this->cssSelect(static::rowSelector($name));
   +    return !empty($element[0]) ? $element[0] : NULL;
   +  }
   

   Not a big deal, but I would suggest that this return the entire NodeElement associated with the row, which can allow future assertions to look for descriptions/checkboxes/etc. in the context of the element, rather than the entire page. (For example, $row_element->find('css', 'div.description')). If you do this with assertSession(), you can also implicitly assert that the row, in fact, exists. So this method could look like:

   return $this->assertSession()->elementExists('css', static::rowSelector($name))

1 is not in scope; that variable already exists and is used in HEAD.

That's true of $role_permissions (plural), but not $role_permission, which is introduced in the foreach loop in the patch. So we can totally give it a better name :)

For 5, I thought we should respect the UI pattern that is already used. Edit: Warnings about the restrict access permission are hidden in the same way and I think those are more important than this one. So I'd leave that out of the scope as well.

That makes sense. Should we maybe open a follow-up to unconditionally display security/deprecation notices on permissions?

I'm not sure about 7; misuse of the title attribute is an accessibility issue. Edit: The other disabled checkboxes on the page for admin permissions don't have such a title so I don't think we should add it.

Makes sense.

I'm confused by 9; it already does return the node element for the row?

Yes, it does, but it requires that the calling code assert that the return value is non-empty. Whereas assertSession()->elementExists() will do that for you implicitly, and return the element itself for further use. So we're doing extra unnecessary work in the current patch.

completed 1 and 9 from #36.

Making the change to rowElement($name) had a tickle down effect to descriptionElement($name). This required three changes to the assertions as AssertEmpty() expected a null response.

Finally, I also updated checkboxElement($permission, $role) to also have an assertion rather than returning a null.

1. +++ b/core/modules/user/src/Form/UserPermissionsForm.php
   @@ -162,10 +178,11 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   -          '#template' => '<div class="permission"><span class="title">{{ title }}</span>{% if description or warning %}<div class="description">{% if warning %}<em class="permission-warning">{{ warning }}</em> {% endif %}{{ description }}</div>{% endif %}</div>',
   +          '#template' => '<div class="permission"><span class="title">{{ title }}</span>{% if description or warning or deprecated_warning %}<div class="description">{% if warning %}<em class="permission-warning">{{ warning }}</em> {% endif %}{% if deprecated_warning %}<em class="permission-warning">{{ deprecated_warning }} </em> {% endif %}{{ description }}</div>{% endif %}</div>',
   

   I think this is now too complex for an inline template, and we need a custom theme hook.

2. +++ b/core/modules/user/src/PermissionHandlerInterface.php
   @@ -35,6 +35,11 @@
   +   *   - deprecated: (optional) Whether the permission is deprecated. The
   +   *     implementation is responsible for retaining backwards compatibility
   +   *     and informing the developer. Be sure to also provide an ugprade path
   +   *     revoking the deprecated permission and (if applicable) granting the
   +   *     new permission.
   

   I don't think the implementation should be responsible for informing the developer, I think we should add code to \Drupal\user\RoleStorage::isPermissionInRoles for that and have it fire a trigger_error

3. +++ b/core/modules/user/tests/modules/user_permission_form_test/user_permission_form_test.permissions.yml
   @@ -0,0 +1,15 @@
   +  deprecated: true
   ...
   +  deprecated: true
   ...
   +  deprecated: true
   

   As mentioned on slack, I don't think these should be a boolean.

   I think they should follow our normal deprecation format that way we can explain to people what to use instead.

   In addition I think 'This permission should no longer be used' isn't very helpful - it doesn't tell people what to use instead or when they need to remove the usages by.

   So by making this a string instead of a boolean, we could have it say This permission was deprecated in Drupal 8.3.0 and will be removed before Drupal 9.0.0. Use 'Administer something' instead. See http://drupal.org/node/the-change-notice-nid.

   We can then use that same message in \Drupal\user\RoleStorage::isPermissionInRoles when we trigger_error for deprecated permissions. So that developers using the deprecated permission get feedback via tests that they're using a deprecated permission and know what to use instead.

We can then use that same message in \Drupal\user\RoleStorage::isPermissionInRoles when we trigger_error for deprecated permissions. So that developers using the deprecated permission get feedback via tests that they're using a deprecated permission and know what to use instead.

This would require us to keep a cache of deprecated permissions for performance sake, as we don't want to be collecting permissions at runtime.

Related #2339487: Static cache permissions

This issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request as a guide.

Yes this would be great feature!

At this time we will need a D10 version of the patch and there were changes requested in #44 that should be addressed

Please do not just reroll.

I've tried to move the changes from the latest patch into a MR.

Things I could do:

• add new key deprecated to permissions.yml that allows setting a deprecation message for a permission
• add new theme hook for the permission information on admin/people/permissions (maybe user_permission_info isn't the best choice but I couldn't come up with a better name for now)
• add test for UserPermissionsForm

Things I couldn't do yet:

• find a place to trigger an error if a deprecated permission is used. RoleStorage::isPermissionInRoles() is likely to be deprecated because it isn't used anymore

Setting to "Needs review" for some thoughts about the current progess.

Did a basic review.

On deprecations, I think you want to do a deprecation when a role with a deprecated permission is saved. We already have checks for non-existing permissions in \Drupal\user\Entity\Role::calculateDependencies, so the info is already available.

This will not cover code that uses it in hasPermission(), but we do not cache permissions atm, and checking it at runtime (access check time) would have way too much overhead. Doing it on role save matches the handling of non-existing permissions and deprecations are mostly for test, any test that tests a certain permission will ultimately need to set it on a role for it to do anything.