8000 [Security] Missing return type in "UserInterface" by turneliusz · Pull Request #13146 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[Security] Missing return type in "UserInterface" #13146

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

Closed
wants to merge 1 commit into from
Closed

[Security] Missing return type in "UserInterface" #13146

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/Symfony/Component/Security/Core/User/UserInterface.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ interface UserInterface
* and populated in any number of different ways when the user object
* is created.
*
* @return Role[] The user roles
* @return Role[]|string[] The user roles
Copy link
Member

Choose a reason for hiding this comment

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

It can be an array containing Role instances and/or strings. Not sure how to express that in a phpdoc.

Copy link
Author

Choose a reason for hiding this comment

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

I think specifying it that way (as above), that it could be or array<Role> or array<string> but not mixed is enough, IDE's are fine with it.

Copy link
Contributor

Choose a reason for hiding this comment

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

PSR-5: (Role|string)[], but I think it's not well supported yet by IDEs.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yuck, that's ugly.

Even if PSR-5 is not an accepted standard yet, some tools are implementing it already: phpDocumentor/phpDocumentor#607 (most likely because the author is involved in PSR-5 ;) ).

Copy link
Author

Choose a reason for hiding this comment

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

First of all it's not ugly and it's well supported for years by IDE (which opens my eyes on fact how stupid it is to create new format...). Second, that (|)[] PSR format doesn't fit at all as it's described as "specified as containing multiple types" because that specific return cannot/shouldn't contain mixed strings and objects in one array as it would create a mess. My change represents and documents 2 completely different outputs. That's my IMHO guys.

Copy link
Member

Choose a reason for hiding this comment

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

@turneliusz the current code supports returning a mix of strings and RoleInterface instances. This is precisely why @fabpot said that your proposal does not describe the code expectations

Copy link
Author

Choose a reason for hiding this comment

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

@stof I've misunderstood it then; for me personally it's quite a mess when any function does that. But if that's status quo there is no discussion. I think we can give PSR way a try.

Copy link
Contributor

Choose a reason for hiding this comment

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

@turneliusz I was referring to what PSR-5 proposes as ugly. In general, I agree we shouldn't mix types in a returned array. In this specific case, we can't change the return type, but we need to update the docblock.

Copy link
Member

Choose a reason for hiding this comment

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

@turneliusz Can you use the PSR-5 notation as it best describes the possible returned values?

Copy link
Member

Choose a reason for hiding this comment

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

@turneliusz Can you make the update?

*/
public function getRoles();

Expand Down
0