[Validator] Add CollectionSize constraint #4149

Herzult · 2012-04-28T21:40:06Z

Bug fix: no
Feature addition: yes
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: -
Todo: -

I will also send a PR to the documentation as soon as this one is accepted.

webmozart · 2012-04-29T08:24:28Z

-1

I dislike the rising amount of very specific constraints in the core. Can't we add this to Size?

vicb · 2012-04-29T09:01:39Z

@bschussek #3918 implements what you propose but then the messages are not valid any more:

<?php
    public $minMessage = 'This value should be {{ limit }} or more';
    public $maxMessage = 'This value should be {{ limit }} or less';
    public $invalidMessage = 'This value should be a valid number';

I can imagine 2 solutions:

adding some more message,
rename the Size constraint to Range and create a new Size constraint for arrays / countables.

What do you think ?

webmozart · 2012-04-29T09:27:53Z

I'd prefer the second solution and merge Size with SizeLength as well.

vicb · 2012-04-29T09:34:50Z

@bschussek It would make sense. @makasim @Herzult any one of you would like to contribute this (i.e. rename the current Size to Range and create a new Size supporting arrays / countables / strings) ?

Herzult · 2012-04-29T14:31:12Z

Yep, I'm on it.

stof · 2012-04-29T15:22:44Z

@Herzult could you take the other comment into account and merge SizeLength into you Size ?

vicb · 2012-04-29T15:33:05Z

The guessers should also be modified (it might also affect the ODM which is in an other repo, if so it would be good to sync the changes).

Herzult · 2012-04-29T16:38:19Z

@stof the problem merging SizeLength into Size is that they don't have the same required options & messages.

Herzult · 2012-04-29T16:47:40Z

And what about renaming Range to Interval and SizeLength to IntervalLength?

stof · 2012-04-29T16:54:38Z

Well, SizeLength is about matching the length of a string currently. Nothing related to intervals

Herzult · 2012-04-29T17:29:40Z

Here are the current names:

Size for collection (countable) size
Range for numbers
SizeLength for strings

Merging SizeLength into Size is maybe not appropriate because collections and strings are different things. It'll be hard to find messages that fit both collections and strings. Maybe we had better to find a better name for both. What do you think?

About the ValidatorTypeGuesser, I'll update it as soon as we know ow to name the constraints.

vicb · 2012-04-29T17:43:01Z

Size is a good name for both strings and "collections", could we have two sets of strings and select according to the type ?

Herzult · 2012-04-29T22:39:55Z

I tried to merge them together, what do you think?

vicb · 2012-04-30T06:41:00Z

src/Symfony/Component/Validator/Constraints/Size.php

+
+    private $stringMinMessage   = 'This value is too short. It should have {{ limit }} characters or more.';
+    private $stringMaxMessage   = 'This value is too long. It should have {{ limit }} characters or less.';
+    private $stringExactMessage = 'This value should have exactly {{ limit }} characters.';


"characters" is not consistent with "element(s)"

I can either replace characters by character(s) or replace element(s) by elements.
The first solution makes more sense to me but also implies some little changes in many places

update every existing message to be consistant

maybe use pluralization in the translations

What do you think?

may be "elements" not to change to many things in this PR

vicb · 2012-04-30T06:52:37Z

I think your changes are great, may be @bschussek has more feedback. The ValidatorTypeGuesser and the translation are yet to be updated.

stof · 2012-04-30T15:51:43Z

src/Symfony/Component/Validator/Constraints/SizeValidator.php

