[Form] add union types #41998
Merged
[Form] add union types #41998
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Member
nicolas-grekas
commented
Jul 6, 2021
| Q | A |
|---|---|
| Branch? | 6.0 |
| Bug fix? | no |
| New feature? | no |
| Deprecations? | no |
| Tickets | Part of #41424 |
| License | MIT |
| Doc PR | - |
e5f1b12 to
2259027
Compare
chalasr
reviewed
Jul 6, 2021
459f57c to
6fe6d29
Compare
|
I would appreciate a closer review on this one:
I'd like a closer check on this second commit please. /cc @HeahDude in case you're around, since this touches ChoiceType :) |
6fe6d29 to
2493e24
Compare
HeahDude
reviewed
Jul 6, 2021
| @@ -303,7 +301,7 @@ public function handleRequest($request = null); | |||
| * | |||
| * @throws Exception\AlreadySubmittedException if the form has already been submitted | |||
| */ | |||
| public function submit($submittedData, bool $clearMissing = true); | |||
| public function submit(string|array|null $submittedData, bool $clearMissing = true); | |||
There was a problem hiding this comment.
I think we should deprecate not passing anything else before doing this, or we might break some code, I'm even surprised that tests are passing.
There was a problem hiding this comment.
This is the interface: implementations can still accept more types. See eg Form, where I put mixed here.
src/Symfony/Component/Form/ChoiceList/Factory/CachingFactoryDecorator.php
Show resolved
Hide resolved
HeahDude
reviewed
Jul 7, 2021
| @@ -259,7 +259,7 @@ private static function resolveValidationGroups(string|GroupSequence|array|calla | |||
| return (array) $groups; | |||
| } | |||
|
|
|||
| private static function getConstraintsInGroups(array $constraints, string $group): array | |||
| private static function getConstraintsInGroups(array $constraints, string|array $group): array | |||
There was a problem hiding this comment.
if $group can be an array then the implementation looks buggy to me, shouldn't it be something like:
return array_filter($constraints, static function (Constraint $constraint) use ($group) {
return \is_array($group) ? 0 < array_intersect($group, $contraint->groups)
: \in_array($group, $constraint->groups, true);
});There was a problem hiding this comment.
Method updated to deal with arrays.
In theory it could also be a GroupSequence, but the code doesn't deal with that.
Would you like to have a look?
2493e24 to
c8a1d0c
Compare
nicolas-grekas
commented
Jul 7, 2021
| @@ -57,7 +57,7 @@ class GroupSequence | |||
| /** | |||
| * The groups in the sequence. | |||
| * | |||
| * @var string[]|string[][]|GroupSequence[] | |||
| * @var array<string|string[]|GroupSequence> | |||
There was a problem hiding this comment.
/cc @HeahDude here you see that nested GroupSequences might be legal
This was referenced Jul 7, 2021
c8a1d0c to
0480be6
Compare
Tobion
added a commit
that referenced
this pull request
Jul 17, 2021
This PR was merged into the 5.2 branch. Discussion ---------- [Form] backport type fixes | Q | A | ------------- | --- | Branch? | 5.2 | Bug fix? | no | New feature? | no | Deprecations? | no | Tickets | - | License | MIT | Doc PR | - Backported from #41998 Commits ------- 75691c6 [Form] backport type fixes
nicolas-grekas
added a commit
that referenced
this pull request
Jul 19, 2021
This PR was merged into the 4.4 branch. Discussion ---------- [Form] fix some type annotations | Q | A | ------------- | --- | Branch? | 4.4 | Bug fix? | no | New feature? | no | Deprecations? | no | Tickets | - | License | MIT | Doc PR | - Backported from #41998 The change in `FormValidator` is likely a bug fix, but it could be an incomplete one. `testFieldsValidateInSequenceWithNestedGroupsArray` triggers a call to `getConstraintsInGroups` with an array. I'd appreciate if `@HeahDude`, `@xabbuh`, or anyone with a better knowledge of Form could have a look please. Commits ------- e49441d [Form] fix some type annotations
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.