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

[2.7] [Form] fix choice value "false" in ChoiceType #17760

Merged
merged 2 commits into from Feb 26, 2016

Conversation

HeahDude
Copy link
Contributor

Q A
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #17292, #14712, #17789
License MIT
Doc PR -

@xabbuh
Copy link
Member

xabbuh commented Feb 11, 2016

👍

Status: Reviewed

ping @symfony/deciders

@fabpot
Copy link
Member

fabpot commented Feb 12, 2016

👍

@webdevilopers
Copy link

Fix works! 👍

@Tobion
Copy link
Member

Tobion commented Feb 12, 2016

This fix contradicts https://github.com/symfony/symfony/pull/16681/files#r45997531
But I think @webmozart was forgetting that false would then conflict with the placeholder optional value.
So changing false to 0 looks better to me indeed.
But then castableToString must also be updated to reflect that change.

Please add a test for that (which would currently fail AFAIK):

'choices' => [
        'Yes'  => true,
        'No'   => false,
        'Undecided'   => null,
    ],

null and false should not conflict and thus can both be used as '' and '0' value (instead of fallback solution to use incrementing numbers).

@Tobion
Copy link
Member

Tobion commented Feb 12, 2016

Btw, this would fix false choices with required => false. But a null choice with required false, would still have the same problem?! IMO a placeholder value should always be different than any given choice.

@Tobion
Copy link
Member

Tobion commented Feb 12, 2016

So basically we need to map all scalars that would result in an empty string to something else to not conflict we the placeholder:

I would propose this mapping:

And if there are still conflicts e.g. because someone has both null and '~' as choices (which would conflict with above mapping), then it still uses the incrementing numbers as fallback which is what castableToString ensures.

@HeahDude
Copy link
Contributor Author

Thanks @Tobion for the review. You're right, castableToString must be refactored so when using both boolean and null values it returns true for the test you mention pass.

I'm fixing it.

Status: Needs Work

@HeahDude HeahDude changed the title [2.7] [Form] fix choice value "false" in ChoiceType [WIP] [2.7] [Form] fix choice value "false" in ChoiceType Feb 13, 2016
@HeahDude HeahDude changed the title [WIP] [2.7] [Form] fix choice value "false" in ChoiceType [2.7] [Form] fix choice value "false" in ChoiceType Feb 13, 2016
@HeahDude
Copy link
Contributor Author

Tests are fixed.

Status: Needs Review

@Tobion
Copy link
Member

Tobion commented Feb 13, 2016

The problem now is

'choices' => [
    'Undecided'   => null,
    'Yes'  => true,
    'No'   => false,
],
'placeholder' => 'Please choose',

will not add the placeholder anymore because null is mapped to '' value and thus the placeholder is ignored because the choice assumes the null is already the placeholder value. And even worse is that people cannot select Undecided in this required select anymore because the browser will treat it as the placeholder as well (cuz it has an empty value). Before it used incrementing integers and thus was not affected by this.
That is why I suggested to change the mapping from null to something like ~ as value to not be treated as placeholder.

@HeahDude
Copy link
Contributor Author

@Tobion, thanks again for the heads-up, so I keep on testing.

Status: Needs Work

HeahDude added a commit to HeahDude/symfony that referenced this pull request Feb 26, 2016
HeahDude added a commit to HeahDude/symfony that referenced this pull request Feb 26, 2016
HeahDude added a commit to HeahDude/symfony that referenced this pull request Feb 26, 2016
fabpot added a commit that referenced this pull request Feb 26, 2016
… `choices_as_values` (HeahDude)

This PR was squashed before being merged into the 3.0 branch (closes #17886).

Discussion
----------

[WIP] [3.0] [Form] fix tests added by #17760 by removing `choices_as_values`

| Q             | A
| ------------- | ---
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | not yet
| Fixed tickets | n/a
| License       | MIT
| Doc PR        | -

- [x] Wait for #17760 being merged in 3.0

Commits
-------

03a7705 [WIP] [3.0] [Form] fix tests added by #17760 by removing
fabpot added a commit that referenced this pull request Feb 26, 2016
* 3.0:
  [WIP] [3.0] [Form] fix tests added by #17760 by removing
  removed obsolete code
  [HttpFoundation] Remove @throws from ParameterBag::get() PHPDoc. This was for the now removed deep flag.
  [Form] refactor `RadioListMapper::mapDataToForm()`
  [Form] fix choice value "false" in ChoiceType
HeahDude added a commit to HeahDude/symfony that referenced this pull request Feb 26, 2016
fabpot added a commit that referenced this pull request Feb 26, 2016
…ahDude)

This PR was merged into the 2.8 branch.

Discussion
----------

[WIP] [2.8] [Form] fix FQCN in tests added by #17760

| Q             | A
| ------------- | ---
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | not yet
| Fixed tickets | n/a
| License       | MIT
| Doc PR        | -

- [x] Update tests from #17760
- [x] Wait for #17760 to be merged in 2.8

Commits
-------

acdd7db [Form] fix tests added by #17760 with FQCN
fabpot added a commit that referenced this pull request Feb 26, 2016
* 2.8:
  [Form] fix tests added by #17760 with FQCN
@HeahDude HeahDude deleted the fix-#17292 branch February 26, 2016 06:26
fabpot added a commit that referenced this pull request Feb 28, 2016
* 3.0:
  #17676 - making the proxy instantiation compatible with ProxyManager 2.x by detecting proxy features
  #17676 - making the proxy instantiation compatible with ProxyManager 2.x by detecting proxy features
  Fix bug when using an private aliased factory service
  [Form] fix tests added by #17798 by removing `choices_as_values`
  [Form] fix FQCN in tests added by #17798
  [DependencyInjection] Remove unused parameter of private property
  bug #17798 [Form] allow `choice_label` option to be `false`
  [Form] fix tests added by #17760 with FQCN
  ChoiceFormField of type "select" could be "disabled"
  Update contributing docs
  [Console] Fix escaping of trailing backslashes
  Fix constraint validator alias being required
  [DependencyInjection] Simplified code in AutowirePass
  [ci] clone with depth=1 to kill push-forced PRs
  Add check on If-Range header
This was referenced Feb 28, 2016
@Taluu
Copy link
Contributor

Taluu commented Feb 29, 2016

This seems to break API ways on Symfony 2.8.3. When before I could do the following :

<?php
class ChoiceType extends AbstractType
{
    public function buildForm(FormBuilderInterface $builder, array $options)
    {
        $builder->add('status', ChoiceType::class, [
            'choices'  => [true, false, 'true', 'false'],
            'choices_as_values' => true
        ]);
    }
}

With the following json sent data :

{
        "choices":[
          {
            "hash":"foo4u",
            "label":"foo",
            "status": true
          },

          {
            "hash":"bar4u",
            "label":"bar",
            "status": "false"
          },

          {
            "hash":"baz4u",
            "label":"baz",
            "status": false
          }
        ]
}

whilre before I could use the values true, "true", false and "false" I can now use only true and false.

@Taluu
Copy link
Contributor

Taluu commented Feb 29, 2016

And when I change the order in the choices key in the options with ['true', 'false', true, false] instead, all my false are changed to null...

@HeahDude
Copy link
Contributor Author

@Taluu, The submission of a ChoiceType actually requires the string value to get properly mapped to the model choice data.

If you submit a true value it will be cast to string so "1" not by the choice list constructor like in this fix but directly in the Form::submit() method. Changing the behavior at this point is a BC break.

So false get casted to an empty string which cannot map the false model choice data expecting the string "0" as set in the choice list constructor (by this PR).

If there is null or an object among the choices, the default ArrayChoiceList::castableToString() method will return false then you should rely on numeric index to send as string or int in a json hash.

false value could get in conflict with placeholder in some cases (solves by this PR).

Another way to resolve this is to return false in castableToString() is there is boolean, but we loose the ability to match true as '"1"' with is a bigger BC break...

You can rely on choice_value option and always send the good strings values on submission, or use choice_label option and a choices array with a numeric index and submit the right index.

@Taluu
Copy link
Contributor

Taluu commented Feb 29, 2016

For legacy reason we have these "true" and "false" values. But we have an internal ticket to use real booleans instead ("one day my prince will come"), and then using a CheckboxType would be the way to go.

Until then, we will have to stick with "true" and "false". Using either ['true', 'false'] or [true, false] works though...

@HeahDude
Copy link
Contributor Author

Edge case then. Happy coding :)

@Taluu
Copy link
Contributor

Taluu commented Feb 29, 2016

Yep, I confirm, this is indeed due to this. Don't know if it was there or not before, but using a CheckboxType when we will have fixed the internal ticket (And chosen between string or boolean) will fix the damn thing. Yeay.

Symfony\Component\Validator\ConstraintViolation
Object(Symfony\Component\Form\Form).children[choices].children[0].children[status] = 1
Caused by:
Symfony\Component\Form\Exception\TransformationFailedException
Unable to reverse value for property path "[status]": The choice "1" does not exist or is not unique
Caused by:
Symfony\Component\Form\Exception\TransformationFailedException
The choice "1" does not exist or is not unique

And using directly the CheckboxType is not possible (Even though it passes with flying colors - a "false" is transformed into false and "true" into true), as we have a need to have "true" and "false" values. So yeah nothing to report.

@HeahDude
Copy link
Contributor Author

@Taluu, note that the ChoiceType when expanded uses CheckboxType or RadioType (which extends the first) to represent as a virtual boolean if the actual string value is selected or not as a choice among other values and to be mapped back to a model value.

That's why it messes with PATCH method and $clearMissing (ref : #17771 (comment)).

When you need to use CheckboxType or RadioType to map a real boolean property in a form, consider using a CollectionType.

@webmozart
Copy link
Contributor

@Taluu You could preprocess your request data and postprocess your responses to transform Booleans to strings and back. That would at least fix your API.

fabpot added a commit that referenced this pull request Apr 28, 2016
…ChoiceType` and its children (HeahDude)

This PR was merged into the 2.7 branch.

Discussion
----------

[Form] fixed BC break with pre selection of choices with `ChoiceType` and its children

| Q             | A
| ------------- | ---
| Branch        | 2.7+
| Bugfix?  | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #18173, #14712, #17789
| License       | MIT
| Doc PR        | -

- f7eea72 reverts BC break introduced in #17760
- 58e8ed0 fixes pre selection of choice with model values such as `false`, `null` or empty string without BC break.

`ChoiceType` now always use `ChoiceToValueTransformer` or `ChoicesToValuesTransformer` depending on `multiple` option.
Hence `CheckboxListMapper` and `RadioListMapper` don't handle the transformation anymore.

Commits
-------

ea5375c [Form] refactor CheckboxListMapper and RadioListMapper
71841c7 Revert "[Form] refactor `RadioListMapper::mapDataToForm()`"
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

10 participants