Skip to content

[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

Merged

Conversation

alexandre-daubois
Copy link
Member

@alexandre-daubois alexandre-daubois commented Jul 5, 2021

Q A
Branch? 5.4
Bug fix? no
New feature? yes
Deprecations? no
Tickets Fix #38503
License MIT
Doc PR symfony/symfony-docs#15541

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.

Capture d’écran du 2021-07-05 17-11-23

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.

@carsonbot
Copy link

Hey!

I think @przemyslaw-bogusz has recently worked with this code. Maybe they can help review this?

Cheers!

Carsonbot

@OskarStark OskarStark modified the milestones: 5.3, 5.4 Jul 6, 2021
@OskarStark
Copy link
Contributor

As this is a new feature it should target 5.4 branch

@alexandre-daubois
Copy link
Member Author

Alright, I'll rebase to 5.4!

@nicolas-grekas nicolas-grekas changed the base branch from 5.3 to 5.4 July 10, 2021 08:58
]),
Assert\Choice(choices: ['A', 'B'], message: 'Must be one of %choices%')
]
public $firstName;
Copy link
Member

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

Copy link
Member

@nicolas-grekas nicolas-grekas left a 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 :)

Comment on lines 35 to 40
Assert\All([
'constraints' => [
new Assert\NotNull(),
new Assert\Range(min: 3),
],
]),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

@derrabus derrabus added Feature and removed Bug labels Jul 18, 2021
@@ -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'];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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

@alexandre-daubois alexandre-daubois force-pushed the feat-nested-attributes-support branch 7 times, most recently from 11508eb to bc996cc Compare July 19, 2021 13:14
@alexandre-daubois
Copy link
Member Author

alexandre-daubois commented Jul 19, 2021

Added missing tests for Sequentially and AtLeastOneOf attributes.
Added support for Collection as well (https://github.com/symfony/symfony/pull/41994/files#diff-d9f229d35c24533de28e1e630ee4e74afa4ce27d7af8f137d788898e0b9efc73R41).
Added documentation pull-request.

Required and Optional don't need update. I still added them in AnnotationLeaderTest.

@derrabus
Copy link
Member

In order to get the PHP 8.0 testsuite green, you need to exclude your new fixtures in .github/patch-types.php.

Copy link
Member

@derrabus derrabus left a 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.

@alexandre-daubois
Copy link
Member Author

In order to get the PHP 8.0 testsuite green, you need to exclude your new fixtures in .github/patch-types.php.

Will do, thanks!

@alexandre-daubois alexandre-daubois force-pushed the feat-nested-attributes-support branch from 1a34fc3 to 1449450 Compare August 2, 2021 06:32
@fabpot
Copy link
Member

fabpot commented Aug 18, 2021

This is now available in the 8.1 PHP branch.

@fabpot
Copy link
Member

fabpot commented Aug 18, 2021

Thank you @alexandre-daubois.

@fabpot fabpot merged commit edecf96 into symfony:5.4 Aug 18, 2021
This was referenced Nov 5, 2021
wouterj added a commit to symfony/symfony-docs that referenced this pull request Jan 14, 2022
…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
@chekalsky
Copy link
Contributor

@alexandre-daubois @fabpot Optional is still not supported in Attributes if I am not mistaken, is it done consciously? Haven't found any mentions of it.

@xabbuh
Copy link
Member

xabbuh commented Sep 10, 2022

@chekalsky Did you experience any issues when using Optional or Required inside an attribute? As far as I can see this should work (and we also have a test case for this).

],
'bar' => new Assert\Range(min: 5),
'baz' => new Assert\Required([new Assert\Email()]),
'qux' => new Assert\Optional([new Assert\NotBlank()]),
Copy link
Member

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

@chekalsky
Copy link
Contributor

@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?

@stof
Copy link
Member

stof commented Sep 12, 2022

Well, Optional and Required don't make sense as top-level attributes. They are meant to be used inside the Collection constraint.

fabpot added a commit that referenced this pull request Aug 22, 2024
…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Validator] Use composite constraints as attributes
9 participants