Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
Fix multiSelect ChoiceQuestion when answers have spaces#32503
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
Simperfit commentedJul 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. |
chalasr commentedJul 14, 2019
Any bugfix might break someone's behavior. This should apply to the 3.4 branch. And trimming should be disabled conditionally as of#31626 (4.4), we missed this part. |
IceMaD commentedJul 15, 2019
Without the trim on multiple answers, it might break cases where answers don't have spaces but user input have. Exemple : Maybe set all ChoiceQuestion as Trimmable by default on 4.4 for backward compatibility ? |
| /** | ||
| * @dataProvider selectUseCases | ||
| */ | ||
| public function testSelectUseCases($isMiltiSelect, $answers, $expected, $message) |
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.
typoisMiltiSelect/isMultiSelect
| $validator = $question->getValidator(); | ||
| $actual = $validator($answer); | ||
| $this->assertEquals($actual, $expected, sprintf($message, $answer)); |
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.
sprintf() seems unnecessary
chalasr commentedJul 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 commentedJul 24, 2019
Thank you@IceMaD. |
…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/AProbleme is explained in the issueCommits-------9104ef1 Fix multiSelect ChoiceQuestion when answers have spaces
chalasr commentedJul 24, 2019
And thank you for contributing to Symfony for the first time. Well done, congratz :) |
Uh oh!
There was an error while loading.Please reload this page.
Probleme is explained in the issue