Skip to content

Navigation Menu

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

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 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
Jan 5, 2024

Conversation

squrious
Copy link
Contributor

@squrious squrious commented Oct 12, 2023

Q A
Branch? 7.1
Bug fix? no
New feature? yes
Deprecations? no
Tickets Part of #51920
License MIT

Add PHPDoc to Validator constraints attributes.

@squrious
Copy link
Contributor Author

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

@squrious squrious marked this pull request as draft October 12, 2023 11:48
@squrious squrious force-pushed the attributes-docs-validator branch 2 times, most recently from 9ef1b0b to 5a3885c Compare October 12, 2023 14:06
@squrious squrious marked this pull request as ready for review October 12, 2023 14:07
Copy link
Member

@GromNaN GromNaN 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.

src/Symfony/Component/Validator/Constraints/Cascade.php Outdated Show resolved Hide resolved
src/Symfony/Component/Validator/Constraints/Currency.php Outdated Show resolved Hide resolved
src/Symfony/Component/Validator/Constraints/IsNull.php Outdated Show resolved Hide resolved
@squrious squrious force-pushed the attributes-docs-validator branch 2 times, most recently from cb93388 to 73e8ceb Compare October 19, 2023 16:45
@squrious
Copy link
Contributor Author

@GromNaN I fixed typos and params alignment :)

@squrious squrious force-pushed the attributes-docs-validator branch from 73e8ceb to 05047c5 Compare October 24, 2023 07:13
@OskarStark OskarStark requested a review from GromNaN October 24, 2023 08:41
Copy link
Member

@GromNaN GromNaN 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.

src/Symfony/Component/Validator/Constraints/Timezone.php Outdated Show resolved Hide resolved
src/Symfony/Component/Validator/Constraints/File.php Outdated Show resolved Hide resolved
@squrious
Copy link
Contributor Author

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

@squrious squrious force-pushed the attributes-docs-validator branch from 102d05e to d9d04f8 Compare October 25, 2023 16:10
@nicolas-grekas nicolas-grekas modified the milestones: 6.4, 7.1 Nov 15, 2023
@OskarStark
Copy link
Contributor

Can you please rebase?

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.

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/Constraints/All.php Outdated Show resolved Hide resolved
src/Symfony/Component/Validator/Constraints/Bic.php Outdated Show resolved Hide resolved
src/Symfony/Component/Validator/Constraints/Blank.php Outdated Show resolved Hide resolved
src/Symfony/Component/Validator/Constraints/Callback.php Outdated Show resolved Hide resolved
@squrious squrious force-pushed the attributes-docs-validator branch from d9d04f8 to 61f719f Compare January 2, 2024 09:10
@squrious squrious force-pushed the attributes-docs-validator branch from 61f719f to aaaef8c Compare January 2, 2024 09:12
@squrious
Copy link
Contributor Author

squrious commented Jan 2, 2024

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

squrious commented Jan 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?

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 in Choice, 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 squrious force-pushed the attributes-docs-validator branch from c2e29f2 to 6269c39 Compare January 2, 2024 10:40
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.

much better :)
let's also remove docblocks that just duplicate the signature - we're fine with partial @param entries

src/Symfony/Component/Validator/Constraints/Cidr.php Outdated Show resolved Hide resolved
src/Symfony/Component/Validator/Constraints/CssColor.php Outdated Show resolved Hide resolved
src/Symfony/Component/Validator/Constraints/Email.php Outdated Show resolved Hide resolved
src/Symfony/Component/Validator/Constraints/Image.php Outdated Show resolved Hide resolved
* @param string|null $uploadExtensionErrorMessage
* @param string|null $uploadErrorMessage Message if an unknown error occurred on upload
* @param string[]|null $groups
* @param mixed|null $payload
Copy link
Member

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

src/Symfony/Component/Validator/Constraints/Image.php Outdated Show resolved Hide resolved
@nicolas-grekas nicolas-grekas force-pushed the attributes-docs-validator branch from 624386d to 1a26ebe Compare January 5, 2024 08:18
@nicolas-grekas
Copy link
Member

Thank you @squrious.

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
Morty Proxy This is a proxified and sanitized view of the page, visit original site.