Would be worth a test case, can you please add one?

Friendly ping @LordZardeck

I like this.
Thank you.

Could you add a small test case so this can be merged?

What is the pull request status? Do you need help?

Anyone willing to take over this PR to finish it? @YaFou perhaps?

I'm in vacation but I will finish it later @fabpot. Do I need to open a new pull request?

You can push directly on this PR.

@fabpot @Nyholm I opened a pull request LordZardeck/symfony#1 to add a test case for this feature.

I've tried to understand why we have the string cast in the first place. It was added in #14648 as a bug fix. @ogizanagi Can you review and comment on this one as you were the one who wrote the code?

I'm fine for removing this cast.

I think the original fix intended to make selecting choices indexed by keys consistent by always returning strings (it never meant to do it for choices sequentially indexed).
But the true "issue" is an array like ['0' => 'foo', 'foo' => 'bar'] is actually interpreted as [0 => 'foo', 'foo' => 'bar'] by PHP. Which means you can either get '0' or 0 as the value returned by ask depending if you used the key or choice value to answer the question (hence the fix suggestion to always return a string).

A proper fix would have been either:

• to only cast to string for assoc choices
• or to properly account for the type of the keys (which is probably not possible actually)

Thanks @ogizanagi. That's clearer to me now. @LordZardeck I think we need to add some test cases to cover the situation explained by @ogizanagi to be sure we don't break anything.

I think we need to add some test cases to cover the situation explained by @ogizanagi to be sure we don't break anything.

These test cases already exist and are failing now, in QuestionHelperTest::testChoiceFromChoicelistWithMixedKeys.

Fixing the code to only cast in the case of assoc choices should solve everything here (we should not forget to get https://github.com/LordZardeck/symfony/pull/1 merged as well in here)

@ogizanagi Would you like to take over?

Sure, I'll update the PR this week

@fabpot : Done. But I'm wondering if this should not be merged as a bug fix into 3.4 since the cast was only meant for keys, not for values (int choices values were casted as well). Support for stringable objects just is a consequence of this fix. Let me know, I'll rebase.

Thank you @LordZardeck.