Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Form] Fix casting regression in DoctrineChoiceLoader#17006
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Tobion commentedDec 14, 2015
What does this fix? What type is the id value that you want to cast? |
bendavies commentedDec 14, 2015
Well, does that even matter? It's a regression right? |
Tobion commentedDec 14, 2015
Just because the new version doesn't behave exactly like the deprecated one doesn't mean it's a regression. |
Tobion commentedDec 14, 2015
Btw, for your use-case the string case is not safe to use anyway, because thehttps://github.com/ramsey/uuid/blob/master/src/UuidInterface.php does not mandate a |
bendavies commentedDec 14, 2015
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. |
bendavies commentedDec 14, 2015
Thoughts@webmozart? Since you wrote it. |
eXtreme commentedDec 14, 2015
Well it is used as a key in an array and ID in doctrine can be a value object. |
webmozart commentedDec 15, 2015
I think it's fine to make this change. Could you add a test case though? |
bendavies commentedDec 15, 2015
@webmozart thanks, will do |
bendavies commentedDec 15, 2015
@webmozart how's that? |
webmozart commentedDec 15, 2015
👍 |
Tobion commentedDec 15, 2015
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/AIn 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
bendavies commentedDec 15, 2015
Thanks@Tobion |
In symfony 2.7, theDoctrineChoiceLoader andIdReader were introduce to replace the deprecatedEntityChoiceList
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 wasmaintained elsewhere, however.Since the new
DoctrineChoiceLoaderdeprecatedEntityChoiceList, i'm calling this a regression.