Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Console] Fixed SymfonyQuestionHelper multi-choice with defaults #19100

Closed

Conversation

sstok
Copy link
Contributor

@sstok sstok commented Jun 18, 2016

Q A
Bug Fix? yes
New Feature? no
BC Breaks? no
Deprecations? no
Tests Pass? yes
Fixed Tickets
License MIT
Doc PR

When you use the SymfonyStyle with a multi-choice question and multiple defaults.
You get an notice because the key is used as-is 0,1 instead of splitting the value.

This pull-request changes the SymfonyQuestionHelper to checks if the Choice is a multi-choice
and displays the selected values correctly [blue, yellow].

Note: Tests are missing, but both the SymfonyStyle and SymfonyQuestionHelper classes
have almost no tests. Making it really hard 😇 hopefully #19097 will make this easier 👍

*
* @return bool
*/
public function isMultiSelect()
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be isMultiselect and i would add it right after setMultiselect

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@sstok sstok force-pushed the bug-console-multi-choice-symfonyStyle branch from 53a0c38 to bcb4f09 Compare June 18, 2016 16:47
@sstok sstok changed the title Fixed SymfonyQuestionHelper multi-choice with defaults [Console] Fixed SymfonyQuestionHelper multi-choice with defaults Jun 18, 2016
@chalasr
Copy link
Member

chalasr commented Jun 19, 2016

👍 (needs some tests).

@sstok
Copy link
Contributor Author

sstok commented Jun 20, 2016

Will add the test later today 👍 thanks for working on it.

Status: Needs work

@sstok
Copy link
Contributor Author

sstok commented Jun 21, 2016

Tests added 👍

Status: Needs review

@chalasr
Copy link
Member

chalasr commented Jun 21, 2016

@sstok Your test fails on appveyor, the output is different than the expected one.
I recently faced this issue.

Your test expects: "What is your favorite superhero? [Spiderman]" but the output is:

It's because the SymfonyQuestionHelper decorates the output, disable it and tests should pass:

- return new StreamOutput(fopen('php://memory', 'r+', false));
+ $output = new StreamOutput(fopen('php://memory', 'r+', false));
+ $output->setDecorated(false);
+
+ return $output;

@ro0NL
Copy link
Contributor

ro0NL commented Jun 21, 2016

Btw specifying defaults as "1,2" is that to reflect user input, format-wise? I.e. why not
array('1', '2')?

@sstok
Copy link
Contributor Author

sstok commented Jun 22, 2016

@ro0NL That would be a new feature, this pr is about fixing the SymfonyQuestionHelper 👍

@chalasr Thanks for the tip 👍

$choices = $question->getChoices();
$default = array_map('trim', explode(',', $default));

foreach ($default as &$defaultVal) {
Copy link
Member

Choose a reason for hiding this comment

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

Can we avoid using references here?

foreach ($default as $key => $value) {
    $default[$key] = $choices[$value];
}

@ro0NL
Copy link
Contributor

ro0NL commented Jun 22, 2016

@sstok understand, just wondered

@sstok sstok force-pushed the bug-console-multi-choice-symfonyStyle branch from 1637d7e to 79815a9 Compare June 22, 2016 09:23
@sstok
Copy link
Contributor Author

sstok commented Jun 22, 2016

Squashed and rebased.

@fabpot
Copy link
Member

fabpot commented Jun 22, 2016

Thank you @sstok.

fabpot added a commit that referenced this pull request Jun 22, 2016
…faults (sstok)

This PR was submitted for the 2.8 branch but it was merged into the 2.7 branch instead (closes #19100).

Discussion
----------

[Console] Fixed SymfonyQuestionHelper multi-choice with defaults

|Q            |A  |
|---          |---|
|Bug Fix?     |yes|
|New Feature? |no |
|BC Breaks?   |no |
|Deprecations?|no |
|Tests Pass?  |yes|
|Fixed Tickets|   |
|License      |MIT|
|Doc PR       |   |

When you use the SymfonyStyle with a multi-choice question and multiple defaults.
You get an notice because the key is used as-is `0,1` instead of splitting the value.

This pull-request changes the SymfonyQuestionHelper to checks if the Choice is a multi-choice
and displays the selected values correctly `[blue, yellow]`.

Note: Tests are missing, but both the SymfonyStyle and SymfonyQuestionHelper classes
have almost no tests. Making it really hard 😇 hopefully #19097 will make this easier 👍

Commits
-------

a8f6f85 Fixed SymfonyQuestionHelper multi-choice with defaults
@fabpot fabpot closed this Jun 22, 2016
@sstok sstok deleted the bug-console-multi-choice-symfonyStyle branch June 22, 2016 12:12
@sstok
Copy link
Contributor Author

sstok commented Jun 28, 2016

@fabpot can you please merge the 2.7 up-to the master branch 👍

@sstok
Copy link
Contributor Author

sstok commented Jun 29, 2016

Thanks ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants