Problem/Motivation
LocaleLookup::getCid() assumes that getRoles() returns an array keyed by role ID's. That's no longer the case.
Proposed resolution
Remove the array_keys(). Possibly write a test, but it's a bit tricky.
Remaining tasks
User interface changes
API changes
Comment | File | Size | Author |
---|---|---|---|
#14 | localelookup_cache_id-2501183-14.patch | 1.83 KB | borisson_ |
#14 | interdiff.txt | 884 bytes | borisson_ |
#3 | localelookup-cache-id-is-using-numeric-indexes-of-the-roles-field-not-role-id-2501183-3.patch | 1.88 KB | dileepmaurya |
Comments
Comment #1
Gábor HojtsyComment #2
dileepmaurya CreditAttribution: dileepmaurya commentedComment #3
dileepmaurya CreditAttribution: dileepmaurya commentedremove array_keys() and replace 0 with 'anonymous' in core/modules/locale/tests/src/Unit/LocaleLookupTest.php because 0 is not more valid.
Comment #4
dileepmaurya CreditAttribution: dileepmaurya commentedremove array_keys() and replace 0 with 'anonymous' in core/modules/locale/tests/src/Unit/LocaleLookupTest.php because 0 is not more valid.
Comment #5
RavindraSingh CreditAttribution: RavindraSingh as a volunteer and at Srijan | A Material+ Company commentedGreat Work Deelip, This would be good if you can add tests also.
One more thing about to use 'anonymous' at the place of '0' (We do have a role id for this role. I am not sure if it is in d8 database)
Comment #6
Gábor HojtsyIs there a maxlength on cids? A user who has 5 or more roles especially if the role machine names are long would make this run pretty long. Should that be considered?
Comment #7
Gábor HojtsyComment #8
joshi.rohit100As per default cache backend, cids are having 256 length, so I think length should not be a problem.
Comment #9
Gábor HojtsyUser roles in core are
authenticated
(length: 13),administrator
(length: 13),anonymous
(length: 9), so average length is about 11. Adding the colon separator, it would be 12 per role.The CID structure is
locale:{$this->langcode}:{$this->context}:$rids
. Some examples of longer (not necessarily longest) contexts (from https://localize.drupal.org/translate/languages/ca/translate), "a drupal commerce payment transaction" (length: 38), "Abbreviated 1 letter weekday Wednesday" (length: 39), "credit card issue number for card types that require it" (length: 56), so let's say plan with 50. So 7 (for locale:), 10 (for language codes like gsw-berne) then 50 adds up to 67 used out of 256, so less than 190 for roles. 12 or more per role would allow for 10-16 roles granted per person. What happens if the user has more roles or the roles are longer named? Eg. localize.drupal.org has longer role names like "translation community manager" (length: 30) and "translation community moderator" (length: 32). Of those, only about 6-7 roles are possible in this cache key then :/Comment #10
BerdirCache ID's are not a problem, because the API has a built-in mechanism that shortens them to 256 characters if they are longer, automatically. So don't worry about that.
The lock system unfortunately doesn't do that, but I've already opened a separate issue for this: #2501191: CacheCollector can result in an exception is cid + classname is too long to be used as a lock name
Comment #11
Gábor HojtsyOk then I guess the only thing left other than tests is that an empty string should be used if lack of roles. Although I am not sure when do we have a user without any roles whatsoever, but there may be transient cases I guess.
Comment #12
deepakaryan1988Removing sprint weekend tag!!
As suggested by @YesCT
Comment #13
Gábor HojtsyNobody is working on this unfortunately, so removing from sprint.
Comment #14
borisson_Attached patch rolls the file mode changes back (files should be 664, not 775).
This also returns an empty string instead of '0' when no roles are found.
A test is still missing, so that should be added still.
Comment #15
Gábor HojtsyMaking unassigned then, and cautiously back on sprint :)
Comment #16
andypost#11 fixed, and there's tests already
Comment #17
alexpottA user can not not have any roles - it's not possible to not have the authenticated role if you're not the anonymous user. See User::getRoles().
Committed 5168706 and pushed to 8.0.x. Thanks!
Comment #19
Gábor HojtsySuperb, thanks all!