[Validator] Add a Length::$allowEmptyString option to reject empty strings

ogizanagi · ogizanagi · commit e113e7f812b9 · 2019-07-01T09:11:48.000+02:00
which defaults to `true` in 4.4 but will trigger a deprecation if not set explicitly
in order to make the default `false` in 5.0.
diff --git a/UPGRADE-4.4.md b/UPGRADE-4.4.md
@@ -87,3 +87,7 @@ Validator
 
  * Deprecated passing an `ExpressionLanguage` instance as the second argument of `ExpressionValidator::__construct()`. 
    Pass it as the first argument instead.
+ * The `Length` constraint expects the `allowEmptyString` option to be defined
+   when the `min` option is used.
+   Set it to `true` to keep the current behavior and `false` to reject empty strings.
+   In 5.0, it'll become optional and will default to `false`.
diff --git a/src/Symfony/Bridge/Doctrine/Tests/Fixtures/BaseUser.php b/src/Symfony/Bridge/Doctrine/Tests/Fixtures/BaseUser.php
@@ -2,6 +2,9 @@
 
 namespace Symfony\Bridge\Doctrine\Tests\Fixtures;
 
+use Symfony\Component\Validator\Constraints as Assert;
+use Symfony\Component\Validator\Mapping\ClassMetadata;
+
 /**
  * Class BaseUser.
  */
@@ -46,4 +49,15 @@ public function getUsername()
     {
         return $this->username;
     }
+
+    public static function loadValidatorMetadata(ClassMetadata $metadata): void
+    {
+        $allowEmptyString = property_exists(Assert\Length::class, 'allowEmptyString') ? ['allowEmptyString' => true] : [];
+
+        $metadata->addPropertyConstraint('username', new Assert\Length([
+            'min' => 2,
+            'max' => 120,
+            'groups' => ['Registration'],
+        ] + $allowEmptyString));
+    }
 }
diff --git a/src/Symfony/Bridge/Doctrine/Tests/Fixtures/DoctrineLoaderEntity.php b/src/Symfony/Bridge/Doctrine/Tests/Fixtures/DoctrineLoaderEntity.php
@@ -14,6 +14,7 @@
 use Doctrine\ORM\Mapping as ORM;
 use Symfony\Bridge\Doctrine\Validator\Constraints\UniqueEntity;
 use Symfony\Component\Validator\Constraints as Assert;
+use Symfony\Component\Validator\Mapping\ClassMetadata;
 
 /**
  * @ORM\Entity
@@ -36,13 +37,11 @@ class DoctrineLoaderEntity extends DoctrineLoaderParentEntity
 
     /**
      * @ORM\Column(length=20)
-     * @Assert\Length(min=5)
      */
     public $mergedMaxLength;
 
     /**
      * @ORM\Column(length=20)
-     * @Assert\Length(min=1, max=10)
      */
     public $alreadyMappedMaxLength;
 
@@ -69,4 +68,12 @@ class DoctrineLoaderEntity extends DoctrineLoaderParentEntity
 
     /** @ORM\Column(type="simple_array", length=100) */
     public $simpleArrayField = [];
+
+    public static function loadValidatorMetadata(ClassMetadata $metadata): void
+    {
+        $allowEmptyString = property_exists(Assert\Length::class, 'allowEmptyString') ? ['allowEmptyString' => true] : [];
+
+        $metadata->addPropertyConstraint('mergedMaxLength', new Assert\Length(['min' => 5] + $allowEmptyString));
+        $metadata->addPropertyConstraint('alreadyMappedMaxLength', new Assert\Length(['min' => 1, 'max' => 10] + $allowEmptyString));
+    }
 }
diff --git a/src/Symfony/Bridge/Doctrine/Tests/Resources/validator/BaseUser.xml b/src/Symfony/Bridge/Doctrine/Tests/Resources/validator/BaseUser.xml
@@ -9,11 +9,6 @@
             <constraint name="NotBlank">
                 <option name="groups">Registration</option>
             </constraint>