+    {
+        if (null === $value || is_scalar($value)) {
+            return Size::TYPE_STRING;
+        } elseif (is_object($value) && method_exists($value, '__toString')) {


use if as the previous if returns

hhamon · 2012-05-01T12:32:28Z

Am I missing something or SizeLength for strings is a duplicate for MinLength and MaxLength constraints?

Herzult · 2012-05-02T13:29:36Z

Yep, that's true. But the only link between this PR and the SizeLength constraint is that I merged it to the one I introduced.

Herzult · 2012-05-07T07:48:01Z

@bschussek what do you think?

vicb · 2012-05-10T19:49:40Z

src/Symfony/Component/Form/Extension/Validator/ValidatorTypeGuesser.php

@@ -224,7 +235,19 @@ public function guessPatternForConstraint(Constraint $constraint)
            case 'Symfony\Component\Validator\Constraints\MinLength':
                return new ValueGuess(sprintf('.{%s,}', (string) $constraint->limit), Guess::LOW_CONFIDENCE);

-            case 'Symfony\Component\Validator\Constraints\SizeLength':
+            case 'Symfony\Component\Validator\Constraints\Size':
+                if ($constraint->min === $constraint->max) {


Should you check the type ?

Is is valid for collections ?

vicb · 2012-05-10T19:51:26Z

@Herzult this PR looks good to me, could you update the changelog and update guides, try to factorize the code and squash the commits ? Thanks.

travisbot · 2012-05-11T15:42:35Z

This pull request passes (merged 8d8e6443 into 4ac3bdd).

vicb · 2012-05-11T21:42:21Z

could Plural validators #4259 be helpful ?
please squash the commits.
please create a PR / issue on symfony-docs

thanks for the updates.

travisbot · 2012-05-13T18:38:18Z

This pull request fails (merged eeda9044 into 4ac3bdd).

travisbot · 2012-05-13T18:45:12Z

This pull request passes (merged 491ca19a into 8b54eb5).

travisbot · 2012-05-14T11:29:39Z

This pull request passes (merged 44865024 into 8b54eb5).

vicb · 2012-05-14T11:49:37Z

@Herzult what about plural translations ?

travisbot · 2012-05-14T16:52:37Z

This pull request passes (merged 93480f95 into 46ffbd5).

travisbot · 2012-05-14T17:03:13Z

This pull request passes (merged 326c3b81 into 46ffbd5).

vicb · 2012-05-14T20:19:18Z

thanks for the updates, this PR looks fine to me. @bschussek ?

vicb · 2012-05-16T06:45:51Z

@Herzult can you squash your commits ?

[Validator] Rename constraint Size to Range [Validator] Rename constraint CollectionSize to Size [Validator] Merge the SizeLength into the Size constraint [Validator] Update messages in Size constraint for consistancy [Validator] Add english and french translation for Size messages [Validator] Tweak expected types for exceptions in SizeValidator [Validator] Fix CS in SizeValidator [Validator] Update the ValidatorTypeGuesser [Validator] Tweak SizeValidator [Validator] Update CHANGELOG [Validator] Complete previous CHANGELOG updates [Form] Update validator type guesser [Validator] Pluralize collection size english messages [Validator] Pluralize Size french messages

travisbot · 2012-05-16T11:20:44Z

This pull request passes (merged 3a5e84f into 58b6ef2).

@makasim

Commits ------- 3a5e84f [Validator] Add CollectionSize constraint Discussion ---------- [Validator] Add CollectionSize constraint Bug fix: no Feature addition: yes Backwards compatibility break: no Symfony2 tests pass: yes Fixes the following tickets: - Todo: - I will also send a PR to the documentation as soon as this one is accepted. --------------------------------------------------------------------------- by bschussek at 2012-04-29T08:24:28Z -1 I dislike the rising amount of very specific constraints in the core. Can't we add this to Size? --------------------------------------------------------------------------- by vicb at 2012-04-29T09:01:39Z @bschussek #3918 implements what you propose but then the messages are not valid any more: ```php <?php public $minMessage = 'This value should be {{ limit }} or more'; public $maxMessage = 'This value should be {{ limit }} or less'; public $invalidMessage = 'This value should be a valid number'; ``` I can imagine 2 solutions: - adding some more message, - rename the `Size` constraint to `Range` and create a new `Size` constraint for arrays / countables. What do you think ? --------------------------------------------------------------------------- by bschussek at 2012-04-29T09:27:53Z I'd prefer the second solution and merge `Size` with `SizeLength` as well. --------------------------------------------------------------------------- by vicb at 2012-04-29T09:34:50Z @bschussek It would make sense. @makasim @Herzult any one of you would like to contribute this (i.e. rename the current Size to Range and create a new Size supporting arrays / countables / strings) ? --------------------------------------------------------------------------- by Herzult at 2012-04-29T14:31:12Z Yep, I'm on it. --------------------------------------------------------------------------- by stof at 2012-04-29T15:22:44Z @Herzult could you take the other comment into account and merge SizeLength into you Size ? --------------------------------------------------------------------------- by vicb at 2012-04-29T15:33:05Z The guessers should also be modified (it might also affect the ODM which is in an other repo, if so it would be good to sync the changes). --------------------------------------------------------------------------- by Herzult at 2012-04-29T16:38:19Z @stof the problem merging SizeLength into Size is that they don't have the same required options & messages. --------------------------------------------------------------------------- by Herzult at 2012-04-29T16:47:40Z And what about renaming Range to Interval and SizeLength to IntervalLength? --------------------------------------------------------------------------- by stof at 2012-04-29T16:54:38Z Well, SizeLength is about matching the length of a string currently. Nothing related to intervals --------------------------------------------------------------------------- by Herzult at 2012-04-29T17:29:40Z Here are the current names: * **Size** for collection (countable) size * **Range** for numbers * **SizeLength** for strings Merging **SizeLength** into **Size** is maybe not appropriate because collections and strings are different things. It'll be hard to find messages that fit both collections and strings. Maybe we had better to find a better name for both. What do you think? About the ValidatorTypeGuesser, I'll update it as soon as we know ow to name the constraints. --------------------------------------------------------------------------- by vicb at 2012-04-29T17:43:01Z Size is a good name for both strings and "collections", could we have two sets of strings and select according to the type ? --------------------------------------------------------------------------- by Herzult at 2012-04-29T22:39:55Z I tried to merge them together, what do you think? --------------------------------------------------------------------------- by vicb at 2012-04-30T06:52:37Z I think your changes are great, may be @bschussek has more feedback. The ValidatorTypeGuesser and the translation are yet to be updated. --------------------------------------------------------------------------- by hhamon at 2012-05-01T12:32:28Z Am I missing something or `SizeLength` for strings is a duplicate for `MinLength` and `MaxLength` constraints? --------------------------------------------------------------------------- by Herzult at 2012-05-02T13:29:36Z Yep, that's true. But the only link between this PR and the SizeLength constraint is that I merged it to the one I introduced. --------------------------------------------------------------------------- by Herzult at 2012-05-07T07:48:01Z @bschussek what do you think? --------------------------------------------------------------------------- by vicb at 2012-05-10T19:51:26Z @Herzult this PR looks good to me, could you update the changelog and update guides, try to factorize the code and squash the commits ? Thanks. --------------------------------------------------------------------------- by travisbot at 2012-05-11T15:42:35Z This pull request [passes](http://travis-ci.org/symfony/symfony/builds/1306112) (merged 8d8e6443 into 4ac3bdd). --------------------------------------------------------------------------- by vicb at 2012-05-11T21:42:21Z * could #4259 be helpful ? * please squash the commits. * please create a PR / issue on [symfony-docs](https://github.com/symfony/symfony-docs) thanks for the updates. --------------------------------------------------------------------------- by travisbot at 2012-05-13T18:38:18Z This pull request [fails](http://travis-ci.org/symfony/symfony/builds/1321123) (merged eeda9044 into 4ac3bdd). --------------------------------------------------------------------------- by travisbot at 2012-05-13T18:45:12Z This pull request [passes](http://travis-ci.org/symfony/symfony/builds/1321146) (merged 491ca19a into 8b54eb5). --------------------------------------------------------------------------- by travisbot at 2012-05-14T11:29:39Z This pull request [passes](http://travis-ci.org/symfony/symfony/builds/1326110) (merged 44865024 into 8b54eb5). --------------------------------------------------------------------------- by vicb at 2012-05-14T11:49:37Z @Herzult what about plural translations ? --------------------------------------------------------------------------- by travisbot at 2012-05-14T16:52:37Z This pull request [passes](http://travis-ci.org/symfony/symfony/builds/1328677) (merged 93480f95 into 46ffbd5). --------------------------------------------------------------------------- by travisbot at 2012-05-14T17:03:13Z This pull request [passes](http://travis-ci.org/symfony/symfony/builds/1328705) (merged 326c3b81 into 46ffbd5). --------------------------------------------------------------------------- by vicb at 2012-05-14T20:19:18Z thanks for the updates, this PR looks fine to me. @bschussek ? --------------------------------------------------------------------------- by vicb at 2012-05-16T06:45:51Z @Herzult can you squash your commits ? --------------------------------------------------------------------------- by travisbot at 2012-05-16T11:20:44Z This pull request [passes](http://travis-ci.org/symfony/symfony/builds/1344811) (merged 3a5e84f into 58b6ef2).

Tobion · 2012-05-20T20:29:30Z

Do I see it correctly, that the validators min and max duplicate range?
And MinLength and MaxLength duplicate Size? So we basically have 4 unneeded constraints+validators. So 8 redundant classes. I guess they only remain in symfony for BC?

vicb · 2012-05-21T05:32:01Z

Not exactly:

Range requires both min & max,
Size validates both strings & collections (you should specify min, max or both)

Tobion · 2012-05-21T18:13:11Z

Well, one could adjust Range to only require one option. So the min and max validators would not be needed anymore.
And you can achieve with Size everything that you could achieve with MinLength and MaxLength. So they are redundant.
From a users perspective this is irritating as they might ask themselves what to use.

stof · 2012-05-21T18:26:51Z

Well, Range will only one option seems weird. It is not a range anymore. And Size cannot be replaced by MinLength and MaxLength (which only support strings, not traversable objects). It can replace them however (so MinLength and MaxLength should probably be deprecated, but not removed because of BC)

Tobion · 2012-05-21T18:40:39Z

@stof that's what I'm saying. Please read carefully.
And a range with one option is still a range: [min, ∞], [0, max]

vicb · 2012-05-21T21:19:40Z

@Tobion even you get confused, it should not be 0 ;)
I agree with @stof that MinLength / MaxLength could be deprecated.

Tobion · 2012-05-21T21:32:46Z

I think we should either deprecate MinLength / MaxLength and Min / Max (when range works with only one option) or none.
Either we have specialized validators (for easier use) that duplicate more generic validators or not.
@vicb I havent looked at the implementation of min/max. Should it be [-∞, max]?

webmozart · 2012-05-24T16:40:37Z

Sorry for being late to the party, I was occupied with the forms and didn't pay attention here. What's the point of Range? I don't see any benefit in that constraint.

Second, what's the benefit of Size over Min/MinLength? I feel rather tempted to deprecate Size.

Third, why is Range marked with @api? New features should not be tagged as stable.

stof · 2012-05-24T16:57:52Z

MinLength and MaxLength only works for strings, not countable objects. And you were the one asking to introduce Size in 2.1: #2674

webmozart · 2012-05-24T17:02:49Z

I'm actually feeling pretty silly now. MinCount and MaxCount sounds much more reasonable, seing that we duplicate MinLength, MaxLength, Min and Max with Range and Size (that seems overly complicated to me). Oh man.

What do you think?

webmozart · 2012-05-24T17:05:02Z

As a follow-up, whatever the solution, I think we need to focus on

either Range or Min/Max
either Length or MinLength/MaxLength
either Count or MinCount/MaxCount

I guess I was doing too much Java when I made that statement in the other thread.

Tobion · 2012-05-24T19:47:13Z

+1 for Range, Length, Count (without Min.../Max...)

I think the benefit of having only Range (instead of Min, Max individually) is that you can better adjust the error message like "Value should be between x and y", instead of "Should be higher than x" or "Should be lower than y".

vicb reviewed Apr 30, 2012
View reviewed changes

stof reviewed Apr 30, 2012
View reviewed changes

vicb reviewed May 10, 2012
View reviewed changes

Herzult mentioned this pull request May 13, 2012

Size validator symfony/symfony-docs#1362

Merged

vicb mentioned this pull request May 16, 2012

[2.1][Validator] Update the constraints doc symfony/symfony-docs#1371

Closed

fabpot merged commit 3a5e84f into symfony:master May 16, 2012

This was referenced May 20, 2012

[Validator] what about to have MinCount and MaxCount validators #2674

Closed

[Validator] NotBlank constraint not work with ManyToMany relation #3518

Closed

Uh oh!

[Validator] Add CollectionSize constraint #4149

[Validator] Add CollectionSize constraint #4149

Uh oh!

Conversation

Herzult commented Apr 28, 2012

Uh oh!

webmozart commented Apr 29, 2012

Uh oh!

vicb commented Apr 29, 2012

Uh oh!

webmozart commented Apr 29, 2012

Uh oh!

vicb commented Apr 29, 2012

Uh oh!

Herzult commented Apr 29, 2012

Uh oh!

stof commented Apr 29, 2012

Uh oh!

vicb commented Apr 29, 2012

Uh oh!

Herzult commented Apr 29, 2012

Uh oh!

Herzult commented Apr 29, 2012

Uh oh!

stof commented Apr 29, 2012

Uh oh!

Herzult commented Apr 29, 2012

Uh oh!

vicb commented Apr 29, 2012

Uh oh!

Herzult commented Apr 29, 2012

Uh oh!

vicb Apr 30, 2012

Choose a reason for hiding this comment

Uh oh!

Herzult Apr 30, 2012

Choose a reason for hiding this comment

Uh oh!

vicb Apr 30, 2012

Choose a reason for hiding this comment

Uh oh!

vicb commented Apr 30, 2012

Uh oh!

stof Apr 30, 2012

Choose a reason for hiding this comment

Uh oh!

hhamon commented May 1, 2012

Uh oh!

Herzult commented May 2, 2012

Uh oh!

Herzult commented May 7, 2012

Uh oh!

vicb May 10, 2012

Choose a reason for hiding this comment

Uh oh!

vicb May 14, 2012

Choose a reason for hiding this comment

Uh oh!

vicb commented May 10, 2012

Uh oh!

travisbot commented May 11, 2012

Uh oh!

vicb commented May 11, 2012

Uh oh!

travisbot commented May 13, 2012

Uh oh!

travisbot commented May 13, 2012

Uh oh!

travisbot commented May 14, 2012

Uh oh!

vicb commented May 14, 2012

Uh oh!

travisbot commented May 14, 2012

Uh oh!

travisbot commented May 14, 2012

Uh oh!

vicb commented May 14, 2012

Uh oh!

vicb commented May 16, 2012