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

Fix multiSelect ChoiceQuestion when answers have spaces #32503

Open
wants to merge 1 commit into
base: 4.3
from

Conversation

Projects
None yet
6 participants
@IceMaD
Copy link

commented Jul 11, 2019

Q A
Branch? 4.3
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #32502
License MIT
Doc PR N/A

Probleme is explained in the issue

@Simperfit

This comment has been minimized.

Copy link
Contributor

commented Jul 11, 2019

If this is really something we want to do, it should be deprecated before doing so, because it will break existing command that rely on this.

@nicolas-grekas nicolas-grekas added this to the 4.3 milestone Jul 12, 2019

@chalasr

This comment has been minimized.

Copy link
Member

commented Jul 14, 2019

Any bugfix might break someone's behavior.
I think that this qualifies as a bug, and that no one reported it because usually people rely on keys for choosing multi-words answers.

This should apply to the 3.4 branch. And trimming should be disabled conditionally as of #31626 (4.4), we missed this part.

@IceMaD

This comment has been minimized.

Copy link
Author

commented Jul 15, 2019

Without the trim on multiple answers, it might break cases where answers don't have spaces but user input have. Exemple :
break

Maybe set all ChoiceQuestion as Trimmable by default on 4.4 for backward compatibility ?

/**
* @dataProvider selectUseCases
*/
public function testSelectUseCases($isMiltiSelect, $answers, $expected, $message)

This comment has been minimized.

Copy link
@chalasr

chalasr Jul 19, 2019

Member

typo isMiltiSelect/isMultiSelect

$validator = $question->getValidator();
$actual = $validator($answer);
$this->assertEquals($actual, $expected, sprintf($message, $answer));

This comment has been minimized.

Copy link
@chalasr

chalasr Jul 19, 2019

Member

sprintf() seems unnecessary

@chalasr

This comment has been minimized.

Copy link
Member

commented Jul 19, 2019

@IceMaD yes, trimming must stay enabled by default.

Can you please rebase against the 3.4 branch and change the PR base branch accordingly?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.