Follow-up from #2163203: Remove calls to deprecated global $user wherever possible (outside bootstrap and authentication).

This is how I'm finding them all, FWIW and used the remaining items from #2163203-62: Remove calls to deprecated global $user wherever possible (outside bootstrap and authentication)
grep -R -e "GLOBALS\['user'\]" -e "global .*\$user" *

Excluding bootstrap and session (thought I may throw a patch in to see what happens).
I'll create a number of patches to see how testbot reacts to the changes then consolidate the green ones into a larger patch.

From #2163203-5: Remove calls to deprecated global $user wherever possible (outside bootstrap and authentication).

Let's expand the scope of this issue to convert all the trivial references outside bootstrap and session handling, and then work on those tougher issues separately.

Before RTBCing a patch for this issue, confirm that no more references to the global remain outside #2061953: Remove calls to deprecated global $user in core/lib/Drupal/Core/Authentication/AuthenticationManager.php , #2062771: #2062771: Remove calls to deprecated global $user in \Drupal\Core\Session\SessionManager, #2062069: Remove calls to deprecated global $user in core/includes/bootstrap.inc . Edit: Fixing TestBase and its child base classes might also merit a separate issue, but uses in normal test classes themselves should be (edit) either in this patch or in that patch, depending on which makes more sense. But not separately.

CommentFileSizeAuthor
#54 interdiff.txt1.57 KBdawehner
#54 remove_global_user-2213899-54.patch2.42 KBdawehner
#48 interdiff.txt3.35 KBjoelpittet
#48 remove_global_user-2213899-48.patch2.43 KBjoelpittet
#46 2213899-global-user-round-2-46.patch3.2 KBjoelpittet
#25 2213899-25-global-user-round-2.patch2.69 KBjoelpittet
#25 2213899-25-global-user-round-2-and-some-from-1.patch8.9 KBjoelpittet
#24 2213899-24-global-user-round-2.patch2.65 KBjoelpittet
#24 2213899-24-global-user-round-2-and-some-from-1.patch8.59 KBjoelpittet
#21 2213899-21-DrupalDateTimeTest.patch1.98 KBjoelpittet
#20 2213899-global-user-round-2-20.patch3.67 KBjoelpittet
#20 interdiff.txt992 bytesjoelpittet
#18 2213899-global-user-round-2-18.patch4.51 KBjoelpittet
#1 2213899-1-user.patch3.06 KBjoelpittet
#1 2213899-1-user.pages_.patch525 bytesjoelpittet
#1 2213899-1-session.patch4.84 KBjoelpittet
#1 2213899-1-HistoryTimestampTest.patch642 bytesjoelpittet
#1 2213899-1-generate-d7-content.patch853 bytesjoelpittet
#1 2213899-1-FormBuilder.patch521 bytesjoelpittet
#1 2213899-1-FormatDateTest.patch1.94 KBjoelpittet
#1 2213899-1-EntityAccessTest.patch1.65 KBjoelpittet
#1 2213899-1-EntityAccessController.patch588 bytesjoelpittet
#1 2213899-1-DrupalDateTimeTest.patch1.78 KBjoelpittet
#1 2213899-1-Cron.patch1.02 KBjoelpittet
#1 2213899-1-Cookie.patch783 bytesjoelpittet
#1 2213899-1-bootstrap.patch1.68 KBjoelpittet
#1 2213899-1-BasicAuth.patch931 bytesjoelpittet
#1 2213899-1-AuthenticationManager.patch1 KBjoelpittet
#1 2213899-1-AuthenticationEnhancer.patch814 bytesjoelpittet
#1 2213899-1-ArgumentDefaultTest.patch1.12 KBjoelpittet
#1 2213899-1-all.patch23.61 KBjoelpittet
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

joelpittet’s picture

Ok here's the combined green. Let's see if they play nice together.

This leaves the following remaining:

