Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Form] Reverted string expectation in ChoiceToValueTransformer#18462
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
walczakmac commentedApr 6, 2016
| Q | A |
|---|---|
| Branch? | 3.0 |
| Bug fix? | yes |
| New feature? | no |
| BC breaks? | no |
| Deprecations? | no |
| Tests pass? | yes |
| Fixed tickets | #18461 |
| License | MIT |
| Doc PR | n/a |
| $this->assertSame($outWithNull,$this->transformerWithNull->reverseTransform($inWithNull)); | ||
| } | ||
| publicfunctionreverseTransformExpectsStringOrNullProvider() |
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 also need to rename those functions with*ExpectsScalarOrNull*.
HeahDude commentedApr 6, 2016
See also#17760 (comment) for a reason to not revert that change. |
HeahDude commentedApr 6, 2016
Another reason would be consistency with the $builder->add('emailNotificationsEnabled', ChoiceType::class, ['required' =>false,'multiple' =>false,'choices' => [0,1],'empty_data' =>1,// works If we revert by merging this PR]);$builder->add('emailNotificationsEnabled', ChoiceType::class, ['required' =>false,'multiple' =>true,// ChoicesToValuesTransformer will be called'choices' => [0,1],'empty_data' =>array(1),// won't work it expects array of strings array('1')]); |
HeahDude commentedApr 6, 2016
And |
HeahDude commentedApr 6, 2016
Also consider that: $builder->add('emailNotificationsEnabled', ChoiceType::class, ['required' =>false,'multiple' =>false,'choices' => [null,0,1],'empty_data' =>1,// will match 0, needs the key '2' to match 1]); |
HeahDude commentedApr 6, 2016
HeahDude commentedApr 6, 2016
Note that if accepted, this PR should be merged in 2.7. |
walczakmac commentedApr 6, 2016
Now I see that there are too many cons when reverting it to previous state, and it will be much easier if I use strings there instead of integers. I'll just close the PR and the ticket, also thanks for your help@HeahDude. |
HeahDude commentedApr 6, 2016
No worry ;) Thank you for the report. |