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

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

Merged
merged 1 commit into from Jul 30, 2023

Conversation

guillaume-a
Copy link
Contributor

@guillaume-a guillaume-a commented Jul 6, 2023

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.

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

src/Symfony/Component/Validator/Constraints/Finite.php Outdated Show resolved Hide resolved
@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.

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

@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

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
@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

@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
7 of 9 checks passed
@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
…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Validator] Type constraint considers NAN and INF as floats
8 participants