8000 [Validator] Update `Type` constraint, add `number`, `finite-float` and `finite-number` validations by guillaume-a · Pull Request #50907 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[Validator] Update Type constraint, add number, finite-float and finite-number validations #50907

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jul 30, 2023
Merged

[Validator] Update Type constraint, add number, finite-float and finite-number validations #50907

merged 1 commit into from
Jul 30, 2023

Conversation

@guillaume-a
Copy link
Contributor
@guillaume-a guillaume-a commented Jul 6, 2023 •
edited
Loading
Q A
Branch? 6.4
Bug fix? no
New feature? yes
Deprecations? no
Tickets Fix #50782
License MIT
Doc PR symfony/symfony-docs#18527

Contraint Type(['float']) can produce positives with INF or NAN.
This new types car narrow validation and limit validation to finite numbers.

#[Assert\Type(['int', 'finite-float'])]
private $value;

Thank you for helping me with this one.

@carsonbot carsonbot added this to the 6.4 milestone Jul 6, 2023
@guillaume-a guillaume-a force-pushed the 50782 branch 3 times, most recently from 222910f to 71dd624 Compare July 6, 2023 19:16
@guillaume-a guillaume-a mentioned this pull request Jul 6, 2023
Merged
nicolas-grekas
nicolas-grekas reviewed Jul 7, 2023
View reviewed changes
stof
stof requested changes Jul 7, 2023
View reviewed changes
Copy link
Member
@stof stof left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is missing tests

@fabpot
Copy link
Member
fabpot commented Jul 7, 2023

@guillaume-a Can you fix fabbot errors?

@derrabus
Copy link
Member
derrabus commented Jul 8, 2023

Contraint Type(['float']) can produce false positives.

No, actually that constraint works as advertised because NaN and infinity are valid float values.

We should change the PR description.

derrabus
derrabus reviewed Jul 8, 2023
View reviewed changes
Copy link
Member
@derrabus derrabus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens if that validator encounters…

  • … a non-numeric string?
  • … null or an empty string?
  • … something completely different lika an array or object?

Can we cover those scenarios with tests?

@guillaume-a guillaume-a force-pushed the 50782 branch 3 times, most recently from 679acd9 to 7019c40 Compare July 8, 2023 08:35
@guillaume-a
Copy link
Contributor Author

Fixed fabbot errors.
Check against is_numeric() in the validator (in case of string, array, object $value)
Added tests scenarios for all thoses last checks.

null was allready checked (considered as valid)
empty is now checked and considered as invalid

javiereguiluz
javiereguiluz approved these changes Jul 10, 2023
View reviewed changes
@nicolas-grekas
Copy link
Member

What about adding new types to the Type constraint instead?

#[Assert\Type(['finite-float'])]
#[Assert\Type(['number'])] # this would be (is_int || is_float) && is_finite

@guillaume-a guillaume-a changed the title [Validator] Add Finite constraint [Validator] Add IsFinite constraint Jul 13, 2023
@guillaume-a
Copy link
Contributor Author

Renamed Finite to IsFinite to match existing validators IsTrue IsNull ...

@nicolas-grekas originally I looked that way, but IMO it will add lots of complexity to Type validator since $types are considered in a global 'OR' condition. And we should then check againts is_finite() only in some cases only which may be hard to deterime.

But i'm open to discussion.

@nicolas-grekas
Copy link
Member
nicolas-grekas commented Jul 13, 2023 •
edited
Loading

It looks like this would be enough to support my proposal. Not too much complexity IMHO.

--- a/src/Symfony/Component/Validator/Constraints/TypeValidator.php
+++ b/src/Symfony/Component/Validator/Constraints/TypeValidator.php
@@ -26,9 +26,11 @@ class TypeValidator extends ConstraintValidator
         'int' => 'is_int',
         'integer' => 'is_int',
         'long' => 'is_int',
