NotificationsYou must be signed in to change notification settings
Fork9.7k
Star30.8k

[Security] add union types#41911

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

nicolas-grekas merged 1 commit intosymfony:6.0fromnicolas-grekas:unions-sec

Jul 1, 2021

Merged

[Security] add union types#41911

nicolas-grekas merged 1 commit intosymfony:6.0fromnicolas-grekas:unions-sec

Jul 1, 2021

Conversation

Copy link

Member

nicolas-grekas commentedJun 30, 2021

Q	A
Branch?	6.0
Bug fix?	no
New feature?	no
Deprecations?	no
Tickets	Part of#41424
License	MIT
Doc PR	-

nicolas-grekas added this to the6.0 milestone

Jun 30, 2021

nicolas-grekas requested review fromchalasr andwouterj ascode owners

June 30, 2021 07:41

carsonbot added Status: Needs Review Security labels

Jun 30, 2021

chalasr reviewed

Jun 30, 2021

View reviewed changes

src/Symfony/Component/Security/Core/Authentication/Token/SwitchUserToken.php

		* @paramstring\|null$originatedFromUri The URI where was the user at the switch
		* @param $user The username (like a nickname, email address, etc.), or a UserInterface instance or an object implementing a __toString method
		* @param $credentials This usually is the password of the user
		* @param $originatedFromUri The URI where was the user at the switch

Copy link

Member

chalasrJun 30, 2021

Choose a reason for hiding this comment

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

Is it valid to omit the type?

Copy link

MemberAuthor

nicolas-grekasJun 30, 2021

Choose a reason for hiding this comment

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

for phpstorm at least it is

Copy link

Member

derrabusJun 30, 2021

Choose a reason for hiding this comment

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

https://docs.phpdoc.org/3.0/guide/references/phpdoc/tags/param.html

@param [<Type>] [name] [<description>]

[…]

When provided it MAY contain a Type to indicate what is expected.

Yes, the type is optional. But I had to look it up myself as well and I'm not yet sure if I like that notation. Maybe we just need to get used to it. 🤔

Copy link

Member

chalasrJun 30, 2021

Choose a reason for hiding this comment

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

If popular IDEs support it, I'm fine with it personally.

Copy link

Member

javiereguiluzJun 30, 2021

Choose a reason for hiding this comment

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

Once you start using PHP attributes for everything ... you want to use it even to describe arguments. I wish this worked:

#[Param('user','The username (like a nickname, email address, etc.), or a ...')]#[Param('credentials','This usually is the password of the user')]#[Param('originatedFromUri','The URI where was the user at the switch')]publicfunction__construct(string|\Stringable|UserInterface$user,mixed$credentials,/* ... */,string$originatedFromUri =null){// ...}

Or even better, this:

publicfunction__construct(    #[Param('The username (like a nickname, email address, etc.), or a ...')]privatestring|\Stringable|UserInterface$user,    #[Param('This usually is the password of the user')]privatemixed$credentials,// ...    #[Param('The URI where was the user at the switch')]privatestring$originatedFromUri =null){}