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] 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

Merged

Conversation

@squrious
Copy link
Contributor

@squrioussqurious commentedOct 12, 2023
edited by OskarStark
Loading

QA
Branch?7.1
Bug fix?no
New feature?yes
Deprecations?no
TicketsPart of#51920
LicenseMIT

Add PHPDoc to Validator constraints attributes.

BafS reacted with rocket emoji
@squrious
Copy link
ContributorAuthor

I move this PR to WIP as I'm currently updating all type hints that could be more specific

@squrioussqurious marked this pull request as draftOctober 12, 2023 11:48
@squrioussquriousforce-pushed theattributes-docs-validator branch 2 times, most recently from9ef1b0b to5a3885cCompareOctober 12, 2023 14:06
@squrioussqurious marked this pull request as ready for reviewOctober 12, 2023 14:07
Copy link
Member

@GromNaNGromNaN left a comment

Choose a reason for hiding this comment

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

Still reviewing.

@squrioussquriousforce-pushed theattributes-docs-validator branch 2 times, most recently fromcb93388 to73e8cebCompareOctober 19, 2023 16:45
@squrious
Copy link
ContributorAuthor

@GromNaN I fixed typos and params alignment :)

@squrioussquriousforce-pushed theattributes-docs-validator branch from73e8ceb to05047c5CompareOctober 24, 2023 07:13
Copy link
Member

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

@squrious
Copy link
ContributorAuthor

I applied your recommendations in a new commit so we can see the changes:

  • Document defaults behaviors when it was possible
  • Replace@link with@see
  • Remove all<pre> tags
  • Fix some mistakes
  • Use more{@see} to reference structural elements like class constants
GromNaN reacted with thumbs up emoji

@squrioussquriousforce-pushed theattributes-docs-validator branch from102d05e tod9d04f8CompareOctober 25, 2023 16:10
@nicolas-grekasnicolas-grekas modified the milestones:6.4,7.1Nov 15, 2023
@OskarStark
Copy link
Contributor

Can you please rebase?

Copy link
Member

@nicolas-grekasnicolas-grekas left a comment
edited
Loading

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?

@squrioussquriousforce-pushed theattributes-docs-validator branch fromd9d04f8 to61f719fCompareJanuary 2, 2024 09:10
@squrioussquriousforce-pushed theattributes-docs-validator branch from61f719f toaaaef8cCompareJanuary 2, 2024 09:12
@squrious
Copy link
ContributorAuthor

Rebased on 7.1!

@nicolas-grekas I also applied your suggestions :)

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?

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
Copy link
Member

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
Copy link
ContributorAuthor

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?

If it's about to be deprecated, yup let's get rid of those too.

I'm all for removing redundant descriptions (groups, payload, etc), can you please update accordingly?

Yes! Just a little question: should I remove the messages documentation if the behavior is a little bit specific to the constraint? Eg inChoice, we have$multipleMessage,$minMessage, etc. Should they be removed too?

@nicolas-grekas
Copy link
Member

We should remove low-value descriptions (things that should be obvious once you get the basics of how things work)

squrious reacted with thumbs up emoji

@squrioussquriousforce-pushed theattributes-docs-validator branch fromc2e29f2 to6269c39CompareJanuary 2, 2024 10:40
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.

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

Choose a reason for hiding this comment

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

Suggested change
* @param mixed|null $payload

@nicolas-grekas
Copy link
Member

Thank you@squrious.

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

Reviewers

@nicolas-grekasnicolas-grekasnicolas-grekas left review comments

@stofstofstof left review comments

@OskarStarkOskarStarkOskarStark left review comments

@GromNaNGromNaNGromNaN approved these changes

Assignees

No one assigned

Projects

None yet

Milestone

7.1

Development

Successfully merging this pull request may close these issues.

6 participants

@squrious@OskarStark@nicolas-grekas@GromNaN@stof@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp