8000 [Security] Add missing docblock in PreAuthenticatedToken by tgalopin · Pull Request #15481 · symfony/symfony · GitHub
[go: up one dir, main page]

Skip to content

[Security] Add missing docblock in PreAuthenticatedToken #15481

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

Closed
wants to merge 1 commit into from

Conversation

tgalopin
Copy link
Contributor
@tgalopin tgalopin commented Aug 6, 2015
Q A
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets -
License MIT
Doc PR -

I noticed the PreAuthenticatedToken does not provide any dockblock about its constructor parameters so PHPStorm uses the AbstractToken parameters and think I'm using PreAuthenticatedToken badly.

This fix it by adding the missing docblock and I also added the string[] possibility for roles in AbstractToken as it's present in the code and the exception message.

@hhamon
Copy link
Contributor
hhamon commented Aug 6, 2015

I think this PR should be submitted against 2.3.

@Nek-
Copy link
Contributor
Nek- commented Aug 6, 2015

I don't see any bugfix :o ... And IMO the addition of the use statement is too much.

@dosten
Copy link
Contributor
dosten commented Aug 6, 2015

@Nek- Why are you saying that the addition of the use statetement is too much? Agree with @hhamon this PR should be merged in the 2.3 branch. This is a doc fix of a maintained version.

@@ -33,7 +33,7 @@
/**
* Constructor.
*
* @param RoleInterface[] $roles An array of roles
* @param RoleInterface[]|string[] $roles An array of roles
Copy link
Contributor

Choose a reason for hiding this comment

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

There are already some debates about this notation, being incomplete. See #13146 (comment) and other related issues.

As @dosten said in one of them:

getRoles can return an array of strings, Role instances or a mix of both, the phpdoc must reflect that.

@tgalopin
Copy link
Contributor Author
tgalopin commented Aug 7, 2015

I applied it on 2.3: #15484

The notation I used for RoleInterface[]|string[] is the one used in PHPStorm and I thought it was universal. Should I remove it?

@tgalopin tgalopin closed this Aug 7, 2015
@ogizanagi
Copy link
Contributor

I think it should be (Rol 78DC eInterface|string)[] according to PSR-5, but it wasn't adopted yet in the framework elsewhere, and IDEs, might not support it properly yet ?

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
0