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

Merged
merged 1 commit into from Jul 24, 2019

Conversation

IceMaD
Copy link
Contributor

@IceMaD IceMaD commented Jul 11, 2019

Q A
Branch? 3.4
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
Copy link
Contributor

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
Copy link
Member

chalasr 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
Copy link
Contributor Author

IceMaD 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)
Copy link
Member

Choose a reason for hiding this comment

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

typo isMiltiSelect/isMultiSelect

$validator = $question->getValidator();
$actual = $validator($answer);

$this->assertEquals($actual, $expected, sprintf($message, $answer));
Copy link
Member

Choose a reason for hiding this comment

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

sprintf() seems unnecessary

@chalasr
Copy link
Member

chalasr 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?

@chalasr chalasr modified the milestones: 4.3, 3.4 Jul 24, 2019
@chalasr chalasr removed the request for review from xabbuh July 24, 2019 14:47
@chalasr
Copy link
Member

chalasr commented Jul 24, 2019

Thank you @IceMaD.

@chalasr chalasr merged commit 9104ef1 into symfony:3.4 Jul 24, 2019
chalasr pushed a commit that referenced this pull request Jul 24, 2019
…ceMaD)

This PR was merged into the 3.4 branch.

Discussion
----------

Fix multiSelect ChoiceQuestion when answers have spaces

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| 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

Commits
-------

9104ef1 Fix multiSelect ChoiceQuestion when answers have spaces
@chalasr
Copy link
Member

chalasr commented Jul 24, 2019

And thank you for contributing to Symfony for the first time. Well done, congratz :)

This was referenced Jul 27, 2019
@fabpot fabpot mentioned this pull request Jul 28, 2019
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.

None yet

6 participants