Skip to content

[Form] Allowed binding false to a checkbox #7789

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.

Sign up for GitHub

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

Jump to bottom
Merged
fabpot merged 2 commits into from
Apr 22, 2013
Merged

[Form] Allowed binding false to a checkbox #7789

fabpot merged 2 commits into from
Apr 22, 2013

Conversation

@jakzal
Copy link
Contributor

@jakzal jakzal commented Apr 22, 2013

Q A
Bug fix? no
New feature? no
BC breaks? yes
Deprecations? no
Tests pass? Build Status
Fixed tickets #7139
License MIT
Doc PR -

@webmozart
Copy link
Contributor

webmozart commented Apr 22, 2013

Looks good, but I'm not sure about the implications of the change in bind(). This means that we're breaking with the assumption that the value transformers always receive either a string or null when a form is submitted. Could this be a problem?

@jakzal
Copy link
Contributor Author

jakzal commented Apr 22, 2013

WIthout the change in bind() the value would be cast to an empty string. We wouldn't be able to tell the difference between an empty string and false.

How about changing false to null (in bind())? We would stick to our promise that value is either a string or null and the BooleanToStringTransformer wouldn't have to be modified.

@webmozart
Copy link
Contributor

webmozart commented Apr 22, 2013

Changing false to null sounds like a good plan! 👍

@jakzal
Copy link
Contributor Author

jakzal commented Apr 22, 2013

@bschussek PR updated.

@webmozart
Copy link
Contributor

webmozart commented Apr 22, 2013

Great! Can you please also add a test to SimpleFormTest that tests the conversion of false => null?

Please add the URI of this PR as comment over all added test cases:

// https://github.com/symfony/symfony/pull/7789

fabpot added a commit that referenced this pull request Apr 22, 2013
@fabpot
merged branch jakzal/checkbox-type-false (PR #7789)
9cdd1c7 
This PR was merged into the master branch.

Discussion
----------

[Form] Allowed binding false to a checkbox

| Q             | A
| ------------- | ---
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | yes
| Deprecations? | no
| Tests pass?   | [![Build Status](https://travis-ci.org/jakzal/symfony.png?branch=checkbox-type-false)](https://travis-ci.org/jakzal/symfony)
| Fixed tickets | #7139
| License       | MIT
| Doc PR        | -

Commits
-------

24ef8d2 [Form] Added a SimpleFormTest test case.
e493984 [Form] Allowed binding false to a checkbox.
@fabpot fabpot merged commit 24ef8d2 into symfony:master Apr 22, 2013
@jakzal jakzal deleted the checkbox-type-false branch April 22, 2013 18:35
@jakzal jakzal mentioned this pull request Apr 23, 2013
Closed
@lyrixx lyrixx mentioned this pull request Jun 21, 2013
Closed
@jakzal jakzal mentioned this pull request Feb 26, 2015
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@jakzal @webmozart @fabpot