Skip to content

[9.x] Add test cases for multiple_of validation rule #34959

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

Closed
wants to merge 5 commits into from

Conversation

tontonsb
Copy link
Contributor

@tontonsb tontonsb commented Oct 25, 2020

Some of the test cases were not conforming to step constraint in HTML and (in my opinion) not correct per se, i.e. 10.5 is a multiple of .3 as much as it is a multiple of .5.

I also added a few more test cases where the behaviour of multiple_of is not what one would expect and not how HTML validation handles them (emphasis mine):

Constraint validation: When the element has an allowed value step, and the result of applying the algorithm to convert a string to a number to the string given by the element's value is a number, and that number subtracted from the step base is not an integral multiple of the allowed value step, the element is suffering from a step mismatch.

Currently the validation rule fails some cases of recognizing that the value is "an integral multiple of the allowed value step" (because PHP's fmod fails that) and doesn't even attempt to tie the check together with min.

https://html.spec.whatwg.org/multipage/input.html#the-step-attribute

@GrahamCampbell GrahamCampbell marked this pull request as draft October 25, 2020 23:21
@driesvints driesvints changed the title Add test cases for multiple_of validation rule [9.x] Add test cases for multiple_of validation rule Oct 26, 2020
@tontonsb
Copy link
Contributor Author

@driesvints Is this is really 9.x? The functionality and the erroneous & misleading test cases are already shipped in Laravel 8.

@driesvints
Copy link
Member

@tontonsb you've sent this to master which is for the upcoming Laravel 9 release.

@tontonsb
Copy link
Contributor Author

@driesvints sorry, double fail on my side 🥇

@driesvints
Copy link
Member

Btw you're free to send this to 8.x if you want.

@driesvints
Copy link
Member

Closing this one as the other PR got merged.

@driesvints driesvints closed this Oct 26, 2020
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.

2 participants