bug #34694 [Validator] Fix auto-mapping constraints should not be validated (ogizanagi)

nicolas-grekas · nicolas-grekas · commit 894ca8a8b257 · 2019-12-10T11:58:52.000+01:00
This PR was squashed before being merged into the 4.4 branch.

Discussion
----------

[Validator] Fix auto-mapping constraints should not be validated

| Q             | A
| ------------- | ---
| Branch?       | 4.4 &lt;!-- see below --&gt;
| Bug fix?      | yes
| New feature?  | no &lt;!-- please update src/**/CHANGELOG.md files --&gt;
| Deprecations? | no &lt;!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files --&gt;
| Tickets       | #34672 &lt;!-- prefix each issue number with "Fix #", if any --&gt;
| License       | MIT
| Doc PR        | N/A

As for `Traverse`, I don't think we should add these "constraints" to the list.

I'm also wondering if it really makes sense to have these annotations as constraints. I think it should rather behave like the `GroupSequence` annotation to add the info the generic metadata at loading time, but we don't need to rely on the constraints behavior at all.

Commits
-------

bc53e4bca0 [Validator] Fix auto-mapping constraints should not be validated
diff --git a/Mapping/AutoMappingStrategy.php b/Mapping/AutoMappingStrategy.php
@@ -0,0 +1,42 @@
+<?php
+
+/*
+ * This file is part of the Symfony package.
+ *
+ * (c) Fabien Potencier <fabien@symfony.com>
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace Symfony\Component\Validator\Mapping;
+
+/**
+ * Specifies how the auto-mapping feature should behave.
+ *
+ * @author Maxime Steinhausser <maxime.steinhausser@gmail.com>
+ */
+final class AutoMappingStrategy
+{
+    /**
+     * Nothing explicitly set, rely on auto-mapping configured regex.
+     */
+    public const NONE = 0;
+
+    /**
+     * Explicitly enabled.
+     */
+    public const ENABLED = 1;
+
+    /**
+     * Explicitly disabled.
+     */
+    public const DISABLED = 2;
+
+    /**
+     * Not instantiable.
+     */
+    private function __construct()
+    {
+    }
+}
diff --git a/Mapping/GenericMetadata.php b/Mapping/GenericMetadata.php
@@ -12,6 +12,8 @@
 namespace Symfony\Component\Validator\Mapping;
 
 use Symfony\Component\Validator\Constraint;
+use Symfony\Component\Validator\Constraints\DisableAutoMapping;
+use Symfony\Component\Validator\Constraints\EnableAutoMapping;
 use Symfony\Component\Validator\Constraints\Length;
 use Symfony\Component\Validator\Constraints\NotBlank;
 use Symfony\Component\Validator\Constraints\Traverse;
@@ -75,6 +77,19 @@ class GenericMetadata implements MetadataInterface
      */
     public $traversalStrategy = TraversalStrategy::NONE;
 
