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.8] Pass notifiable email through reset link #28475

Merged
merged 3 commits into from May 13, 2019

Conversation

Projects
None yet
6 participants
@dwightwatson
Copy link
Contributor

commented May 10, 2019

When you click the link from the password reset email, the controller/view try to get your email address from the request to pre-fill the email field of the form. It's a nice improvement to the user experience as it doesn't force them to enter their email again, but it isn't actually working at the moment.

This change will pass the notifiable's email address through the query string so that this feature actually works.

@dwightwatson dwightwatson changed the title Pass notifiable email through reset link [5.8] Pass notifiable email through reset link May 10, 2019

@taylorotwell

This comment has been minimized.

Copy link
Member

commented May 11, 2019

Attempted here: #21633

As you can see, you should probably use the contract method to get the email. Secondly, several people commented it would break their applications - though I'm not sure how or why.

@devcircus

This comment has been minimized.

Copy link
Contributor

commented May 11, 2019

This was breaking for me due to my own customizations. Not relevant any longer and many people have wanted this. Not sure why but the PRs for it will likely never stop so I say 👍🏻.

@dwightwatson

This comment has been minimized.

Copy link
Contributor Author

commented May 12, 2019

Gotcha, that looks a lot better. I'll update this PR to match, but let me know if you'd prefer to target 5.9 instead.

I feel like it's worth fixing this because the rest of the piping is already in place, it's just one missing piece of the puzzle, and it's a better experience for users.

@@ -47,7 +47,7 @@ public function via($notifiable)
/**
* Build the mail representation of the notification.
*
* @param mixed $notifiable
* @param \Illuminate\Contracts\Auth\CanResetPassword $notifiable

This comment has been minimized.

Copy link
@driesvints

driesvints May 12, 2019

Member

This doesn't makes sense because you expect a notifiable here and not a CanResetPassword instance. You can't be 100% sure that it's a User instance you're receiving here. The change below is ok for me but let's keep the mixed type-hint.

This comment has been minimized.

Copy link
@dwightwatson

dwightwatson May 12, 2019

Author Contributor

That's fair, I was just lining up with the aforementioned PR. I'll fix this now.

This comment has been minimized.

Copy link
@martinbean

martinbean May 15, 2019

Contributor

Why would a non-CanResetPassword implementation be trying to, well, reset their password?

This comment has been minimized.

Copy link
@driesvints

driesvints May 16, 2019

Member

@martinbean There's, for example, the if (static::$toMailCallback) { at the beginning which could let you do whatever you want with whatever implementation you have.

@taylorotwell taylorotwell merged commit d8a626c into laravel:5.8 May 13, 2019

2 checks passed

continuous-integration/styleci/pr The analysis has passed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@dwightwatson dwightwatson deleted the dwightwatson:reset-email branch May 14, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.