core/includes/bootstrap.inc: x3
core/includes/session.inc: x7
core/lib/Drupal/Core/Authentication/AuthenticationManager.php: x3
core/lib/Drupal/Core/Authentication/Provider/Cookie.php:  x1
core/lib/Drupal/Core/Entity/EntityAccessController.php:  x1
core/lib/Drupal/Core/Form/FormBuilder.php:        x1
core/lib/Drupal/Core/Routing/Enhancer/AuthenticationEnhancer.php:  x3
core/modules/system/lib/Drupal/system/Tests/Common/FormatDateTest.php:   x2
core/modules/system/lib/Drupal/system/Tests/Datetime/DrupalDateTimeTest.php:   x2
core/modules/system/lib/Drupal/system/Tests/Entity/EntityAccessTest.php:    x3
core/modules/user/user.module: x7
core/modules/user/user.pages.inc: x1
tstoeckler’s picture

Status: Needs review » Needs work
+++ b/core/scripts/generate-d7-content.sh
@@ -257,11 +257,11 @@
-  $original_user = $GLOBALS['user'];
+  $original_user = \Drupal::currentUser();
...
-    $GLOBALS['user'] = drupal_anonymous_user();// We should have already allowed anon to vote.
+    \Drupal::getContainer()->set('current_user', drupal_anonymous_user()); // We should have already allowed anon to vote.

As this script is supposed to run on a D7 site we should leave those calls alone.

Otherwise looks great!

joelpittet’s picture

Status: Needs work » Needs review
FileSize
992 bytes
3.67 KB

Thanks for the review @tstoeckler, this removes the d7 change, I knew there was a reason that didn't show up in the previous round.

Not a whole lot now but we be widdl'n.

joelpittet’s picture

A better version of 2213899-1-DrupalDateTimeTest.patch but it will still fail because it needs the drupal_get_user_timezone() to be changed but that causes a circular reference.

joelpittet’s picture

1: 2213899-1-all.patch queued for re-testing.

joelpittet’s picture

Re-roll of #20

Also, grabbed a few from #1 that still applied to see where they are at.

joelpittet’s picture

The last submitted patch, 25: 2213899-25-global-user-round-2-and-some-from-1.patch, failed testing.

joelpittet’s picture

1: 2213899-1-user.patch queued for re-testing.

joelpittet’s picture

1: 2213899-1-user.pages_.patch queued for re-testing.

joelpittet’s picture

1: 2213899-1-FormBuilder.patch queued for re-testing.

joelpittet’s picture

1: 2213899-1-FormatDateTest.patch queued for re-testing.

joelpittet’s picture

joelpittet’s picture

joelpittet’s picture

joelpittet’s picture

1: 2213899-1-Cookie.patch queued for re-testing.

joelpittet’s picture

joelpittet’s picture

The last submitted patch, 1: 2213899-1-user.patch, failed testing.

The last submitted patch, 1: 2213899-1-AuthenticationEnhancer.patch, failed testing.

The last submitted patch, 1: 2213899-1-Cookie.patch, failed testing.

The last submitted patch, 1: 2213899-1-DrupalDateTimeTest.patch, failed testing.

The last submitted patch, 1: 2213899-1-EntityAccessController.patch, failed testing.

The last submitted patch, 1: 2213899-1-EntityAccessTest.patch, failed testing.

The last submitted patch, 1: 2213899-1-FormatDateTest.patch, failed testing.

The last submitted patch, 1: 2213899-1-AuthenticationManager.patch, failed testing.

The last submitted patch, 1: 2213899-1-user.pages_.patch, failed testing.

joelpittet’s picture

Adding formbuilder patch from #1 because it passes now.

dawehner’s picture

