Skip to content

[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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Apr 1, 2022

Conversation

norkunas
Copy link
Contributor

@norkunas norkunas commented Jan 19, 2022

Q A
Branch? 6.1
Bug fix? no
New feature? yes
Deprecations? no
Tickets N/A
License MIT
Doc PR symfony/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 overriding getRequiredOptions 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.

@carsonbot carsonbot added this to the 6.1 milestone Jan 19, 2022
@norkunas norkunas force-pushed the validator-constraint-required-args branch 2 times, most recently from 169216f to bc1205d Compare January 19, 2022 09:27
@norkunas norkunas force-pushed the validator-constraint-required-args branch from 16ef147 to f72c5c0 Compare March 18, 2022 07:28
@norkunas norkunas force-pushed the validator-constraint-required-args branch 2 times, most recently from 5d6405d to 63f4d0c Compare March 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 on getNumberOfRequiredParameters 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 add AbstractConstraint and use an instanceof check against it?

@norkunas
Copy link
Contributor Author

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 on getNumberOfRequiredParameters 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 add AbstractConstraint and use an instanceof check against it?

And what that AbstractConstraint 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 commented Mar 29, 2022

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
Contributor Author

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!

@norkunas norkunas force-pushed the validator-constraint-required-args branch from 63f4d0c to 44c1c1d Compare March 29, 2022 07:26
@norkunas
Copy link
Contributor Author

That'd work for me also!

Thank you, updated.

@norkunas norkunas force-pushed the validator-constraint-required-args branch from 44c1c1d to 5aec890 Compare March 29, 2022 08:10
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.

almost good to me, I just have some questions

@norkunas norkunas force-pushed the validator-constraint-required-args branch from 5aec890 to f22433f Compare April 1, 2022 07:14
@fabpot
Copy link
Member

fabpot commented Apr 1, 2022

Thank you @norkunas.

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.

6 participants