[Doctrine][Bridge] EntityType : default choices value

[RapotOR]

If the choices property is equal to array(), the EntityChoiceList don't use empty choices and try query_builder or findAll.

We should use false as default instead of array() to be able to have an empty list if a custom filter gives an empty result. With array() as default, the EntityType populates choices with findAll even if our result was empty; and it is not expected.

(PR created instead of #2500 to pull it into 2.0 branch)

[beberlei]

This is valid, however as a fix i am not sure if an is_array() check would be better here, since "null" now fails for this case as well.

[ericclemmons]

First, it would be useful if you went ahead & created a test case (that would fail on 2.0) illustrating how it's darn near impossible to have empty choices.

Second, I would assume the behavior would be the following because of semantics:

• Default options uses "choices" => null. NULL should be any uninitialized state so we can distinguish between user-set values.
• If a user wants an empty list, they should explicitly use an empty array() for choices.
• The check would then be if (is_array($choices)) { ... }.
• false or null should not be allowed options, as neither are a collection of "choices".

[RapotOR]

It is my first PR and I was quite lost regarding PR.

Thanks for advises. It makes sense.
If the null value is the chosen way, the issue affects also ArrayChoiceList because it accept array for the choices options. A way is to avoid the _ArrayChoiceList::_construct

[ericclemmons]

We'll have to get some feedback from @beberlei. I'm usually hesitant to encourage extra work, since sometimes simple improvements go through a lot of architecture discussions. (Especially considering the required change to ArrayChoiceList, which I forgot was used via parent::__construct)

[RapotOR]

Some coments:
The parent::__construct only checks the $choice type (array or closure). As the null value should be added to admitted types, I guess it makes sense to replace the parent::__construct by new check allowing null too. I think there is no utility to change the ArrayChoiceList because an empty array works with array().

[fabpot]

@beberlei: Can you comment on this PR?

[webda2l]

+1

[beberlei]

@fabpot You can merge this PR, its good this way.

[fabpot]

@RapotOR: Can you squash your commits before I merge this PR? Thanks.

[stof]

it also need to be rebased on top of master as github says it conflicts

[RapotOR]

I've squashed the commits and github created a new PR #2868. I close this one.