Status: Needs review » Needs work
  1. +++ b/core/lib/Drupal/Core/Form/FormBuilder.php
    @@ -1800,8 +1800,7 @@ protected function currentUser() {
             $this->currentUser = \Drupal::currentUser();
           }
           else {
    -        global $user;
    -        $this->currentUser = $user;
    +        $this->currentUser = \Drupal::currentUser();
           }
    

    Together this won't really need the two code paths ...

  2. +++ b/core/modules/basic_auth/lib/Drupal/basic_auth/Authentication/Provider/BasicAuth.php
    @@ -138,7 +138,7 @@ public function cleanup(Request $request) {}
    -    if ($GLOBALS['user']->isAnonymous() && $exception instanceof AccessDeniedHttpException) {
    +    if (\Drupal::currentUser()->isAnonymous() && $exception instanceof AccessDeniedHttpException) {
    

    This is part of the session handling and so potentially problematic.

  3. +++ b/core/modules/history/lib/Drupal/history/Tests/Views/HistoryTimestampTest.php
    @@ -50,7 +50,7 @@ public function testHandlers() {
    +    \Drupal::getContainer()->set('current_user', $account);
    
    +++ b/core/modules/user/lib/Drupal/user/Tests/Views/ArgumentDefaultTest.php
    @@ -35,17 +35,16 @@ public function test_plugin_argument_default_current_user() {
    +    $admin = \Drupal::currentUser();
    ...
    +    \Drupal::getContainer()->set('current_user', $account);
    ...
    +    \Drupal::getContainer()->set('current_user', $admin);
    

    The recommended API is now \Drupal::currentUser()->setCurrentUser($account);

joelpittet’s picture

@dawehner thanks for the review.

1. Merged the paths
2. Reverted this change
3. Great that looks nice to work with, thanks, changed.

joelpittet’s picture

Status: Needs work » Needs review

aw rookie mistake ... :P

Status: Needs review » Needs work

The last submitted patch, 48: remove_global_user-2213899-48.patch, failed testing.

joelpittet’s picture

@dawehner it seems that the setCurrentUser isn't working as it should in those tests. I'll dig in deeper in a while.

The last submitted patch, 48: remove_global_user-2213899-48.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
2.42 KB
1.57 KB

Let's just fix the tests.

moshe weitzman’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Will cause commit conflicts

Looks sane and modern.

webchick’s picture

Status: Reviewed & tested by the community » Needs review
+++ b/core/lib/Drupal/Core/Form/FormBuilder.php
@@ -1148,14 +1148,8 @@ protected function drupalStaticReset($name = NULL) {
   protected function currentUser() {
-    if (!$this->currentUser) {
-      if (\Drupal::hasService('current_user')) {
-        $this->currentUser = \Drupal::currentUser();
-      }
-      else {
-        global $user;
-        $this->currentUser = $user;
-      }
+    if (!$this->currentUser && \Drupal::hasService('current_user')) {
+      $this->currentUser = \Drupal::currentUser();
     }
     return $this->currentUser;
   }

This changes the logic such that $this->currentUser could end up being NULL. On the one hand, tests are passing, and chx said in IRC he didn't think it was a big deal because FormBuilder should come well after session loading stuff when that ceases to be an issue. On the other hand, it seems to have come from #2112807: Move the form builder functions in form.inc to a form service and doesn't seem to be a copy/paste of anything else that I can find, indicating it was a deliberate change.

Can we look more into this before committing?

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

This changes the logic such that $this->currentUser could end up being NULL. On the one hand, tests are passing, and chx said in IRC he didn't think it was a big deal because FormBuilder should come well after session loading stuff when that ceases to be an issue. On the other hand, it seems to have come from #2112807: Move the form builder functions in form.inc to a form service and doesn't seem to be a copy/paste of anything else that I can find, indicating it was a deliberate change.

Technically the current user is a lazy initialized object. So unless someone calls for example ->id() or ->hasPermission() on it, the user does not have to be initialized.

Afaik the reason why we have this hasService call is that first step of the installer doesn't have a proper container yet.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Cool, thanks for the explanation.

Committed and pushed to 8.x. Thanks!

  • webchick committed 1b97ba1 on 8.0.x
    Issue #2213899 by dawehner, joelpittet: Remove global $user wherever...

Status: Fixed » Closed (fixed)

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