Skip to content

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

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Dec 10, 2019
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 10 additions & 6 deletions src/Symfony/Bridge/Doctrine/Tests/Validator/DoctrineLoaderTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,11 +21,12 @@
use Symfony\Bridge\Doctrine\Tests\Fixtures\DoctrineLoaderParentEntity;
use Symfony\Bridge\Doctrine\Validator\Constraints\UniqueEntity;
use Symfony\Bridge\Doctrine\Validator\DoctrineLoader;
use Symfony\Component\Validator\Constraints\DisableAutoMapping;
use Symfony\Component\Validator\Constraints\Length;
use Symfony\Component\Validator\Mapping\AutoMappingStrategy;
use Symfony\Component\Validator\Mapping\CascadingStrategy;
use Symfony\Component\Validator\Mapping\ClassMetadata;
use Symfony\Component\Validator\Mapping\Loader\AutoMappingTrait;
use Symfony\Component\Validator\Mapping\PropertyMetadata;
use Symfony\Component\Validator\Mapping\TraversalStrategy;
use Symfony\Component\Validator\Tests\Fixtures\Entity;
use Symfony\Component\Validator\Validation;
Expand Down Expand Up @@ -141,11 +142,12 @@ public function testLoadClassMetadata()
$this->assertInstanceOf(Length::class, $textFieldConstraints[0]);
$this->assertSame(1000, $textFieldConstraints[0]->max);

/** @var PropertyMetadata[] $noAutoMappingMetadata */
$noAutoMappingMetadata = $classMetadata->getPropertyMetadata('noAutoMapping');
$this->assertCount(1, $noAutoMappingMetadata);
$noAutoMappingConstraints = $noAutoMappingMetadata[0]->getConstraints();
$this->assertCount(1, $noAutoMappingConstraints);
$this->assertInstanceOf(DisableAutoMapping::class, $noAutoMappingConstraints[0]);
$this->assertCount(0, $noAutoMappingConstraints);
$this->assertSame(AutoMappingStrategy::DISABLED, $noAutoMappingMetadata[0]->getAutoMappingStrategy());
}

public function testFieldMappingsConfiguration()
Expand Down Expand Up @@ -207,13 +209,15 @@ public function testClassNoAutoMapping()
$classMetadata = $validator->getMetadataFor(new DoctrineLoaderNoAutoMappingEntity());

$classConstraints = $classMetadata->getConstraints();
$this->assertCount(1, $classConstraints);
$this->assertInstanceOf(DisableAutoMapping::class, $classConstraints[0]);
$this->assertCount(0, $classConstraints);
$this->assertSame(AutoMappingStrategy::DISABLED, $classMetadata->getAutoMappingStrategy());

$maxLengthMetadata = $classMetadata->getPropertyMetadata('maxLength');
$this->assertEmpty($maxLengthMetadata);

/** @var PropertyMetadata[] $autoMappingExplicitlyEnabledMetadata */
$autoMappingExplicitlyEnabledMetadata = $classMetadata->getPropertyMetadata('autoMappingExplicitlyEnabled');
$this->assertCount(2, $autoMappingExplicitlyEnabledMetadata[0]->constraints);
$this->assertCount(1, $autoMappingExplicitlyEnabledMetadata[0]->constraints);
$this->assertSame(AutoMappingStrategy::ENABLED, $autoMappingExplicitlyEnabledMetadata[0]->getAutoMappingStrategy());
}
}
18 changes: 10 additions & 8 deletions src/Symfony/Bridge/Doctrine/Validator/DoctrineLoader.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,9 @@
use Doctrine\ORM\Mapping\ClassMetadataInfo;
use Doctrine\ORM\Mapping\MappingException as OrmMappingException;
use Symfony\Bridge\Doctrine\Validator\Constraints\UniqueEntity;
use Symfony\Component\Validator\Constraints\DisableAutoMapping;
use Symfony\Component\Validator\Constraints\EnableAutoMapping;
use Symfony\Component\Validator\Constraints\Length;
use Symfony\Component\Validator\Constraints\Valid;
use Symfony\Component\Validator\Mapping\AutoMappingStrategy;
use Symfony\Component\Validator\Mapping\ClassMetadata;
use Symfony\Component\Validator\Mapping\Loader\AutoMappingTrait;
use Symfony\Component\Validator\Mapping\Loader\LoaderInterface;
Expand Down Expand Up @@ -76,13 +75,16 @@ public function loadClassMetadata(ClassMetadata $metadata): bool
$enabledForProperty = $enabledForClass;
$lengthConstraint = null;
foreach ($metadata->getPropertyMetadata($mapping['fieldName']) as $propertyMetadata) {
// Enabling or disabling auto-mapping explicitly always takes precedence
if (AutoMappingStrategy::DISABLED === $propertyMetadata->getAutoMappingStrategy()) {
continue 2;
}
if (AutoMappingStrategy::ENABLED === $propertyMetadata->getAutoMappingStrategy()) {
$enabledForProperty = true;
}

foreach ($propertyMetadata->getConstraints() as $constraint) {
// Enabling or disabling auto-mapping explicitly always takes precedence
if ($constraint instanceof DisableAutoMapping) {
continue 3;
} elseif ($constraint instanceof EnableAutoMapping) {
$enabledForProperty = true;
} elseif ($constraint instanceof Length) {
if ($constraint instanceof Length) {
$lengthConstraint = $constraint;
}
}
Expand Down
4 changes: 2 additions & 2 deletions src/Symfony/Bridge/Doctrine/composer.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@
"symfony/proxy-manager-bridge": "^3.4|^4.0|^5.0",
"symfony/security-core": "^4.4|^5.0",
"symfony/expression-language": "^3.4|^4.0|^5.0",
"symfony/validator": "^4.4|^5.0",
"symfony/validator": "^4.4.1|^5.0.1",
"symfony/var-dumper": "^3.4|^4.0|^5.0",
"symfony/translation": "^3.4|^4.0|^5.0",
"doctrine/annotations": "~1.7",
Expand All @@ -53,7 +53,7 @@
"symfony/http-kernel": "<4.3.7",
"symfony/messenger": "<4.3",
"symfony/security-core": "<4.4",
"symfony/validator": "<4.4"
"symfony/validator": "<4.4.1|<5.0.1,>=5.0"
},
"suggest": {
"symfony/form": "",
Expand Down
42 changes: 42 additions & 0 deletions src/Symfony/Component/Validator/Mapping/AutoMappingStrategy.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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()
{
}
}
31 changes: 31 additions & 0 deletions src/Symfony/Component/Validator/Mapping/GenericMetadata.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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.
*
Expand All @@ -87,6 +102,7 @@ public function __sleep()
'constraintsByGroup',
'cascadingStrategy',
'traversalStrategy',
'autoMappingStrategy',
];
}

Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -213,6 +236,14 @@ public function getTraversalStrategy()
return $this->traversalStrategy;
}

/**
* @see AutoMappingStrategy
*/
public function getAutoMappingStrategy(): int
Copy link
Contributor Author

@ogizanagi ogizanagi Nov 28, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Worth adding to the interface (@method to get the deprec), or adding a new AutoMappingMetadataInterface?
AFAIK, implementations all rely on GenericMetadata classes, so might only be an implementation detail.

{
return $this->autoMappingStrategy;
}

private function configureLengthConstraints(array $constraints): void
{
$allowEmptyString = true;
Expand Down
13 changes: 3 additions & 10 deletions src/Symfony/Component/Validator/Mapping/Loader/AutoMappingTrait.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;

/**
Expand All @@ -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
Expand Down
19 changes: 9 additions & 10 deletions src/Symfony/Component/Validator/Mapping/Loader/PropertyInfoLoader.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;

/**
Expand Down Expand Up @@ -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) {
Expand Down
12 changes: 8 additions & 4 deletions src/Symfony/Component/Validator/Tests/Mapping/Loader/PropertyInfoLoaderTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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');
}

/**
Expand Down Expand Up @@ -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());
}
}