Support from Acquia helps fund testing for Drupal Acquia logo

Comments

victor-shelepen’s picture

I've done small modifications.

victor-shelepen’s picture

victor-shelepen’s picture

Status: Active » Needs review

I was playing with DIC I see that it is a model hidden deep in objects. It is a model. I do not know why we need user roles here.
If we delete this part of the key we will kill our headache.

// for anonymous users.
-    $rids = isset($GLOBALS['user']) ? implode(':', array_keys($GLOBALS['user']->getRoles())) : '0';
+    $user = \Drupal::request()->attributes->get('_account');
+    $rids = $user ? implode(':', array_keys($user->getRoles())) : '0';
 // @todo: If the global user is an EntityBCDecorator, getting the roles
-    // from it within LocaleLookup results in a loop that invokes LocaleLookup
-    // again.
-    global $user;
-    $user = drupal_anonymous_user();
-

It had been removed because the user initialized few lines beneath.

$user = $this->drupalCreateUser(array('translate interface', 'access administration pages'));
victor-shelepen’s picture

Fixed. It works by manual testing. I see the language module also has problems with tests.

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +D8MI, +sprint, +language-base

This looks very straightforward and all are in global functions, so global invocation of Drupal::* is appropriate.

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Needs review

Wait, what! #4 is not a patch for this issue. Haha! Ok so looking at #2, is $user not used inbetween the re-initialization? Can you re-upload #2 which is the relevant patch.

Gábor Hojtsy’s picture

Issue tags: -D8MI-meta
victor-shelepen’s picture

Sorry. This #2, #4 are my mistakes.

victor-shelepen’s picture

Status: Needs review » Needs work

The last submitted patch, language-global_user_var-2061929-1.patch, failed testing.

jair’s picture

Issue tags: +Needs reroll
pfrenssen’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
1.79 KB

#9 is for the Language module, not for the Locale module, see Gábor's comment at #6.

Here's a straight reroll of the patch in #2.

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Looks good as per above discussion.

catch’s picture

Status: Reviewed & tested by the community » Postponed
Gábor Hojtsy’s picture

Status: Postponed » Needs work
sergeypavlenko’s picture

Assigned: victor-shelepen » Unassigned
Status: Needs work » Needs review
FileSize
1.76 KB

Status: Needs review » Needs work

The last submitted patch, drupal8.locale-module.2061923-16.patch, failed testing.

m1r1k’s picture

sergeypavlenko’s picture

Status: Needs work » Needs review
FileSize
2.17 KB

New patch.

sergeypavlenko’s picture

Title: Remove calls to deprecated global $user » Remove calls to deprecated global $user in locale module
andypost’s picture

Status: Needs review » Postponed
  1. +++ b/core/core.services.yml
    @@ -613,7 +613,6 @@ services:
    -    scope: request
    

    let's postpone on it

  2. +++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleUninstallTest.php
    @@ -63,12 +63,6 @@ function testUninstallProcess() {
    -    // @todo: If the global user is an EntityBCDecorator, getting the roles
    

    outdated

m1r1k’s picture

Issue tags: +blocked-by-request-scope

Add tag for easy tracking

joelpittet’s picture

Status: Postponed » Needs review
Issue tags: -sprint, -likin, -blocked-by-request-scope
FileSize
1.78 KB

Re-rolled without the scope: request bit from #19

joelpittet’s picture

Issue tags: +blocked-by-request-scope

Didn't mean to remove the blocked-by-request-scope tag

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

As per above.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.x. Thanks!

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