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] add number constraints #28637

Merged
merged 1 commit into from Mar 31, 2019

Conversation

Projects
None yet
8 participants
@jschaedl
Copy link
Contributor

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 jschaedl:validator-number_constraints branch from ac485ee to 77ba93a Sep 29, 2018

@jschaedl jschaedl force-pushed the jschaedl:validator-number_constraints branch from 6f25560 to 5a92e1b Sep 29, 2018

@nicolas-grekas nicolas-grekas added this to the next milestone Sep 29, 2018

@ostrolucky

This comment has been minimized.

Copy link
Contributor

commented Sep 30, 2018

What about null values?

@jschaedl jschaedl force-pushed the jschaedl:validator-number_constraints branch from 5a92e1b to 0b6e408 Sep 30, 2018

@jschaedl

This comment has been minimized.

Copy link
Contributor Author

commented Sep 30, 2018

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

@xabbuh

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Contributor

commented Sep 30, 2018

like Positive extends Range?

@xabbuh

This comment has been minimized.

Copy link
Member

commented Sep 30, 2018

yes

@ostrolucky

This comment has been minimized.

Copy link
Contributor

commented Sep 30, 2018

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

@xabbuh

This comment has been minimized.

Copy link
Member

commented Sep 30, 2018

indeed, those fit better than Range

@jschaedl

This comment has been minimized.

Copy link
Contributor Author

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 jschaedl:validator-number_constraints branch from 73e1e81 to 43aef65 Oct 5, 2018

@jschaedl

This comment has been minimized.

Copy link
Contributor Author

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 jschaedl:validator-number_constraints branch from 43aef65 to 40aa6cd Oct 5, 2018

@ro0NL

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link
Contributor

commented Oct 5, 2018

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 jschaedl:validator-number_constraints branch 2 times, most recently from 84de93c to 63fa241 Nov 2, 2018

@OskarStark
Copy link
Contributor

left a comment

Nice improvement

@jschaedl jschaedl force-pushed the jschaedl:validator-number_constraints branch from 63fa241 to d2073e5 Nov 7, 2018

@ostrolucky
Copy link
Contributor

left a comment

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

@ro0NL

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link
Contributor

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 jschaedl:validator-number_constraints branch from 8f7388a to 2c07ac2 Nov 30, 2018

@jschaedl jschaedl force-pushed the jschaedl:validator-number_constraints branch 2 times, most recently from 7119701 to 35bfa5a Dec 1, 2018

@jschaedl jschaedl force-pushed the jschaedl:validator-number_constraints branch from 189d578 to 0e31c3b Mar 17, 2019

@jschaedl jschaedl force-pushed the jschaedl:validator-number_constraints branch from e476040 to c77ef3b Mar 22, 2019

@fabpot

fabpot approved these changes Mar 31, 2019

Copy link
Member

left a comment

Ready to be merged after the translation files are fixed.

@OskarStark

This comment has been minimized.

Copy link
Contributor

commented Mar 31, 2019

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

@jschaedl jschaedl force-pushed the jschaedl:validator-number_constraints branch 2 times, most recently from e718297 to c973cee Mar 31, 2019

@jschaedl

This comment has been minimized.

Copy link
Contributor Author

commented Mar 31, 2019

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

@fabpot fabpot force-pushed the jschaedl:validator-number_constraints branch from c973cee to 0187039 Mar 31, 2019

@fabpot

This comment has been minimized.

Copy link
Member

commented Mar 31, 2019

Thank you @jschaedl.

@fabpot fabpot merged commit 0187039 into symfony:master Mar 31, 2019

1 of 3 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
fabbot.io Your code looks good.
Details

fabpot added a commit that referenced this pull request Mar 31, 2019

feature #28637 [Validator] add number constraints (jschaedl)
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

@jschaedl jschaedl referenced this pull request Mar 31, 2019

Closed

[Validator] Add docs for number constraints #11254

5 of 5 tasks complete

javiereguiluz added a commit to symfony/symfony-docs that referenced this pull request Apr 5, 2019

minor #11254 [Validator] Add docs for number constraints (jschaedl)
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 referenced this pull request May 9, 2019

Merged

Release v4.3.0-BETA1 #31435

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.