[Form] bool2string strict comparison #18005
Conversation
| @@ -80,6 +94,23 @@ public function reverseTransform($value) | |||
| throw new TransformationFailedException('Expected a string.'); | |||
| } | |||
|
|
|||
| return true; | |||
| if (in_array($value, array_merge($this->defaultTrueValues, array($this->trueValue)), true)) { | |||
There was a problem hiding this comment.
Instead of the expensive expensive array_merge(), could you rearrange this to:
if ($this->trueValue === $value || in_array($value, $this->defaultValues, true)) {
|
This new behaviour looks wrong to me as it complicates things. Why do you send values via JavaScript at all when a checkbox is not checked? If I am not mistaken, this is not how checkboxes are supposed to behave. |
|
The point is that you may deal with a frontend model where your checkbox(es) may be bound to a model that reflects those value as "boolean" (eg checkbox:check ["true"] checkbox:not-check ["false"]). The latter would cause the bool2string transformer to evaluate "0", "" or "false" to You're totally right to say that's how checkboxes supposed to behave but that's actually not the intention of this PR (as i stated in the PRs decription) and will remain as is but eases the pain of the transforming "boolean" values (in the sense of it's real meaning "boolean") where false is false and true is true. sorry @xabbuh but i'm kind of fed up with this PR cuz I'm waiting since 06/15 to get qualified feedback on this, rebase, push, rebase and push and starting all over again. @webmozart please make a decision on that now or never ... i'm out |
|
@phramz Sorry for upsetting you. We need to take care only to merge what's really necessary in order to not bloat the code base. I have one more question: In the example you gave, do you send the form via CGI (regular form submission) or as part of a JSON object? i.e., could the values |
|
Ideally both Don't get me wrong, I really appreciate your and the work of all symfony members and contributors and totally understand that there is always lack of time and also cumbersome stuff to care about. What makes me unhappy about how that PR evolved is not wether it'll be merged or not nor that it took such a long time but that I think it would have been less frustrating to discuss those pros and cons before you asked me to write more tests and refactor error-messages and such. I'm totally fine with any decision about if it's a "necessary" or even useful feature or not. |
|
@phramz I understand that both are possible now. What I'm asking is: Can your JavaScript be changed to achieve the desired behavior without merging this PR? Is this a must-have or a nice-to-have for you? I'm very sorry for your bad experience with working on this PR. I totally understand your frustration. It's often easier for us to spot "obvious" issues such as missing tests or error messages than to judge whether a change actually makes sense. We try to do that, but sometimes that only gets clear after a certain amount of discussion. |
|
No worries ... I just put it in a dedicated bool formtype. Thanks for your time. |
…oString transformer (leberknecht) This PR was squashed before being merged into the 4.1-dev branch (closes #25741). Discussion ---------- [Form] issue-13589: adding custom false-values to BooleanToString transformer | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #13589 | License | MIT | Doc PR | symfony/symfony-docs#9031 As suggested in #13589 , this PR adds the option to specify custom false-values, so we can use the CheckBoxType to handle strings '1' / '0' and eval them to true / false. While HTTP defines that checkbox types are either submitted=true or not-submitted=false, no matter of what value was submitted, it would be nice to have this option. Also refs (which read like "the basic idea of the feature was accepted, PR almost done, then something-happend so PR wasnt merged"..?) symfony/symfony#15054 symfony/symfony#18005 Commits ------- a3e5ac496f [Form] issue-13589: adding custom false-values to BooleanToString transformer
…oString transformer (leberknecht) This PR was squashed before being merged into the 4.1-dev branch (closes #25741). Discussion ---------- [Form] issue-13589: adding custom false-values to BooleanToString transformer | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #13589 | License | MIT | Doc PR | symfony/symfony-docs#9031 As suggested in #13589 , this PR adds the option to specify custom false-values, so we can use the CheckBoxType to handle strings '1' / '0' and eval them to true / false. While HTTP defines that checkbox types are either submitted=true or not-submitted=false, no matter of what value was submitted, it would be nice to have this option. Also refs (which read like "the basic idea of the feature was accepted, PR almost done, then something-happend so PR wasnt merged"..?) #15054 #18005 Commits ------- a3e5ac4 [Form] issue-13589: adding custom false-values to BooleanToString transformer
The current behaviour of the
BooleanToStringTransformerperfectly fits the need of theCheckBoxTypeand how browsers handle form submissions of checkbox-values.The downside is that any value e.g. "0" will transform to
trueeven if thetrueValueis set to "1". Due to this it may cause trouble when you want to handle form-submission by Javascript. For a JS-Dev is may not be 100% clear that "0" will transform totruein the backend.This PR makes it possible to uncheck values without omitting the form-field in the post-data by submitting "0" or "" as value.
*This PR is a follow up of #15054. Head there to see the full discussion *