Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Console] add union types#41912
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
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 | - |
| * @return $this | ||
| */ | ||
| publicfunctionaddArgument(string$name,int$mode =null,string$description ='',$default =null) | ||
| publicfunctionaddArgument(string$name,int$mode =null,string$description ='',mixed$default =null) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Was the parameter annotation wrong here?
| publicfunction addArgument(string$name,int$mode =null,string$description ='',mixed$default =null) | |
| publicfunction addArgument(string$name,int$mode =null,string$description ='',string|array|null$default =null) |
nicolas-grekasJun 30, 2021 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Tests fail if we restrict here. We should at least allowint|float|bool. But I don't see the need to restrict when it was allowed before. We should avoid needless BC breaks IMHO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
It does not make much sense to pass a default value that cannot be produced by invoking the command via the console.null would be the only reasonable exception here.
If we don't want to break BC, I'm fine withmixed but please let's keep the more precise type in the doc block then.
nicolas-grekasJun 30, 2021 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
What would be the point of the annotation? If technically we don't care, we shouldn't bother our users IMHO.
I'm sure there are ppl out there that already found some creative use of that :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I'm sure there are ppl out there that already found some creative use of that :)
That's precisely what scares me. 🙈
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
We should be proud of what ppl do with Symfony :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I did somegit blame + some code inspection. We use mixed in many places already. Having this restricted list is both wrong (as proved by tests) and not needed. I confirm this change.
PR ready.
fde34d8 to0b1d337CompareUh oh!
There was an error while loading.Please reload this page.
| * @param string $description A description text | ||
| * @param string|string[]|bool|null $default The default value (must be null for self::VALUE_NONE) | ||
| * @param $shortcut The shortcuts, can be null, a string of shortcuts delimited by | or an array of shortcuts | ||
| * @param $mode The option mode: One of the VALUE_* constants |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
IIUC we dont duplicate the type as per CS?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
oh wait, this is totally fine 👍
When provided it MAY contain a Type to indicate what is expected
https://docs.phpdoc.org/3.0/guide/references/phpdoc/tags/param.html#param
for now 🤦
When provided it MUST contain a "Type" to indicate what is expected
https://github.com/php-fig/fig-standards/blob/master/proposed/phpdoc-tags.md#59-param
nicolas-grekasJul 1, 2021 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
We settled on:
- PSR19 is not a thing and is outdated anyway, especially considering PHP8
- phpdoc.org is the most up-to-date specification/authority for annotations.