-            <constraint name="Length">
-                <option name="min">2</option>
-                <option name="max">120</option>
-                <option name="groups">Registration</option>
-            </constraint>
         </property>
     </class>
 </constraint-mapping>
diff --git a/src/Symfony/Bridge/Doctrine/Tests/Validator/DoctrineLoaderTest.php b/src/Symfony/Bridge/Doctrine/Tests/Validator/DoctrineLoaderTest.php
@@ -40,6 +40,7 @@ public function testLoadClassMetadata()
         }
 
         $validator = Validation::createValidatorBuilder()
+            ->addMethodMapping('loadValidatorMetadata')
             ->enableAnnotationMapping()
             ->addLoader(new DoctrineLoader(DoctrineTestHelper::createTestEntityManager(), '{^Symfony\\\\Bridge\\\\Doctrine\\\\Tests\\\\Fixtures\\\\DoctrineLoader}'))
             ->getValidator()
@@ -142,6 +143,7 @@ public function testFieldMappingsConfiguration()
         }
 
         $validator = Validation::createValidatorBuilder()
+            ->addMethodMapping('loadValidatorMetadata')
             ->enableAnnotationMapping()
             ->addXmlMappings([__DIR__.'/../Resources/validator/BaseUser.xml'])
             ->addLoader(
diff --git a/src/Symfony/Component/Form/Tests/Extension/Validator/Type/FormTypeValidatorExtensionTest.php b/src/Symfony/Component/Form/Tests/Extension/Validator/Type/FormTypeValidatorExtensionTest.php
@@ -57,13 +57,15 @@ public function testValidConstraint()
 
     public function testGroupSequenceWithConstraintsOption()
     {
+        $allowEmptyString = property_exists(Length::class, 'allowEmptyString') ? ['allowEmptyString' => true] : [];
+
         $form = Forms::createFormFactoryBuilder()
             ->addExtension(new ValidatorExtension(Validation::createValidator()))
             ->getFormFactory()
             ->create(FormTypeTest::TESTED_TYPE, null, (['validation_groups' => new GroupSequence(['First', 'Second'])]))
             ->add('field', TextTypeTest::TESTED_TYPE, [
                 'constraints' => [
-                    new Length(['min' => 10, 'groups' => ['First']]),
+                    new Length(['min' => 10, 'groups' => ['First']] + $allowEmptyString),
                     new Email(['groups' => ['Second']]),
                 ],
             ])
diff --git a/src/Symfony/Component/Form/Tests/Extension/Validator/ValidatorTypeGuesserTest.php b/src/Symfony/Component/Form/Tests/Extension/Validator/ValidatorTypeGuesserTest.php
@@ -61,11 +61,13 @@ protected function setUp()
 
     public function guessRequiredProvider()
     {
+        $allowEmptyString = property_exists(Length::class, 'allowEmptyString') ? ['allowEmptyString' => true] : [];
+
         return [
             [new NotNull(), new ValueGuess(true, Guess::HIGH_CONFIDENCE)],
             [new NotBlank(), new ValueGuess(true, Guess::HIGH_CONFIDENCE)],
             [new IsTrue(), new ValueGuess(true, Guess::HIGH_CONFIDENCE)],
-            [new Length(10), new ValueGuess(false, Guess::LOW_CONFIDENCE)],
+            [new Length(['min' => 10, 'max' => 10] + $allowEmptyString), new ValueGuess(false, Guess::LOW_CONFIDENCE)],
             [new Range(['min' => 1, 'max' => 20]), new ValueGuess(false, Guess::LOW_CONFIDENCE)],
         ];
     }
@@ -101,7 +103,9 @@ public function testGuessMaxLengthForConstraintWithMaxValue()
 
     public function testGuessMaxLengthForConstraintWithMinValue()
     {
-        $constraint = new Length(['min' => '2']);
+        $allowEmptyString = property_exists(Length::class, 'allowEmptyString') ? ['allowEmptyString' => true] : [];
+
+        $constraint = new Length(['min' => '2'] + $allowEmptyString);
 
         $result = $this->guesser->guessMaxLengthForConstraint($constraint);
         $this->assertNull($result);
diff --git a/src/Symfony/Component/Validator/CHANGELOG.md b/src/Symfony/Component/Validator/CHANGELOG.md
@@ -8,6 +8,7 @@ CHANGELOG
  * added the `compared_value_path` parameter in violations when using any 
    comparison constraint with the `propertyPath` option.
  * added support for checking an array of types in `TypeValidator`
+ * added a new `allowEmptyString` option to the `Length` constraint to allow rejecting empty strings when `min` is set, by setting it to `false`.
 
 4.3.0
 -----
diff --git a/src/Symfony/Component/Validator/Constraints/Length.php b/src/Symfony/Component/Validator/Constraints/Length.php
@@ -41,6 +41,7 @@ class Length extends Constraint
     public $min;
     public $charset = 'UTF-8';
     public $normalizer;
+    public $allowEmptyString;
 
     public function __construct($options = null)
     {
@@ -56,6 +57,13 @@ public function __construct($options = null)
 
         parent::__construct($options);
 
+        if (null === $this->allowEmptyString) {
+            $this->allowEmptyString = true;
+            if (null !== $this->min) {
+                @trigger_error(sprintf('Using the "%s" constraint with the "min" option without setting the "allowEmptyString" one is deprecated and defaults to true. In 5.0, it will become optional and default to false.', self::class), E_USER_DEPRECATED);
+            }
+        }
+
         if (null === $this->min && null === $this->max) {
             throw new MissingOptionsException(sprintf('Either option "min" or "max" must be given for constraint %s', __CLASS__), ['min', 'max']);
         }
diff --git a/src/Symfony/Component/Validator/Constraints/LengthValidator.php b/src/Symfony/Component/Validator/Constraints/LengthValidator.php
@@ -30,7 +30,7 @@ public function validate($value, Constraint $constraint)
             throw new UnexpectedTypeException($constraint, __NAMESPACE__.'\Length');
         }
 
-        if (null === $value || '' === $value) {
+        if (null === $value || ('' === $value && $constraint->allowEmptyString)) {
             return;
         }
 
diff --git a/src/Symfony/Component/Validator/Tests/Constraints/LengthTest.php b/src/Symfony/Component/Validator/Tests/Constraints/LengthTest.php
@@ -21,7 +21,7 @@ class LengthTest extends TestCase
 {
     public function testNormalizerCanBeSet()
     {
-        $length = new Length(['min' => 0, 'max' => 10, 'normalizer' => 'trim']);
+        $length = new Length(['min' => 0, 'max' => 10, 'normalizer' => 'trim', 'allowEmptyString' => false]);
 
         $this->assertEquals('trim', $length->normalizer);
     }
@@ -32,7 +32,7 @@ public function testNormalizerCanBeSet()
      */
     public function testInvalidNormalizerThrowsException()
     {
-        new Length(['min' => 0, 'max' => 10, 'normalizer' => 'Unknown Callable']);
+        new Length(['min' => 0, 'max' => 10, 'normalizer' => 'Unknown Callable', 'allowEmptyString' => false]);
     }
 
     /**
@@ -41,6 +41,6 @@ public function testInvalidNormalizerThrowsException()
      */
     public function testInvalidNormalizerObjectThrowsException()
     {
-        new Length(['min' => 0, 'max' => 10, 'normalizer' => new \stdClass()]);
+        new Length(['min' => 0, 'max' => 10, 'normalizer' => new \stdClass(), 'allowEmptyString' => false]);
     }
 }
diff --git a/src/Symfony/Component/Validator/Tests/Constraints/LengthValidatorTest.php b/src/Symfony/Component/Validator/Tests/Constraints/LengthValidatorTest.php
@@ -22,26 +22,47 @@ protected function createValidator()
         return new LengthValidator();
     }
 
-    public function testNullIsValid()
+    public function testLegacyNullIsValid()
     {
-        $this->validator->validate(null, new Length(6));
+        $this->validator->validate(null, new Length(['value' => 6, 'allowEmptyString' => false]));
 
         $this->assertNoViolation();
     }
 
-    public function testEmptyStringIsValid()
+    /**
+     * @group legacy
+     * @expectedDeprecation Using the "Symfony\Component\Validator\Constraints\Length" constraint with the "min" option without setting the "allowEmptyString" one is deprecated and defaults to true. In 5.0, it will become optional and default to false.
+     */
+    public function testLegacyEmptyStringIsValid()
     {
         $this->validator->validate('', new Length(6));
 
         $this->assertNoViolation();
     }
 
+    public function testEmptyStringIsInvalid()
+    {
+        $this->validator->validate('', new Length([
+            'value' => $limit = 6,
+            'allowEmptyString' => false,
+            'exactMessage' => 'myMessage',
+        ]));
+
+        $this->buildViolation('myMessage')
+            ->setParameter('{{ value }}', '""')
+            ->setParameter('{{ limit }}', $limit)
+            ->setInvalidValue('')
+            ->setPlural($limit)
+            ->setCode(Length::TOO_SHORT_ERROR)
+            ->assertRaised();
+    }
+
     /**
      * @expectedException \Symfony\Component\Validator\Exception\UnexpectedValueException
      */
     public function testExpectsStringCompatibleType()
     {
-        $this->validator->validate(new \stdClass(), new Length(5));
+        $this->validator->validate(new \stdClass(), new Length(['value' => 5, 'allowEmptyString' => false]));
     }
 
     public function getThreeOrLessCharacters()
@@ -109,7 +130,7 @@ public function getThreeCharactersWithWhitespaces()
      */
     public function testValidValuesMin($value)
     {
-        $constraint = new Length(['min' => 5]);
+        $constraint = new Length(['min' => 5, 'allowEmptyString' => false]);
         $this->validator->validate($value, $constraint);
 
         $this->assertNoViolation();
@@ -131,7 +152,7 @@ public function testValidValuesMax($value)
      */
     public function testValidValuesExact($value)
     {
-        $constraint = new Length(4);
+        $constraint = new Length(['value' => 4, 'allowEmptyString' => false]);
         $this->validator->validate($value, $constraint);
 
         $this->assertNoViolation();
@@ -142,7 +163,7 @@ public function testValidValuesExact($value)
      */
     public function testValidNormalizedValues($value)
     {
-        $constraint = new Length(['min' => 3, 'max' => 3, 'normalizer' => 'trim']);
+        $constraint = new Length(['min' => 3, 'max' => 3, 'normalizer' => 'trim', 'allowEmptyString' => false]);
         $this->validator->validate($value, $constraint);
 
         $this->assertNoViolation();
@@ -156,6 +177,7 @@ public function testInvalidValuesMin($value)
         $constraint = new Length([
             'min' => 4,
             'minMessage' => 'myMessage',
+            'allowEmptyString' => false,
         ]);
 
         $this->validator->validate($value, $constraint);
@@ -199,6 +221,7 @@ public function testInvalidValuesExactLessThanFour($value)
             'min' => 4,
             'max' => 4,
             'exactMessage' => 'myMessage',
+            'allowEmptyString' => false,
         ]);
 
         $this->validator->validate($value, $constraint);
@@ -221,6 +244,7 @@ public function testInvalidValuesExactMoreThanFour($value)
             'min' => 4,
             'max' => 4,
             'exactMessage' => 'myMessage',
+            'allowEmptyString' => false,
         ]);
 
         $this->validator->validate($value, $constraint);
@@ -244,6 +268,7 @@ public function testOneCharset($value, $charset, $isValid)
             'max' => 1,
             'charset' => $charset,
             'charsetMessage' => 'myMessage',
+            'allowEmptyString' => false,
         ]);
 
         $this->validator->validate($value, $constraint);
