Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
6faddcd to1a977b8Comparesqurious commentedOct 12, 2023
I move this PR to WIP as I'm currently updating all type hints that could be more specific |
9ef1b0b to5a3885cCompare
GromNaN left a comment
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.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
cb93388 to73e8cebComparesqurious commentedOct 23, 2023
@GromNaN I fixed typos and params alignment :) |
73e8ceb to05047c5Compare
GromNaN left a comment
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.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
squrious commentedOct 25, 2023
I applied your recommendations in a new commit so we can see the changes:
|
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
102d05e tod9d04f8CompareOskarStark commentedDec 29, 2023
Can you please rebase? |
nicolas-grekas left a comment• edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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?
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
d9d04f8 to61f719fCompare61f719f toaaaef8cComparesqurious commentedJan 2, 2024
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. |
nicolas-grekas commentedJan 2, 2024
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? |
squrious commentedJan 2, 2024
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 |
nicolas-grekas commentedJan 2, 2024
We should remove low-value descriptions (things that should be obvious once you get the basics of how things work) |
c2e29f2 to6269c39Compare
nicolas-grekas left a comment
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
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
| * @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 |
Uh oh!
There was an error while loading.Please reload this page.
624386d to1a26ebeComparenicolas-grekas commentedJan 5, 2024
Thank you@squrious. |
Uh oh!
There was an error while loading.Please reload this page.
Add PHPDoc to Validator constraints attributes.