[Serializer] Allow to pass csv encoder options in context #22537
Conversation
| } else { | ||
| $result[$parentKey.$key] = $value; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * @param array $context |
There was a problem hiding this comment.
useless, just repeats the signature
| @@ -21,6 +21,10 @@ | |||
| class CsvEncoder implements EncoderInterface, DecoderInterface | |||
| { | |||
| const FORMAT = 'csv'; | |||
| const DELIMITER_KEY = 'csv_delimiter'; | |||
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Actually the KEY suffix is not about csv at all. It's about the constant representing the key of the option to pass in the context argument. See DateTimeNormalizer::FORMAT_KEY for reference. 😄
There was a problem hiding this comment.
Perhaps your confusion is legit and the naming should be more explicit though. I named it after DateTimeNormalizer::FORMAT_KEY, but that's currently the only occurence of such a suffixed constant in the serializer component. Other constants for options like in the AbstractNormalizer are simply named after the option itself, without any suffix.
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?
ping @dunglas.
There was a problem hiding this comment.
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.
|
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/A 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. 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/A As 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/A As seen in symfony/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.