-
Notifications
You must be signed in to change notification settings - Fork 11.1k
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
[10.x] Improve decimal shape validation #47954
Conversation
if (preg_match('/^[+-]?\d*\.?(\d*)$/', $value, $matches) !== 1) { | ||
return false; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The "dot" has been escaped here. I believe this was a mistake in the original PR.
The dot allows 1 of any character. I believe it should only be a literal .
.
Nice, you have fixed the concerns I had in #45356 (comment) and following, which didn't get interest then… With your code:
I noticed a caveat though: it doesn't make sure there is at least one digit, |
Empty strings would also pass the regex. BUT, there is the edit: Same with |
After trying various regex features, I figured out a nice way to validate everything without relying on a Let's make use of branch reset: if (preg_match('/^[+-]?(?|\d+(\.\d*)?|(\.\d+))$/', $value, $matches) !== 1) {
return false;
}
$decimals = isset($matches[1]) ? (strlen($matches[1]) - 1) : 0;
|
Also, leading/trailing spaces should be supported, for consistency with |
The decimal rules checks that a value is numeric and then checks the value against a regex.
The current decimal validation regex is expecting:
[+-]?
an optional positive / negative symbol\d*
zero or more digits.
any character, except new lines\d*
zero of more digitsAll of the following values would pass this regex:
However, we first check that the value is
numeric
in PHP's eyes, so that reduces the values that get checked against the regex to the following set:but scientific notation will also pass the
is_numeric
check. So we add the following to the possible values. You will note, however, that these do not match the regex:When the
preg_match
happens, it has0
results in$matches
, and the rule interprets this as the number having0
decimal places.I believe this is a bug and the decimal rule should in fact check that the string matches the expected pattern, i.e., the decimal rule fails for scientific notation.