@@ -262,15 +287,15 @@ public function testOneCharset($value, $charset, $isValid)
 
     public function testConstraintDefaultOption()
     {
-        $constraint = new Length(5);
+        $constraint = new Length(['value' => 5, 'allowEmptyString' => false]);
 
         $this->assertEquals(5, $constraint->min);
         $this->assertEquals(5, $constraint->max);
     }
 
     public function testConstraintAnnotationDefaultOption()
     {
-        $constraint = new Length(['value' => 5, 'exactMessage' => 'message']);
+        $constraint = new Length(['value' => 5, 'exactMessage' => 'message', 'allowEmptyString' => false]);
 
         $this->assertEquals(5, $constraint->min);
         $this->assertEquals(5, $constraint->max);
diff --git a/src/Symfony/Component/Validator/Tests/Validator/RecursiveValidatorTest.php b/src/Symfony/Component/Validator/Tests/Validator/RecursiveValidatorTest.php
@@ -103,7 +103,7 @@ public function testRelationBetweenChildAAndChildB()
     public function testCollectionConstraintValidateAllGroupsForNestedConstraints()
     {
         $this->metadata->addPropertyConstraint('data', new Collection(['fields' => [
-            'one' => [new NotBlank(['groups' => 'one']), new Length(['min' => 2, 'groups' => 'two'])],
+            'one' => [new NotBlank(['groups' => 'one']), new Length(['min' => 2, 'groups' => 'two', 'allowEmptyString' => false])],
             'two' => [new NotBlank(['groups' => 'two'])],
         ]]));
 
@@ -121,16 +121,17 @@ public function testAllConstraintValidateAllGroupsForNestedConstraints()
     {
         $this->metadata->addPropertyConstraint('data', new All(['constraints' => [
             new NotBlank(['groups' => 'one']),
-            new Length(['min' => 2, 'groups' => 'two']),
+            new Length(['min' => 2, 'groups' => 'two', 'allowEmptyString' => false]),
         ]]));
 
         $entity = new Entity();
         $entity->data = ['one' => 't', 'two' => ''];
 
         $violations = $this->validator->validate($entity, null, ['one', 'two']);
 
-        $this->assertCount(2, $violations);
+        $this->assertCount(3, $violations);
         $this->assertInstanceOf(NotBlank::class, $violations->get(0)->getConstraint());
         $this->assertInstanceOf(Length::class, $violations->get(1)->getConstraint());
+        $this->assertInstanceOf(Length::class, $violations->get(2)->getConstraint());
     }
 }

Original file line number	Diff line number	Diff line change
`@@ -40,6 +40,7 @@ public function testLoadClassMetadata()`
`40`	`40`	`}`
`41`	`41`
`42`	`42`	`$validator = Validation::createValidatorBuilder()`
	`43`	`+ ->addMethodMapping('loadValidatorMetadata')`
`43`	`44`	`->enableAnnotationMapping()`
`44`	`45`	`->addLoader(new DoctrineLoader(DoctrineTestHelper::createTestEntityManager(), '{^Symfony\\\\Bridge\\\\Doctrine\\\\Tests\\\\Fixtures\\\\DoctrineLoader}'))`
`45`	`46`	`->getValidator()`
`@@ -142,6 +143,7 @@ public function testFieldMappingsConfiguration()`
`142`	`143`	`}`
`143`	`144`
`144`	`145`	`$validator = Validation::createValidatorBuilder()`
	`146`	`+ ->addMethodMapping('loadValidatorMetadata')`
`145`	`147`	`->enableAnnotationMapping()`
`146`	`148`	`->addXmlMappings([__DIR__.'/../Resources/validator/BaseUser.xml'])`
`147`	`149`	`->addLoader(`
Original file line number	Diff line number	Diff line change
`@@ -30,7 +30,7 @@ public function validate($value, Constraint $constraint)`
`30`	`30`	`throw new UnexpectedTypeException($constraint, __NAMESPACE__.'\Length');`
`31`	`31`	`}`
`32`	`32`
`33`		`- if (null === $value \|\| '' === $value) {`
	`33`	`+ if (null === $value \|\| ('' === $value && $constraint->allowEmptyString)) {`
`34`	`34`	`return;`
`35`	`35`	`}`
`36`	`36`