Skip to content

[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

Merged
merged 1 commit into from
Nov 6, 2016

Conversation

yceruto
Copy link
Member

@yceruto yceruto commented Nov 1, 2016

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 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):

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) as choice values always (unless have duplicates values).

The Problem

But in this situation it doesn't happen:

$form = $this->createFormBuilder()
    ->add('foo', ChoiceType::class, [
        'choices' => [
            'Min' => 0.5,
            'Mid' => 1.0,
            'Max' => 1.5,
        ]
    ])
    ->getForm();

Output:

<select id="form_foo" name="form[foo]">
  <option value="0">Min</option>
  <option value="1">Mid</option>
  <option value="2">Max</option>
</select>

Why?

If the key of the array is a float number, it's interpreted as integer automatically:

// ...

$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 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:

<select id="form_foo" name="form[foo]">
  <option value="0">Min</option>
  <option value="1">Mid</option>
  <option value="2">Max</option>
</select>

After:

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

@javiereguiluz
Copy link
Member

👍 the change looks solid and your explanation was perfect. Thanks!

} elseif (isset($cache[$choice])) {
}

$choice = false === $choice ? '0' : (string) $choice;
Copy link
Member

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()?

Copy link
Member Author

@yceruto yceruto Nov 2, 2016

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.

Copy link
Member Author

@yceruto yceruto Nov 5, 2016

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

@linaori
Copy link
Contributor

linaori commented Nov 2, 2016

To be honest, I would expect 1.0 to be shown here:

  <option value="1">Mid</option>

@yceruto
Copy link
Member Author

yceruto commented Nov 2, 2016

@iltar there aren't differences as float numbers in JavaScript, so no matter.

@linaori
Copy link
Contributor

linaori commented Nov 2, 2016

It's a matter of consistency

@yceruto
Copy link
Member Author

yceruto commented Nov 2, 2016

You are right, but this can't be solved from PHP, because we can't differentiate between 1.0 and 1.00 (as floats) at all, so IMHO the inconsistency is irrelevant in this case.

@yceruto yceruto force-pushed the form/fixed-float-choices branch from 65d1af6 to 3564228 Compare November 5, 2016 18:02
@HeahDude
Copy link
Contributor

HeahDude commented Nov 6, 2016

👍

@fabpot
Copy link
Member

fabpot commented Nov 6, 2016

Thank you @yceruto.

@fabpot fabpot merged commit 3564228 into symfony:2.7 Nov 6, 2016
fabpot added a commit that referenced this pull request Nov 6, 2016
…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
@yceruto yceruto deleted the form/fixed-float-choices branch November 6, 2016 16:51
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.

7 participants