From 441c80603e77ceccb1e56db80086220fd80b92ca Mon Sep 17 00:00:00 2001 From: "Alexander M. Turek" Date: Sat, 7 Nov 2020 20:15:36 +0100 Subject: [PATCH] [Validator] Allow load mappings from attributes without doctrine/annotations. --- UPGRADE-5.2.md | 30 ++++++++ UPGRADE-6.0.md | 30 ++++++++ .../Tests/Validator/DoctrineLoaderTest.php | 9 ++- .../FrameworkExtension.php | 9 ++- .../CacheWarmer/ValidatorCacheWarmerTest.php | 4 +- .../FrameworkExtensionTest.php | 30 ++++---- src/Symfony/Component/Validator/CHANGELOG.md | 1 + .../Tests/Constraints/ValidValidatorTest.php | 4 +- .../Mapping/Loader/PropertyInfoLoaderTest.php | 6 +- .../Validator/Tests/ValidatorBuilderTest.php | 71 +++++++++++++++++++ .../Component/Validator/ValidatorBuilder.php | 48 ++++++++++--- 11 files changed, 206 insertions(+), 36 deletions(-) diff --git a/UPGRADE-5.2.md b/UPGRADE-5.2.md index bc6c4b69bf..e62963fe2d 100644 --- a/UPGRADE-5.2.md +++ b/UPGRADE-5.2.md @@ -126,6 +126,36 @@ Validator * Deprecated the `NumberConstraintTrait` trait. + * Deprecated setting a Doctrine annotation reader via `ValidatorBuilder::enableAnnotationMapping()` + + Before: + + ```php + $builder->enableAnnotationMapping($reader); + ``` + + After: + + ```php + $builder->enableAnnotationMapping(true) + ->setDoctrineAnnotationReader($reader); + ``` + + * Deprecated creating a Doctrine annotation reader via `ValidatorBuilder::enableAnnotationMapping()` + + Before: + + ```php + $builder->enableAnnotationMapping(); + ``` + + After: + + ```php + $builder->enableAnnotationMapping(true) + ->addDefaultDoctrineAnnotationReader(); + ``` + Security -------- diff --git a/UPGRADE-6.0.md b/UPGRADE-6.0.md index abeb497d9c..6e84d2c0de 100644 --- a/UPGRADE-6.0.md +++ b/UPGRADE-6.0.md @@ -189,6 +189,36 @@ Validator * Removed the `NumberConstraintTrait` trait. +* `ValidatorBuilder::enableAnnotationMapping()` does not accept a Doctrine annotation reader anymore. + + Before: + + ```php + $builder->enableAnnotationMapping($reader); + ``` + + After: + + ```php + $builder->enableAnnotationMapping(true) + ->setDoctrineAnnotationReader($reader); + ``` + +* `ValidatorBuilder::enableAnnotationMapping()` won't automatically setup a Doctrine annotation reader anymore. + + Before: + + ```php + $builder->enableAnnotationMapping(); + ``` + + After: + + ```php + $builder->enableAnnotationMapping(true) + ->addDefaultDoctrineAnnotationReader(); + ``` + Yaml ---- diff --git a/src/Symfony/Bridge/Doctrine/Tests/Validator/DoctrineLoaderTest.php b/src/Symfony/Bridge/Doctrine/Tests/Validator/DoctrineLoaderTest.php index 35a90044f7..31129a8c61 100644 --- a/src/Symfony/Bridge/Doctrine/Tests/Validator/DoctrineLoaderTest.php +++ b/src/Symfony/Bridge/Doctrine/Tests/Validator/DoctrineLoaderTest.php @@ -46,7 +46,8 @@ class DoctrineLoaderTest extends TestCase public function testLoadClassMetadata() { $validator = Validation::createValidatorBuilder() - ->enableAnnotationMapping() + ->enableAnnotationMapping(true) + ->addDefaultDoctrineAnnotationReader() ->addLoader(new DoctrineLoader(DoctrineTestHelper::createTestEntityManager(), '{^Symfony\\\\Bridge\\\\Doctrine\\\\Tests\\\\Fixtures\\\\DoctrineLoader}')) ->getValidator() ; @@ -151,7 +152,8 @@ class DoctrineLoaderTest extends TestCase public function testFieldMappingsConfiguration() { $validator = Validation::createValidatorBuilder() - ->enableAnnotationMapping() + ->enableAnnotationMapping(true) + ->addDefaultDoctrineAnnotationReader() ->addXmlMappings([__DIR__.'/../Resources/validator/BaseUser.xml']) ->addLoader( new DoctrineLoader( @@ -192,7 +194,8 @@ class DoctrineLoaderTest extends TestCase public function testClassNoAutoMapping() { $validator = Validation::createValidatorBuilder() - ->enableAnnotationMapping() + ->enableAnnotationMapping(true) + ->addDefaultDoctrineAnnotationReader() ->addLoader(new DoctrineLoader(DoctrineTestHelper::createTestEntityManager(), '{.*}')) ->getValidator(); diff --git a/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php b/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php index aabf476d41..ea5e139e3f 100644 --- a/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php +++ b/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php @@ -1303,11 +1303,14 @@ class FrameworkExtension extends Extension $definition->replaceArgument(0, $config['email_validation_mode']); if (\array_key_exists('enable_annotations', $config) && $config['enable_annotations']) { - if (!$this->annotationsConfigEnabled) { - throw new \LogicException('"enable_annotations" on the validator cannot be set as Annotations support is disabled.'); + if (!$this->annotationsConfigEnabled && \PHP_VERSION_ID < 80000) { + throw new \LogicException('"enable_annotations" on the validator cannot be set as Doctrine Annotations support is disabled.'); } - $validatorBuilder->addMethodCall('enableAnnotationMapping', [new Reference('annotation_reader')]); + $validatorBuilder->addMethodCall('enableAnnotationMapping', [true]); + if ($this->annotationsConfigEnabled) { + $validatorBuilder->addMethodCall('setDoctrineAnnotationReader', [new Reference('annotation_reader')]); + } } if (\array_key_exists('static_method', $config) && $config['static_method']) { diff --git a/src/Symfony/Bundle/FrameworkBundle/Tests/CacheWarmer/ValidatorCacheWarmerTest.php b/src/Symfony/Bundle/FrameworkBundle/Tests/CacheWarmer/ValidatorCacheWarmerTest.php index 92ef379b1b..8a54680c0f 100644 --- a/src/Symfony/Bundle/FrameworkBundle/Tests/CacheWarmer/ValidatorCacheWarmerTest.php +++ b/src/Symfony/Bundle/FrameworkBundle/Tests/CacheWarmer/ValidatorCacheWarmerTest.php @@ -26,7 +26,7 @@ class ValidatorCacheWarmerTest extends TestCase $validatorBuilder->addXmlMapping(__DIR__.'/../Fixtures/Validation/Resources/person.xml'); $validatorBuilder->addYamlMapping(__DIR__.'/../Fixtures/Validation/Resources/author.yml'); $validatorBuilder->addMethodMapping('loadValidatorMetadata'); - $validatorBuilder->enableAnnotationMapping(); + $validatorBuilder->enableAnnotationMapping(true)->addDefaultDoctrineAnnotationReader(); $file = sys_get_temp_dir().'/cache-validator.php'; @unlink($file); @@ -46,7 +46,7 @@ class ValidatorCacheWarmerTest extends TestCase { $validatorBuilder = new ValidatorBuilder(); $validatorBuilder->addYamlMapping(__DIR__.'/../Fixtures/Validation/Resources/categories.yml'); - $validatorBuilder->enableAnnotationMapping(); + $validatorBuilder->enableAnnotationMapping(true)->addDefaultDoctrineAnnotationReader(); $file = sys_get_temp_dir().'/cache-validator-with-annotations.php'; @unlink($file); diff --git a/src/Symfony/Bundle/FrameworkBundle/Tests/DependencyInjection/FrameworkExtensionTest.php b/src/Symfony/Bundle/FrameworkBundle/Tests/DependencyInjection/FrameworkExtensionTest.php index 48ac02ccc1..b902d3b636 100644 --- a/src/Symfony/Bundle/FrameworkBundle/Tests/DependencyInjection/FrameworkExtensionTest.php +++ b/src/Symfony/Bundle/FrameworkBundle/Tests/DependencyInjection/FrameworkExtensionTest.php @@ -870,7 +870,7 @@ abstract class FrameworkExtensionTest extends TestCase $annotations = !class_exists(FullStack::class) && class_exists(Annotation::class); - $this->assertCount($annotations ? 7 : 6, $calls); + $this->assertCount($annotations ? 8 : 6, $calls); $this->assertSame('setConstraintValidatorFactory', $calls[0][0]); $this->assertEquals([new Reference('validator.validator_factory')], $calls[0][1]); $this->assertSame('setTranslator', $calls[1][0]); @@ -882,6 +882,7 @@ abstract class FrameworkExtensionTest extends TestCase $i = 3; if ($annotations) { $this->assertSame('enableAnnotationMapping', $calls[++$i][0]); + $this->assertSame('setDoctrineAnnotationReader', $calls[++$i][0]); } $this->assertSame('addMethodMapping', $calls[++$i][0]); $this->assertSame(['loadValidatorMetadata'], $calls[$i][1]); @@ -923,13 +924,14 @@ abstract class FrameworkExtensionTest extends TestCase $calls = $container->getDefinition('validator.builder')->getMethodCalls(); - $this->assertCount(7, $calls); + $this->assertCount(8, $calls); $this->assertSame('enableAnnotationMapping', $calls[4][0]); - $this->assertEquals([new Reference('annotation_reader')], $calls[4][1]); - $this->assertSame('addMethodMapping', $calls[5][0]); - $this->assertSame(['loadValidatorMetadata'], $calls[5][1]); - $this->assertSame('setMappingCache', $calls[6][0]); - $this->assertEquals([new Reference('validator.mapping.cache.adapter')], $calls[6][1]); + $this->assertSame('setDoctrineAnnotationReader', $calls[5][0]); + $this->assertEquals([new Reference('annotation_reader')], $calls[5][1]); + $this->assertSame('addMethodMapping', $calls[6][0]); + $this->assertSame(['loadValidatorMetadata'], $calls[6][1]); + $this->assertSame('setMappingCache', $calls[7][0]); + $this->assertEquals([new Reference('validator.mapping.cache.adapter')], $calls[7][1]); // no cache this time } @@ -944,14 +946,15 @@ abstract class FrameworkExtensionTest extends TestCase $calls = $container->getDefinition('validator.builder')->getMethodCalls(); - $this->assertCount(8, $calls); + $this->assertCount(9, $calls); $this->assertSame('addXmlMappings', $calls[3][0]); $this->assertSame('addYamlMappings', $calls[4][0]); $this->assertSame('enableAnnotationMapping', $calls[5][0]); - $this->assertSame('addMethodMapping', $calls[6][0]); - $this->assertSame(['loadValidatorMetadata'], $calls[6][1]); - $this->assertSame('setMappingCache', $calls[7][0]); - $this->assertEquals([new Reference('validator.mapping.cache.adapter')], $calls[7][1]); + $this->assertSame('setDoctrineAnnotationReader', $calls[6][0]); + $this->assertSame('addMethodMapping', $calls[7][0]); + $this->assertSame(['loadValidatorMetadata'], $calls[7][1]); + $this->assertSame('setMappingCache', $calls[8][0]); + $this->assertEquals([new Reference('validator.mapping.cache.adapter')], $calls[8][1]); $xmlMappings = $calls[3][1][0]; $this->assertCount(3, $xmlMappings); @@ -1004,11 +1007,12 @@ abstract class FrameworkExtensionTest extends TestCase $annotations = !class_exists(FullStack::class) && class_exists(Annotation::class); - $this->assertCount($annotations ? 6 : 5, $calls); + $this->assertCount($annotations ? 7 : 5, $calls); $this->assertSame('addXmlMappings', $calls[3][0]); $i = 3; if ($annotations) { $this->assertSame('enableAnnotationMapping', $calls[++$i][0]); + $this->assertSame('setDoctrineAnnotationReader', $calls[++$i][0]); } $this->assertSame('setMappingCache', $calls[++$i][0]); $this->assertEquals([new Reference('validator.mapping.cache.adapter')], $calls[$i][1]); diff --git a/src/Symfony/Component/Validator/CHANGELOG.md b/src/Symfony/Component/Validator/CHANGELOG.md index 41de63da39..69fc8a1b3f 100644 --- a/src/Symfony/Component/Validator/CHANGELOG.md +++ b/src/Symfony/Component/Validator/CHANGELOG.md @@ -34,6 +34,7 @@ CHANGELOG * added support for UUIDv6 in `Uuid` constraint * enabled the validator to load constraints from PHP attributes * deprecated the `NumberConstraintTrait` trait + * deprecated setting or creating a Doctrine annotation reader via `ValidatorBuilder::enableAnnotationMapping()`, pass `true` as first parameter and additionally call `setDoctrineAnnotationReader()` or `addDefaultDoctrineAnnotationReader()` to set up the annotation reader 5.1.0 ----- diff --git a/src/Symfony/Component/Validator/Tests/Constraints/ValidValidatorTest.php b/src/Symfony/Component/Validator/Tests/Constraints/ValidValidatorTest.php index 0ec1d42137..93de4244cc 100644 --- a/src/Symfony/Component/Validator/Tests/Constraints/ValidValidatorTest.php +++ b/src/Symfony/Component/Validator/Tests/Constraints/ValidValidatorTest.php @@ -12,7 +12,7 @@ class ValidValidatorTest extends TestCase public function testPropertyPathsArePassedToNestedContexts() { $validatorBuilder = new ValidatorBuilder(); - $validator = $validatorBuilder->enableAnnotationMapping()->getValidator(); + $validator = $validatorBuilder->enableAnnotationMapping(true)->addDefaultDoctrineAnnotationReader()->getValidator(); $violations = $validator->validate(new Foo(), null, ['nested']); @@ -23,7 +23,7 @@ class ValidValidatorTest extends TestCase public function testNullValues() { $validatorBuilder = new ValidatorBuilder(); - $validator = $validatorBuilder->enableAnnotationMapping()->getValidator(); + $validator = $validatorBuilder->enableAnnotationMapping(true)->addDefaultDoctrineAnnotationReader()->getValidator(); $foo = new Foo(); $foo->fooBar = null; diff --git a/src/Symfony/Component/Validator/Tests/Mapping/Loader/PropertyInfoLoaderTest.php b/src/Symfony/Component/Validator/Tests/Mapping/Loader/PropertyInfoLoaderTest.php index c1f147a802..767a5ab5c6 100644 --- a/src/Symfony/Component/Validator/Tests/Mapping/Loader/PropertyInfoLoaderTest.php +++ b/src/Symfony/Component/Validator/Tests/Mapping/Loader/PropertyInfoLoaderTest.php @@ -89,7 +89,8 @@ class PropertyInfoLoaderTest extends TestCase $propertyInfoLoader = new PropertyInfoLoader($propertyInfoStub, $propertyInfoStub, $propertyInfoStub, '{.*}'); $validator = Validation::createValidatorBuilder() - ->enableAnnotationMapping() + ->enableAnnotationMapping(true) + ->addDefaultDoctrineAnnotationReader() ->addLoader($propertyInfoLoader) ->getValidator() ; @@ -220,7 +221,8 @@ class PropertyInfoLoaderTest extends TestCase $propertyInfoLoader = new PropertyInfoLoader($propertyInfoStub, $propertyInfoStub, $propertyInfoStub, '{.*}'); $validator = Validation::createValidatorBuilder() - ->enableAnnotationMapping() + ->enableAnnotationMapping(true) + ->addDefaultDoctrineAnnotationReader() ->addLoader($propertyInfoLoader) ->getValidator() ; diff --git a/src/Symfony/Component/Validator/Tests/ValidatorBuilderTest.php b/src/Symfony/Component/Validator/Tests/ValidatorBuilderTest.php index a09c8fc3c0..4f7b6d66b5 100644 --- a/src/Symfony/Component/Validator/Tests/ValidatorBuilderTest.php +++ b/src/Symfony/Component/Validator/Tests/ValidatorBuilderTest.php @@ -11,12 +11,18 @@ namespace Symfony\Component\Validator\Tests; +use Doctrine\Common\Annotations\CachedReader; +use Doctrine\Common\Annotations\Reader; use PHPUnit\Framework\TestCase; use Psr\Cache\CacheItemPoolInterface; +use Symfony\Bridge\PhpUnit\ExpectDeprecationTrait; +use Symfony\Component\Validator\Mapping\Loader\AnnotationLoader; use Symfony\Component\Validator\ValidatorBuilder; class ValidatorBuilderTest extends TestCase { + use ExpectDeprecationTrait; + /** * @var ValidatorBuilder */ @@ -74,9 +80,74 @@ class ValidatorBuilderTest extends TestCase $this->assertSame($this->builder, $this->builder->addMethodMappings([])); } + /** + * @group legacy + */ public function testEnableAnnotationMapping() { + $this->expectDeprecation('Since symfony/validator 5.2: Not passing true as first argument to "Symfony\Component\Validator\ValidatorBuilder::enableAnnotationMapping" is deprecated. Pass true and call "addDefaultDoctrineAnnotationReader()" if you want to enable annotation mapping with Doctrine Annotations.'); $this->assertSame($this->builder, $this->builder->enableAnnotationMapping()); + + $loaders = $this->builder->getLoaders(); + $this->assertCount(1, $loaders); + $this->assertInstanceOf(AnnotationLoader::class, $loaders[0]); + + $r = new \ReflectionProperty(AnnotationLoader::class, 'reader'); + $r->setAccessible(true); + + $this->assertInstanceOf(CachedReader::class, $r->getValue($loaders[0])); + } + + public function testEnableAnnotationMappingWithDefaultDoctrineAnnotationReader() + { + $this->assertSame($this->builder, $this->builder->enableAnnotationMapping(true)); + $this->assertSame($this->builder, $this->builder->addDefaultDoctrineAnnotationReader()); + + $loaders = $this->builder->getLoaders(); + $this->assertCount(1, $loaders); + $this->assertInstanceOf(AnnotationLoader::class, $loaders[0]); + + $r = new \ReflectionProperty(AnnotationLoader::class, 'reader'); + $r->setAccessible(true); + + $this->assertInstanceOf(CachedReader::class, $r->getValue($loaders[0])); + } + + /** + * @group legacy + */ + public function testEnableAnnotationMappingWithCustomDoctrineAnnotationReaderLegacy() + { + $reader = $this->createMock(Reader::class); + + $this->expectDeprecation('Since symfony/validator 5.2: Passing an instance of "'.\get_class($reader).'" as first argument to "Symfony\Component\Validator\ValidatorBuilder::enableAnnotationMapping" is deprecated. Pass true instead and call setDoctrineAnnotationReader() if you want to enable annotation mapping with Doctrine Annotations.'); + $this->assertSame($this->builder, $this->builder->enableAnnotationMapping($reader)); + + $loaders = $this->builder->getLoaders(); + $this->assertCount(1, $loaders); + $this->assertInstanceOf(AnnotationLoader::class, $loaders[0]); + + $r = new \ReflectionProperty(AnnotationLoader::class, 'reader'); + $r->setAccessible(true); + + $this->assertSame($reader, $r->getValue($loaders[0])); + } + + public function testEnableAnnotationMappingWithCustomDoctrineAnnotationReader() + { + $reader = $this->createMock(Reader::class); + + $this->assertSame($this->builder, $this->builder->enableAnnotationMapping(true)); + $this->assertSame($this->builder, $this->builder->setDoctrineAnnotationReader($reader)); + + $loaders = $this->builder->getLoaders(); + $this->assertCount(1, $loaders); + $this->assertInstanceOf(AnnotationLoader::class, $loaders[0]); + + $r = new \ReflectionProperty(AnnotationLoader::class, 'reader'); + $r->setAccessible(true); + + $this->assertSame($reader, $r->getValue($loaders[0])); } public function testDisableAnnotationMapping() diff --git a/src/Symfony/Component/Validator/ValidatorBuilder.php b/src/Symfony/Component/Validator/ValidatorBuilder.php index e85bba34d1..59698f327c 100644 --- a/src/Symfony/Component/Validator/ValidatorBuilder.php +++ b/src/Symfony/Component/Validator/ValidatorBuilder.php @@ -17,7 +17,6 @@ use Doctrine\Common\Annotations\Reader; use Doctrine\Common\Cache\ArrayCache; use Psr\Cache\CacheItemPoolInterface; use Symfony\Component\Validator\Context\ExecutionContextFactory; -use Symfony\Component\Validator\Exception\LogicException; use Symfony\Component\Validator\Exception\ValidatorException; use Symfony\Component\Validator\Mapping\Factory\LazyLoadingMetadataFactory; use Symfony\Component\Validator\Mapping\Factory\MetadataFactoryInterface; @@ -53,6 +52,7 @@ class ValidatorBuilder * @var Reader|null */ private $annotationReader; + private $enableAnnotationMapping = false; /** * @var MetadataFactoryInterface|null @@ -212,23 +212,28 @@ class ValidatorBuilder /** * Enables annotation based constraint mapping. * + * @param bool $skipDoctrineAnnotations + * * @return $this */ - public function enableAnnotationMapping(Reader $annotationReader = null) + public function enableAnnotationMapping(/* bool $skipDoctrineAnnotations = true */) { if (null !== $this->metadataFactory) { throw new ValidatorException('You cannot enable annotation mapping after setting a custom metadata factory. Configure your metadata factory instead.'); } - if (null === $annotationReader) { - if (!class_exists(AnnotationReader::class) || !class_exists(ArrayCache::class)) { - throw new LogicException('Enabling annotation based constraint mapping requires the packages doctrine/annotations and doctrine/cache to be installed.'); - } - - $annotationReader = new CachedReader(new AnnotationReader(), new ArrayCache()); + $skipDoctrineAnnotations = 1 > \func_num_args() ? false : func_get_arg(0); + if (false === $skipDoctrineAnnotations || null === $skipDoctrineAnnotations) { + trigger_deprecation('symfony/validator', '5.2', 'Not passing true as first argument to "%s" is deprecated. Pass true and call "addDefaultDoctrineAnnotationReader()" if you want to enable annotation mapping with Doctrine Annotations.', __METHOD__); + $this->addDefaultDoctrineAnnotationReader(); + } elseif ($skipDoctrineAnnotations instanceof Reader) { + trigger_deprecation('symfony/validator', '5.2', 'Passing an instance of "%s" as first argument to "%s" is deprecated. Pass true instead and call setDoctrineAnnotationReader() if you want to enable annotation mapping with Doctrine Annotations.', get_debug_type($skipDoctrineAnnotations), __METHOD__); + $this->setDoctrineAnnotationReader($skipDoctrineAnnotations); + } elseif (true !== $skipDoctrineAnnotations) { + throw new \TypeError(sprintf('"%s": Argument 1 is expected to be a boolean, "%s" given.', __METHOD__, get_debug_type($skipDoctrineAnnotations))); } - $this->annotationReader = $annotationReader; + $this->enableAnnotationMapping = true; return $this; } @@ -240,11 +245,32 @@ class ValidatorBuilder */ public function disableAnnotationMapping() { + $this->enableAnnotationMapping = false; $this->annotationReader = null; return $this; } + /** + * @return $this + */ + public function setDoctrineAnnotationReader(?Reader $reader): self + { + $this->annotationReader = $reader; + + return $this; + } + + /** + * @return $this + */ + public function addDefaultDoctrineAnnotationReader(): self + { + $this->annotationReader = new CachedReader(new AnnotationReader(), new ArrayCache()); + + return $this; + } + /** * Sets the class metadata factory used by the validator. * @@ -252,7 +278,7 @@ class ValidatorBuilder */ public function setMetadataFactory(MetadataFactoryInterface $metadataFactory) { - if (\count($this->xmlMappings) > 0 || \count($this->yamlMappings) > 0 || \count($this->methodMappings) > 0 || null !== $this->annotationReader) { + if (\count($this->xmlMappings) > 0 || \count($this->yamlMappings) > 0 || \count($this->methodMappings) > 0 || $this->enableAnnotationMapping) { throw new ValidatorException('You cannot set a custom metadata factory after adding custom mappings. You should do either of both.'); } @@ -346,7 +372,7 @@ class ValidatorBuilder $loaders[] = new StaticMethodLoader($methodName); } - if ($this->annotationReader) { + if ($this->enableAnnotationMapping) { $loaders[] = new AnnotationLoader($this->annotationReader); }