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

Change controller validate helper method to behave like Laravel (#1247) #1248

Merged
merged 8 commits into from
Sep 12, 2022

Conversation

MattApril
Copy link
Contributor

@driesvints
Copy link
Member

Tests are failing here.

@driesvints driesvints marked this pull request as draft September 9, 2022 14:13
@MattApril MattApril marked this pull request as ready for review September 9, 2022 14:39
@MattApril
Copy link
Contributor Author

Whoops, I forgot that I overwrote my own exception handler. Looks like Lumen's default handler excepts the response to be set on the ValidationException. Fixed.

@taylorotwell
Copy link
Member

Would this break existing applications?

@MattApril
Copy link
Contributor Author

Yes, it would cause a breaking change if anyone overwrote the protected extractInputFromRules or throwValidationException methods. I just updated it to use those methods if they are defined.

@taylorotwell taylorotwell merged commit 94445c4 into laravel:9.x Sep 12, 2022
@halloei
Copy link
Contributor

halloei commented Sep 15, 2022

This broke my application. I'm not overwriting extractInputFromRules() and throwValidationException(), I'm calling them directly from my controller.

@driesvints
Copy link
Member

We're reverting this.

taylorotwell pushed a commit that referenced this pull request Sep 15, 2022
@MattApril
Copy link
Contributor Author

Hmm, that was an unfortunate oversight. Sorry about that.

I think this should be released as a breaking change in the next major version though. The current validation is not behaving as expected.

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