-
-
Notifications
You must be signed in to change notification settings - Fork 9.8k
[Validator] remove support for generic constraint option handling #61063
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
Conversation
b8ba9e7 to
0c03730
Compare
| } | ||
|
|
||
| $this->constraints = $this->getConstraints($this->normalizeOptions($options)); | ||
| $this->constraints = $this->getConstraints($options ?? []); |
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.
We probably need to deprecate using options in this Compound abstract class first, to be able to remove support properly.
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.
Indeed, that's not a really useful feature anymore. I'm gonna send a PR for the deprecation targeting the 7.4 branch.
| parent::__construct(null, $groups, $payload); | ||
| } | ||
|
|
||
| protected function getCompositeOption(): string |
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.
this method should also be removed, as it does not make sense anymore once the parent does not handle an array of options anymore.
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.
getCompositeOption() is used independently from the $options array in the constructor of the Composite base class:
symfony/src/Symfony/Component/Validator/Constraints/Composite.php
Lines 64 to 115 in b49a855
| foreach ((array) $this->getCompositeOption() as $option) { | |
| /** @var Constraint[] $nestedConstraints */ | |
| $nestedConstraints = $this->$option; | |
| if (!\is_array($nestedConstraints)) { | |
| $nestedConstraints = [$nestedConstraints]; | |
| } | |
| foreach ($nestedConstraints as $constraint) { | |
| if (!$constraint instanceof Constraint) { | |
| if (\is_object($constraint)) { | |
| $constraint = get_debug_type($constraint); | |
| } | |
| throw new ConstraintDefinitionException(\sprintf('The value "%s" is not an instance of Constraint in constraint "%s".', $constraint, get_debug_type($this))); | |
| } | |
| if ($constraint instanceof Valid) { | |
| throw new ConstraintDefinitionException(\sprintf('The constraint Valid cannot be nested inside constraint "%s". You can only declare the Valid constraint directly on a field or method.', get_debug_type($this))); | |
| } | |
| } | |
| if (!isset(((array) $this)['groups'])) { | |
| $mergedGroups = []; | |
| foreach ($nestedConstraints as $constraint) { | |
| foreach ($constraint->groups as $group) { | |
| $mergedGroups[$group] = true; | |
| } | |
| } | |
| // prevent empty composite constraint to have empty groups | |
| $this->groups = array_keys($mergedGroups) ?: [self::DEFAULT_GROUP]; | |
| $this->$option = $nestedConstraints; | |
| continue; | |
| } | |
| foreach ($nestedConstraints as $constraint) { | |
| if (isset(((array) $constraint)['groups'])) { | |
| $excessGroups = array_diff($constraint->groups, $this->groups); | |
| if (\count($excessGroups) > 0) { | |
| throw new ConstraintDefinitionException(\sprintf('The group(s) "%s" passed to the constraint "%s" should also be passed to its containing constraint "%s".', implode('", "', $excessGroups), get_debug_type($constraint), get_debug_type($this))); | |
| } | |
| } else { | |
| $constraint->groups = $this->groups; | |
| } | |
| } | |
| $this->$option = $nestedConstraints; | |
| } |
The important part when not being able anymore to use the options array is that the property/properties referenced with getCompositeOption() are initialised in child class before the base constructor is called (that's part of the upgrade/changelog files).
src/Symfony/Component/Validator/Constraints/PasswordStrength.php
Outdated
Show resolved
Hide resolved
5428b30 to
e934b07
Compare
71a8b66 to
8397d33
Compare
17fcad8 to
9df2c71
Compare
|
This is ready to be reviewed. Status: Needs Review |
nicolas-grekas
left a comment
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.
Wow, such nice diff! Thanks for working on this!
| public function __construct(?array $options = null, ?string $message = null, ?array $groups = null, mixed $payload = null) | ||
| { | ||
| if (\is_array($options)) { | ||
| trigger_deprecation('symfony/validator', '7.3', 'Passing an array of options to configure the "%s" constraint is deprecated, use named arguments instead.', static::class); |
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.
in cases like this, we still have a legacy $options argument
would it make sense to try to remove it, using some BC layer?
or should we throw to be sure passing anything else than null is forbidden?
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.
We cannot easily detect if options are passed when changing the order of arguments in 7.4 (we can try again in 8.1). But I added checks for $options to be null and throw otherwise.
9df2c71 to
28644a9
Compare
28644a9 to
f70d261
Compare
f70d261 to
01771b4
Compare
nicolas-grekas
left a comment
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.
I'm wondering about how we could remove those $options arguments, but let's figure this out later (if there's a simple enough way)
About Choice, I made a comment in #61255, no blocker for this one.
Thanks for tackling this!
01771b4 to
a85e03b
Compare
a85e03b to
09299fc
Compare
|
Thank you @xabbuh. |
opening the PR already even if some polishing needs to be done (and not all tests pass yet) to avoid someone else wasting their time on this topic