-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[Validator] Add support of nested attributes #41994
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
[Validator] Add support of nested attributes #41994
Conversation
Hey! I think @przemyslaw-bogusz has recently worked with this code. Maybe they can help review this? Cheers! Carsonbot |
As this is a new feature it should target |
Alright, I'll rebase to 5.4! |
]), | ||
Assert\Choice(choices: ['A', 'B'], message: 'Must be one of %choices%') | ||
] | ||
public $firstName; |
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.
the Ci reports PHP Fatal error: Constant expression contains invalid operations
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.
Let's wait for the RFC to be merged before merging :)
Assert\All([ | ||
'constraints' => [ | ||
new Assert\NotNull(), | ||
new Assert\Range(min: 3), | ||
], | ||
]), |
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.
Assert\All([ | |
'constraints' => [ | |
new Assert\NotNull(), | |
new Assert\Range(min: 3), | |
], | |
]), | |
Assert\All( | |
constraints: [ | |
new Assert\NotNull(), | |
new Assert\Range(min: 3), | |
], | |
), |
Let's switch to named arguments, like we did for the other constraints.
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.
Done!
@@ -185,5 +185,9 @@ public function provideNamespaces(): iterable | |||
if (\PHP_VERSION_ID >= 80000) { | |||
yield 'attributes' => ['Symfony\Component\Validator\Tests\Fixtures\Attribute']; | |||
} | |||
|
|||
if (\PHP_VERSION_ID >= 80100) { | |||
yield 'nested_attributes' => ['Symfony\Component\Validator\Tests\Fixtures\Attribute\Nested']; |
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.
yield 'nested_attributes' => ['Symfony\Component\Validator\Tests\Fixtures\Attribute\Nested']; | |
yield 'nested_attributes' => ['Symfony\Component\Validator\Tests\Fixtures\NestedAttribute']; |
Let's keep the directory structure flat
11508eb
to
bc996cc
Compare
Added missing tests for
|
bc996cc
to
6635f91
Compare
0a9b7f5
to
1a34fc3
Compare
In order to get the PHP 8.0 testsuite green, you need to exclude your new fixtures in |
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.
LGTM, once patch-types.php has been updated.
src/Symfony/Component/Validator/Tests/Mapping/Loader/AnnotationLoaderTest.php
Outdated
Show resolved
Hide resolved
Will do, thanks! |
1a34fc3
to
1449450
Compare
This is now available in the 8.1 PHP branch. |
Thank you @alexandre-daubois. |
…constraints (alexandre-daubois) This PR was merged into the 5.4 branch. Discussion ---------- [Validator] Add attributes documentation of composite constraints Related pull-request: symfony/symfony#41994 Not really sure how to precise it only works for attributes in PHP 8.1, not 8.0. If you have a solution, please let me know! Commits ------- 7099e3f [Validator] Add attributes documentation of composite constraints
@alexandre-daubois @fabpot |
@chekalsky Did you experience any issues when using |
], | ||
'bar' => new Assert\Range(min: 5), | ||
'baz' => new Assert\Required([new Assert\Email()]), | ||
'qux' => new Assert\Optional([new Assert\NotBlank()]), |
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.
@chekalsky this is the test I mean
@xabbuh Yes, but only when it is not nested (e.g. I am trying to make one of the properties itself optional). Does it make sense? |
Well, |
…ructor (derrabus) This PR was merged into the 7.2 branch. Discussion ---------- [Validator] Add $groups and $payload to Compound constructor | Q | A | ------------- | --- | Branch? | 7.2 | Bug fix? | no | New feature? | yes | Deprecations? | no | Issues | N/A | License | MIT While reviewing #49547 I noticed that the `$groups` and `$payload` parameters introduced in #41994 cannot be leveraged in compound constraints. I'd like to change that. Commits ------- bf4207c [Validator] Add $groups and $payload to Compound constructor
Although the RFC (https://wiki.php.net/rfc/new_in_initializers) is in the voting phase (until 14 July), it is already well on its way to passing.
Based on @nikic's development (php/php-src#7153), this makes the development of support possible. It will obviously take a little while before this pull request is merged.
If this pull request is OK for you, I'll get to work on writing the existing documentation for the attribute validation constraints.
Not sure about the Symfony version to target, as
AtLeastOneOf
has been introduced in 5.1. Although, I couldn't find attributes validation documentation for 4.4.