-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[Validator] Add PHPDoc to validator constraints #52012
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
Conversation
6faddcd
to
1a977b8
Compare
I move this PR to WIP as I'm currently updating all type hints that could be more specific |
9ef1b0b
to
5a3885c
Compare
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.
Still reviewing.
src/Symfony/Component/Validator/Attribute/HasNamedArguments.php
Outdated
Show resolved
Hide resolved
cb93388
to
73e8ceb
Compare
@GromNaN I fixed typos and params alignment :) |
73e8ceb
to
05047c5
Compare
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.
Thank you for your hard work. I've made a few comments, nothing certain, I let others give their opinion.
src/Symfony/Component/Validator/Constraints/PasswordStrength.php
Outdated
Show resolved
Hide resolved
I applied your recommendations in a new commit so we can see the changes:
|
102d05e
to
d9d04f8
Compare
Can you please rebase? |
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.
Thanks for working on this. I have mixed feelings here and I'm already sorry about that.
That's a lot of added lines to review and maintain. Are we sure all this is not just boilerplate? Eg is the repetition of the description for groups, payload, options, error message, etc. that useful? I'm not the best person to answer this question because I'm too much into this already but I'm still wondering.
Any other opinion?
src/Symfony/Component/Validator/Attribute/HasNamedArguments.php
Outdated
Show resolved
Hide resolved
d9d04f8
to
61f719f
Compare
61f719f
to
aaaef8c
Compare
Rebased on 7.1! @nicolas-grekas I also applied your suggestions :)
I agree the repetitions are very redundant and add a lot to maintain. Maybe it's not so useful for groups, payload and error messages as their behavior is likely to be known when working with the validator. But for the options it's a bit different, as the first constructor argument may be used as the default option depending on the constraint. Documenting this aspect may worth it I guess. |
In #51393, we're wondering about a way to deprecate passing options as an array and require using named arguments instead, so maybe we shouldn't document the array-style? I'm all for removing redundant descriptions (groups, payload, etc), can you please update accordingly? |
If it's about to be deprecated, yup let's get rid of those too.
Yes! Just a little question: should I remove the messages documentation if the behavior is a little bit specific to the constraint? Eg in |
We should remove low-value descriptions (things that should be obvious once you get the basics of how things work) |
c2e29f2
to
6269c39
Compare
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.
much better :)
let's also remove docblocks that just duplicate the signature - we're fine with partial @param
entries
* @param string|null $uploadExtensionErrorMessage | ||
* @param string|null $uploadErrorMessage Message if an unknown error occurred on upload | ||
* @param string[]|null $groups | ||
* @param mixed|null $payload |
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.
* @param mixed|null $payload |
624386d
to
1a26ebe
Compare
Thank you @squrious. |
Add PHPDoc to Validator constraints attributes.