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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Gábor Hojtsy’s picture

Issue tags: +D8MI, +language-ui
dileepmaurya’s picture

Assigned: Unassigned » dileepmaurya
dileepmaurya’s picture

remove array_keys() and replace 0 with 'anonymous' in core/modules/locale/tests/src/Unit/LocaleLookupTest.php because 0 is not more valid.

dileepmaurya’s picture

Issue tags: +SprintWeekend2015

remove array_keys() and replace 0 with 'anonymous' in core/modules/locale/tests/src/Unit/LocaleLookupTest.php because 0 is not more valid.

RavindraSingh’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests

Great 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)

Gábor Hojtsy’s picture

Is 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?

Gábor Hojtsy’s picture

Issue tags: +sprint
joshi.rohit100’s picture

As per default cache backend, cids are having 256 length, so I think length should not be a problem.

Gábor Hojtsy’s picture

User 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 :/

Berdir’s picture

Cache 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

Gábor Hojtsy’s picture

+++ b/core/modules/locale/src/LocaleLookup.php
@@ -117,7 +117,7 @@ protected function getCid() {
+      $rids = $user ? implode(':', $user->getRoles()) : '0';

Ok 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.

deepakaryan1988’s picture

Issue tags: -SprintWeekend2015

Removing sprint weekend tag!!
As suggested by @YesCT

Gábor Hojtsy’s picture

Issue tags: -sprint +SprintWeekend2015

Nobody is working on this unfortunately, so removing from sprint.

borisson_’s picture

Status: Needs work » Needs review
FileSize
884 bytes
1.83 KB

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.

Gábor Hojtsy’s picture

Assigned: dileepmaurya » Unassigned
Issue tags: +sprint

Making unassigned then, and cautiously back on sprint :)

andypost’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs tests

#11 fixed, and there's tests already

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

A 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!

  • alexpott committed 5168706 on 8.0.x
    Issue #2501183 by borisson_, dileepmaurya, Gábor Hojtsy, Berdir:...
Gábor Hojtsy’s picture

Issue tags: -sprint

Superb, thanks all!

Status: Fixed » Closed (fixed)

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