Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[5.5] delete function id #24960

Closed
wants to merge 1 commit into from
Closed

[5.5] delete function id #24960

wants to merge 1 commit into from

Conversation

apollopy
Copy link
Contributor

@apollopy apollopy commented Jul 24, 2018

@apollopy apollopy changed the title delete function id [5.5]delete function id Jul 24, 2018
@apollopy apollopy changed the title [5.5]delete function id [5.5] delete function id Jul 24, 2018
@browner12
Copy link
Contributor

the function contents are different. how is this the same?

@apollopy
Copy link
Contributor Author

apollopy commented Jul 25, 2018

read

https://github.com/laravel/framework/blob/5.5/src/Illuminate/Auth/SessionGuard.php#L112-L125

and

https://github.com/laravel/framework/blob/5.5/src/Illuminate/Auth/SessionGuard.php#L205-L207

public function id()
{
    if ($this->loggedOut) {
        return;
    }
    return $this->user()
        ? $this->user()->getAuthIdentifier()
        : $this->session->get($this->getName());
}

->

public function id()
{
    if ($this->loggedOut) {
        return;
    } else {
        if ($this->user()) {
            return $this->user()->getAuthIdentifier();
        } else {
            /**
             * This is superfluous.
             * if $this->loggedOut == false and can get by session,
             * $this->user() will return user object.
             * if $this->loggedOut == false and not can get by session,
             * $this->user() will get by "remember me" cookie or return null.
             */
            return $this->session->get($this->getName());
        }
    }
}

->

public function id()
{
    if ($this->loggedOut) { // This is superfluous, $this->user() Already judged
        return;
    } else {
        if ($this->user()) {
            return $this->user()->getAuthIdentifier();
        }
    }
}

->

/**
 * it's the same as
 * https://github.com/laravel/framework/blob/5.5/src/Illuminate/Auth/GuardHelpers.php#L68
 */
public function id()
{
        if ($this->user()) {
            return $this->user()->getAuthIdentifier();
        }
}

@apollopy
Copy link
Contributor Author

#16373

After this pr, there is no practical significance.

@jmarcher
Copy link
Contributor

They could do the same or not, but deleting a public method is a massive breaking change.

So big 👎

@apollopy
Copy link
Contributor Author

apollopy commented Jul 25, 2018

This is not deleting. The id function exists in Traits GuardHelpers, and SessionGuard use GuardHelpers.

https://github.com/laravel/framework/blob/5.5/src/Illuminate/Auth/GuardHelpers.php#L68
https://github.com/laravel/framework/blob/5.5/src/Illuminate/Auth/SessionGuard.php#L20

The current class override Trait methods, try not to get the User object from the database, only get id from session. But brought a lot of bugs:

this #13769 and this #16373

After several fixed bugs, the original purpose no exists.
Either completely override the id function to the extent that there is no bug, or remove the override

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants