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] Allow to pass csv encoder options in context#22537
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
[Serializer] Allow to pass csv encoder options in context#22537
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| } | ||
| /** | ||
| * @param array $context |
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.
useless, just repeats the signature
| class CsvEncoderimplements EncoderInterface, DecoderInterface | ||
| { | ||
| constFORMAT ='csv'; | ||
| constDELIMITER_KEY ='csv_delimiter'; |
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.
Just asking: is there any reason to name these constants*_KEY instead of*_CHAR? I was checking the popular CSV library published by ThePhpLeague and they say that the separator, enclosure, etc. can only be 1 char:https://github.com/thephpleague/csv/blob/master/src/AbstractCsv.php#L42
ogizanagiApr 27, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
Actually theKEY suffix is not about csv at all. It's about the constant representing the key of the option to pass in the context argument. SeeDateTimeNormalizer::FORMAT_KEY for reference. 😄
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.
Sorry for my confusion!
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.
Perhaps your confusion is legit and the naming should be more explicit though. I named it afterDateTimeNormalizer::FORMAT_KEY, but that's currently the only occurence of such a suffixed constant in the serializer component. Other constants for options like in theAbstractNormalizer are simply named after the option itself, without any suffix.
Also in a different topic, some options likeallow_extra_attributes,attributes, orkey_type don't even have any constant. Maybe it's worth adding them and always ask for constants for new options in the future?
ping@dunglas.
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.
Also in a different topic, some options like allow_extra_attributes, attributes, or key_type don't even have any constant. Maybe it's worth adding them and always ask for constants for new options in the future?
I definitely agree.
fabpot commentedApr 29, 2017
Thank you@ogizanagi. |
…ext (ogizanagi)This PR was merged into the 3.3-dev branch.Discussion----------[Serializer] Allow to pass csv encoder options in context| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | yes| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets | N/A| License | MIT| Doc PR | N/ACSV contents typically are provided by one or many third-parties, not always allowing you to get control over the provided format. In case you need to import csv files with different formats, either you have to instantiate a decoder yourself/inject it instead of the main serializer instance, either you need another serializer instance with a differently configured csv encoder registered within.This PR allows to configure any encoder option through the context, so you can keep injecting and using the same serializer instance.Commits-------10a76aa [Serializer] Allow to pass csv encoder options in context
…gizanagi)This PR was merged into the 3.3-dev branch.Discussion----------[Serializer] Add missing normalizer options constants| Q | A| ------------- | ---| Branch? | master (3.3)| Bug fix? | not really| New feature? | yesish, but for 3.3 as those options were added on this branch and not released yet| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets |#22537 (comment)| License | MIT| Doc PR | N/AAs seen in#22537 (comment).@dunglas : I'm not sure about the exposing the `key_type` option as a constant in `ArrayDenormalizer`/`AbstractObjectNormalizer`, as it looks more or less like a detail of the AbstractObjectNormalizer implementation, but anyway it should be in 3.2 if we add it, so I haven't included it here.However, I wonder if this option shouldn't directly accept a string too, rather than just a `Symfony\Component\PropertyInfo\Type` instance if we want to consider this option "public"?Commits-------b0c414f [Serializer] Add missing normalizer options constants
…gizanagi)This PR was merged into the 3.3-dev branch.Discussion----------[Serializer] Add missing normalizer options constants| Q | A| ------------- | ---| Branch? | master (3.3)| Bug fix? | not really| New feature? | yesish, but for 3.3 as those options were added on this branch and not released yet| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets |symfony/symfony#22537 (comment)| License | MIT| Doc PR | N/AAs seen insymfony/symfony#22537 (comment).@dunglas : I'm not sure about the exposing the `key_type` option as a constant in `ArrayDenormalizer`/`AbstractObjectNormalizer`, as it looks more or less like a detail of the AbstractObjectNormalizer implementation, but anyway it should be in 3.2 if we add it, so I haven't included it here.However, I wonder if this option shouldn't directly accept a string too, rather than just a `Symfony\Component\PropertyInfo\Type` instance if we want to consider this option "public"?Commits-------b0c414f2c8 [Serializer] Add missing normalizer options constants
CSV contents typically are provided by one or many third-parties, not always allowing you to get control over the provided format. In case you need to import csv files with different formats, either you have to instantiate a decoder yourself/inject it instead of the main serializer instance, either you need another serializer instance with a differently configured csv encoder registered within.
This PR allows to configure any encoder option through the context, so you can keep injecting and using the same serializer instance.