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] fix a bug when you are passing a default value and passing -n would output the index#25521
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
sroze left a comment
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.
You are missing a test sir!
| } | ||
| if (!$input->isInteractive()) { | ||
| $isInteractive = !$input->isInteractive(); |
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.
$isNotInteractive ?
| if (!$input->isInteractive()) { | ||
| $isInteractive = !$input->isInteractive(); | ||
| if ($isInteractive &&$questioninstanceof ChoiceQuestion) { |
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.
Or I think that you should just putif ($question instanceof ChoiceQuestion) { within the existingif (!$input->isInteractive()) {.
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.
shouldn’t theChoiceQuestion itself take care of returning the correct default value ?
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 don't know if we should nest theses ifs, IMO its clearer like this.
@mathroc I wanted to it like this firstly but it's hard since the getDefault is called without the choices being changed.
SimperfitDec 16, 2017 • 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.
Nvm@sroze you are right, it avoids one test for nothing.
338c78d to56ccf18Compare56ccf18 to066108aCompareSimperfit commentedDec 16, 2017
Test added |
Simperfit commentedDec 16, 2017
Status: Needs Review |
| $this->assertEquals(array('Superman','Batman'),$questionHelper->ask($this->createInputInterfaceMock(),$this->createOutputInterface(),$question)); | ||
| $question =newChoiceQuestion('What is your favorite superhero?',$heroes,0); | ||
| // We are supposed to get the default value since we are in interactive mode |
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.
since we arenot in interactive mode
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.
Thanks! forgot that little word :p
mathroc commentedDec 16, 2017 • 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.
thx@Simperfit to have taken a look at this. there’s still something odd. I’m not sure the default value passed to the question should be its key, the doc comment says ps: but to be fair, that’s whatthe documentation is doing |
Simperfit commentedDec 16, 2017
@mathroc SymfonyStyle is transforming this array. So it has value -> key. This fixes both behaviour when passing the key or the value look at the reproducer i've done to be sure that nothing will be broken :Simperfit/symfony-reproducer@b37a2a5 |
f878424 to0704f80Compare…-n would ouput the index
0704f80 to41ffc69CompareSimperfit commentedDec 16, 2017
Travis failure is unrelated |
nicolas-grekas commentedDec 18, 2017
Thank you@Simperfit. |
…nd passing -n would output the index (Simperfit)This PR was merged into the 2.7 branch.Discussion----------[Console] fix a bug when you are passing a default value and passing -n would output the index| Q | A| ------------- | ---| Branch? | 2.7| Bug fix? | yes| New feature? | no <!-- don't forget to update src/**/CHANGELOG.md files -->| BC breaks? | no| Deprecations? | no <!-- don't forget to update UPGRADE-*.md files -->| Tests pass? | yes| Fixed tickets |#25470| License | MIT| Doc PR |We are fixing a bug when you are using a default value with -n, it would output the index instead of the value.Simple reproducerSimperfit/symfony-reproducer@5079dd1?diff=split#diff-77efcc28bc5309e1af9ac07a1e073009R40Commits-------41ffc69 [Console] fix a bug when you are passing a default value and passing -n would ouput the index
This is how the QuestionHelper API was intended to be used, but it onlyhas been so since a recent bugfix.Seesymfony/symfony#25521FixesBehat#1151
This is how the QuestionHelper API was intended to be used, but it onlyhas been so since a recent bugfix.Seesymfony/symfony#25521FixesBehat#1151
This is how the QuestionHelper API was intended to be used, but it onlyhas been so since a recent bugfix.Seesymfony/symfony#25521FixesBehat#1151
Uh oh!
There was an error while loading.Please reload this page.
We are fixing a bug when you are using a default value with -n, it would output the index instead of the value.
Simple reproducerSimperfit/symfony-reproducer@5079dd1?diff=split#diff-77efcc28bc5309e1af9ac07a1e073009R40