Problem/Motivation

#2571235: [regression] Roles should depend on objects that are building the granted permissions was just fixed, and should be included in Drupal 9.3.0.

When saving a user role, Drupal now issues a deprecation message if there are any invalid permissions (i.e., permissions not defined by an enabled module). In Drupal 10, we will throw an exception instead of creating a deprecation message.

Steps to reproduce

Proposed resolution

We should check for invalid permissions used.

We could reuse the 9.3.x code, but that is not available before Drupal 9.3 and we need to display results in the runtime environment summary where the best is to be brief and concise, so its best for us to format the message ourselves. (Also that leads to no side effects).

Remaining tasks

User interface changes

Environment check notes if there are invalid permissions or none.

API changes

None

Data model changes

None

CommentFileSizeAuthor
#9 3223068.patch3.23 KBGábor Hojtsy
#8 3223068.patch3.21 KBGábor Hojtsy
#7 3223068.patch3.2 KBGábor Hojtsy
#5 3223068.patch3.18 KBGábor Hojtsy
#4 3223068.patch3.01 KBGábor Hojtsy
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

benjifisher created an issue. See original summary.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy at Acquia commented

Hm, should we run the core user role save and try to catch exceptions from there or just do a cross-check ourselves?

This seems to be enough to get a full list of all permissions available (assuming $container):

$permissions = $container->get('user.permissions')->getPermissions();

So we could use this to check against the list of permissions in roles. I fear there may be other side effects if we try to save roles. Eg. some sites may run a rules kind of rule or call out to third party systems, etc.

benjifisher’s picture

benjifisher
he/him or they/them
Primary language English
Location Boston area
CreditAttribution: benjifisher at FRUITION commented

The deprecation notices are added in Drupal\user\Entity\Role::calculateDependencies() (current 9.3.x). That is called from ConfigEntityBase::preSave(). But it is a public method, so you can just call calculateDependencies() directly. Since that is designed to be called by preSave(), it should not do any saving itself.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy at Acquia commented
Status: Active » Needs review
FileSize
3223068.patch3.01 KB

Thanks! That one triggering errors is also a side effect that I think we want to avoid.

I think this would look something like the attached.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy at Acquia commented
FileSize
3223068.patch3.18 KB

Add role object to test.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy at Acquia commented
Title: Check for deprecation messages from invalid permissions » Check for invalid permissions, will throw runtime exceptions in Drupal 10
Issue summary: View changes

Updated the issue summary. Note this bit especially.

We could reuse the 9.3.x code, but that is not available before Drupal 9.3 and we need to display results in the runtime environment summary where the best is to be brief and concise, so its best for us to format the message ourselves. (Also that leads to no side effects).

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy at Acquia commented
FileSize
3223068.patch3.2 KB

Wrong variable name :D

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy at Acquia commented
FileSize
3223068.patch3.21 KB

Aaand the role name was wrong.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy at Acquia commented
FileSize
3223068.patch3.23 KB

It is HTML encoded. Let's use $this->assertSession()->pageTextContains() which will decode it.

Testbot driven development.

  • 08f7324 committed on 8.x-3.x 
    Issue #3223068 by Gábor Hojtsy, benjifisher: Check for invalid...
Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy at Acquia commented
Status: Needs review » Fixed

Yay!! Fixed a code comment on commit too.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.