-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[Form] ignore whitespaces in choicelist optionkeys #24712
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
hliebe
commented
Oct 27, 2017
Q | A |
---|---|
Branch? | 2.8 |
Bug fix? | yes |
New feature? | no |
BC breaks? | no |
Deprecations? | no |
Tests pass? | yes |
Fixed tickets | #24247 |
License | MIT |
Why 2.8? Shouldn't it be 2.7? |
It's a fix for issue #24247. The reported version is 2.8 so i decided to do it for this version. Before v2.8 ArrayKeyChoiceList was used which is deprecated since v2.8. I could do a patch for 2.7 but i was not really sure. |
@ro0NL any review here, since you commented on the linked issue? |
it's a new feature no? still think we should not implicitly trim (user known) choice options =/ |
@ro0NL agreed, we should not trim model data. What bugs me is : why are input keys trimmed in the first place? I understand why values are trimmed by default, but keys, it's wrong. Isn't that the root issue? |
Que? That's exactly what's not happening right?
(contains a space) Am i missing a trim call? DX-wise what could be done is resolve a default |
If we need to trim the model, it means the user input keys have been trimmed before. Why? That's what I mean. |
Ah right.. isnt the selected choice HTML wise the submitted value? Thus trimmed depending on trim=true|false. edit: i see that's in fact model data we implicitly trim.. |
So.. setting trim=false by default for choicetype would be sufficient actually? |
@ro0NL looks like so! |
Changing the value of the trim option as a bug fix may introduce weird behavior in some existing applications (because values submitted before were mapped to choice models thanks to the trim, even if this is the responsibility of the client to send a trim value). Ok to change the value of the trim option, but this change must be documented |
@HeahDude would be great if you could submit a PR doing so. |
See #26932. |
This PR was merged into the 2.7 branch. Discussion ---------- [Form] Fixed trimming choice values | Q | A | ------------- | --- | Branch? | 2.7 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #24247, #24712 | License | MIT | Doc PR | symfony/symfony-docs#9598 Follows #24712 discussion. Commits ------- 00cdf5e [Form] Fixed trimming choice values