+    /**
+     * Is auto-mapping enabled?
+     *
+     * @var int
+     *
+     * @see AutoMappingStrategy
+     *
+     * @internal This property is public in order to reduce the size of the
+     *           class' serialized representation. Do not access it. Use
+     *           {@link getAutoMappingStrategy()} instead.
+     */
+    public $autoMappingStrategy = AutoMappingStrategy::NONE;
+
     /**
      * Returns the names of the properties that should be serialized.
      *
@@ -87,6 +102,7 @@ public function __sleep()
             'constraintsByGroup',
             'cascadingStrategy',
             'traversalStrategy',
+            'autoMappingStrategy',
         ];
     }
 
@@ -139,6 +155,13 @@ public function addConstraint(Constraint $constraint)
             return $this;
         }
 
+        if ($constraint instanceof DisableAutoMapping || $constraint instanceof EnableAutoMapping) {
+            $this->autoMappingStrategy = $constraint instanceof EnableAutoMapping ? AutoMappingStrategy::ENABLED : AutoMappingStrategy::DISABLED;
+
+            // The constraint is not added
+            return $this;
+        }
+
         $this->constraints[] = $constraint;
 
         foreach ($constraint->groups as $group) {
@@ -213,6 +236,14 @@ public function getTraversalStrategy()
         return $this->traversalStrategy;
     }
 
+    /**
+     * @see AutoMappingStrategy
+     */
+    public function getAutoMappingStrategy(): int
+    {
+        return $this->autoMappingStrategy;
+    }
+
     private function configureLengthConstraints(array $constraints): void
     {
         $allowEmptyString = true;
diff --git a/Mapping/Loader/AutoMappingTrait.php b/Mapping/Loader/AutoMappingTrait.php
@@ -11,8 +11,7 @@
 
 namespace Symfony\Component\Validator\Mapping\Loader;
 
-use Symfony\Component\Validator\Constraints\DisableAutoMapping;
-use Symfony\Component\Validator\Constraints\EnableAutoMapping;
+use Symfony\Component\Validator\Mapping\AutoMappingStrategy;
 use Symfony\Component\Validator\Mapping\ClassMetadata;
 
 /**
@@ -25,14 +24,8 @@ trait AutoMappingTrait
     private function isAutoMappingEnabledForClass(ClassMetadata $metadata, string $classValidatorRegexp = null): bool
     {
         // Check if AutoMapping constraint is set first
-        foreach ($metadata->getConstraints() as $constraint) {
-            if ($constraint instanceof DisableAutoMapping) {
-                return false;
-            }
-
-            if ($constraint instanceof EnableAutoMapping) {
-                return true;
-            }
+        if (AutoMappingStrategy::NONE !== $strategy = $metadata->getAutoMappingStrategy()) {
+            return AutoMappingStrategy::ENABLED === $strategy;
         }
 
         // Fallback on the config
diff --git a/Mapping/Loader/PropertyInfoLoader.php b/Mapping/Loader/PropertyInfoLoader.php
@@ -16,11 +16,10 @@
 use Symfony\Component\PropertyInfo\PropertyTypeExtractorInterface;
 use Symfony\Component\PropertyInfo\Type as PropertyInfoType;
 use Symfony\Component\Validator\Constraints\All;
-use Symfony\Component\Validator\Constraints\DisableAutoMapping;
-use Symfony\Component\Validator\Constraints\EnableAutoMapping;
 use Symfony\Component\Validator\Constraints\NotBlank;
 use Symfony\Component\Validator\Constraints\NotNull;
 use Symfony\Component\Validator\Constraints\Type;
+use Symfony\Component\Validator\Mapping\AutoMappingStrategy;
 use Symfony\Component\Validator\Mapping\ClassMetadata;
 
 /**
@@ -77,16 +76,16 @@ public function loadClassMetadata(ClassMetadata $metadata): bool
             $hasNotBlankConstraint = false;
             $allConstraint = null;
             foreach ($metadata->getPropertyMetadata($property) as $propertyMetadata) {
-                foreach ($propertyMetadata->getConstraints() as $constraint) {
-                    // Enabling or disabling auto-mapping explicitly always takes precedence
-                    if ($constraint instanceof DisableAutoMapping) {
-                        continue 3;
-                    }
+                // Enabling or disabling auto-mapping explicitly always takes precedence
+                if (AutoMappingStrategy::DISABLED === $propertyMetadata->getAutoMappingStrategy()) {
+                    continue 2;
+                }
 
-                    if ($constraint instanceof EnableAutoMapping) {
-                        $enabledForProperty = true;
-                    }
+                if (AutoMappingStrategy::ENABLED === $propertyMetadata->getAutoMappingStrategy()) {
+                    $enabledForProperty = true;
+                }
 
+                foreach ($propertyMetadata->getConstraints() as $constraint) {
                     if ($constraint instanceof Type) {
                         $hasTypeConstraint = true;
                     } elseif ($constraint instanceof NotNull) {
diff --git a/Tests/Mapping/Loader/PropertyInfoLoaderTest.php b/Tests/Mapping/Loader/PropertyInfoLoaderTest.php
@@ -15,13 +15,14 @@
 use Symfony\Component\PropertyInfo\PropertyInfoExtractorInterface;
 use Symfony\Component\PropertyInfo\Type;
 use Symfony\Component\Validator\Constraints\All;
-use Symfony\Component\Validator\Constraints\DisableAutoMapping;
 use Symfony\Component\Validator\Constraints\Iban;
 use Symfony\Component\Validator\Constraints\NotBlank;
 use Symfony\Component\Validator\Constraints\NotNull;
 use Symfony\Component\Validator\Constraints\Type as TypeConstraint;
+use Symfony\Component\Validator\Mapping\AutoMappingStrategy;
 use Symfony\Component\Validator\Mapping\ClassMetadata;
 use Symfony\Component\Validator\Mapping\Loader\PropertyInfoLoader;
+use Symfony\Component\Validator\Mapping\PropertyMetadata;
 use Symfony\Component\Validator\Tests\Fixtures\Entity;
 use Symfony\Component\Validator\Tests\Fixtures\PropertyInfoLoaderEntity;
 use Symfony\Component\Validator\Tests\Fixtures\PropertyInfoLoaderNoAutoMappingEntity;
@@ -164,11 +165,12 @@ public function testLoadClassMetadata()
         $readOnlyMetadata = $classMetadata->getPropertyMetadata('readOnly');
         $this->assertEmpty($readOnlyMetadata);
 
+        /** @var PropertyMetadata[] $noAutoMappingMetadata */
         $noAutoMappingMetadata = $classMetadata->getPropertyMetadata('noAutoMapping');
         $this->assertCount(1, $noAutoMappingMetadata);
+        $this->assertSame(AutoMappingStrategy::DISABLED, $noAutoMappingMetadata[0]->getAutoMappingStrategy());
         $noAutoMappingConstraints = $noAutoMappingMetadata[0]->getConstraints();
-        $this->assertCount(1, $noAutoMappingConstraints);
-        $this->assertInstanceOf(DisableAutoMapping::class, $noAutoMappingConstraints[0]);
+        $this->assertCount(0, $noAutoMappingConstraints, 'DisableAutoMapping constraint is not added in the list');
     }
 
     /**
@@ -222,8 +224,10 @@ public function testClassNoAutoMapping()
             ->getValidator()
         ;
 
+        /** @var ClassMetadata $classMetadata */
         $classMetadata = $validator->getMetadataFor(new PropertyInfoLoaderNoAutoMappingEntity());
         $this->assertEmpty($classMetadata->getPropertyMetadata('string'));
-        $this->assertCount(3, $classMetadata->getPropertyMetadata('autoMappingExplicitlyEnabled')[0]->constraints);
+        $this->assertCount(2, $classMetadata->getPropertyMetadata('autoMappingExplicitlyEnabled')[0]->constraints);
+        $this->assertSame(AutoMappingStrategy::ENABLED, $classMetadata->getPropertyMetadata('autoMappingExplicitlyEnabled')[0]->getAutoMappingStrategy());
     }
 }
