10000 [Validation][FrameworkBundle] Allow EnableAutoMapping to work without auto-mapping namespaces by ogizanagi · Pull Request #34707 · symfony/symfony · GitHub
[go: up one dir, main page]

Skip to content

[Validation][FrameworkBundle] Allow EnableAutoMapping to work without auto-mapping namespaces #34707

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.

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 17, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
5 changes: 3 additions & 2 deletions UPGRADE-4.4.md
Original file line number Diff line number Diff line change
Expand Up @@ -164,8 +164,8 @@ Lock
* Deprecated `Symfony\Component\Lock\StoreInterface` in favor of `Symfony\Component\Lock\BlockingStoreInterface` and
`Symfony\Component\Lock\PersistingStoreInterface`.
* `Factory` is deprecated, use `LockFactory` instead
* Deprecated services `lock.store.flock`, `lock.store.semaphore`, `lock.store.memcached.abstract` and `lock.store.redis.abstract`,
use `StoreFactory::createStore` instead.
* Deprecated services `lock.store.flock`, `lock.store.semaphore`, `lock.store.memcached.abstract` and `lock.store.redis.abstract`,
use `StoreFactory::createStore` instead.

Mailer
------
Expand Down Expand Up @@ -360,6 +360,7 @@ TwigBundle
Validator
---------

* [BC BREAK] Using null as `$classValidatorRegexp` value in `DoctrineLoader::__construct` or `PropertyInfoLoader::__construct` will not enable auto-mapping for all classes anymore, use `'{.*}'` instead.
Copy link
Member

Choose a reason for hiding this comment

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

{.*} or just //, but OK :)

* Deprecated passing an `ExpressionLanguage` instance as the second argument of `ExpressionValidator::__construct()`.
* Deprecated using anything else than a `string` as the code of a `ConstraintViolation`, a `string` type-hint will
be added to the constructor of the `ConstraintViolation` class and to the `ConstraintViolationBuilder::setCode()`
Expand Down
1 change: 1 addition & 0 deletions src/Symfony/Bridge/Doctrine/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ CHANGELOG
4.4.0
-----

* [BC BREAK] using null as `$classValidatorRegexp` value in `DoctrineLoader::__construct` will not enable auto-mapping for all classes anymore, use `'{.*}'` instead.
* added `DoctrineClearEntityManagerWorkerSubscriber`
* deprecated `RegistryInterface`, use `Doctrine\Persistence\ManagerRegistry`
* added support for invokable event listeners
Expand Down
16 changes: 4 additions & 12 deletions src/Symfony/Bridge/Doctrine/Tests/Validator/DoctrineLoaderTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,6 @@
use Symfony\Component\Validator\Mapping\TraversalStrategy;
use Symfony\Component\Validator\Tests\Fixtures\Entity;
use Symfony\Component\Validator\Validation;
use Symfony\Component\Validator\ValidatorBuilder;

/**
* @author Kévin Dunglas <dunglas@gmail.com>
Expand Down Expand Up @@ -152,10 +151,6 @@ public function testLoadClassMetadata()

public function testFieldMappingsConfiguration()
{
if (!method_exists(ValidatorBuilder::class, 'addLoader')) {
$this->markTestSkipped('Auto-mapping requires symfony/validation 4.2+');
}

$validator = Validation::createValidatorBuilder()
->addMethodMapping('loadValidatorMetadata')
->enableAnnotationMapping()
Expand All @@ -180,7 +175,7 @@ public function testFieldMappingsConfiguration()
*/
public function testClassValidator(bool $expected, string $classValidatorRegexp = null)
{
$doctrineLoader = new DoctrineLoader(DoctrineTestHelper::createTestEntityManager(), $classValidatorRegexp);
$doctrineLoader = new DoctrineLoader(DoctrineTestHelper::createTestEntityManager(), $classValidatorRegexp, false);

$classMetadata = new ClassMetadata(DoctrineLoaderEntity::class);
$this->assertSame($expected, $doctrineLoader->loadClassMetadata($classMetadata));
Expand All @@ -189,21 +184,18 @@ public function testClassValidator(bool $expected, string $classValidatorRegexp
public function regexpProvider()
{
return [
[true, null],
[false, null],
[true, '{.*}'],
[true, '{^'.preg_quote(DoctrineLoaderEntity::class).'$|^'.preg_quote(Entity::class).'$}'],
[false, '{^'.preg_quote(Entity::class).'$}'],
];
}

public function testClassNoAutoMapping()
{
if (!method_exists(ValidatorBuilder::class, 'addLoader')) {
$this->markTestSkipped('Auto-mapping requires symfony/validation 4.2+');
}

$validator = Validation::createValidatorBuilder()
->enableAnnotationMapping()
->addLoader(new DoctrineLoader(DoctrineTestHelper::createTestEntityManager()))
->addLoader(new DoctrineLoader(DoctrineTestHelper::createTestEntityManager(), '{.*}'))
->getValidator();

$classMetadata = $validator->getMetadataFor(new DoctrineLoaderNoAutoMappingEntity());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -861,7 +861,7 @@ private function addValidationSection(ArrayNodeDefinition $rootNode)
->end()
->end()
->arrayNode('auto_mapping')
->info('A collection of namespaces for which auto-mapping will be enabled.')
->info('A collection of namespaces for which auto-mapping will be enabled by default, or null to opt-in with the EnableAutoMapping constraint.')
->example([
'App\\Entity\\' => [],
'App\\WithSpecificLoaders\\' => ['validator.property_info_loader'],
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1312,7 +1312,7 @@ private function registerValidationConfiguration(array $config, ContainerBuilder
}

$container->setParameter('validator.auto_mapping', $config['auto_mapping']);
if (!$propertyInfoEnabled || !$config['auto_mapping'] || !class_exists(PropertyInfoLoader::class)) {
if (!$propertyInfoEnabled || !class_exists(PropertyInfoLoader::class)) {
Copy link
Member

Choose a reason for hiding this comment

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

Could we clarify in Configuration (maybe with an ->info()) the valid values for auto_mapping and what they mean?

  • Pass an array to enable auto-mapping & activate it automatically for matching namespaces
  • Pass null to turn the auto-mapping feature on only and activate with config

The wording/explanation might be more clear if we (in this PR) add a proper enable/disable flag to this config. And maybe instead of auto_mapping: ~ we make it be auto_mapping: true.

$container->removeDefinition('validator.property_info_loader');
}

Expand Down
7 changes: 4 additions & 3 deletions src/Symfony/Component/Validator/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,19 +4,20 @@ CHANGELOG
4.4.0
-----

* added `EnableAutoMapping` and `DisableAutoMapping` constraints to enable or disable auto mapping for class or a property
* [BC BREAK] using null as `$classValidatorRegexp` value in `PropertyInfoLoader::__construct` will not enable auto-mapping for all classes anymore, use `'{.*}'` instead.
* added `EnableAutoMapping` and `DisableAutoMapping` constraints to enable or disable auto mapping for class or a property
* using anything else than a `string` as the code of a `ConstraintViolation` is deprecated, a `string` type-hint will
be added to the constructor of the `ConstraintViolation` class and to the `ConstraintViolationBuilder::setCode()`
method in 5.0
* deprecated passing an `ExpressionLanguage` instance as the second argument of `ExpressionValidator::__construct()`. Pass it as the first argument instead.
* added the `compared_value_path` parameter in violations when using any
* 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`.
* Added new `minPropertyPath` and `maxPropertyPath` options
to `Range` constraint in order to get the value to compare
from an array or object
* added the `min_limit_path` and `max_limit_path` parameters in violations when using
* added the `min_limit_path` and `max_limit_path` parameters in violations when using
`Range` constraint with respectively the `minPropertyPath` and
`maxPropertyPath` options
* added a new `notInRangeMessage` option to the `Range` constraint that will
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -59,13 +59,8 @@ public function process(ContainerBuilder $container)
$validatorBuilder = $container->getDefinition($this->validatorBuilderService);
foreach ($container->findTaggedServiceIds($this->tag) as $id => $tags) {
$regexp = $this->getRegexp(array_merge($globalNamespaces, $servicesToNamespaces[$id] ?? []));
if (null === $regexp) {
$container->removeDefinition($id);
continue;
}

$container->getDefinition($id)->setArgument('$classValidatorRegexp', $regexp);
$validatorBuilder->addMethodCall('addLoader', [new Reference($id)]);
$container->getDefinition($id)->setArgument('$classValidatorRegexp', $regexp);
}

$container->getParameterBag()->remove('validator.auto_mapping');
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,6 @@ private function isAutoMappingEnabledForClass(ClassMetadata $metadata, string $c
}

// Fallback on the config
return null === $classValidatorRegexp || preg_match($classValidatorRegexp, $metadata->getClassName());
return null !== $classValidatorRegexp && preg_match($classValidatorRegexp, $metadata->getClassName());
Copy link
Contributor Author
@ogizanagi ogizanagi Nov 29, 2019

Choose a reason for hiding this comment

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

⚠️ This is a BC break for the ones using the component directly:

  • before, setting $classValidatorRegexp to null would always consider the auto-mapping enabled for class by default
  • now, null will consider auto-mapping disabled for class by default, but can still be enabled by using EnableAutoMapping.

This needs a proper BC layer in any case, but I think the current way it works is not the most relevant. If you want it to be enabled by default for all classes, just use ".*". null should consider the auto-mapping feature not enabled for the class.

EDIT: see second commit

Copy link
Member

Choose a reason for hiding this comment

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

What about just doing the BC break? The trait is new to 4.4.0, let's handle this as a bug.

Copy link
Contributor Author
@ogizanagi ogizanagi Nov 29, 2019

Choose a reason for hiding this comment

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

That's not about the trait actually. That's about 4.3 behavior of Doctrine/PropertyInfo loaders with a null regex. On 4.3 it was expected to enable the automapping with a null regex:

if (null !== $this->classValidatorRegexp && !preg_match($this->classValidatorRegexp, $className)) {
return false;
}

Or do you mean handle this as a bug in 4.3?

Copy link
Member

Choose a reason for hiding this comment

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

/cc @dunglas WDYT?

Copy link
Member

Choose a reason for hiding this comment

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

see also #33828

Copy link
Member

Choose a reason for hiding this comment

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

this behavior already changed several times so... I'm in favor of introducing this change now, even if it's a real BC break (we'll have to document this in UPGRADE.md).

}
}
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,6 @@ public function testDoNotMapAllClassesWhenConfigIsEmpty()

(new AddAutoMappingConfigurationPass())->process($container);

$this->assertFalse($container->hasDefinition('loader'));
$this->assertNull($container->getDefinition('loader')->getArguments()['$classValidatorRegexp'] ?? null);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ public function testLoadClassMetadata()
))
;

$propertyInfoLoader = new PropertyInfoLoader($propertyInfoStub, $propertyInfoStub, $propertyInfoStub);
$propertyInfoLoader = new PropertyInfoLoader($propertyInfoStub, $propertyInfoStub, $propertyInfoStub, '{.*}');

$validator = Validation::createValidatorBuilder()
->enableAnnotationMapping()
Expand Down Expand Up @@ -197,7 +197,8 @@ public function testClassValidator(bool $expected, string $classValidatorRegexp
public function regexpProvider()
{
return [
[true, null],
[false, null],
[true, '{.*}'],
[true, '{^'.preg_quote(PropertyInfoLoaderEntity::class).'$|^'.preg_quote(Entity::class).'$}'],
[false, '{^'.preg_quote(Entity::class).'$}'],
];
Expand All @@ -217,7 +218,7 @@ public function testClassNoAutoMapping()
[new Type(Type::BUILTIN_TYPE_BOOL)]
);

$propertyInfoLoader = new PropertyInfoLoader($propertyInfoStub, $propertyInfoStub, $propertyInfoStub);
$propertyInfoLoader = new PropertyInfoLoader($propertyInfoStub, $propertyInfoStub, $propertyInfoStub, '{.*}');
$validator = Validation::createValidatorBuilder()
->enableAnnotationMapping()
->addLoader($propertyInfoLoader)
Expand Down
0