-
-
Notifications
You must be signed in to change notification settings - Fork 9.7k
[Form] Fixed show float values as choice value in ChoiceType #20378
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
Conversation
👍 the change looks solid and your explanation was perfect. Thanks! |
} elseif (isset($cache[$choice])) { | ||
} | ||
|
||
$choice = false === $choice ? '0' : (string) $choice; |
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.
Couldn't we simplify this a bit and just always use var_export()
?
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.
I like your idea (perhaps that allows render "false"
instead of "0"
for boolean values) but we need to detect duplicates now when (0
and false
) or (1
and true
) coexist in the same choices, else it breaks the current behavior when above these values are casted to (string) again.
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.
However, the result looks weird by using var_export()
for float values in some PHP versions, for instance https://3v4l.org/beeGa#v5422 😕 ?
To be honest, I would expect <option value="1">Mid</option> |
@iltar there aren't differences as float numbers in JavaScript, so no matter. |
It's a matter of consistency |
You are right, but this can't be solved from PHP, because we can't differentiate between |
65d1af6
to
3564228
Compare
👍 |
Thank you @yceruto. |
…pe (yceruto) This PR was merged into the 2.7 branch. Discussion ---------- [Form] Fixed show float values as choice value in ChoiceType | Q | A | ------------- | --- | Branch? | 2.7 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #13817 | License | MIT | Doc PR | - There is a closed [issue][1] related to this before Symfony 2.7 (when choice value still was the key from the `array`). This issue already happened in 2.7+ but now inside Symfony core. Information ----------- [This method][3] checks whether the given choices can be cast to strings without generating duplicates (3ab8189 in #16705): ```php private function castableToString(array $choices, array &$cache = array()) { foreach ($choices as $choice) { if (is_array($choice)) { if (!$this->castableToString($choice, $cache)) { return false; } continue; } elseif (!is_scalar($choice)) { return false; } elseif (isset($cache[$choice])) { // <---- red breakpoint return false; } $cache[$choice] = true; // <---- red breakpoint } return true; } ``` So it should to keep [scalar values (integer, float, string or boolean)][2] as choice values always (unless have duplicates values). The Problem ----------- But in this situation it doesn't happen: ```php $form = $this->createFormBuilder() ->add('foo', ChoiceType::class, [ 'choices' => [ 'Min' => 0.5, 'Mid' => 1.0, 'Max' => 1.5, ] ]) ->getForm(); ``` **Output:** ```html <select id="form_foo" name="form[foo]"> <option value="0">Min</option> <option value="1">Mid</option> <option value="2">Max</option> </select> ``` [**Why?**][5] If the key of the array is a float number, it's interpreted as integer automatically: ```php // ... $cache[$choice] = true; // when $choice = 0.5: $cache = [0 => true] // when $choice = 1.0: $cache = [0 => true, 1 => true] ``` Then, when `$choice = 1.5` [this sentence][4] `isset($cache[1.5])` returns `true` because really checks `isset($cache[1])` and this key already exists, so `castableToString()` returns `false` (detected as duplicate) and the choices values are generated incrementing integers as values. The PR's Effect ------------- **Before:** ```html <select id="form_foo" name="form[foo]"> <option value="0">Min</option> <option value="1">Mid</option> <option value="2">Max</option> </select> ``` **After:** ```html <select id="form_foo" name="form[foo]"> <option value="0.5">Min</option> <option value="1">Mid</option> <option value="1.5">Max</option> </select> ``` [1]: #13817 [2]: http://php.net/manual/en/function.is-scalar.php [3]: https://github.com/symfony/symfony/blob/2.7/src/Symfony/Component/Form/ChoiceList/ArrayChoiceList.php#L228 [4]: https://github.com/symfony/symfony/blob/2.7/src/Symfony/Component/Form/ChoiceList/ArrayChoiceList.php#L239 [5]: https://github.com/symfony/symfony/blob/2.7/src/Symfony/Component/Form/ChoiceList/ArrayChoiceList.php#L243 Commits ------- 3564228 [Form] Fix show float values as choices values in ChoiceType
There is a closed issue related to this before Symfony 2.7 (when choice value still was the key from the
array
). This issue already happened in 2.7+ but now inside Symfony core.Information
This method checks whether the given choices can be cast to strings without generating duplicates (3ab8189 in #16705):
So it should to keep scalar values (integer, float, string or boolean) as choice values always (unless have duplicates values).
The Problem
But in this situation it doesn't happen:
Output:
Why?
If the key of the array is a float number, it's interpreted as integer automatically:
Then, when
$choice = 1.5
this sentenceisset($cache[1.5])
returnstrue
because really checksisset($cache[1])
and this key already exists, socastableToString()
returnsfalse
(detected as duplicate) and the choices values are generated incrementing integers as values.The PR's Effect
Before:
After: