[5.1] Remove redundant function call

[apollopy]

The login function will call updateSession function which will excute

$this->session->set($this->getName(), $id);

https://github.com/laravel/framework/blob/5.1/src/Illuminate/Auth/Guard.php#L432

[GrahamCampbell]

What does this do please?

[apollopy]

@GrahamCampbell
the explanation updated.

[taylorotwell]

If it's not broken don't fix it. Had too many broken PRs lately.

[vlakoff]

This is a leftover from the first implementation which wasn't making any sense:

public function loginUsingId($id, $remember = false)
{
    $this->session->put($this->getName(), $id);

    return $this->login($this->user(), $remember);
}

Then it was fixed five months later in #1712, see how this retrieveById is welcome:

public function loginUsingId($id, $remember = false)
{
    $this->session->put($this->getName(), $id);

    return $this->login($this->provider->retrieveById($id), $remember);
}

(Then, it was again modified #2871 to return the user as stated in the phpdoc.)

So, it's not a redundant function call but rather a leftover from the first implementation which was entirely wrong.

[apollopy]

The corresponding test cases are not understood

https://github.com/laravel/framework/blob/5.1/tests/Auth/AuthGuardTest.php#L245

the login function is return void, why test set will return user?

[vlakoff]

Just an overlook I suppose. The test wasn't impacted as the login return value is not used actually.

More importantly:

• If no user is found for the given id, bad things happen. We should check retrieveById found an user before continuing, otherwise throw a proper error.
• As login is mocked in the test, we aren't testing much. We should either:
  • test the whole thing, as in the tests above,
  • or simply test we are retrieving the expected user, logging with it, and returning it (basically what is done currently).

[vlakoff]

@taylorotwell What should be the behavior when Guard::loginUsingId (Laravel 5.1) or SessionGuard::loginUsingId (Laravel 5.2) is called with an invalid id? (throw exception, return false...) Current behavior: login is then called with null and errors happen.

[vlakoff]

Thoughts?

I'm ready to submit code for this, but I have to know what should be thrown if ever SessionGuard::loginUsingId were to be called with an invalid id.