backdrop / backdrop-issues

Issue tracker for Backdrop core.
144 stars 40 forks source link

[DX][D8] Provide a mechanism to deprecate permissions #5030

Open klonos opened 3 years ago

klonos commented 3 years ago

Issues like #3787 and #4487 were the triggers for this issue here.

Our Drupal brethren started implementing this and were originally planning to add it in Drupal 8.5.0:

Problem/Motivation

Wherever possible, Drupal 8 code should deprecate instead of removing old code, behaviors, etc.

Permission machine names are stored in user role configurations, but they are also used in code for many things like access checks. This means that if core updates the machine name of a permission, even if it provides an upgrade path, contributed module code that checks against the core permissions might start to deny access that should be allowed.

Marked major because it affects how thoroughly we can implement our backwards compatibility policy.

Proposed resolution

Provide a mechanism to deprecate permissions in the permission definition.

Remaining tasks

  • Needs review.
  • Followup to explore how to raise warnings when a deprecated permission is used.

User interface changes

If no deprecated permissions are used, there is no UI change: unused deprecated permissions are simply not presented to the user.

If a deprecated permission is in use, the user will see a short message under the permission. The permission can be revoked from roles that have it through the UI, but not granted to additional roles (unchecked checkboxes for it will be disabled).

image

A message will also be provided on the status report.

image

API changes

A deprecated key is added to the permission definition API.

klonos commented 3 years ago

Turns out that this is very simple to implement after all, with relatively minimal changes:

The hook_requirements() thing is fine to implement on top of the current 1.x code, but the deprecation logic and the display of the warning in the Permissions page needs to happen in user_admin_permissions() and there is duplication of that code all over the place that we'd better clean up first. See: #5031.

klonos commented 3 years ago

...another thing that we could implement for additional DX++ is logging a watchdog message each time a deprecated permission is checked via user_access().

klonos commented 1 year ago

Waiting on #5031 before I resume work on this - in the meantime, I did push the code from my local (which includes the changes from the PR for #5031), so that we can get a sandbox going, and see if there are any obvious things to fix (coding standards, spelling/typos etc.).