-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[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
[Validation][FrameworkBundle] Allow EnableAutoMapping to work without auto-mapping namespaces #34707
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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)) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could we clarify in
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 |
||
$container->removeDefinition('validator.property_info_loader'); | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change | ||||||
---|---|---|---|---|---|---|---|---|
|
@@ -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()); | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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 EDIT: see second commit There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 symfony/src/Symfony/Component/Validator/Mapping/Loader/PropertyInfoLoader.php Lines 50 to 52 in bbda69d
Or do you mean handle this as a bug in 4.3? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. /cc @dunglas WDYT? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. see also #33828 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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). |
||||||||
} | ||||||||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
{.*}
or just//
, but OK :)