Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

[Validator] UpdateType constraint, addnumber,finite-float andfinite-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.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
fabpot merged 1 commit intosymfony:6.4fromguillaume-a:50782
Jul 30, 2023

Conversation

@guillaume-a
Copy link
Contributor

@guillaume-aguillaume-a commentedJul 6, 2023•
edited
Loading

QA
Branch?6.4
Bug fix?no
New feature?yes
Deprecations?no
TicketsFix#50782
LicenseMIT
Doc PRsymfony/symfony-docs#18527

ContraintType(['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

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

@guillaume-a Can you fix fabbot errors?

@derrabus
Copy link
Member

ContraintType(['float']) can produce false positives.

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

We should change the PR description.

Copy link
Member

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

OskarStark reacted with thumbs up emoji
@guillaume-aguillaume-aforce-pushed the50782 branch 3 times, most recently from679acd9 to7019c40CompareJuly 8, 2023 08:35
@guillaume-a
Copy link
ContributorAuthor

Fixed fabbot errors.
Check againstis_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-aguillaume-a changed the title[Validator] AddFinite constraint[Validator] AddIsFinite constraintJul 13, 2023
@guillaume-a
Copy link
ContributorAuthor

RenamedFinite toIsFinite to match existing validatorsIsTrueIsNull ...

@nicolas-grekas originally I looked that way, but IMO it will add lots of complexity toType validator since$types are considered in a global 'OR' condition. And we should then check againtsis_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 commentedJul 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-aguillaume-a changed the title[Validator] AddIsFinite constraint[Validator] UpdateType constraint, addnumber,finite-float andfinite-number validationsJul 13, 2023
@guillaume-aguillaume-aforce-pushed the50782 branch 2 times, most recently fromc043d02 to85d802fCompareJuly 13, 2023 16:39
@guillaume-aguillaume-aforce-pushed the50782 branch 2 times, most recently from38bfc13 to4d1e2b5CompareJuly 17, 2023 07:24
@guillaume-a
Copy link
ContributorAuthor

Can someone explain me why we must write\is_float but we must writeis_nan oris_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 reacted with thumbs up emoji

@guillaume-a
Copy link
ContributorAuthor

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

Done. (And rebased too.)

@fabpot
Copy link
Member

Thank you@guillaume-a.

guillaume-a reacted with hooray emoji

@fabpotfabpot merged commitf404704 intosymfony:6.4Jul 30, 2023
@guillaume-aguillaume-a deleted the 50782 branchJuly 31, 2023 06:43
OskarStark added a commit to symfony/symfony-docs that referenced this pull requestJul 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` validationsDocumentation for PR*symfony/symfony#50907Related to*symfony/symfony#50782This 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 referencedOct 21, 2023
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@derrabusderrabusderrabus left review comments

@fabpotfabpotfabpot approved these changes

@nicolas-grekasnicolas-grekasnicolas-grekas approved these changes

@stofstofAwaiting requested review from stof

@javiereguiluzjaviereguiluzAwaiting requested review from javiereguiluz

+1 more reviewer

@ro0NLro0NLro0NL left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

6.4

Development

Successfully merging this pull request may close these issues.

[Validator]Type constraint considersNAN andINF as floats

8 participants

@guillaume-a@fabpot@derrabus@nicolas-grekas@javiereguiluz@stof@ro0NL@carsonbot
[8]ページ先頭
©2009-2025 Movatter.jp