Skip to content

[Validator] add number constraints #28637

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

Merged
merged 1 commit into from
Mar 31, 2019

Conversation

jschaedl
Copy link
Contributor

@jschaedl jschaedl commented Sep 29, 2018

Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #28608
License MIT
Doc PR symfony/symfony-docs#11254

I added the following constraints:

  • Positive
  • PositiveOrZero
  • Negative
  • NegativeOrZero

@jschaedl jschaedl force-pushed the validator-number_constraints branch from 6f25560 to 5a92e1b Compare September 29, 2018 18:43
@nicolas-grekas nicolas-grekas added this to the next milestone Sep 29, 2018
@ostrolucky
Copy link
Contributor

What about null values?

@jschaedl jschaedl force-pushed the validator-number_constraints branch from 5a92e1b to 0b6e408 Compare September 30, 2018 07:05
@jschaedl
Copy link
Contributor Author

@ostrolucky I think it should do nothing for null values. I will add a condition and a test.

@xabbuh
Copy link
Member

xabbuh commented Sep 30, 2018

I think we could save plenty of code here if the constraints were just specialised versions of the already existing Range constraint. WDYT?

@ro0NL
Copy link
Contributor

ro0NL commented Sep 30, 2018

like Positive extends Range?

@xabbuh
Copy link
Member

xabbuh commented Sep 30, 2018

yes

@ostrolucky
Copy link
Contributor

Better fit is GreaterThan*/LessThan*. No new validators would be required then

@xabbuh
Copy link
Member

xabbuh commented Sep 30, 2018

indeed, those fit better than Range

@jschaedl
Copy link
Contributor Author

jschaedl commented Oct 5, 2018

@ostrolucky Do you mean we don't need the new Positive*/Negative* validators at all and users should use the already existing GreaterThan*/LessThan* validators?

@jschaedl jschaedl force-pushed the validator-number_constraints branch from 73e1e81 to 43aef65 Compare October 5, 2018 08:40
@jschaedl
Copy link
Contributor Author

jschaedl commented Oct 5, 2018

@xabbuh @ro0NL I changed the PositiveValidator which is now extending the GreaterThanValidator. Is this what you had in mind?

@jschaedl jschaedl force-pushed the validator-number_constraints branch from 43aef65 to 40aa6cd Compare October 5, 2018 08:43
@ro0NL
Copy link
Contributor

ro0NL commented Oct 5, 2018

@jschaedl actually i expected it like:

class Postive extends GreaterThan {
   public function __construct() {
       parent::__construct(array('min' => 1));
   }
}

then Postive is validated by GreaterThanValidator as regular.

@ostrolucky
Copy link
Contributor

Yep, like @ro0NL says. Well, not exactly (validatedBy needs to be specified and minimum value maybe tweaked) but mostly just that.

@jschaedl jschaedl force-pushed the validator-number_constraints branch 2 times, most recently from 84de93c to 63fa241 Compare November 2, 2018 08:51
Copy link
Contributor

@OskarStark OskarStark left a comment

Choose a reason for hiding this comment

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

Nice improvement

@jschaedl jschaedl force-pushed the validator-number_constraints branch from 63fa241 to d2073e5 Compare November 7, 2018 07:17
Copy link
Contributor

@ostrolucky ostrolucky left a comment

Choose a reason for hiding this comment

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

Like I explained in #28637 (comment) since these constraints don't accept any default value, their constructor shouldn't default to null

@ro0NL
Copy link
Contributor

ro0NL commented Nov 16, 2018

IMHO it should, so we get the expected exception here https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Validator/Constraint.php#L136

Also we could make message the default option for these :)

@ostrolucky
Copy link
Contributor

ostrolucky commented Nov 16, 2018

These constraints are designed in a way they will never pass null to parent.

It would make some sense in case message is default property, but I don't think somebody would approve such change.

@jschaedl jschaedl force-pushed the validator-number_constraints branch from 8f7388a to 2c07ac2 Compare November 30, 2018 08:10
@jschaedl jschaedl force-pushed the validator-number_constraints branch 2 times, most recently from 7119701 to 35bfa5a Compare December 1, 2018 22:57
@jschaedl jschaedl force-pushed the validator-number_constraints branch from 189d578 to 0e31c3b Compare March 17, 2019 21:34
@jschaedl jschaedl force-pushed the validator-number_constraints branch from e476040 to c77ef3b Compare March 22, 2019 11:09
Copy link
Member

@fabpot fabpot left a comment

Choose a reason for hiding this comment

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

Ready to be merged after the translation files are fixed.

@OskarStark
Copy link
Contributor

@jschaedl could you please send a Docs PR for this new feature?
Thank you

@jschaedl jschaedl force-pushed the validator-number_constraints branch 2 times, most recently from e718297 to c973cee Compare March 31, 2019 16:48
@jschaedl
Copy link
Contributor Author

@OskarStark I am already working on it. :-)

@fabpot fabpot force-pushed the validator-number_constraints branch from c973cee to 0187039 Compare March 31, 2019 17:19
@fabpot
Copy link
Member

fabpot commented Mar 31, 2019

Thank you @jschaedl.

@fabpot fabpot merged commit 0187039 into symfony:master Mar 31, 2019
fabpot added a commit that referenced this pull request Mar 31, 2019
This PR was squashed before being merged into the 4.3-dev branch (closes #28637).

Discussion
----------

[Validator] add number constraints

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #28608
| License       | MIT
| Doc PR        | tbd.

I added the following constraints:
* `Positive`
* `PositiveOrZero`
* `Negative`
* `NegativeOrZero`

Commits
-------

0187039 [Validator] add number constraints
javiereguiluz added a commit to symfony/symfony-docs that referenced this pull request Apr 5, 2019
This PR was squashed before being merged into the master branch (closes #11254).

Discussion
----------

[Validator] Add docs for number constraints

Feature PR: symfony/symfony#28637

- [x] Positive
- [x] PositiveOrZero
- [x] Negative
- [x] NegativeOrZero

### Todo:

- [x] find a better example for `NegativeOrZero` constraint

Commits
-------

8022292 [Validator] Add docs for number constraints
@nicolas-grekas nicolas-grekas modified the milestones: next, 4.3 Apr 30, 2019
@fabpot fabpot mentioned this pull request May 9, 2019
@jschaedl jschaedl deleted the validator-number_constraints branch February 23, 2020 08:03
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.

8 participants