[Console] add union types #41912
[Console] add union types #41912
Conversation
nicolas-grekas
commented
Jun 30, 2021
| Q | A |
|---|---|
| Branch? | 6.0 |
| Bug fix? | no |
| New feature? | no |
| Deprecations? | no |
| Tickets | Part of #41424 |
| License | MIT |
| Doc PR | - |
7882d3f to
d228d0c
Compare
| * | ||
| * @throws InvalidArgumentException When argument mode is not valid | ||
| * | ||
| * @return $this | ||
| */ | ||
| public function addArgument(string $name, int $mode = null, string $description = '', $default = null) | ||
| public function addArgument(string $name, int $mode = null, string $description = '', mixed $default = null) |
There was a problem hiding this comment.
Was the parameter annotation wrong here?
| public function addArgument(string $name, int $mode = null, string $description = '', mixed $default = null) | |
| public function addArgument(string $name, int $mode = null, string $description = '', string|array|null $default = null) |
There was a problem hiding this comment.
Tests fail if we restrict here. We should at least allow int|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.
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 with mixed but please let's keep the more precise type in the doc block then.
There was a problem hiding this comment.
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.
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.
We should be proud of what ppl do with Symfony :)
There was a problem hiding this comment.
I did some git 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 to
0b1d337
Compare
| * @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.
IIUC we dont duplicate the type as per CS?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
0b1d337 to
66b6865
Compare