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

Validation rule multiple_of fails on many cases with decimal step values #34960

Closed
tontonsb opened this issue Oct 25, 2020 · 9 comments
Closed
Assignees

Comments

@tontonsb
Copy link
Contributor

tontonsb commented Oct 25, 2020

  • Laravel Version: 8.11.2
  • PHP Version: 7.4.3

Description:

The multiple_of validation rule fails in all those cases where PHP's fmod works in an unexpected way.

The initial PR #34788 intended to make this a counterpart of HTML's step:

This validation rule can be useful if you use a number input with a step attribute.

But as it currently works it does not mirror the behaviour of HTML's step on contemporary browsers, for example

<input type=number min=0 step=0.3 >

allows me to enter 0.9, but it fails the multiple_of:0.3 validation.

Steps To Reproduce:

See #34959 for failing tests.

Solutions?

I could attempt to implement a solution, but I am not sure what Laravel wants to have here. There's a lot of possible directions

  • Leave it as is and claim to only support integer steps
  • Implement a direct and more correct computation instead of using fmod
  • Add a more proper fmod replacement to helpers
  • Use a third party library that does this correctly (I don't think there's anything native in PHP that solves this) like https://github.com/simplito/bn-php

And do we want to support the HTML's behaviour that step is evaluated relative to min? I.e. if min=10 step=3 HTML would allow 10, 13, 16, ... while Laravel's validator with 'min:10', 'multiple_of:3' would allow 12, 15, ...

@timacdonald
Copy link
Member

😩 This is terribly embarrassing. I cannot believe I let these issues slip through!!

Thank you very much @tontonsb for raising them. I would also love to help do anything I can to get this back into a good state for Laravel.

I can only put these mistakes on my end down to my late night hacking and am very sorry to have PR'd this with such glaring holes in the implementation and test data! FML.

I can't dive into this right this second, but will be able to help hack on solutions in the evenings this week if we want to push forward with any of the proposed solutions. I'll also see if I can think of any other solutions that we can implement to make this work as expected.

My apologies to the Laravel team and to anyone hitting this issue.

@timacdonald
Copy link
Member

timacdonald commented Oct 25, 2020

Perhaps we should hotfix core and the docs so that it explicity requires integers and then we can roll out float support later this week.

@tontonsb
Copy link
Contributor Author

Don't worry @timacdonald this is not about you. This is about getting the rule working properly ;)

I think an "only for integers" note on docs might be enough at the moment.

It seems that at least one of the browsers solves the floating arithmetic issues by allowing a small error:
https://github.com/chromium/chromium/blob/master/third_party/blink/renderer/core/html/forms/step_range.cc#L168

@driesvints driesvints added the bug label Oct 26, 2020
@driesvints
Copy link
Member

Think a note to the docs might indeed be enough for now.

@driesvints
Copy link
Member

driesvints commented Oct 26, 2020

laravel/docs#6526

Gonna wait to see what Taylor says about marking this as a bug or not. Imo this can be closed once that note to the docs is merged in. @timacdonald you might want to update your description on the PR so people don't get confused.

@taylorotwell
Copy link
Member

It seems like we would probably want to support this... is it possible to just commit a fix directly to the validation rule without adding any additional helpers or pulling in any additional libraries?

@timacdonald
Copy link
Member

Waking up to this all being resolved is amazing. You all rock. I am sorry I wasn’t around to help with the final fix.

I really appreciate your time and input to fix this one @tontonsb

@driesvints i don’t think it is needed now, but if you’d like me to update my PR description still, please let me know.

@timacdonald
Copy link
Member

timacdonald commented Oct 26, 2020

@tontonsb regarding a min value on an input, I don’t we should rely on the standard min validation and change multiple of functionality.

I was thinking multiple_of should probably accept a second parameter “starting from” which would allow for more rich validation constraints with a mixture of min and multiple of.

Min: 3
Multiple of: 0.3, 2.8

2.8 not allowed due to MIN)
3 not allowed due to multiple of
3.1 is allowed

What are your thoughts on this?

@tontonsb
Copy link
Contributor Author

@timacdonald that's a good idea. It would support the same features as HTML's step while not adding another rule or making multiple_of magically counterintuitive.

I would only suggest to call that parameter something else, maybe offset. Because starting from might imply it's a minimum, but we shouldn't enforce it (and it would be harder to do). I.e. multiple_of:1,.5 should accept n + .5 for any integer n if there's no explicit min constraint. At least I would find it more useful.

Btw, if you are going to tackle this, be careful to provide bcmod with string input. I came across some issues when checking if it would be appropriate replacement for fmod.

For example, bcmod('.0000015', '.000001', 16) is '0.0000005000000000' as expected, but bcmod(.0000015, '.000001', 16) is 0.0000000000000000 and a PHP warning because (string) .0000015 is '1.5E-6'.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants