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

Handling escape parameter in PHP8.4+ #532

Closed
nyamsprod opened this issue Aug 20, 2024 · 4 comments
Closed

Handling escape parameter in PHP8.4+ #532

nyamsprod opened this issue Aug 20, 2024 · 4 comments

Comments

@nyamsprod
Copy link
Member

nyamsprod commented Aug 20, 2024

In PHP8.4 the escape parameter is deprecated to use with csv methods and functions in PHP.

  • The only value acceptable is the empty string.
  • Any other valid value (ie. any one byte long character) will trigger a deprecation warning.
  • Invalid value behaviour does not change
  • The deprecation warning will be emitted on each method/function call

This means for league/csv that

  • the current default value for the escape character will emit the deprecation
  • every time a insertion or a read is performed using a non empty value a deprecation will be triggered.
$reader = Reader::createFromPath('path/to/document.csv');
foreach ($reader as $record) {
 // on each iteration a deprecation warning is issued starting with PHP8.4!
}

$writer = Writer::createFromString();
$writer->insertAll($records);
// on each insertion a deprecation warning is issued starting with PHP8.4!

An easy workaround would be to tell users via documentation and communication that they now need to do the following:

$reader = Reader::createFromPath('path/to/document.csv');
$reader->setEscape('');
foreach ($reader as $record) {
 // no deprecation warning at all
}

$writer = Writer::createFromString();
$writer->setEscape('');
$writer->insertAll($records);
// no deprecation warning at all

But since the end goal it to remove the escape character mechanism completely this means that setEscape and getEscape should also be deprecated which leads to a chicken and a egg issue. If you are on version 9 to fix the issue in 8.4 you need to add a method call which will be removed once you are on version 10 with PHP8.4 or PHP9. Which adds burden to migration.

Another way to deal with this issue would be to release a new major version of league/csv with the escape mechanism removed entirely and internally always using the empty string. This is possible for every supported version of PHP. But releasing a new major version only for this is cumbersome as they are other major area of the package that would benefit for a clean up and an upgrade and the time constraint before the release of PHP8.4 is limiting the time to refactor correctly the package in a state that would be satisfying from maintenance POV.

Of course we could think of other strategies or solutions if you have one, you are welcomed to submit them in the comment

@nyamsprod
Copy link
Member Author

@cedric-anne I am leaning into doing nothing regarding the default value and the PHP8.4 deprecation notice. I will try to write a lengthy blog post explaining the why of my decision> TL;DR: it is best for DX usage to not try to outsmart PHP expectations in the particular case.

@cedric-anne
Copy link
Contributor

I figured out that, in the application I maintain, the double quotes are replaced by two single quotes prior to the CSV operations. The result is that the escaping char is never used, so I just set it to an empty string and the deprecation notice dissapeared without any functional impact.

That said, I do not know how it should be handled in the league/csv library. I guess the solution could be to change the default escaping char to an empty string in the next major release, with a BC-break warning in the changelog.

@nyamsprod
Copy link
Member Author

@cedric-anne indeed that's also my conclusion BC Break in the next major regarding the default value for the escape parameter is the only sustainable solution. Everything else would result in more headache for the developer using the library.

@nyamsprod
Copy link
Member Author

After much consideration I decided to do nothing about this issue.
here's my full explanation as to why

https://nyamsprod.com/blog/csv-and-php8-4/

TL;DR:

  • there is no easy way to prevent the deprecation notices (it is a case-by-case basis)
  • removing the escape parameter would be a huge BC break (removing the ability to read old CSV documents)
  • changing the default escape parameter would also be a BC Break for the package with a small gain.

I prefer communicating than releasing. a solution that works less hat 50% of the time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants