Problem/Motivation

There are still some left-over usages of global $user, especially in the low-level session/authentication API's.

Those have had cross-dependencies which made it impossible to remove them separately.

Proposed resolution

Move the initialization of the current timezone from the SessionHandler, where it required global $user, to AccountProxy::setAccount(). This is the primary API to set the currently logged in user, doing it there allows us to remove it in three different places, and it works whenever a session is activated, a user is logged in, or even when we switch to a user temporarily, using the account switcher.

Removing that allows to remove global user in all but one case. That case is the interaction between the Cookie authentication provider and the session reader, those two currently have to use a global variable to communicate. To make it clear that this is just temporary and a workaround and will be cleaned up in #2286971: Remove dependency of current_user on request and authentication manager, it has been renamed, so that global $user is offically and finally *gone*.

Remaining tasks

User interface changes

API changes

Global $user is definitely gone now, but it was officially deprecated for a very long time, the change record was written in july, 20*13*.

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Task because it is deprecated API removal.
Prioritized changes Prioritized because it removes a deprecated API
Disruption Not disruptive for core/contributed and custom modules/themes because the global $user was deprecated since July 2013 and nobody is encouraged to use it.

Original report by @andypost

Part of #2047951: [META] Remove calls to deprecated global $user and $GLOBALS['user']

There's a still some usage in tests and user.module:

grep -R -e   "GLOBALS\['user'\]"    -e "global .*\$user"  *
core/scripts/generate-d7-content.sh:  $original_user = $GLOBALS['user'];
core/scripts/generate-d7-content.sh:    $GLOBALS['user'] = drupal_anonymous_user();// We should have already allowed anon to vote.
core/modules/system/src/Tests/Datetime/DrupalDateTimeTest.php:    global $user;
core/modules/system/src/Tests/Datetime/DrupalDateTimeTest.php:    // Disable session saving as we are about to modify the global $user.
core/modules/user/user.module: * Drupal has a global $user object, which represents the currently-logged-in
core/modules/user/user.module: * user. So to avoid confusion and to avoid clobbering the global $user object,
core/modules/user/user.module: * The global $user object is replaced with the passed in account.
core/modules/user/user.module:  global $user;
core/modules/user/user.module:  global $user;
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

andypost’s picture

It looks that date tests are affected by #2062069: Remove calls to deprecated global $user in core/includes/bootstrap.inc
Suppose better to remove that code from tests because this out of scope

skipyT’s picture

Issue summary: View changes
Berdir’s picture

Title: Remove global $user wherever possible (outside bootstrap and authentication) Round 3 » Remove remaining global $user
Status: Active » Needs review
FileSize
10.56 KB

It is not possible to further separate the remaining calls, they all overlap and need to be fixed together.

This patch deals with almost everything.

The only place where global $user is left is when reading the session, for the interaction between SessionHandler and Cookie authentication provider, I believe there are other issues that are trying to address this.

Status: Needs review » Needs work

The last submitted patch, 3: remove-global-user-2328645-3.patch, failed testing.

