Skip to content

[Console][EventDispatcher][Security][Serializer][Workflow] Add PHPDoc to attribute classes and properties #51974

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jan 2, 2024
Merged

[Console][EventDispatcher][Security][Serializer][Workflow] Add PHPDoc to attribute classes and properties #51974

merged 1 commit into from
Jan 2, 2024

Conversation

alexandre-daubois
Copy link
Member

@alexandre-daubois alexandre-daubois commented Oct 10, 2023 •
edited
Loading

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

One more round.

ℹ️ A first review of this kind is being done here, I'll adjust this PR accordingly once done 🙂

@carsonbot carsonbot added this to the 6.4 milestone Oct 10, 2023
@alexandre-daubois alexandre-daubois force-pushed the attributes-serializer-workflow-console-security branch from de837c7 to d3eccf1 Compare October 10, 2023 14:08
GromNaN
GromNaN reviewed Oct 10, 2023
View reviewed changes
@alexandre-daubois alexandre-daubois force-pushed the attributes-serializer-workflow-console-security branch 2 times, most recently from c3c514f to 2fcdd38 Compare October 11, 2023 07:13
javiereguiluz
javiereguiluz reviewed Oct 11, 2023
View reviewed changes
@alexandre-daubois alexandre-daubois force-pushed the attributes-serializer-workflow-console-security branch from 2fcdd38 to e24303f Compare October 11, 2023 07:14
javiereguiluz
javiereguiluz reviewed Oct 11, 2023
View reviewed changes
Copy link
Member

@javiereguiluz javiereguiluz left a comment

Choose a reason for hiding this comment

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

Alex, thanks a lot for taking care of improving attributes doc.

We want to make these descriptions both complete and concise. They need to be very short, but they also need to explain all the basics needed to fully understand them (what happens if I enable this? what's the format required to pass data here? etc.)

So, thanks in advance for your patience because we'll add many comments 🙏

@alexandre-daubois alexandre-daubois force-pushed the attributes-serializer-workflow-console-security branch 2 times, most recently from ffdcbda to 3adfbc6 Compare October 11, 2023 07:24
@alexandre-daubois
Copy link
Member Author

alexandre-daubois commented Oct 11, 2023 •
edited
Loading

@GromNaN I revamped the PR with the feedback that was given in the other one and by taking into account your review on this one 🙂

@javiereguiluz Thanks, I agree with every point! I updated the PR accordingly. I understand how challenging this kind of PR it is, we don't have that much "space" to explain things like in the doc 😄

@derrabus derrabus mentioned this pull request Oct 11, 2023
Closed
stof
stof reviewed Oct 12, 2023
View reviewed changes
@alexandre-daubois alexandre-daubois force-pushed the attributes-serializer-workflow-console-security branch 2 times, most recently from 0c28525 to aa72c94 Compare October 15, 2023 09:16
@alexandre-daubois
Copy link
Member Author

Thanks, I addressed your comments 🙂

@GromNaN GromNaN mentioned this pull request Oct 18, 2023
Closed
@nicolas-grekas nicolas-grekas modified the milestones: 6.4, 7.1 Nov 15, 2023
@OskarStark
Copy link
Contributor

Rebase please

@alexandre-daubois alexandre-daubois force-pushed the attributes-serializer-workflow-console-security branch 2 times, most recently from 70fd5d6 to 2e03065 Compare December 30, 2023 16:46
@alexandre-daubois
Copy link
Member Author

Rebased and updated with @param 🙂

derrabus
derrabus reviewed Dec 30, 2023
View reviewed changes
@alexandre-daubois alexandre-daubois force-pushed the attributes-serializer-workflow-console-security branch 2 times, most recently from d5495ac to b6d6cef Compare December 30, 2023 16:59
nicolas-grekas
nicolas-grekas reviewed Dec 30, 2023
View reviewed changes
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.

As a general note, it'd be nice to add more titles to classes. Sometimes, this can even replace an @param FTW

@alexandre-daubois alexandre-daubois force-pushed the attributes-serializer-workflow-console-security branch 2 times, most recently from 707ed2d to fa3b3e3 Compare December 31, 2023 09:15
nicolas-grekas
nicolas-grekas approved these changes Jan 2, 2024
View reviewed changes
src/Symfony/Component/Security/Http/Attribute/IsGranted.php Outdated
* @param string|Expression $attribute The attribute that will be checked against a given authentication token and optional subject
* @param array|string|Expression|null $subject An optional subject - e.g. the current object being voted on
* @param string|null $message A custom message when access is not granted
* @param int|null $statusCode If set, will throw HttpKernel's HttpException with the given $statusCode. If null, Security\Core's AccessDeniedException will be used
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 int|null $statusCode If set, will throw HttpKernel's HttpException with the given $statusCode. If null, Security\Core's AccessDeniedException will be used
* @param int|null $statusCode If set, will throw HttpKernel's HttpException with the given $statusCode; if null, Security\Core's AccessDeniedException will be used

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated 👍

@alexandre-daubois alexandre-daubois force-pushed the attributes-serializer-workflow-console-security branch from fa3b3e3 to 8a2ac5a Compare January 2, 2024 10:00
@nicolas-grekas
Copy link
Member

Thank you @alexandre-daubois.

@nicolas-grekas nicolas-grekas merged commit 0685c42 into symfony:7.1 Jan 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@javiereguiluz javiereguiluz javiereguiluz left review comments

@GromNaN GromNaN GromNaN left review comments

@stof stof stof left review comments

@nicolas-grekas nicolas-grekas nicolas-grekas approved these changes

@derrabus derrabus derrabus approved these changes

@lyrixx lyrixx Awaiting requested review from lyrixx lyrixx is a code owner

@chalasr chalasr Awaiting requested review from chalasr chalasr is a code owner

@dunglas dunglas Awaiting requested review from dunglas dunglas is a code owner

Assignees
No one assigned
Labels
Console EventDispatcher Feature Security Serializer Status: Reviewed Workflow
Projects
None yet
Milestone
7.1
Development

Successfully merging this pull request may close these issues.
8 participants
@alexandre-daubois @OskarStark @nicolas-grekas @javiereguiluz @GromNaN @stof @derrabus @carsonbot