8000 [Form] Fix #11694 - Enforce options value type check in some form types by kix · Pull Request #11696 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[Form] Fix #11694 - Enforce options value type check in some form types #11696

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
Closed
wants to merge 5 commits into from
Closed

[Form] Fix #11694 - Enforce options value type check in some form types #11696

wants to merge 5 commits into from

Conversation

kix
Copy link
Contributor
@kix kix commented Aug 18, 2014
Q A
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #11694
License MIT
Doc PR none

Symfony\Component\Form\Extension\Core\Type\RepeatedType allowed passing strings as options, which caused problems as in #11694.

stof
stof reviewed Aug 18, 2014
View reviewed changes
src/Symfony/Component/Form/Extension/Core/Type/RepeatedType.php

$resolver->setAllowedTypes(array(
'first_options' => 'array',
'second_options' => 'array',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would add it for options too

@stof stof added the Form label Aug 18, 2014
@stof
Copy link
Member
stof commented Aug 18, 2014

note to maintainers: should be merged in 2.3

@Tobion
Copy link
Contributor
Tobion commented Aug 18, 2014

👍

@kix
Copy link
Contributor Author
kix commented Aug 19, 2014

I'd like to note that many built-in form types are also prone to this behaviour:

  • BirthdayType has 'years' parameter that's supposed to be an array that gets passed to DateType;
  • ChoiceType expects 'preferred_choices' to be an array as this option is then passed to SimpleChoiceList constructor without any checks, and before setAllowedTypes is called;
  • CollectionType expects 'options' to be an array as options are then passed to $optionsNormalizer which uses string keys
  • CountryType, LanguageType, LocaleType, TimezoneType and CurrencyType could behave strange when a string is passed as 'choices' value; no checks here, too;
  • DateType::listMonths, listYears and listDays all expect an array (it's typehinted) though the allowed type is not set
  • TimeType has lots of logic that could possibly misbehave due to 'hours', 'minutes', 'seconds'
  • TimeTypecould possibly misbehave due to 'emptyValue' option, though there are some dependencies there.
  • UrlType can behave strange when a 'default_protocol' option is not an array.

Could fixing these types improve [DX]?

@webmozart
Copy link
Contributor

@kix Thanks for your investigation! Do you want to add fixes for these other problems to this PR?

@kix
Copy link
Contributor Author
kix commented Aug 19, 2014

@webmozart, sure, I've been doing some testing; currently thinking of a correct way to implement this all (some types are not so easy to figure out, like the Date)

@webmozart
Copy link
Contributor

@kix A good approach is to start with the trivial fixes first, and keep the more complicated ones for a later PR where we discuss some more.

@kix
Copy link
Contributor Author
kix commented Aug 19, 2014

@webmozart, sure, basically that's what I intended to do :)

@kix
Copy link
Contributor Author
kix commented Aug 19, 2014

Actually, the problem in ChoiceType and its derivatives is caused by OptionsResolver\OptionsResolver and OptionsResolver\Options.
The Options::all() call in OptionsResolver::resolve() leads to normalization being called, and this happens before options validation, which causes our problems.
Hence, any FormType that needs normalized options is not guaranteed to have valid options passed to the normalizers.

@kix
Copy link
Contributor Author
kix commented Aug 19, 2014

ChoiceType, CollectionType, CountryType, LanguageType, LocaleType, TimezoneType, TimeType and CurrencyType depend on normalizer logic implemented in OptionsResolver\Options.
Basically, this pull request is complete, and a new issue needs to be created on OptionsResolver::resolve() and Options::all() behaviour.

@kix kix mentioned this pull request Aug 19, 2014
Closed
@kix kix changed the title [Form] Fix #11694 - Enforce RepeatedType options value type check [Form] Fix #11694 - Enforce options value type check in some form types Aug 19, 2014
@kix
Copy link
Contributor Author
kix commented Aug 20, 2014

OptionsResolver problem is described in #4500, fixing it should open the way for some more form type fixes.

8000
webmozart added a commit that referenced this pull request Oct 16, 2014
@webmozart
bug #11696 [Form] Fix #11694 - Enforce options value type check in so…
7a13556 
…me form types (kix)

This PR was submitted for the master branch but it was merged into the 2.3 branch instead (closes #11696).

Discussion
----------

[Form] Fix #11694 - Enforce options value type check in some form types

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

`Symfony\Component\Form\Extension\Core\Type\RepeatedType` allowed passing strings as options, which caused problems as in #11694.

Commits
-------

0af6467 [Form] Fix #11694 - Enforce options value type check in some form types
@webmozart
Copy link
Contributor

Thank you so much Stepan! This is merged into 2.3 now. #4500 is being fixed in #12156, do you want to work on the remaining types after that?

@webmozart webmozart closed this Oct 16, 2014
@webmozart webmozart mentioned this pull request Oct 16, 2014
Closed
@kix
Copy link
Contributor Author
kix commented Oct 16, 2014

@webmozart, well, if it's merged then why not? I'll make another PR then.

@Tobion Tobion mentioned this pull request Oct 16, 2014
Merged
webmozart added a commit that referenced this pull request Oct 20, 2014
@webmozart
minor #12242 [Form] no need to add the url listener when it does not …
91fde69 
…do anything (Tobion)

This PR was merged into the 2.3 branch.

Discussion
----------

[Form] no need to add the url listener when it does not do anything

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

In line with #11696

Commits
-------

7aea1c9 [Form] no need to add the url listener when it does not do anything
@kix kix mentioned this pull request Nov 6, 2014
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
Form
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@kix @stof @Tobion @webmozart
0