Skip to content

[Form] Choice loader is not validated against unknown choice(s) #23679

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
ro0NL opened this issue Jul 26, 2017 · 7 comments
Closed

[Form] Choice loader is not validated against unknown choice(s) #23679

ro0NL opened this issue Jul 26, 2017 · 7 comments

Comments

@ro0NL
Copy link
Contributor

ro0NL commented Jul 26, 2017

Q A
Bug report? yes
Feature request? no
BC Break report? a bit?
RFC? no
Symfony version 3.2

While having this form wtih a language type and custom choice type, i noticed submitting an unknown value triggers a transformation value (thus form error) on the choice field but not for the language field.

This worked OK in 2.7

$form = $this->createFormBuilder(null, array('csrf_protection' => false)))
    ->add('a', LanguageType::class)
    ->add('b', ChoiceType::class, array('choices' => array(1,2,3)))
    ->getForm();
$form->handleRequest($request);
POST /
form[a]=x&form[b]=x

3.2
image

2.7
image

@HeahDude
Copy link
Contributor

HeahDude commented Jul 26, 2017

This is the drawback of lazy loading choices, we cannot validate a submitted value without loading all choices.

I agree this is a kind of BC break though, we should find a way to deprecate this path, to enforce usage of proper constraints.

@ro0NL
Copy link
Contributor Author

ro0NL commented Jul 26, 2017

Hm.. defining a constraint kinda defeats laziness as well no? Cant we use loadChoicesForValues as an optimized check?

I dont think we should deprecate this behavior.

@HeahDude
Copy link
Contributor

Hm.. defining a constraint kinda defeats laziness as well no?

Indeed, but without violating SRP.

@ro0NL
Copy link
Contributor Author

ro0NL commented Jul 26, 2017

I always liked choice type validating out-of-the-box for unknown choices. It's a nice safety net.

But you're right i never defined constraints for choice fields :)) i rely on the behavior of forms for this. Basically i choose between forms and the standalone validator per use-case, and mostly it's forms TBH.

It's at least weird to have different behavior between "choices" and "choice_loader".

@HeahDude
Copy link
Contributor

HeahDude commented Jul 26, 2017

But you're right i never defined constraints for choice fields

And this is why I think a deprecation is necessary to make it clear.

Relying on internal form behavior to validate data is wrong.

@ro0NL
Copy link
Contributor Author

ro0NL commented Jul 26, 2017

Okay.. as forms already integrates constraint validation. Why not auto register a choice constraint... isnt that plain convenient? I.e. normalize constraints option and prepend it.

@HeahDude
Copy link
Contributor

IMO This is a good idea to consider when revamping type guessing.

nicolas-grekas added a commit that referenced this issue Dec 8, 2018
This PR was merged into the 3.4 branch.

Discussion
----------

[Form] filter out invalid Intl values

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #23679
| License       | MIT
| Doc PR        |

Commits
-------

a2d6940 filter out invalid Intl values
nicolas-grekas added a commit that referenced this issue Dec 8, 2018
This PR was merged into the 4.1 branch.

Discussion
----------

[Form] filter out invalid language values

| Q             | A
| ------------- | ---
| Branch?       | 4.1
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #23679
| License       | MIT
| Doc PR        |

Commits
-------

ed97568 filter out invalid language values
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants