NotificationsYou must be signed in to change notification settings
Fork9.7k
Star30.8k

[Serializer] Add CsvEncoder tests for PHP 7.4#32051

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

Jump to bottom

Merged

fabpot merged 1 commit intosymfony:3.4fromro0NL:csv

Sep 30, 2019

Merged

[Serializer] Add CsvEncoder tests for PHP 7.4#32051

fabpot merged 1 commit intosymfony:3.4fromro0NL:csv

Sep 30, 2019

Conversation

Copy link

Contributor

ro0NL commentedJun 14, 2019

Q	A
Branch?	3.4
Bug fix?	no
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#...
License	MIT
Doc PR	symfony/symfony-docs#...

Some CSV encoder tests to show the broken behavior of a trailing slash. Spotted in#31867, not sure what to do with it :)

carsonbot added Status: Needs Review Serializer labels

Jun 14, 2019

ro0NL commented

Jun 14, 2019

View reviewed changes

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

Copy link

Contributor

Simperfit commentedJun 15, 2019

cc@dunglas

nicolas-grekas added this to the3.4 milestone

Jun 15, 2019

Copy link

Member

nicolas-grekas commentedJul 18, 2019

What should we do with this PR?

Copy link

ContributorAuthor

ro0NL commentedJul 18, 2019

we could merge as-is, maybe add the expected output using comments. Utimately this is broken in PHP:https://3v4l.org/reb5N

however we may want to look at

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

Line 38 inba988ac

self::ESCAPE_CHAR_KEY =>'\\',

as well

Copy link

ContributorAuthor

ro0NL commentedJul 18, 2019

Oh, and here's the interesting part: as of 7.4 this seems fixed at the escape level:

https://3v4l.org/fkQDt

ro0NL closed this

Jul 18, 2019

ro0NL reopened this

Jul 18, 2019

Copy link

ContributorAuthor

ro0NL commentedJul 18, 2019

Im not sure what the point of$escape_char is

The optional escape_char parameter sets the escape character (at most one character). An empty string ("") disables the proprietary escape mechanism.

7.4.0 | The escape_char parameter now also accepts an empty string to disable the proprietary escape mechanism.

Copy link

Member

nicolas-grekas commentedJul 18, 2019

I'm not for merging a PR that proves a PHP bug...

Copy link

Member

fabpot commentedJul 18, 2019

I agree with@nicolas-grekas

Copy link

ContributorAuthor

ro0NL commentedJul 18, 2019

I think the solution is to change the default escape char from\ to an empty string and see what current tests do.

after#32289, we can test targeting php 7.4 as well.

I'll look into this 👍

Copy link

ContributorAuthor

ro0NL commentedJul 18, 2019

Right, we cant use 'empty string' on older PHP versions :)

If an enclosure character is contained in a field, it will be escaped by doubling it, unless it is immediately preceded by an escape_char. (https://php.net/fputcsv)

we always hit the issue if the last char in a cell value is the escaping character

https://3v4l.org/3NIb6 (fixes trailing slash by using tilde as escape char)
https://3v4l.org/YE3ht (breaks again with trailing tilde)

So i think the only way is to update the escape char in master for php 7.4 to an empty string.

Copy link

Member

nicolas-grekas commentedJul 18, 2019

Why not 3.4? PHP 7.4 will be supported there too.

Copy link

Member

fabpot commentedSep 25, 2019

What's the status here?

ro0NL changed the title~~[Serializer] Add CsvEncoder tests~~[Serializer] Add CsvEncoder tests for PHP 7.4

Sep 28, 2019

Copy link

ContributorAuthor

ro0NL commentedSep 28, 2019

@fabpot @nicolas-grekas all good 👍

ro0NL mentioned this pull request

Sep 28, 2019

[DI] Add CSV env var processor tests / support PHP 7.4#33744

Merged

dunglas approved these changes

Sep 28, 2019

View reviewed changes

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

Sep 28, 2019

Copy link

ContributorAuthor

ro0NL commentedSep 28, 2019

note merging this in 4.3 requires a bit more work, let me know if you want a PR for that.

nicolas-grekas reviewed

Sep 29, 2019

View reviewed changes

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

maxhelias approved these changes

Sep 29, 2019

View reviewed changes

nicolas-grekas approved these changes

Sep 30, 2019

View reviewed changes

[Serializer] Add CsvEncoder tests for PHP 7.4

760354d

Copy link

Member

fabpot commentedSep 30, 2019

Thank you@ro0NL.

fabpot added a commit that referenced this pull request

Sep 30, 2019

bug#32051[Serializer] Add CsvEncoder tests for PHP 7.4 (ro0NL)

871e8e9

This PR was squashed before being merged into the 3.4 branch (closes#32051).Discussion----------[Serializer] Add CsvEncoder tests for PHP 7.4| Q             | A| ------------- | ---| Branch?       | 3.4| Bug fix?      | no| New feature?  | no| BC breaks?    | no     <!-- seehttps://symfony.com/bc -->| Deprecations? |no| Tests pass?   | yes    <!-- please add some, will be required by reviewers -->| Fixed tickets | #...   <!-- #-prefixed issue number(s), if any -->| License       | MIT| Doc PR        | symfony/symfony-docs#... <!-- required for new features -->Some CSV encoder tests to show the broken behavior of a trailing slash. Spotted in#31867, not sure what to do with it :)Commits-------760354d [Serializer] Add CsvEncoder tests for PHP 7.4

fabpot merged commit760354d intosymfony:3.4

Sep 30, 2019

nicolas-grekas added a commit that referenced this pull request

Oct 2, 2019

bug#33744[DI] Add CSV env var processor tests / support PHP 7.4 (ro…

1fea533

…0NL)This PR was merged into the 4.3 branch.Discussion----------[DI] Add CSV env var processor tests / support PHP 7.4| Q             | A| ------------- | ---| Branch?       | 4.3| Bug fix?      | yes| New feature?  | no| Deprecations? | no| Tickets       | Fix #... <!-- prefix each issue number with "Fix #", if any -->| License       | MIT| Doc PR        | symfony/symfony-docs#... <!-- required for new features -->Similar as#32051Commits-------82f3418 [DI] Add CSV env var processor tests

ro0NL deleted the csv branch

October 2, 2019 13:12

This was referencedOct 7, 2019

Release v3.4.32#33889

Merged

Release v4.3.5#33891

Merged

dependabotbot mentioned this pull request

Mar 17, 2021

Bump symfony/symfony from 3.4.8 to 3.4.35SereicikasTomas/Web-application-with-Symfony-3#2

Closed

hultberg pushed a commit to hultberg/symfony that referenced this pull request

Sep 17, 2021

bugsymfony#32051[Serializer] Add CsvEncoder tests for PHP 7.4 (ro0NL)

14e29c5

This PR was squashed before being merged into the 3.4 branch (closessymfony#32051).Discussion----------[Serializer] Add CsvEncoder tests for PHP 7.4| Q             | A| ------------- | ---| Branch?       | 3.4| Bug fix?      | no| New feature?  | no| BC breaks?    | no     <!-- seehttps://symfony.com/bc -->| Deprecations? |no| Tests pass?   | yes    <!-- please add some, will be required by reviewers -->| Fixed tickets | #...   <!-- #-prefixed issue number(s), if any -->| License       | MIT| Doc PR        | symfony/symfony-docs#... <!-- required for new features -->Some CSV encoder tests to show the broken behavior of a trailing slash. Spotted insymfony#31867, not sure what to do with it :)Commits-------760354d [Serializer] Add CsvEncoder tests for PHP 7.4