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.7] Use Request::validate() macro in Auth traits #26314

Merged

Conversation

Projects
None yet
5 participants
@cmorbitzer
Copy link
Contributor

commented Oct 30, 2018

The traits AuthenticatesUsers, ResetsPasswords, and SendsPasswordResetEmails currently call ValidatesRequests::validate() in order to validate request data. However, ValidatesRequests is not listed as a dependency of these traits and even though the base controller of the default Laravel installation includes this trait, it is technically optional.

This PR uses the Request::validate() function that is macroed in FoundationServiceProvider since 5.5 (#19063) because that is more likely to be available.

This is technically a breaking change because it's possible for an installation not to include FoundationServiceProvider. However, all examples of validation in the Laravel documentation use this method instead and it is much more likely that a developer would remove the ValidatesRequests trait from the base controller for disuse than for the FoundationServiceProvider to be removed, especially considering that there is no mention of FoundationServiceProvider or of removing the framework service providers in config/app.php in the documentation. Still, I'm willing to resubmit this for 5.8.

@taylorotwell taylorotwell merged commit d1dcac5 into laravel:5.7 Oct 30, 2018

2 checks passed

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

This comment has been minimized.

Copy link
Contributor

commented Oct 31, 2018

This change will silently stop calling people's overridden LoginController::validate method. It's basically a huge silent change in behavior. Code that has worked for ages are suddenly no longer invoked, and custom validation of user-data in authentication controllers is no longer performed. There's no exception, nothing logged, that the custom security checks implemented for the last few years (since Laravel 5.0) is no longer checked.

A breaking change doesn't mean that the new code will not work. This is not only a breaking change because the small possibility that the FoundationServiceProvider is missing; it's a enormous breaking change because any existing application logic needs to be refactored into new places to have effect again.

@cmorbitzer

This comment has been minimized.

Copy link
Contributor Author

commented Oct 31, 2018

I'm sympathetic and willing to submit this for 5.8.

However, in defense of the change, overriding ValidatesRequests::validate doesn't seem like such a good idea even for only one controller since the under-the-hood mechanics of that method are undocumented and subject to change. Every tutorial I've seen so far on how to customize the login input validation recommends overriding the validateLogin/validateEmail methods instead, which won't break with this change.

@cmorbitzer cmorbitzer deleted the cmorbitzer:use-request-validate-macro-in-auth-traits branch Oct 31, 2018

@rekrios

This comment has been minimized.

Copy link

commented Nov 9, 2018

rly? how can developers make such changes between minor versions?

@antimech

This comment has been minimized.

Copy link
Contributor

commented Nov 13, 2018

I am confused. Why is it a minor change?

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.