[Console] Fix type of InputOption::$default #40428
Conversation
|
Hey! I see that this is your first PR. That is great! Welcome! Symfony has a contribution guide which I suggest you to read. In short:
Review the GitHub status checks of your pull request and try to solve the reported issues. If some tests are failing, try to see if they are failing because of this change. When two Symfony core team members approve this change, it will be merged and you will become an official Symfony contributor! I am going to sit back now and wait for the reviews. Cheers! Carsonbot |
| @@ -122,7 +122,7 @@ public function getOptions(); | |||
| * | |||
| * @param string $name The option name | |||
| * | |||
| * @return string|string[]|bool|null The option value | |||
| * @return string|string[]|int|bool|null The option value | |||
There was a problem hiding this comment.
Thank for this PR, but this change looks wrong, The options can be:
string:--option=foostring[]:--option=foo --option=barbool:--option+VALUE_NONEnull:--option+VALUE_OPTIONAL
But there is no case where the value can be an integer.
Note: --option=123 returns the string "123".
There was a problem hiding this comment.
Indeed, see also #28374 (comment).
There was a problem hiding this comment.
We should rather remove int from the getDefault() doc block.
There was a problem hiding this comment.
InputOption declares int as a valid type for the default value: in the constructor signature as well as for getDefault() and setDefault(). This looks like a mistake to me because you can never produce an integer on the command line. But it means that it is possible, when following the documented public API, to receive an integer from getOption().
There was a problem hiding this comment.
Indeed when the default value is used then it can be the case that getOption returns an integer. I just tested it quickly.
public function configure(): void
{
$this->addOption('foo', null, InputOption::VALUE_REQUIRED, '', 123);
}
protected function execute(InputInterface $input, OutputInterface $output): int
{
var_dump($input->getOption('foo'));die;
}And then executed without passing any option at all:
==> int(123)
Edit: It actually seems one can pass anything as a default currently. This also works:
->addOption('foo', null, InputOption::VALUE_REQUIRED, '', new \stdClass())There was a problem hiding this comment.
Would you mind to change this PR to fix the default signature? @oliverklee
Will do. |
38eb3c4 to
34d044c
Compare
|
I have updated the PR accordingly and repushed. |
The types accepted and provided by `InputInterface::getOption` and `setOption` do not include `int` (and passing something like `--option=123` will set the option to the string `"123"`, not to the integer `123`). The `InputOption` default types should match this.
34d044c to
3a2d1b4
Compare
Thanks! Done and repushed. |
|
The test failures do not look like they could be related to my change. |
|
Thank you @oliverklee. |
Description ----------- Because command options cannot be integers (see symfony/symfony#40428). Commits ------- 3257cda Fix the PHPStan issues
Description ----------- Because command options cannot be integers (see symfony/symfony#40428). Commits ------- 3257cdac Fix the PHPStan issues
|
(reply to a comment that has been deleted meanwhile) An immediate solution would be to ignore that failure in your static analyser configuration. |
Options can also be
ints. So add this type to the PHPDoc parametertype annotation of
InputInterface::setOptionand the return typeannotation of
InputInterface::getOption.