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] Make inequality validation fail on different types rather than 500 #28174

Conversation

Projects
None yet
5 participants
@JonathanGawrych
Copy link

commented Apr 10, 2019

There are four types of inequality validators: lt, lte, gt, gte. If these validators reference another attribute, the attribute must be of the same type. Before this change, if the payload has different types, an error is thrown and if uncaught, a 500 is return to the user. However, the type is not determine by the rule configuration, but rather the HTTP request. It shouldn't be an Internal Server Error, but rather be a validation failure.

This change fixes the behaviour by returning false if the types are different. Tests cases were added for both a different type ('int' != 'string') and if the related attribute was missing entirely. This is not a breaking change, because the validator is handling more than it did before, so I've targeted 5.8 as taylorotwell said in #28168

Jonathan Gawrych
Make inequality validation fail on different types rather than 500
There are four types of inequality validators: lt, lte, gt, gte.
If these validators reference another attribute, the attribute
must be of the same type. Before this change, if the payload has
different types, a 500 error is given. However, the type is not
determine by the rule configuration, but rather the HTTP request.
Instead of throwing an error, just fail the validation.
@devcircus

This comment has been minimized.

Copy link
Contributor

commented Apr 10, 2019

IMO although this could be considered a bug fix, it seems like a serious behavior change for those using these validations and will inevitably break apps.

It’s debatable whether this should be changed but if so I think it should go to master.

@ankurk91

This comment has been minimized.

Copy link
Contributor

commented Apr 11, 2019

i would like this to be merged in 5.8

@JonathanGawrych

This comment has been minimized.

Copy link
Author

commented Apr 11, 2019

@devcircus I could retarget into master, but I'd prefer 5.8. But, whatever is best for Laravel and its users.
However, I would be very surprised if this breaks anyone. They would have to be running the validator, using an inequality evaluation, and expecting a InvalidArgumentException or be looking for a 500. This compared to the validator's normal call to failedValidation which if not overridden would throw a ValidationException with a redirect. Maybe I'm a little bit naive on this, but it feels like the current behavior isn't a scenario that people wouldn't desire or code against. Right now we are writing a workaround to manually call failValidation, but that didn't seem like the right solution, thus the PR :)

@dwightwatson

This comment has been minimized.

Copy link
Contributor

commented Apr 11, 2019

I noticed this error popping up a lot recently, having not seen it before (from memory). I think a validation failure is the excepted outcome and that this fix should go straight into 5.8.

@taylorotwell taylorotwell merged commit 9cf256c into laravel:5.8 Apr 11, 2019

2 checks passed

continuous-integration/styleci/pr The analysis has passed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
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.