+        'finite-float' => 'is_float && is_finite',
         'float' => 'is_float',
         'double' => 'is_float',
         'real' => 'is_float',
+        'number' => 'is_int || is_float && is_finite',
         'numeric' => 'is_numeric',
         'string' => 'is_string',
         'scalar' => 'is_scalar',
@@ -69,7 +71,13 @@ class TypeValidator extends ConstraintValidator
 
         foreach ($types as $type) {
             $type = strtolower($type);
-            if (isset(self::VALIDATION_FUNCTIONS[$type]) && self::VALIDATION_FUNCTIONS[$type]($value)) {
+            if (isset(self::VALIDATION_FUNCTIONS[$type]) && match (self::VALIDATION_FUNCTIONS[$type]) {
+                'finite-float' => \is_float($value) && \is_finite($value),
+                'number' => \is_int($value) || \is_float($value) && \is_finite($value),
+                default => self::VALIDATION_FUNCTIONS[$type]($value),
+            }) {
                 return;
             }

@guillaume-a guillaume-a changed the title [Validator] Add IsFinite constraint [Validator] Update Type constraint, add number, finite-float and finite-number validations Jul 13, 2023
nicolas-grekas
nicolas-grekas reviewed Jul 13, 2023
View reviewed changes
nicolas-grekas
nicolas-grekas reviewed Jul 13, 2023
View reviewed changes
@guillaume-a guillaume-a force-pushed the 50782 branch 2 times, most recently from c043d02 to 85d802f Compare July 13, 2023 16:39
nicolas-grekas
nicolas-grekas reviewed Jul 13, 2023
View reviewed changes
@guillaume-a guillaume-a force-pushed the 50782 branch 2 times, most recently from 38bfc13 to 4d1e2b5 Compare July 17, 2023 07:24
@guillaume-a
Copy link
Contributor Author

Can someone explain me why we must write \is_float but we must write is_nan or is_finite without ?
What are the exact rules ?

@nicolas-grekas
Copy link
Member

@guillaume-a check PHP-CS-Fixer/PHP-CS-Fixer#3048

@nicolas-grekas
Copy link
Member

(can you have a look at the test failure?)

@guillaume-a
Copy link
Contributor Author

(can you have a look at the test failure?)

Done. (And rebased too.)

@fabpot
Copy link
Member
fabpot commented Jul 30, 2023

Thank you @guillaume-a.

@fabpot fabpot merged commit f404704 into symfony:6.4 Jul 30, 2023
@guillaume-a guillaume-a deleted the 50782 branch July 31, 2023 06:43
OskarStark added a commit to symfony/symfony-docs that referenced this pull request Jul 31, 2023
@OskarStark
feature #18527 [Validator] Update Type constraint, add number, `f…
03d356b 
…inite-float` and `finite-number` validations (guillaume-a)

This PR was merged into the 6.4 branch.

Discussion
----------

[Validator] Update `Type` constraint, add `number`, `finite-float` and `finite-number` validations

Documentation for PR
* symfony/symfony#50907

Related to
* symfony/symfony#50782

This is my first sf-docs PR, please tell me if I did anything wrong.
Thank you.

Commits
-------

458fdd5 [Validator] Update `Type` constraint, add `number`, `finite-float` and `finite-number` validations
This was referenced Oct 21, 2023
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@derrabus derrabus derrabus left review comments

@fabpot fabpot fabpot approved these changes

@nicolas-grekas nicolas-grekas nicolas-grekas approved these changes

@stof stof Awaiting requested review from stof

@javiereguiluz javiereguiluz Awaiting requested review from javiereguiluz

+1 more reviewer

@ro0NL ro0NL ro0NL left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Feature Status: Reviewed Validator

Projects

None yet

Milestone

6.4

Development

Successfully merging this pull request may close these issues.

[Validator] Type constraint considers NAN and INF as floats

8 participants

@guillaume-a @fabpot @derrabus @nicolas-grekas @javiereguiluz @stof @ro0NL @carsonbot
0