Skip to content

[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

Closed
wants to merge 3 commits into from

Conversation

hliebe
Copy link

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

@nicolas-grekas nicolas-grekas changed the title [BUGFIX] ignore whitespaces in choicelist optionkeys [Form] ignore whitespaces in choicelist optionkeys Oct 28, 2017
@nicolas-grekas nicolas-grekas added this to the 2.8 milestone Oct 28, 2017
@nicolas-grekas
Copy link
Member

Why 2.8? Shouldn't it be 2.7?

@hliebe
Copy link
Author

hliebe commented Nov 1, 2017

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.

@nicolas-grekas
Copy link
Member

@ro0NL any review here, since you commented on the linked issue?

@ro0NL
Copy link
Contributor

ro0NL commented Jan 21, 2018

it's a new feature no? still think we should not implicitly trim (user known) choice options =/

@nicolas-grekas
Copy link
Member

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

@ro0NL
Copy link
Contributor

ro0NL commented Jan 22, 2018

why are input keys trimmed in the first place?

Que? That's exactly what's not happening right?

The name "foo " contains illegal characters. Names should start with a letter, digit or underscore and only contain letters, digits, numbers, underscores ("_"), hyphens ("-") and colons (":").

(contains a space)

Am i missing a trim call?

DX-wise what could be done is resolve a default trim=true|false option for the choice type. If we know options contains spaces we should set trim=false i guess.

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Jan 22, 2018

If we need to trim the model, it means the user input keys have been trimmed before. Why? That's what I mean.

@ro0NL
Copy link
Contributor

ro0NL commented Jan 22, 2018

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

@ro0NL
Copy link
Contributor

ro0NL commented Jan 22, 2018

So.. setting trim=false by default for choicetype would be sufficient actually?

@nicolas-grekas
Copy link
Member

@ro0NL looks like so!

@nicolas-grekas
Copy link
Member

@ro0NL or @hliebe up to do the PR? (setting trim=false on choicetype)

@HeahDude
Copy link
Contributor

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 ⚠️.

@nicolas-grekas
Copy link
Member

@HeahDude would be great if you could submit a PR doing so.

@HeahDude
Copy link
Contributor

See #26932.

fabpot added a commit that referenced this pull request Apr 16, 2018
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
@fabpot fabpot closed this Apr 16, 2018
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.

6 participants