[Form] fix some type annotations #42012
Conversation
| @@ -56,7 +56,7 @@ class GroupSequence | ||
| /** | ||
| * The groups in the sequence. | ||
| * | ||
| * @var string[]|string[][]|GroupSequence[] | ||
| * @var array<string|string[]|GroupSequence> |
There was a problem hiding this comment.
Do we actually have tests that confirm that nested group sequences did ever fully work?
There was a problem hiding this comment.
We don't, because constraints are empty on the only test case that does so... But a deep TypeError looks like something is like something is missing. Maybe just a check with a clear error message. The doc mentions nested groups btw.
There was a problem hiding this comment.
Nested groups should be okay. The GroupSequence part looks suspicious to me. What error would we get when changing the annotation to array<string|string[]>|GroupSequence?
There was a problem hiding this comment.
RecursiveContextualValidator has many instanceof GroupSequence checks that map to this possibility. But FormValidator doesn't handle it at all.
|
PR updated according to https://github.com/symfony/symfony/pull/42013/files#r666967532 |
2817fed to
868410f
Compare
868410f to
e49441d
Compare
|
See rationale at #42013 (comment) |
| * | ||
| * @return ChoiceListView The choice list view | ||
| * @return ChoiceListView | ||
| */ | ||
| public function createView(ChoiceListInterface $list, $preferredChoices = null, $label = null, $index = null, $groupBy = null, $attr = null) |
There was a problem hiding this comment.
I mean using mixed for a decorator makes sense as the class does not know what it decorates that might have widened the range of accepted parameters types.
But in theory this applies to all arguments. So in theory you would also need to remove the type from ChoiceListInterface $list and accept mixed there. This might go to far. But it's a thought expirement.
There was a problem hiding this comment.
This might go to far. But it's a thought experiment.
I agree with this :)
| @@ -260,8 +260,16 @@ private static function resolveValidationGroups($groups, FormInterface $form) | ||
|
|
||
| private static function getConstraintsInGroups($constraints, $group) | ||
| { | ||
| return array_filter($constraints, static function (Constraint $constraint) use ($group) { | ||
| return \in_array($group, $constraint->groups, true); | ||
| $groups = (array) $group; |
There was a problem hiding this comment.
This is coming from \Symfony\Component\Validator\Constraints\GroupSequence::$groups and according to it's type, $group itself could also be a GroupSequence again. So there is still something not right.
There was a problem hiding this comment.
That's true, yet nobody reported this as a bug ever, so that the urgency on this is zero on my side :)
There was a problem hiding this comment.
Using a nested array seems to be the same as using a nested GroupSequence. If the nested group sequence is not supported, we probably should just remove the type from GroupSequence::$groups. It's also not documented in https://symfony.com/doc/current/validation/sequence_provider.html
Backported from #41998
The change in
FormValidatoris likely a bug fix, but it could be an incomplete one.testFieldsValidateInSequenceWithNestedGroupsArraytriggers a call togetConstraintsInGroupswith an array.I'd appreciate if @HeahDude, @xabbuh, or anyone with a better knowledge of Form could have a look please.