Skip to content
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

[Form] fix Catchable Fatal Error if choices is not an array #17163

Closed
wants to merge 1 commit into from
Closed

[Form] fix Catchable Fatal Error if choices is not an array #17163

wants to merge 1 commit into from

Conversation

Gladhon
Copy link

@Gladhon Gladhon commented Dec 29, 2015

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

Since 2.7.8 I got a BC-Break Error

Catchable Fatal Error: Argument 1 passed to Symfony\Component\Form\Extension\Core\Type\ChoiceType::normalizeLegacyChoices() must be of the type array, null given

normalizeLegacyChoices work only with array, so if choices not an array, just don't try to normlize.

@xabbuh
Copy link
Member

xabbuh commented Dec 29, 2015

The choice list normalizer does this:

// Harden against NULL values (like in EntityType and ModelType)
$choices = null !== $options['choices'] ? $options['choices'] : array();

What about using the same here?

@Gladhon
Copy link
Author

Gladhon commented Dec 29, 2015

If the choicesNormalizer have to make an array in every case, you're right.
IMHO has the choicesNormalizer to normalize arrays to avoid bc's and leave everything else as it is.

@Gladhon
Copy link
Author

Gladhon commented Dec 29, 2015

@webmozart @nicolas-grekas
Would be nice to merge these in, cause 2.7.8 is broken

Depends on:
#16685

@nicolas-grekas
Copy link
Member

👍

'expanded' => false,
'choices' => null,
));
$this->assertEquals($form->getConfig()->getOption('choices'), null);
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure but I think assertEquals doesn't make a strict comparison. An assertion will be considered valid if you receive false whereas you expect null (and vice-versa). I suggest to use the proper assertion methods instead for both readability and strict comparison.

$this->assertNull($form->getConfig()->getOption('choices'));
$this->assertFalse($form->getConfig()->getOption('multiple'));
$this->assertFalse($form->getConfig()->getOption('expanded'));

Copy link
Author

Choose a reason for hiding this comment

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

I will change this as you wish.

@GrahamCampbell
Copy link
Contributor

Surely checking if is null is more correct? If something else is passed that's not null or an array, we want the type error to happen, right?

@Tobion
Copy link
Member

Tobion commented Dec 29, 2015

This conflicts with #16796. I think #16796 should have been merged in 2.7 and not 2.8 and it also covers this case here.

@nicolas-grekas
Copy link
Member

@Tobion you're right!
@Gladhon can you then cherry-pick the patch in #16796 and keep your test case?

@Gladhon
Copy link
Author

Gladhon commented Dec 30, 2015

@nicolas-grekas done

@nicolas-grekas
Copy link
Member

@Gladhon it looks like the phpdoc block wasn't formatted correctly (see fabbot's patch) can you please fix it and also squash all the commits in one?

@nicolas-grekas nicolas-grekas changed the title Catchable Fatal Error: Argument 1 passed to Symfony\Component\Form\Extension\Core\Type\ChoiceType::normalizeLegacyChoices() must be of the type array, null given [Form] fix Catchable Fatal Error if choices is not an array Dec 30, 2015
@nicolas-grekas
Copy link
Member

Thank you @Gladhon.

nicolas-grekas added a commit that referenced this pull request Dec 30, 2015
…y (Gladhon, nicolas-grekas)

This PR was merged into the 2.7 branch.

Discussion
----------

[Form] fix Catchable Fatal Error if choices is not an array

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

Since 2.7.8 I got a BC-Break Error

Catchable Fatal Error: Argument 1 passed to Symfony\Component\Form\Extension\Core\Type\ChoiceType::normalizeLegacyChoices() must be of the type array, null given

normalizeLegacyChoices work only with array, so if choices not an array, just don't try to normlize.

Commits
-------

f3c2a9b [Form] fix Catchable Fatal Error if choices is not an array
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.

None yet

7 participants