[Form] Allow empty choices array for ChoiceType #3299
Conversation
|
Sorry, I messed up with the tickets. Didn't know a pull request opens a ticket. |
|
This ticket depends on #3290 for being merged. Apart from this, a test case is missing. Add this to ChoiceTypeTest: |
| @@ -37,7 +37,7 @@ public function buildForm(FormBuilder $builder, array $options) | ||
| throw new FormException('The "choice_list" must be an instance of "Symfony\Component\Form\Extension\Core\ChoiceList\ChoiceListInterface".'); | ||
| } | ||
|
|
||
| if (!$options['choice_list'] && !$options['choices']) { | ||
| if (!$options['choice_list'] && !is_array($options['choices'])) { |
There was a problem hiding this comment.
You should do a null-check instead.
null === $options['choices']
In some cases, choices might be a traversable.
There was a problem hiding this comment.
false should also not be allowed. Maybe it's better to explicitly check for valid values:
if (!$options['choice_list'] && !is_array($options['choices']) && !$options['choices'] instanceof \Traversable) {
👍 for getting this fixed soon.
|
Ok, I updated the branch with the test case and craue's suggestion on explicitly checking valid values. |
| @@ -662,4 +662,11 @@ public function testAdjustFullNameForMultipleNonExpanded() | ||
|
|
||
| $this->assertSame('name[]', $view->get('full_name')); | ||
| } | ||
|
|
There was a problem hiding this comment.
Can you add the URL of the ticket here please?
// https://github.com/symfony/symfony/issues/3298
|
Should be fine now. |
|
Good. I see that you added a check for \Traversable too - can you add a test for this too? It should be fine to pass an empty \ArrayObject(). |
|
@bschussek: But even if there's passed something different than an array, the c'tor of |
|
@craue Yes. But in EntityType the choices option is reused and passed to EntityChoiceList, which extends ChoiceList and accepts any array or Traversable. You're right though that it's not possible to add a test covering this in ChoiceTypeTest, and in EntityTypeTest this is already covered. @fabpot Ready to merge. |
Commits ------- dbaddbb [Form] Allow empty choices array for ChoiceType Discussion ---------- [Form] Allow empty choices array for ChoiceType Bug fix: yes Feature addition: no Backwards compatibility break: no Symfony2 tests pass: yes The documentation about ChoiceType says, that default for 'choices' is array(). This is not allowed because an empty array evaluates to false: if (!$options['choice_list'] && !$options['choices']) { Use case: choices are empty and items are added dynamically. --------------------------------------------------------------------------- by chmielot at 2012-02-07T21:03:03Z Sorry, I messed up with the tickets. Didn't know a pull request opens a ticket. --------------------------------------------------------------------------- by bschussek at 2012-02-08T08:23:29Z This ticket depends on #3290 for being merged. Apart from this, a test case is missing. Add this to ChoiceTypeTest: // #3298 public function testInitializeWithEmptyChoices() { $this->factory->createNamed('choice', 'name', null, array( 'choices' => array(), )); } --------------------------------------------------------------------------- by craue at 2012-02-10T20:32:44Z @bschussek: Why does it depend on #3290? I have issues with the `!$options['choices']` check even without #3290 applied. --------------------------------------------------------------------------- by chmielot at 2012-02-10T21:24:28Z Ok, I updated the branch with the test case and craue's suggestion on explicitly checking valid values. --------------------------------------------------------------------------- by chmielot at 2012-02-11T09:07:05Z Should be fine now. --------------------------------------------------------------------------- by bschussek at 2012-02-11T09:15:10Z Good. I see that you added a check for \Traversable too - can you add a test for this too? It should be fine to pass an empty \ArrayObject(). --------------------------------------------------------------------------- by craue at 2012-02-11T10:05:36Z @bschussek: But even if there's passed something different than an array, the c'tor of `SimpleChoiceList` (which is called in line 45) is type hinted with `array` and won't allow passing a `Traversable` anyway, right? --------------------------------------------------------------------------- by bschussek at 2012-02-11T10:16:36Z @craue Yes. But in EntityType the choices option is reused and passed to EntityChoiceList, which extends ChoiceList and accepts any array or Traversable. You're right though that it's not possible to add a test covering this in ChoiceTypeTest, and in EntityTypeTest this is already covered. @fabpot Ready to merge.
Bug fix: yes
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: yes
The documentation about ChoiceType says, that default for 'choices' is array().
This is not allowed because an empty array evaluates to false:
if (!$options['choice_list'] && !$options['choices']) {
Use case: choices are empty and items are added dynamically.