[Security] Missing return type in "UserInterface" #13146
Conversation
| Q | A |
|---|---|
| Bug fix? | no |
| New feature? | no |
| BC breaks? | no |
| Deprecations? | no |
| Tests pass? | yes |
| Fixed tickets | #13037 |
| License | MIT |
| Doc PR |
| @@ -47,7 +47,7 @@ | |||
| * 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 | |||
There was a problem hiding this comment.
It can be an array containing Role instances and/or strings. Not sure how to express that in a phpdoc.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
PSR-5: (Role|string)[], but I think it's not well supported yet by IDEs.
There was a problem hiding this comment.
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 ;) ).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
@turneliusz Can you use the PSR-5 notation as it best describes the possible returned values?
There was a problem hiding this comment.
@turneliusz Can you make the update?
This PR was merged into the 2.3 branch. Discussion ---------- Fixed the phpDoc of UserInterface | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #13037 | License | MIT | Doc PR | - #13037 is the original issue ... which was tried to be fixed in #13146 ... which was closed without merging it. Commits ------- 8e2a33e Fixed the phpDoc of UserInterface