Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Serializer] Add CsvEncoder tests for PHP 7.4 #32051

Merged
merged 1 commit into from Sep 30, 2019

Conversation

@ro0NL
Copy link
Contributor

ro0NL commented Jun 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 :)

@Simperfit

This comment has been minimized.

Copy link
Contributor

Simperfit commented Jun 15, 2019

@nicolas-grekas nicolas-grekas added this to the 3.4 milestone Jun 15, 2019
@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Jul 18, 2019

What should we do with this PR?

@ro0NL

This comment has been minimized.

Copy link
Contributor Author

ro0NL commented Jul 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

as well

@ro0NL

This comment has been minimized.

Copy link
Contributor Author

ro0NL commented Jul 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 ro0NL closed this Jul 18, 2019
@ro0NL ro0NL reopened this Jul 18, 2019
@ro0NL

This comment has been minimized.

Copy link
Contributor Author

ro0NL commented Jul 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.

@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Jul 18, 2019

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

@fabpot

This comment has been minimized.

Copy link
Member

fabpot commented Jul 18, 2019

I agree with @nicolas-grekas

@ro0NL

This comment has been minimized.

Copy link
Contributor Author

ro0NL commented Jul 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 👍

@ro0NL

This comment has been minimized.

Copy link
Contributor Author

ro0NL commented Jul 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.

@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Jul 18, 2019

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

@fabpot

This comment has been minimized.

Copy link
Member

fabpot commented Sep 25, 2019

What's the status here?

@ro0NL ro0NL force-pushed the ro0NL:csv branch from b22a5b5 to 8fb494b Sep 28, 2019
@ro0NL ro0NL changed the title [Serializer] Add CsvEncoder tests [Serializer] Add CsvEncoder tests for PHP 7.4 Sep 28, 2019
@ro0NL ro0NL force-pushed the ro0NL:csv branch 4 times, most recently from 1b63872 to 6572256 Sep 28, 2019
@ro0NL

This comment has been minimized.

Copy link
Contributor Author

ro0NL commented Sep 28, 2019

@fabpot @nicolas-grekas all good 👍

@ro0NL

This comment has been minimized.

Copy link
Contributor Author

ro0NL commented Sep 28, 2019

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

@fabpot fabpot force-pushed the ro0NL:csv branch from 03d1394 to 760354d Sep 30, 2019
@fabpot

This comment has been minimized.

Copy link
Member

fabpot commented Sep 30, 2019

Thank you @ro0NL.

fabpot added a commit that referenced this pull request Sep 30, 2019
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     <!-- see https://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 fabpot merged commit 760354d into symfony:3.4 Sep 30, 2019
1 of 3 checks passed
1 of 3 checks passed
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
fabbot.io Your code looks good.
Details
nicolas-grekas added a commit that referenced this pull request Oct 2, 2019
…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 #32051

Commits
-------

82f3418 [DI] Add CSV env var processor tests
@ro0NL ro0NL deleted the ro0NL:csv branch Oct 2, 2019
This was referenced Oct 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.