Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Serializer] Deprecate CsvEncoder as_collection false default value#27715
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
Merged
fabpot merged 1 commit intosymfony:masterfromogizanagi:serializer/deprecate_as_collection_false_defaultJun 30, 2018
Merged
[Serializer] Deprecate CsvEncoder as_collection false default value#27715
fabpot merged 1 commit intosymfony:masterfromogizanagi:serializer/deprecate_as_collection_false_defaultJun 30, 2018
Uh oh!
There was an error while loading.Please reload this page.
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
Member
nicolas-grekas commentedJun 28, 2018
(rebase needed) |
dunglas approved these changesJun 28, 2018
ContributorAuthor
ogizanagi commentedJun 29, 2018
Rebased |
chalasr approved these changesJun 29, 2018
fabpot previously requested changesJun 30, 2018
UPGRADE-4.2.md Outdated
| -------------- | ||
| * The`lazy` attribute on`doctrine.event_listener` tags was removed. | ||
| Listeners are now lazy by default. So any`lazy` attributes can safely be removed from those tags. |
Member
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Does not seem related to this PR.
ContributorAuthor
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Yes, indeed. It's just to respect the alpha order we use on UPGRADE files.
I can do another PR if you want, but IIRC we already accepted such changes inside unrelated PRs in the past.
ContributorAuthor
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
See#27781
fabpot approved these changesJun 30, 2018
sroze approved these changesJun 30, 2018
Member
fabpot commentedJun 30, 2018
Thank you@ogizanagi. |
fabpot added a commit that referenced this pull requestJun 30, 2018
…default value (ogizanagi)This PR was merged into the 4.2-dev branch.Discussion----------[Serializer] Deprecate CsvEncoder as_collection false default value| Q | A| ------------- | ---| Branch? | master <!-- see below -->| Bug fix? | no| New feature? | no <!-- don't forget to update src/**/CHANGELOG.md files -->| BC breaks? | no <!-- seehttps://symfony.com/bc -->| Deprecations? | yes <!-- don't forget to update UPGRADE-*.md and src/**/CHANGELOG.md files -->| Tests pass? | yes <!-- please add some, will be required by reviewers -->| Fixed tickets | N/A <!-- #-prefixed issue number(s), if any -->| License | MIT| Doc PR | N/AAs already expressed in#25369 and related issues, this behavior is quite counter-intuitive. It may be fine for write-API with a single document in the body but I think such CSV APIs are way less common than file-based ones, expecting collections. So I think this behavior should be opt-in explicitly in required cases, always dealing with collections by default.This is still an arbitrary decision, but trying to make it based on use-cases and user's experience with CSV.Note: perhaps we could find a better name for this as the semantic of setting `as_collection` to `false` to get the single row directly would not be really obvious.Also, it could throw an exception when getting multiple rows where only one was expected.Commits-------bce59c8 [Serializer] Deprecate CsvEncoder as_collection false default value
nicolas-grekas added a commit that referenced this pull requestMay 29, 2019
…on & change default value (ogizanagi)This PR was merged into the 5.0-dev branch.Discussion----------[Serializer] Remove CsvEncoder "as_collection" deprecation & change default value| Q | A| ------------- | ---| Branch? | master <!-- see below -->| Bug fix? | no| New feature? | no <!-- please update src/**/CHANGELOG.md files -->| BC breaks? | no <!-- seehttps://symfony.com/bc -->| Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files -->| Tests pass? | yes (AppVeyor failure unrelated. Seehttps://github.com/symfony/symfony/pull/31685/files) <!-- please add some, will be required by reviewers -->| Fixed tickets |#27715 <!-- #-prefixed issue number(s), if any -->| License | MIT| Doc PR | N/A <!-- required for new features -->As planned in#27715Commits-------22dd071 [Serializer] Remove CsvEncoder "as_collection" deprecation & change default value
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading.Please reload this page.
As already expressed in#25369 and related issues, this behavior is quite counter-intuitive. It may be fine for write-API with a single document in the body but I think such CSV APIs are way less common than file-based ones, expecting collections. So I think this behavior should be opt-in explicitly in required cases, always dealing with collections by default.
This is still an arbitrary decision, but trying to make it based on use-cases and user's experience with CSV.
Note: perhaps we could find a better name for this as the semantic of setting
as_collectiontofalseto get the single row directly would not be really obvious.Also, it could throw an exception when getting multiple rows where only one was expected.