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

ogizanagi · 2019-11-29T10:41:58Z

Q	A
Branch?	4.4
Bug fix?	yes?
New feature?	no
Deprecations?	no
Tickets	N/A
License	MIT
Doc PR	N/A

Currently, when no framework.validation.auto_mapping namespaces are configured, the EnableAutoMapping annotation has no effect on its own, because the loaders were removed by the fwb extension and the compiler pass.
Simply adding a namespace (even a foolish one) will make it work:

    validation:
        auto_mapping:
            '*': ~ # this does not really map anything, but allows `EnableAutoMapping` to work as loaders won't be removed.

So for those only wanting to use the auto-mapping feature by explicitly setting the EnableAutoMapping annotation, it'll be counter-intuitive, as it cannot work without declaring at least one namespace.

ogizanagi · 2019-11-29T10:45:54Z

src/Symfony/Component/Validator/Mapping/Loader/AutoMappingTrait.php

@@ -36,6 +36,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());


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

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

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:

symfony/src/Symfony/Component/Validator/Mapping/Loader/PropertyInfoLoader.php

Lines 50 to 52 in bbda69d

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

return false;

}

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

/cc @dunglas WDYT?

see also #33828

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

weaverryan · 2019-12-06T16:50:28Z

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php

@@ -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)) {


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.

ogizanagi · 2019-12-09T08:52:11Z

Alright, I've removed the BC layer and documented the BC break & alternative in CHANGELOG & UPGRADE files.

I'd suggest to:

allow to disable auto-mapping entirely by setting framework.validation.auto_mapping: false in another PR. It's not critical to me and could go in 5.1. Auto-mapping is for now enabled by default, services are loaded, but have no effect unless the EnableAutoMapping constraint is set, or a configured namespace matches.
backport ->info() and add ->example() in 4.3 configuration following @weaverryan's suggestion in [Validation][FrameworkBundle] Allow EnableAutoMapping to work without auto-mapping namespaces #34707 (comment)

Failure high is expected. First build is failing but does not seems related to these changes.

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Configuration.php

nicolas-grekas · 2019-12-15T12:38:44Z

UPGRADE-4.4.md

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


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

…fig (ogizanagi) This PR was merged into the 4.3 branch. Discussion ---------- [FrameworkBundle] Add info & example to auto_mapping config | Q | A | ------------- | --- | Branch? | 4.3  | Bug fix? | no | New feature? | no  | Deprecations? | no  | Tickets | N/A  | License | MIT | Doc PR | N/A Backport part of #34707 and add example. Commits ------- 8eb29a7 [FrameworkBundle] Add info & example to auto_mapping config

… auto-mapping namespaces

…ork without auto-mapping namespaces (ogizanagi) This PR was squashed before being merged into the 4.4 branch (closes #34707). Discussion ---------- [Validation][FrameworkBundle] Allow EnableAutoMapping to work without auto-mapping namespaces | Q | A | ------------- | --- | Branch? | 4.4  | Bug fix? | yes? | New feature? | no  | Deprecations? | no  | Tickets | N/A  | License | MIT | Doc PR | N/A Currently, when no `framework.validation.auto_mapping` namespaces are configured, the `EnableAutoMapping` annotation has no effect on its own, because the loaders were removed by the fwb extension and the compiler pass. Simply adding a namespace (even a foolish one) will make it work: ```yml validation: auto_mapping: '*': ~ # this does not really map anything, but allows `EnableAutoMapping` to work as loaders won't be removed. ``` So for those only wanting to use the auto-mapping feature by explicitly setting the `EnableAutoMapping` annotation, it'll be counter-intuitive, as it cannot work without declaring at least one namespace. Commits ------- 00b46fa [Validation][FrameworkBundle] Allow EnableAutoMapping to work without auto-mapping namespaces

ogizanagi added the Status: Needs Work label Nov 29, 2019

carsonbot added Status: Needs Review FrameworkBundle Bug and removed Status: Needs Work labels Nov 29, 2019

ogizanagi commented Nov 29, 2019

View reviewed changes

ogizanagi added the BC Break label Nov 29, 2019

nicolas-grekas added this to the 4.4 milestone Nov 29, 2019

ogizanagi added DoctrineBridge and removed BC Break labels Nov 29, 2019

ogizanagi mentioned this pull request Dec 6, 2019

[RFC][Validator] Change Validator auto_mapping to opt-in #34860

Closed

weaverryan reviewed Dec 6, 2019

View reviewed changes

ogizanagi added the Deprecation label Dec 7, 2019

dunglas approved these changes Dec 8, 2019

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels Dec 8, 2019

ogizanagi added BC Break and removed Deprecation labels Dec 9, 2019

ogizanagi mentioned this pull request Dec 9, 2019

[FrameworkBundle] Add info & example to auto_mapping config #34887

Merged

weaverryan approved these changes Dec 9, 2019

View reviewed changes

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Configuration.php Outdated Show resolved Hide resolved

nicolas-grekas approved these changes Dec 15, 2019

View reviewed changes

nicolas-grekas added the Ready label Dec 15, 2019

[Validation][FrameworkBundle] Allow EnableAutoMapping to work without…

00b46fa

… auto-mapping namespaces

ogizanagi merged commit 00b46fa into symfony:4.4 Dec 17, 2019

ogizanagi deleted the fwb_valid_automapping_dont_rm_loaders branch December 17, 2019 08:15

This was referenced Dec 19, 2019

Release v4.4.2 #35053

Merged

Release v5.0.2 #35055

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

ogizanagi commented Nov 29, 2019 •

edited

ogizanagi Nov 29, 2019 •

edited

nicolas-grekas Nov 29, 2019

ogizanagi Nov 29, 2019 •

edited

nicolas-grekas Nov 29, 2019

xabbuh Nov 29, 2019

dunglas Dec 8, 2019

weaverryan Dec 6, 2019

ogizanagi commented Dec 9, 2019

nicolas-grekas Dec 15, 2019

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

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

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

Conversation

ogizanagi commented Nov 29, 2019 • edited

ogizanagi Nov 29, 2019 • edited

Choose a reason for hiding this comment

nicolas-grekas Nov 29, 2019

Choose a reason for hiding this comment

ogizanagi Nov 29, 2019 • edited

Choose a reason for hiding this comment

nicolas-grekas Nov 29, 2019

Choose a reason for hiding this comment

xabbuh Nov 29, 2019

Choose a reason for hiding this comment

dunglas Dec 8, 2019

Choose a reason for hiding this comment

weaverryan Dec 6, 2019

Choose a reason for hiding this comment

ogizanagi commented Dec 9, 2019

nicolas-grekas Dec 15, 2019

Choose a reason for hiding this comment

ogizanagi commented Nov 29, 2019 •

edited

ogizanagi Nov 29, 2019 •

edited

ogizanagi Nov 29, 2019 •

edited