Problem/Motivation

When a user role gets deleted, the statical cache of user_roles() is not invalidated accordingly. This breaks code using that function.

The attached test-only patch proves this bug. Also, we've found that the static cache is never hit, so it's useless.

Proposed resolution

Remove this static cache because is never used.

Remaining tasks

None.

User interface changes

None.

API changes

None.

Data model changes

None.

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Bug because it breaks code that is trying to use user_roles() after deleting a role.
Issue priority Major because it returns a false list of role and that triggers fatal errors when trying to use unexistent roles.
Disruption No disruption.
CommentFileSizeAuthor
#18 static-cache-not-hit.patch2.54 KBclaudiu.cristea
#18 failure_test.patch1.21 KBclaudiu.cristea
#18 2572667-18.patch1.01 KBclaudiu.cristea
#17 2572667-17.patch2.21 KBclaudiu.cristea
#17 interdiff.txt2.04 KBclaudiu.cristea
#16 cache_hit.patch2.19 KBclaudiu.cristea
#9 interdiff.txt1.13 KBclaudiu.cristea
#9 2572667-9.patch2.24 KBclaudiu.cristea
#6 interdiff.txt1.36 KBclaudiu.cristea
#6 2572667-6.patch2.24 KBclaudiu.cristea
#2 2572667.patch2.13 KBclaudiu.cristea
test_only.patch1.21 KBclaudiu.cristea

Comments

claudiu.cristea created an issue. See original summary.

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented
StatusFileSize
new 2572667.patch2.13 KB

Here's tha patch.

The last submitted patch, test_only.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 2: 2572667.patch, failed testing.

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented
Status: Needs work » Needs review

The failure is expected on the test-only patch. NR for #2.

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented
StatusFileSize
new 2572667-6.patch2.24 KB
new interdiff.txt1.36 KB

The entire cid of the static cache must be cleared.

Status: Needs review » Needs work

The last submitted patch, 6: 2572667-6.patch, failed testing.

The last submitted patch, 2: 2572667.patch, failed testing.

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented
StatusFileSize
new 2572667-9.patch2.24 KB
new interdiff.txt1.13 KB

Course, the callback of array_filter() should return TRUE for items that remain.

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented
Status: Needs work » Needs review

The last submitted patch, 6: 2572667-6.patch, failed testing.

The last submitted patch, test_only.patch, failed testing.

googletorp’s picture

googletorp commented
Status: Needs review » Reviewed & tested by the community

This looks good. We have a well defined issue, a failing test that is fixed by the patch.

I found a small issue which can fixed on commit, it should "either ... or" instead of "either ... either".

+++ b/core/modules/user/src/RoleStorage.php
@@ -30,4 +30,26 @@ public function isPermissionInRoles($permission, array $rids) {
+      // The cache cid can be either RoleInterface::ANONYMOUS_ID, either
+      // RoleInterface::AUTHENTICATED_ID.

Small nit, should be

// The cache cid can be either RoleInterface::ANONYMOUS_ID or
// RoleInterface::AUTHENTICATED_ID.

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Needs review
Issue tags: +needs profiling

Could we confirm the static cache actually gets hit on a standard profile page request and if so what that is? Not sure what's relying on it at this point.

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented
Related issues: +#2569741: [PP-1] FilterFormat should add the roles as config dependencies

@catch, I discovered this bug trying to fix #2569741: [PP-1] FilterFormat should add the roles as config dependencies. That issue is not finished and that uses user_roles(). I'm getting unexpected test failures and those because of this bug. You suggest that we can get rid of that static cache because it seems is not used?

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented
StatusFileSize
new cache_hit.patch2.19 KB

Here's a temporary patch proving that the static cache receives no hit.

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented
StatusFileSize
new interdiff.txt2.04 KB
new 2572667-17.patch2.21 KB

No more static cache. Based on #16 test.

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented
Issue summary: View changes
Issue tags: -needs profiling
StatusFileSize
new 2572667-18.patch1.01 KB
new failure_test.patch1.21 KB
new static-cache-not-hit.patch2.54 KB

Changing the issue summary to reflect the fact that we are removing an unused static cache. Adding beta evaluation.

Patches:

  • 2572667-18.patch: [should pass] patch to be committed.
  • failure_test.patch: [should fail] the test revealing the bug. This test will not be committed because the test will lose its scope. It's posted here only to prove the bug.
  • static-cache-not-hit.patch: [should pass] this is a test ran with the standard profile that proves the static cache is never hit, at least for the pages provided in the test.

The last submitted patch, 18: failure_test.patch, failed testing.

The last submitted patch, 18: failure_test.patch, failed testing.

googletorp’s picture

googletorp commented
Status: Needs review » Needs work
Issue tags: +Needs issue summary update

The patch looks good, but we need to update this issue summery as we are now doing a completely different thing.

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented
Issue summary: View changes
Issue tags: -Needs issue summary update

@googletorp, thank you. In fact I updated the issue summary in #18. Or, you mean it's not clear enough?

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented
Status: Needs work » Needs review

Setting back to NR

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented
Title: Deleting a role doesn't invalidate user_role() static cache » Deleting a role doesn't invalidate user_roles() static cache

Title fixing.

sam152’s picture

sam152 commented
Status: Needs review » Reviewed & tested by the community

The test that proves the static cache isn't required is really cool. If none of those core pages make use of the static cache, I doubt there is much case to keep it there for 3rd party code that might make use of it, especially when it introduces a bug as demonstrated.

I think this is a clear RTBC given the amount of validation the patches provide.

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.0.x, thanks!

  • catch committed e54454a on 8.0.x 
    Issue #2572667 by claudiu.cristea: Deleting a role doesn't invalidate...

Status: Fixed » Closed (fixed)

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