thephpleague / csv

CSV data manipulation made easy in PHP
https://csv.thephpleague.com
MIT License
3.33k stars 333 forks source link

Handling escape parameter in PHP8.4+ #532

Closed nyamsprod closed 2 weeks ago

nyamsprod commented 3 weeks ago

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

This means for league/csv that

$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 commented 3 weeks ago

@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 commented 3 weeks ago

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 commented 3 weeks ago

@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 commented 2 weeks ago

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:

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