Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

[Validator] Allow creating constraints with required arguments#45072

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged

Conversation

@norkunas
Copy link
Contributor

@norkunasnorkunas commentedJan 19, 2022
edited by lyrixx
Loading

QA
Branch?6.1
Bug fix?no
New feature?yes
Deprecations?no
TicketsN/A
LicenseMIT
Doc PRsymfony/symfony-docs#16770

Before PHP8 validation constraint usage was only possible with providing options as an array, but now with native PHP attributes we can provide as named arguments. And to require some arguments overridinggetRequiredOptions method in Constraint was necessary to get proper validation. But since PHP8.1 we can just make arguments required in the Attribute constructor and try to unpack them because it is possible now.

@carsonbotcarsonbot added this to the6.1 milestoneJan 19, 2022
@norkunasnorkunasforce-pushed thevalidator-constraint-required-args branch 2 times, most recently from169216f tobc1205dCompareJanuary 19, 2022 09:27
@norkunasnorkunasforce-pushed thevalidator-constraint-required-args branch from16ef147 tof72c5c0CompareMarch 18, 2022 07:28
@norkunasnorkunasforce-pushed thevalidator-constraint-required-args branch 2 times, most recently from5d6405d to63f4d0cCompareMarch 18, 2022 08:21
@nicolas-grekas
Copy link
Member

If I'm not wrong, it's already possible to write constraints with required args, but then they can only be loaded via annotations, and not via yaml/xml, isn't it?

Relying ongetNumberOfRequiredParameters feels fragile. I think this won't work as nicely as we'd like in practice. Can't we think of another check to replace the call? Eg addAbstractConstraint and use aninstanceof check against it?

@norkunas
Copy link
ContributorAuthor

If I'm not wrong, it's already possible to write constraints with required args, but then they can only be loaded via annotations, and not via yaml/xml, isn't it?

yes

Relying ongetNumberOfRequiredParameters feels fragile. I think this won't work as nicely as we'd like in practice. Can't we think of another check to replace the call? Eg addAbstractConstraint and use aninstanceof check against it?

And what thatAbstractConstraint would give us, except an empty marker?Constraint is already abstract :) From my side I don't see what wouldn't work because it's now just determining if the constructor requires something that are mandatory and properly instantiating it instead of making the old assumption that we are instantiating it with single argument.

@nicolas-grekas
Copy link
Member

nicolas-grekas commentedMar 29, 2022
edited
Loading

And what that AbstractConstraint would give us, except an empty marker?

Exactly that: a marker to express that some constraints opt-in to get their arguments by a named constructor.

From my side I don't see what wouldn't work because it's now just determining if the constructor requires something that are mandatory and properly instantiating it instead of making the old assumption that we are instantiating it with single argument.

The next logical step would be to allow constructing constraints using named arguments, even if no arguments are required.

@norkunas
Copy link
ContributorAuthor

Exactly that: a marker to express that some constraints opt-in to get their arguments by a named constructor.

Maybe then an attribute could be used for this? because if I'd extend an existing constraint it wouldn't work

@nicolas-grekas
Copy link
Member

That'd work for me also!

@norkunasnorkunasforce-pushed thevalidator-constraint-required-args branch from63f4d0c to44c1c1dCompareMarch 29, 2022 07:26
@norkunas
Copy link
ContributorAuthor

That'd work for me also!

Thank you, updated.

@norkunasnorkunasforce-pushed thevalidator-constraint-required-args branch from44c1c1d to5aec890CompareMarch 29, 2022 08:10
Copy link
Member

@nicolas-grekasnicolas-grekas left a comment

Choose a reason for hiding this comment

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

almost good to me, I just have some questions

@norkunasnorkunasforce-pushed thevalidator-constraint-required-args branch from5aec890 tof22433fCompareApril 1, 2022 07:14
@fabpot
Copy link
Member

Thank you@norkunas.

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@GromNaNGromNaNGromNaN left review comments

@derrabusderrabusderrabus requested changes

@nicolas-grekasnicolas-grekasnicolas-grekas approved these changes

Assignees

No one assigned

Projects

None yet

Milestone

6.1

Development

Successfully merging this pull request may close these issues.

6 participants

@norkunas@nicolas-grekas@fabpot@GromNaN@derrabus@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp