Skip to content

Conversation

@xabbuh
Copy link
Member

@xabbuh xabbuh commented Aug 16, 2024

Q A
Branch? 5.4
Bug fix? no
New feature? no
Deprecations? no
Issues
License MIT

using custom escape character will be deprecated from PHP 8.4

@nicolas-grekas
Copy link
Member

Do we keep one as @group legacy for testing purposes?

@xabbuh
Copy link
Member Author

xabbuh commented Aug 16, 2024

none of the tests actually depends on the value of the escape character, not sure if we want to add a new test covering it explicitly

Copy link
Member

@chalasr chalasr left a comment

Choose a reason for hiding this comment

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

Should we deprecate the CsvEncoder::ESCAPE_CHAR_KEYcontext key on 7.2?

@nicolas-grekas
Copy link
Member

@chalasr see #57827

@xabbuh
Copy link
Member Author

xabbuh commented Aug 16, 2024

@chalasr I think @alexandre-daubois should have that covered in #57827

@alexandre-daubois
Copy link
Member

Indeed, I'll address the remaining comment in a couple of days. Thanks for this complementary PR!

@nicolas-grekas
Copy link
Member

Isn't the failing test related?

@xabbuh
Copy link
Member Author

xabbuh commented Aug 16, 2024

indeed, fixed

@nicolas-grekas
Copy link
Member

Thank you @xabbuh.

@nicolas-grekas nicolas-grekas merged commit 9348c49 into symfony:5.4 Aug 16, 2024
@xabbuh xabbuh deleted the php-8.4-csv branch August 16, 2024 10:40
fabpot added a commit that referenced this pull request Aug 17, 2024
This PR was merged into the 6.4 branch.

Discussion
----------

[Serializer] clean up PHP version checks

| Q             | A
| ------------- | ---
| Branch?       | 6.4
| Bug fix?      | no
| New feature?  | no
| Deprecations? | no
| Issues        |
| License       | MIT

these checks are not needed after merging #58021 up into the `6.4` branch

Commits
-------

6e19c0f clean up PHP version checks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants