Movatterモバイル変換

malarzm changed the title~~Add context options to handle BOM~~[Serializer][CSV] Add context options to handle BOM

malarzm commented

src/Symfony/Component/Serializer/Encoder/CsvEncoder.php OutdatedShow resolvedHide resolved

malarzm commented

src/Symfony/Component/Serializer/Tests/Encoder/CsvEncoderTest.php OutdatedShow resolvedHide resolved

malarzm force-pushed thecsv-bom branch from618b90d toc1d9694Compare

October 7, 2019 22:21

nicolas-grekas added this to thenext milestone

Oct 8, 2019

nicolas-grekas reviewed

Oct 8, 2019

Copy link

Member

nicolas-grekas left a comment•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

When decoding a value, we should always deal with the BOM to me, per the robustness principle.
Then, does it really make sense to add an option to prefix the BOM in the output, when the alternative is just a concatention away? I'm not sure...

src/Symfony/Component/Serializer/ByteOrderMark.php OutdatedShow resolvedHide resolved

Copy link

Member

stof commentedOct 8, 2019

I agree with@nicolas-grekas that we should always handle BOM in the input.
And I'm not sure we need the ByteOrderMask class.

Copy link

ContributorAuthor

malarzm commentedOct 8, 2019

@nicolas-grekas so you're proposing we always add BOM to output and handle it in input (if present)?

Copy link

Member

nicolas-grekas commentedOct 8, 2019

I'm proposing to never add the BOM in the output, and always handle it in the input.

Copy link

ContributorAuthor

malarzm commentedOct 8, 2019

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.

Copy link

Member

stof commentedOct 8, 2019

@malarzm as@nicolas-grekas said, you can still concatenate the BOM before the serialized data.

Copy link

ContributorAuthor

malarzm commentedOct 8, 2019

@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?

malarzm force-pushed thecsv-bom branch 2 times, most recently fromebe7fd3 tof6b1405Compare

October 8, 2019 19:01

Copy link

ContributorAuthor

malarzm commentedOct 8, 2019

@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.

dunglas approved these changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels

nicolas-grekas reviewed

src/Symfony/Component/Serializer/Encoder/CsvEncoder.php OutdatedShow resolvedHide resolved

malarzm force-pushed thecsv-bom branch fromf6b1405 to424da8bCompare

October 9, 2019 07:03

nicolas-grekas reviewed

src/Symfony/Component/Serializer/CHANGELOG.md OutdatedShow resolvedHide resolved

nicolas-grekas reviewed

List CSV encoder's context optionssymfony/symfony-docs#12461

src/Symfony/Component/Serializer/Encoder/CsvEncoder.php OutdatedShow resolvedHide resolved

malarzm force-pushed thecsv-bom branch 3 times, most recently from93602d0 to35d8e61Compare

October 9, 2019 19:40

malarzm mentioned this pull request

Oct 10, 2019

Merged

nicolas-grekas approved these changes

fabpot approved these changes

Copy link

Member

fabpot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Small typo

src/Symfony/Component/Serializer/CHANGELOG.md Outdated

		-----

		* 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

Copy link

Member

fabpotOct 13, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

a UTF-8

src/Symfony/Component/Serializer/Encoder/CsvEncoder.php Outdated


		if ($outputBom) {
		if (!preg_match('//u',$value)) {
		thrownewUnexpectedValueException('You are trying to add an UTF-8 BOM to a non UTF-8 text.');

Copy link

Member

fabpotOct 13, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

a UTF-8

Add context options to handle BOM

3eb3668

malarzm force-pushed thecsv-bom branch from35d8e61 to3eb3668Compare

October 13, 2019 08:45

Copy link

ContributorAuthor

malarzm commentedOct 13, 2019

@fabpot thanks for noticing, fixed :)

Copy link

Member

fabpot commentedOct 13, 2019

Thank you@malarzm.

fabpot added a commit that referenced this pull request

feature#33896[Serializer][CSV] Add context options to handle BOM (m…

004ee73

…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#12461This 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

fabpot merged commit3eb3668 intosymfony:4.4