Remove drupal_goto from user_login()

Marking as a task/

This is my #1 complaint about Drupal, but I could have sworn there's another issue for this already open somewhere...

I'm a n00b at core hacking, but it seems to me the drupal_goto() in user_login() is redundant anyways. As far as I can tell, user_login() is called from user_page() in user.pages.inc like this:

function user_page() {
  global $user;
  if ($user->uid) {
    menu_set_active_item('user/' . $user->uid);
    return menu_execute_active_handler(NULL, FALSE);
  }
  else {
    return drupal_get_form('user_login');
  }
}

So it looks like the user's logged-in status is being checked before we even call user_login(). I removed the drupal_goto() stuff from user_login() and it seems to work fine.

setting to need review for the bot

The patch in #3 no longer applies.

The patch from #3 has been re-rolled to account for directory changes in D8 since the patch was first written.

Please note, the function being removed HERE, drupal_goto(), is in the process of being re-written as of August 2012. #1668866: Replace drupal_goto() with RedirectResponse.

Thanks @Transition! The reroll looks correct.

The next step is to check whether this is the correct fix, and explore what behavior might be changed if we remove this code. I looked up the following information on api.d.o:

• user_login() is the form builder for the user login page.
• Above, @mdm points out that the user_login() form is built from user_page() (which is the page callback for user/login defined in user_menu()) only when the user ID is false, which means that the code removed by the patch in #6 would never get executed, because the user ID is always empty whenever the user login form gets built from the page callback.
• However, it's also possible that the user login form might be built on a different page, or even built directly and then manipulated as a render array. So, it's possible that this patch could change the behavior in those circumstances.
• Since user_login() is a form builder callback, it won't generally be called directly (except maybe for testing or to build a larger form). On api.d.o, it shows no direct calls to user_login() in core, but also lists these string references to user_login (which usually means places where it's used as a callback, but sometimes might include false positives).

Note that the above is exactly the situation that the original post is trying to fix. Suppose you wanted to build the user form manually in a test. You might add the following code:

  $user_form = drupal_get_form('user_login');

...and, since the function checks the global $user variable directly, and a user ID is set while the test is being run, boom, you're suddenly logged out redirected! So this is a correct patch from my perspective.

However, we still need to make sure this patch won't affect anything else that we might not have test coverage for. So, let's investigate the other references to user_login() I link above, and see if anything else would be affected by this patch. Probably not, but we should be sure. You can both look at the code on api.d.o and manually test those instances with and without the patch applied. (You can start by testing visiting user/login with and without the patch applied to confirm there's no change in behavior.)

Unfortunately, I don't think this is backportable, because it's possible some contributed module out there is (unwisely) relying on the behavior that they get logged out if no user ID is set when user_login() is called. However, this is a good fix for Drupal 8 (and as a bonus means less stuff to clean up following #1668866: Replace drupal_goto() with RedirectResponse). Once the other references to user_login() in core are checked, I think this is RTBC. @Transition, do you want to check those references too?

Edit: Forgot to mention, it doesn't make sense to add a test for this unless one of the other uses turns something up. There's no actual bug, just some silly code that doesn't get executed under normal circumstances. The only test we could add would be to manually build the user form array while logged in and confirm that the user isn't logged out, but that seems excessive even to me. :)

Also related #1772584: Get rid of user_login_block() & user_login() in favour of user_login_form()

Sorry, I linked the Drupal 7 string references; here are the Drupal 8 ones:
http://api.drupal.org/api/drupal/core%21modules%21user%21user.module/fun...

So @Transition and I went over the references above today. Six of the eight are block names used in the block plugin tests, and one is the block name used in the sample code in the hook_page_alter() documentation. So user_page() is the only place this function is called in Drupal core.

Marking RTBC. Thanks @Transition!

Nice clean-up, and nice research to back up the clean-up. :)

Committed and pushed to 8.x. Thanks!

Thanks @xjm and @mdm. Wouldn't have been possible without you two!