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

[8.x] Revert PR #38552 #38711

Merged
merged 1 commit into from
Sep 8, 2021
Merged

[8.x] Revert PR #38552 #38711

merged 1 commit into from
Sep 8, 2021

Conversation

driesvints
Copy link
Member

@driesvints driesvints commented Sep 8, 2021

This PR reverts #38552 because it's a breaking change. Any Laravel app that doesn't has a password reset route will see their app break.

See laravel/nova-issues#3542 & the comments in #38552

@driesvints driesvints changed the title Revert PR #38552 [8.x] Revert PR #38552 Sep 8, 2021
@GrahamCampbell
Copy link
Member

Do we need another patch release right after this?

@driesvints
Copy link
Member Author

Probably best.

@Jubeki
Copy link
Contributor

Jubeki commented Sep 8, 2021

Wouldn't it also be possible to check if the route exists and only then generate the link? otherwise pass null to the creating function.

@driesvints
Copy link
Member Author

@Jubeki what code do you propose? Can you send in a PR?

@Jubeki
Copy link
Contributor

Jubeki commented Sep 8, 2021

protected function resetUrl($notifiable)
{
    if (static::$createUrlCallback) {
        return call_user_func(static::$createUrlCallback, $notifiable, $this->token);
    }
    
    if(Route::has('password.reset') {
        return url(route('password.reset', [
            'token' => $this->token,
            'email' => $notifiable->getEmailForPasswordReset(),
        ], false));
    }
    
    return null;
}

Something like this?

@Jubeki
Copy link
Contributor

Jubeki commented Sep 8, 2021

I can send in a PR

@crynobone
Copy link
Member

I personally would prefer static method to generate reset url if needed. ResetPassword::url(string $token)

@Jubeki
Copy link
Contributor

Jubeki commented Sep 8, 2021

My idea is also not really good, because it should provide a default URL and not null. No idea what a good default URL is though.

@taylorotwell taylorotwell merged commit 408dc67 into 8.x Sep 8, 2021
@taylorotwell taylorotwell deleted the revert-38552 branch September 8, 2021 13:12
victorvilella pushed a commit to cdsistemas/framework that referenced this pull request Oct 12, 2021
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.

5 participants