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

Non-digits allowed in digits_between #42326

Closed
taai opened this issue May 10, 2022 · 6 comments
Closed

Non-digits allowed in digits_between #42326

taai opened this issue May 10, 2022 · 6 comments
Labels

Comments

@taai
Copy link
Contributor

taai commented May 10, 2022

  • Laravel Version: 9.11.0

Description:

Meaning of digit: any one of the ten numbers 0 to 9

A dot (.) is not a digit.

The problem is that the validation rule digits_between accepts fractions (a dot), multiple dots and even dots only (....). 🤣

Note that the validation rule digits does not accept fractions (that is good) and that makes an inconsistency between these two validation rules.

Please, let the digits stay digits! In my opinion, this PR should be rolled back.

I suggest to make a separate and a valid validation rule for fractions. Name it floats and floats_between, or something like that.

This is how validation rule digits_between looks like in Laravel v9.11.0 (look at preg_match):

public function validateDigitsBetween($attribute, $value, $parameters)
{
    $this->requireParameterCount(2, $parameters, 'digits_between');

    $length = strlen((string) $value);

    return ! preg_match('/[^0-9.]/', $value)
                && $length >= $parameters[0] && $length <= $parameters[1];
}

I found that this validation rule looks like that in these versions: v6.20.44, v8.79.0 (and newer, including v9.x.x)

This was fun. No hard feelings about my presentation of the issue, I hope. 😉

Steps To Reproduce:

$data = [
    'pin' => '1234.78',
    // 'pin' => '1234..78', // ok, weird
    // 'pin' => '....',     // OMG
];

$validator = \Illuminate\Support\Facades\Validator::make($data, [
    'pin' => 'digits_between:4,8',
]);

if (!$validator->fails()) {
    echo 'These are not the digits you are looking for.';
} else {
    echo 'When you see this, the validation rule digits_between is now fixed.';
}
@driesvints
Copy link
Member

I don't think we're going to change this any time soon, sorry.

@driesvints
Copy link
Member

Also see the original issue for that: #40264. This should be a numeric value. Maybe digit isn't an 100% word but the rule atm does what it's suppose to do.

@driesvints
Copy link
Member

Ah the multiple dots thing is a concern. I'll send in a PR for that.

@driesvints
Copy link
Member

I sent in a PR for this: #42330

@taai
Copy link
Contributor Author

taai commented May 10, 2022

@driesvints I disagree.

Which one is it then – a digit or a numeric?

  • In the code there is a comment for that validation rule: Validate that an attribute is between a given number of digits.
  • In the Laravel's documentation it says The field under validation must be numeric and must have a length between the given min and max.

More arguments:

  • Previously there were no unit tests for fractions in unit tests for validation rule digits_between – I take that as a sign that this validation rule was never made for fractions, only for digits (as the name suggests).
  • There is validation rule numeric – it can be used in combination with validation rules string and between
  • So what if the documentation said numeric? The code (and the comment in docblocks * ) says it is digits. Documentation is sometimes wrong. A "digit" is a term even outside programming.
  • So what if the author of the issue was exploiting the validation rule in a way it was not built for?!
  • Apparently, the term "numeric" has been used in PHP documentation when talking about only digits.
  • As I understand, the validation rule was created very long time ago by @taylorotwell - maybe it would be better to ask him what to do with this validation rule...

* In programming, a docblock or DocBlock is a specially formatted comment specified in source code that is used to document a specific segment of code.

@driesvints
Copy link
Member

I'm sorry but I'm not going to change my mind over this. Changing this would be a large breaking change for many apps. It's only terminology and it's clearly documented. If you want, you can attempt a PR that adds a rule that does exactly what you want.

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

No branches or pull requests

2 participants