Make WordPress Core

Opened 7 years ago

Closed 20 months ago

#39385 closed defect (bug) (invalid)

Set $current_user global in wp_signon() after successful authentication

Reported by: fjarrett's profile fjarrett Owned by:
Milestone: Priority: normal
Severity: normal Version:
Component: Login and Registration Keywords: close
Focuses: Cc:

Description

The $current_user global should be set after successful authentication inside wp_signon() instead of waiting for the next load of WordPress.

Although the $user_login string and $user object are passed through the wp_login hook, there are some functions that don't allow user parameters and rely solely on get_current_user_id(), such as wp_destroy_other_sessions().

This is easy enough to work around, but it seems reasonable to expect that user-related function calls should "just work" at any point after authentication.

<?php

add_action( 'wp_login', function ( $user_login, $user ) {

        var_dump( get_current_user_id() ); // int(0)

        $GLOBALS['current_user'] = $user;

        var_dump( get_current_user_id() ); // int(1)

        exit;

}, 10, 2 );

Attachments (2)

39385.patch (396 bytes) - added by fjarrett 7 years ago.
39385.2.patch (399 bytes) - added by dotancohen 6 years ago.

Download all attachments as: .zip

Change History (7)

#1 @johnbillion
7 years ago

  • Keywords needs-patch added
  • Version trunk deleted

@fjarrett
7 years ago

#2 @ocean90
7 years ago

  • Keywords close added

See #28116 and #35488 for why this is more of a documentation issue than an actual bug.

#3 @dotancohen
6 years ago

This is not a documentation error for the simple reason that the wp_login action does not have the current user set, even though it ostensibly "Fires after the user has successfully logged in".

The current patches don't use the core Wordpress function for setting the current user, even though it pretty much does the same thing. I feel that the attached patch is better from a maintainability standpoint by using the appropriate WP API. It is also updated to the current WP code base (just different line numbers in the diff, though).

@dotancohen
6 years ago

#4 @dotancohen
6 years ago

To preempt arguments that the wp_login does not need to have the current user set because one of its parameters is the logged-in user, I counter that the code in the hooked method may (as, _does_ on one system I'm writing) can a non-coupled class that itself _does_ require the current user to be set.

add_action('wp_login', 'actionWpLogin', 10, 2);

function actionWpLogin($user_login, $user)
{
    Foo::bar();
}


/**
 * A class that I don't maintain.
 */
class Foo {
    public static function bar()
    {
        $user = wp_get_current_user();
        // Some $user magic here...
    }
}

#5 @hellofromTonya
20 months ago

  • Keywords needs-patch removed
  • Milestone Awaiting Review deleted
  • Resolution set to invalid
  • Status changed from new to closed

Hello 👋,

Thank you for the ticket and everyone who has contributed.

As previously state, it's not a defect, but rather by design.

By design, wp_signon() does not handle setting the current user. It's job is to check the credentials and set the authentication cookies as SergeyBiryukov notes here.

#28116 / [40943] added a note into the function's DocBlock explaining why and providing further guidance if a custom requirement is needed:

 * Note: wp_signon() doesn't handle setting the current user. This means that if the
 * function is called before the {@see 'init'} hook is fired, is_user_logged_in() will
 * evaluate as false until that point. If is_user_logged_in() is needed in conjunction
 * with wp_signon(), wp_set_current_user() should be called explicitly.

https://developer.wordpress.org/reference/functions/wp_signon/

Why by design?

After logging in, a redirect happens, which clears in-memory status (including the globals). After the redirect is done, the $current_user global is empty. It's populated after plugins are loaded, setup_theme, and auth_cookie_valid all run.

Setting the current user within wp_signon() does not retain the current user in-memory once the login and redirect are done. Adding that to wp_signon() adds more tasks to the process without a major benefit for the majority users.

While I understand the need, there are hooks available for customization.

Since this is not a defect, but by design, and it's been 4 years since last activity, I'm closing this ticket. The classification of invalid seems harsh (sorry about that), but it's used to denote tickets that are not a defect/bug (see in the handbook).

Thank you again everyone!

Note: See TracTickets for help on using tickets.