feature #31528 [Validator] Add a Length::$allowEmptyString option to reject empty strings (ogizanagi)

This PR was merged into the 4.4 branch.

Discussion
----------

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

| Q             | A
| ------------- | ---
| Branch?       | 4.4 <!-- see below -->
| Bug fix?      | no
| New feature?  | yes <!-- please update src/**/CHANGELOG.md files -->
| BC breaks?    | no     <!-- see https://symfony.com/bc -->
| Deprecations? | yes <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files -->
| Tests pass?   | yes    <!-- please add some, will be required by reviewers -->
| Fixed tickets | N/A   <!-- #-prefixed issue number(s), if any -->
| License       | MIT
| Doc PR        | Todo (change the warning on top of https://symfony.com/doc/current/reference/constraints/Length.html)

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.

While it could be solved now thanks to #29641 by using both `@Length(min=1)` & `@NotBlank(allowNull=true)` constraints,
as expressed in https://github.com/symfony/symfony/issues/27876#issuecomment-403307783 and following comments, the `@Length(min=1)` behavior doesn't match our expectations when reading it: it feels logical to invalidate empty strings, but it actually doesn't.
Hence the proposal of making the behavior of rejecting empty strings the default in 5.0.

In my opinion, the flag could even be removed later.

Commits
-------

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

UPGRADE-4.4.md

@@ -107,3 +107,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`.

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 @@ class BaseUser
     {
         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));
+    }
 }

src/Symfony/Bridge/Doctrine/Tests/Fixtures/DoctrineLoaderEntity.php

@@ -14,6 +14,7 @@ namespace Symfony\Bridge\Doctrine\Tests\Fixtures;
 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));
+    }
 }

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>

src/Symfony/Bridge/Doctrine/Tests/Validator/DoctrineLoaderTest.php

@@ -40,6 +40,7 @@ class DoctrineLoaderTest extends TestCase
         }
 
         $validator = Validation::createValidatorBuilder()
+            ->addMethodMapping('loadValidatorMetadata')
             ->enableAnnotationMapping()
             ->addLoader(new DoctrineLoader(DoctrineTestHelper::createTestEntityManager(), '{^Symfony\\\\Bridge\\\\Doctrine\\\\Tests\\\\Fixtures\\\\DoctrineLoader}'))
             ->getValidator()
@@ -142,6 +143,7 @@ class DoctrineLoaderTest extends TestCase
         }
 
         $validator = Validation::createValidatorBuilder()
+            ->addMethodMapping('loadValidatorMetadata')
             ->enableAnnotationMapping()
             ->addXmlMappings([__DIR__.'/../Resources/validator/BaseUser.xml'])
             ->addLoader(

src/Symfony/Component/Form/Tests/Extension/Validator/Type/FormTypeValidatorExtensionTest.php

@@ -57,13 +57,15 @@ class FormTypeValidatorExtensionTest extends BaseValidatorExtensionTest
 
     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']]),
                 ],
             ])

src/Symfony/Component/Form/Tests/Extension/Validator/ValidatorTypeGuesserTest.php

@@ -61,11 +61,13 @@ class ValidatorTypeGuesserTest extends TestCase
 
     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 @@ class ValidatorTypeGuesserTest extends TestCase
 
     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);

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

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 @@ class Length extends Constraint
 
         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']);
         }

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

@@ -30,7 +30,7 @@ class LengthValidator extends ConstraintValidator
             throw new UnexpectedTypeException($constraint, __NAMESPACE__.'\Length');
         }
 
-        if (null === $value || '' === $value) {
+        if (null === $value || ('' === $value && $constraint->allowEmptyString)) {
             return;
         }
 

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 @@ class LengthTest extends TestCase
      */
     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 @@ class LengthTest extends TestCase
      */
     public function testInvalidNormalizerObjectThrowsException()
     {
-        new Length(['min' => 0, 'max' => 10, 'normalizer' => new \stdClass()]);
+        new Length(['min' => 0, 'max' => 10, 'normalizer' => new \stdClass(), 'allowEmptyString' => false]);
     }
 }

src/Symfony/Component/Validator/Tests/Constraints/LengthValidatorTest.php

@@ -22,26 +22,47 @@ class LengthValidatorTest extends ConstraintValidatorTestCase
         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 @@ class LengthValidatorTest extends ConstraintValidatorTestCase
      */
     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 @@ class LengthValidatorTest extends ConstraintValidatorTestCase
      */
     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 @@ class LengthValidatorTest extends ConstraintValidatorTestCase
      */
     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 @@ class LengthValidatorTest extends ConstraintValidatorTestCase
         $constraint = new Length([
             'min' => 4,
             'minMessage' => 'myMessage',
+            'allowEmptyString' => false,
         ]);
 
         $this->validator->validate($value, $constraint);
@@ -199,6 +221,7 @@ class LengthValidatorTest extends ConstraintValidatorTestCase
             'min' => 4,
             'max' => 4,
             'exactMessage' => 'myMessage',
+            'allowEmptyString' => false,
         ]);
 
         $this->validator->validate($value, $constraint);
@@ -221,6 +244,7 @@ class LengthValidatorTest extends ConstraintValidatorTestCase
             'min' => 4,
             'max' => 4,
             'exactMessage' => 'myMessage',
+            'allowEmptyString' => false,
         ]);
 
         $this->validator->validate($value, $constraint);
@@ -244,6 +268,7 @@ class LengthValidatorTest extends ConstraintValidatorTestCase
             'max' => 1,
             'charset' => $charset,
             'charsetMessage' => 'myMessage',
+            'allowEmptyString' => false,
         ]);
 
         $this->validator->validate($value, $constraint);
@@ -262,7 +287,7 @@ class LengthValidatorTest extends ConstraintValidatorTestCase
 
     public function testConstraintDefaultOption()
     {
-        $constraint = new Length(5);
+        $constraint = new Length(['value' => 5, 'allowEmptyString' => false]);
 
         $this->assertEquals(5, $constraint->min);
         $this->assertEquals(5, $constraint->max);
@@ -270,7 +295,7 @@ class LengthValidatorTest extends ConstraintValidatorTestCase
 
     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);

src/Symfony/Component/Validator/Tests/Validator/RecursiveValidatorTest.php

@@ -103,7 +103,7 @@ class RecursiveValidatorTest extends AbstractTest
     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,7 +121,7 @@ class RecursiveValidatorTest extends AbstractTest
     {
         $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();
@@ -129,8 +129,9 @@ class RecursiveValidatorTest extends AbstractTest
 
         $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());
     }
 }