[Serializer][CSV] Add context options to handle BOM #33896
Conversation
src/Symfony/Component/Serializer/Tests/Encoder/CsvEncoderTest.php
Outdated
Show resolved
Hide resolved
|
I agree with @nicolas-grekas that we should always handle BOM in the input. |
|
@nicolas-grekas so you're proposing we always add BOM to output and handle it in input (if present)? |
|
I'm proposing to never add the BOM in the output, and always handle it in the input. |
|
I'm 👎 for never adding BOM in the output as then I'm as a consumer responsible for checking whether data is serialized to CSV and if so, add BOM to it. |
|
@malarzm as @nicolas-grekas said, you can still concatenate the BOM before the serialized data. |
|
@nicolas-grekas @stof I'm not sure when exactly you want to manually concatenate BOM, may I ask for an example how do you see it? |
ebe7fd3 to
f6b1405
Compare
|
@nicolas-grekas I've removed the ByteOrderMask class keeping only UTF-8 BOM in the encoder itself, the BOM is always stripped while decoding and, for now, I've left a bool switch to include BOM in encoded string as I believe this is encoder's responsibility. |
93602d0 to
35d8e61
Compare
| @@ -5,6 +5,7 @@ CHANGELOG | |||
| ----- | |||
|
|
|||
| * deprecated the `XmlEncoder::TYPE_CASE_ATTRIBUTES` constant, use `XmlEncoder::TYPE_CAST_ATTRIBUTES` instead | |||
| * added option to output an UTF-8 BOM in CSV encoder via `CsvEncoder::OUTPUT_UTF8_BOM_KEY` context option | |||
| @@ -114,6 +119,14 @@ public function encode($data, $format, array $context = []) | |||
| $value = stream_get_contents($handle); | |||
| fclose($handle); | |||
|
|
|||
| if ($outputBom) { | |||
| if (!preg_match('//u', $value)) { | |||
| throw new UnexpectedValueException('You are trying to add an UTF-8 BOM to a non UTF-8 text.'); | |||
|
@fabpot thanks for noticing, fixed :) |
|
Thank you @malarzm. |
…alarzm) This PR was merged into the 4.4 branch. Discussion ---------- [Serializer][CSV] Add context options to handle BOM | Q | A | ------------- | --- | Branch? | 4.4 | Bug fix? | no | New feature? | yes | Deprecations? | no | Tickets | Fix #33684 | License | MIT | Doc PR | symfony/symfony-docs#12461 This allows BOM handling in en/decoded CSV files. To keep current behaviour intact both skipping BOM at the beginning of the CSV and outputting BOM are an opt-in feature. Personally I'd propose to make `SKIP_INPUT_BOM` default to `false` in 5.0 so the BOM is transparent and people that for some reasons expect BOM characters to be present in the parsed text explicitly opt-out of trimming it. Commits ------- 3eb3668 Add context options to handle BOM
This PR was merged into the 4.4 branch. Discussion ---------- List CSV encoder's context options I needed to add new option `output_utf8_bom` introduced in symfony/symfony#33896 and since context options weren't documented I figured I'll list them all. Format and text is based on XML's part of the docs. Commits ------- b600aa0 List CSV encoder's context options
This allows BOM handling in en/decoded CSV files. To keep current behaviour intact both skipping BOM at the beginning of the CSV and outputting BOM are an opt-in feature.
Personally I'd propose to make
SKIP_INPUT_BOMdefault tofalsein 5.0 so the BOM is transparent and people that for some reasons expect BOM characters to be present in the parsed text explicitly opt-out of trimming it.