andypost’s picture

  1. +++ b/core/lib/Drupal/Core/Session/AccountProxy.php
    @@ -67,6 +67,8 @@ public function setAccount(AccountInterface $account) {
    +    // @todo: Who should call this?
    +    date_default_timezone_set(drupal_get_user_timezone());
    

    Suppose authentication

  2. +++ b/core/lib/Drupal/Core/Session/SessionManager.php
    @@ -146,7 +146,6 @@ public function start() {
    -    date_default_timezone_set(drupal_get_user_timezone());
    

    otoh timezone is session level setting

  3. +++ b/core/lib/Drupal/Core/Session/SessionManager.php
    @@ -241,13 +240,8 @@ public function regenerate($destroy = FALSE, $lifetime = NULL) {
    -    date_default_timezone_set(drupal_get_user_timezone());
    

    regenerate maybe needed too

Berdir’s picture

1. I thought the same, but the interesting thing is that the current_user is not set while you are still in AuthenticationManager::authenticate(). The user is returned and then set in the caller, which is AccountProxy::getAccount(), which calls setAccount().

3. That's the interesting side-effect of doing it in setAccount(). regenerate() alone doesn't change the user. Someone actually has to call setAccount() first, e.g. in user_login_finalize().

After writing that patch, doing it there actually makes a lot of sense to me. AccountProxy::setAccount() is *the* API to set the current user. And the timezone is based on the current user, so this immediately updates that. So I doubt that we currently have a better place to do this. See also the cleaned up DrupalDateTimeTest, all that login/session stuff is gone. Just setAccount($user) and it works.

Berdir’s picture

Status: Needs work » Needs review
FileSize
11.4 KB
861 bytes
Berdir’s picture

joelpittet’s picture

Eagerly awaiting testbot to come back! Changed the parent issue and just did a quick review.

  1. +++ b/core/lib/Drupal/Core/Session/AccountProxy.php
    @@ -67,6 +67,8 @@ public function setAccount(AccountInterface $account) {
    +    // @todo: Who should call this?
    +    date_default_timezone_set(drupal_get_user_timezone());
    

    Very good question, I see this was removed from other hunks.

  2. +++ b/core/lib/Drupal/Core/Session/SessionHandler.php
    @@ -155,17 +155,17 @@ public function close() {
       public function destroy($sid) {
    -    global $user;
    +
     
         // Delete session data.
    

    Extra line break.

Berdir’s picture

Patch is green ;)

1. See #6, it is removed from other places because it is now called in the one place that matters IMHO. And that place is also after the current user service is guaranteed to be set, so we no longer need to worry about it being there or not.

You did something strange with the parent/related issues, making this its own parent :) Should probably be reverted to what I did in #8.

Berdir’s picture

joelpittet’s picture

@Berdir sorry, this is what I meant to do there for the parents, as this is a direct child of the meta.

Nice work on getting it green, that was a lot of re-rolls when the session handling wasn't finished, glad that's over.

So should that @todo just be removed then? I've no opinion on if that is right or wrong way to deal with that, just that we should try to avoid adding @todo without tying it to an issue, though it looked like it just needed to be discussed so wanted to surface that.

Berdir’s picture

That's still wrong, I closed the meta as duplicate.

I don't know, the session stuff is actually not yet done, I'm waiting for reviews from the involved people for now..

andypost’s picture

Issue tags: +Needs issue summary update
FileSize
1.56 KB
12.96 KB

Looking at \Drupal\Core\Session\AccountSwitcher::switchTo() I agree exactly

Also found a few places that should be fixed.

Maybe context should should return a drupal_get_user_timezone() as result or somehow conditionally

Berdir’s picture

+++ b/core/lib/Drupal/Component/Gettext/PoHeader.php
@@ -77,9 +77,8 @@ class PoHeader {
     $this->_langcode = $langcode;
-    // Ignore errors when run during site installation before
-    // date_default_timezone_set() is called.
-    $this->_po_date = @date("Y-m-d H:iO");
+    // Use properly formatted date.
+    $this->_po_date = date("Y-m-d H:iO");
     $this->_pluralForms = 'nplurals=2; plural=(n > 1);';
   }

I'm not sure if this is safe to remove, it could result in errors on a server that does not have a default timezone set in the installer...

almaudoh’s picture

Great to see we can remove so many of the global $user (especially in SessionHandler) and still get green. Most of the code here is being refactored over at #2286971: Remove dependency of current_user on request and authentication manager.

I wonder if this should wait for that. OTOH since this is green I guess we can always get this in.

Berdir’s picture

Yeah, I wasn't sure.

Thing is, I can't really make sense of #2286971: Remove dependency of current_user on request and authentication manager, atleast not without spending a lot of time on it I think, because I really don't understand yet what it is changing, and how, and why the result is better. And was thinking that cleaning this up first might make the issue there easier, there are a few tricky things uncovered and fixed here, like the fact that user_login_finalize() does not update the current user service after a login, only global $user.

almaudoh’s picture

And was thinking that cleaning this up first might make the issue there easier, there are a few tricky things uncovered and fixed here, like the fact that user_login_finalize() does not update the current user service after a login, only global $user.

You are right, this will actually help #2286971: Remove dependency of current_user on request and authentication manager along. I'm RTBC+ on this one.

Berdir’s picture

One part that I'm not sure about is the last global $user, that is used through reading/starting the session. That's something you're working on in #2286971: Remove dependency of current_user on request and authentication manager. Wondering if we should keep that or possibly explicitly rename it to $_user or something, so that we can be sure that nothing is left that still relies on global $user.

andypost’s picture

FileSize
1.22 KB
12.34 KB

Added a link for @todo to file #2427077: Find a right place to call date_default_timezone_set() #12
Reverted #15 change

Summary update is needed to rtbc

znerol’s picture

Status: Needs review » Needs work

Needs a reroll due to a conflict in DrupalDateTimeTest.php.

  1. +++ b/core/lib/Drupal/Core/Session/AccountProxy.php
    @@ -67,6 +67,8 @@ public function setAccount(AccountInterface $account) {
    +    // @todo: Who should call this?
    +    date_default_timezone_set(drupal_get_user_timezone());
    

    This is exactly where I'd expect date_default_timezone_set().

  2. +++ b/core/lib/Drupal/Core/Session/SessionManager.php
    @@ -146,7 +146,6 @@ public function start() {
    -    date_default_timezone_set(drupal_get_user_timezone());
    

    Very much agree.

  3. +++ b/core/lib/Drupal/Core/Session/SessionManager.php
    @@ -241,13 +240,8 @@ public function regenerate($destroy = FALSE, $lifetime = NULL) {
    -      // Preserve the logged in user, as it will be reset to anonymous
    -      // by \Drupal\Core\Session\SessionHandler::read().
    -      $account = $user;
           $this->startNow();
    -      $user = $account;
    

    So we can remove this because the global $user variable is only used to tunnel the user read from the session table to the cookie authentication provider. Fantastic!

  4. +++ b/core/lib/Drupal/Core/Session/SessionManager.php
    @@ -241,13 +240,8 @@ public function regenerate($destroy = FALSE, $lifetime = NULL) {
    -    date_default_timezone_set(drupal_get_user_timezone());
    

    Ditto.

  5. +++ b/core/modules/system/src/Tests/Datetime/DrupalDateTimeTest.php
    @@ -93,28 +92,14 @@ public function testDateTimezone() {
    -    \Drupal::service('session_manager')->disable();
    ...
    +    $this->container->get('current_user')->setAccount(User::load($test_user->id()));
    

    Please use the account_switcher here.

  6. +++ b/core/modules/user/user.module
    @@ -595,8 +586,7 @@ function user_menu_breadcrumb_alter(&$active_trail, $item) {
    -  global $user;
    -  $user = $account;
    +  \Drupal::currentUser()->setAccount($account);
    

    Agree. I guess this never was a real problem because the login form redirects, therefore only little code relying on the current user service actually was affected. In contrast the session manager / session handler still worked with the global user, so the correct user is written to the new session record.

Wondering if we should keep that or possibly explicitly rename it to $_user or something, so that we can be sure that nothing is left that still relies on global $user.

Yes please. If contrib modules still rely on the global $user, then that is a) wrong anyway and b) easy to fix.

andypost’s picture

Status: Needs work » Needs review
FileSize
794 bytes
12.34 KB

Fixed #21.5
Any reason to rename? we are going to remove that.

andypost’s picture

catch’s picture

Renaming would break any unconverted contrib modules earlier than later, which would be a good thing, in case it takes longer to fully remove. Don't mind if it's done here though.

Berdir’s picture

Status: Needs review » Needs work

Thanks for the review @znerol:

1. Great. I actually tried to add it in the authentication manager first, that's when I added the @todo. Then I noticed that won't work, searched for a better place and found setAccount() but kept the @todo. Looks like we all agree that this is actually the best place, great. So, remove the @todo, close the follow-up.

5. I'm actually not sure that makes sense. switcher implies that we want to switch back again and might actually break if we do not call reset, because we also switch in the test runner, wondering if that still works in the UI then. See drupalLogin(), that also does a setAccount(), and not a switch.

6. Actually, I had a very real problem with that, I have a dialog/ajax based form workflow where I log in in the first step, and then display the next form. That didn't work because the injected user in form cache wasn't updated, so it didn't generate a token as it was supposed to. It actually still doesn't work, because the csrf token thing in the meta session bag is somehow last, will need your help with that, but unrelated to this issue.

Berdir’s picture

Also, agreed, lets rename global $user to global $_session_user, with a @todo that says to remove it, possibly in #2286971: Remove dependency of current_user on request and authentication manager?

andypost’s picture

Status: Needs work » Needs review
FileSize
4.58 KB
15 KB

Renamed, reverted to setAccount() and removed todo

almaudoh’s picture

FWIW, I also think renaming is a good idea.

Berdir’s picture

Issue summary: View changes
Issue tags: -Needs issue summary update

Updated the issue summary. I think this is good to go?

almaudoh’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

Yeaaaah! global $user...erm $_session_user... reduced to < 10 mentions in core. Great patch. RTBC from me.
Also added beta evaluation

znerol’s picture

See drupalLogin(), that also does a setAccount(), and not a switch.

In fact drupalLogin() (and logout) is more or less the only place where AccountProxy::setAccount() should be used. In most other places, especially in tests we do masquerading, and hence should use the account switcher. If this does not work then there is either something wrong with the account switcher or with the test runner.

It actually still doesn't work, because the csrf token thing in the meta session bag is somehow last

I guess s/last/lost. This is likely because SessionManager::regenerate() clears the csrf token seed.

Berdir’s picture

It does work, it just isn't necessary, because we already are masquerading in the testrunner itself, and that will switch back at the end just fine. I just don't see why it is fine in drupalLogin() but not there. In fact, I could have just called drupalLogin(), the only reason I did not is that this would prevent us from converting this test to a kernel test for no good reason.

But if you feel strongly about, I'm fine with switching to switching, I just see the point :)

Berdir’s picture

I guess s/last/lost. This is likely because SessionManager::regenerate() clears the csrf token seed.

Yes, but my code runs after that (in a new submit handler of the login form), generates a new token, and then that is not persisted. somehow...

almaudoh’s picture

Status: Reviewed & tested by the community » Needs work

I agree with @znerol #31 that AccountSwitcher::switchTo() and ::switchBack() should be used in all tests where user impersonation is needed.

Just didn't want to hold up this patch since a similar change is being done at #2426031: Remove deprecated uses of SessionManager::isEnabled(), SessionManager::enable() and SessionManager::disable(). But then again, maybe we should do the right thing here too.

almaudoh’s picture

Status: Needs work » Reviewed & tested by the community

Okay. Took another good look at this patch: while the AccountSwitcher service should be used whenever user impersonation / masquerading is done, DrupalDateTimeTest is not actually doing impersonation.

It's just checking to verify that the user's timezone is used when no timezone is specified. So I guess in this case AccountProxy::setAccount() would be okay - please ignore previous comment #34 didn't look closely.

So I'll set this back to RTBC if that's ok.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll
almaudoh’s picture

Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs reroll
FileSize
15.12 KB
760 bytes

Rerolled and added a little comment for why we're using AccountProxy::setAccount() in DrupalDateTimeTest

Back to RTBC since I only added a comment.

alexpott’s picture

Status: Reviewed & tested by the community » Needs review
+++ b/core/lib/Drupal/Core/Session/SessionHandler.php
@@ -86,29 +87,29 @@ public function read($sid) {
-    global $user;
+    $user = \Drupal::currentUser();

@@ -155,17 +156,17 @@ public function close() {
-    $user = new AnonymousUserSession();
+    \Drupal::currentUser()->setAccount(new AnonymousUserSession());

+++ b/core/lib/Drupal/Core/Session/SessionManager.php
@@ -181,7 +180,7 @@ protected function startNow() {
-    global $user;
+    $user = \Drupal::currentUser();

@@ -212,7 +211,7 @@ public function save() {
-    global $user;
+    $user = \Drupal::currentUser();

Removing the global for \Drupal usage here does not feel like a win. Is this going to be addressed by #2286971: Remove dependency of current_user on request and authentication manager Or should be somehow by injecting the currentUser service into SessionManager and SessionHandler?

Setting back to needs review to get an answer.

Berdir’s picture

Oh, I missed that question.

Why do you think that is not an improvement? This finally removes global $user, which took us 1.5 years to achieve. We have a lot of cross-dependencies. Yes, this doesn't make this very pretty, but we can remove a bunch of other workarounds, which will in make other issues easier.

Note that the latest patches in #2286971: Remove dependency of current_user on request and authentication manager no longer address this AFAIK, this was partially merged in from #2228393: Decouple session from cookie based user authentication and reverted again. We either have to merge all those issues together, which I don't think is going to make us move any faster, or make smaller, non-perfect steps. Any further improvement of that code is IMHO not useful here, we want to get rid knowing about the current user at least in SessionHandler, not make it nicer.

Btw, my redirect port is completely broken in HEAD, because of a fatal error in SessionManager, when you return a response before authentication happens, this fixes that. Not in a pretty way, because it just makes authentication happen then, but still, it is a fix, just verified that the tests pass again with this applied.

Berdir’s picture

Status: Needs review » Reviewed & tested by the community

Moving back to RTBC to get it into your queue again :)

znerol’s picture

+1

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 37: remove-global-user-2328645-37.patch, failed testing.

Berdir’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
14.46 KB

Conflicted with #2429103: Remove deprecated methods SessionManager::enable(), SessionManager::disable() and SessionManager::isEnabled(), that removed a part where we made small doc change here, no other change.

$ diff remove-global-user-2328645-37.patch patches/remove-global-user-2328645-43.patch
2c2
< index fee191a..c700848 100644
---
> index 5d81ac1..fcf4d44 100644
187c187
< index 8249805..e3fcd24 100644
---
> index f9496d3..07790cd 100644
250,262d249
< diff --git a/core/lib/Drupal/Core/Session/SessionManagerInterface.php b/core/lib/Drupal/Core/Session/SessionManagerInterface.php
< index fad62b7..b14b056 100644
< --- a/core/lib/Drupal/Core/Session/SessionManagerInterface.php
< +++ b/core/lib/Drupal/Core/Session/SessionManagerInterface.php
< @@ -38,7 +38,7 @@ public function isEnabled();
<     *
<     * This function allows the caller to temporarily disable writing of
<     * session data, should the request end while performing potentially
< -   * dangerous operations, such as manipulating the global $user object.
< +   * dangerous operations, such as manipulating the current user.
<     *
<     * @see https://drupal.org/node/218104
<     *
alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Thanks for answering my question @Berdir. I think it is okay for this issue not to try to untangle the use of \Drupal::currentUser(); as a service locator.

Committed 8db8dd0 and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation to the issue summary.

  • alexpott committed 8db8dd0 on 8.0.x
    Issue #2328645 by andypost, Berdir, almaudoh: Remove remaining global $...

Status: Fixed » Closed (fixed)

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