[Form] Fix casting regression in DoctrineChoiceLoader #17006
Conversation
|
What does this fix? What type is the id value that you want to cast? |
|
Well, does that even matter? It's a regression right? |
|
Just because the new version doesn't behave exactly like the deprecated one doesn't mean it's a regression. |
|
Btw, for your use-case the string case is not safe to use anyway, because the https://github.com/ramsey/uuid/blob/master/src/UuidInterface.php does not mandate a |
|
If you don't want to classify it as a regression, fine, although the feels like a stretch in this particular case. My change is making the behaviour consistent throughout the class. It's casted to string in one place but not a other. My particular use case has no relevance in the discussion. |
|
Thoughts @webmozart? Since you wrote it. |
|
Well it is used as a key in an array and ID in doctrine can be a value object. |
|
I think it's fine to make this change. Could you add a test case though? |
|
@webmozart thanks, will do |
|
@webmozart how's that? |
|
👍 |
|
Thank you @bendavies. |
…davies) This PR was merged into the 2.7 branch. Discussion ---------- [Form] Fix casting regression in DoctrineChoiceLoader | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | N/A | License | MIT | Doc PR | N/A In symfony 2.7, the [DoctrineChoiceLoader](https://github.com/symfony/symfony/blob/9543b36a34221aaca9f2f3cfe34112e8468a591a/src/Symfony/Bridge/Doctrine/Form/ChoiceList/DoctrineChoiceLoader.php) and [IdReader](https://github.com/symfony/symfony/blob/9543b36a34221aaca9f2f3cfe34112e8468a591a/src/Symfony/Bridge/Doctrine/Form/ChoiceList/IdReader.php) were introduce to replace the deprecated [EntityChoiceList](https://github.com/symfony/symfony/blob/9543b36a34221aaca9f2f3cfe34112e8468a591a/src/Symfony/Bridge/Doctrine/Form/ChoiceList/EntityChoiceList.php) There appears to have been a change in behaviour in the refactor, as the old `EntityChoiceList` [casts ID to strings](https://github.com/symfony/symfony/blob/9543b36a34221aaca9f2f3cfe34112e8468a591a/src/Symfony/Bridge/Doctrine/Form/ChoiceList/EntityChoiceList.php#L248), whereas the new `DoctrineChoiceLoader` [does not](https://github.com/symfony/symfony/blob/2.7/src/Symfony/Bridge/Doctrine/Form/ChoiceList/DoctrineChoiceLoader.php#L159). The casting behavior was [maintained elsewhere](https://github.com/symfony/symfony/blob/2.7/src/Symfony/Bridge/Doctrine/Form/ChoiceList/DoctrineChoiceLoader.php#L122), however. Since the new `DoctrineChoiceLoader` deprecated `EntityChoiceList`, i'm calling this a regression. Commits ------- 54bbade [Form] cast IDs to match deprecated behaviour of EntityChoiceList
|
Thanks @Tobion |
In symfony 2.7, the DoctrineChoiceLoader and IdReader were introduce to replace the deprecated EntityChoiceList
There appears to have been a change in behaviour in the refactor, as the old
EntityChoiceListcasts ID to strings, whereas the newDoctrineChoiceLoaderdoes not. The casting behavior was maintained elsewhere, however.Since the new
DoctrineChoiceLoaderdeprecatedEntityChoiceList, i'm calling this a regression.