Safe\fgetcsv throws Exception when csv file ends with a new line

[Gemineye]

When iterating over a file we get an Exception if the last line is empty. Is this intended?

[Kharhamel]

this is definitely not intended, the exception should only trigger if the base function would return false.
What kind of exception do you get? A FilesystemException? Do you correctly get null if you use the base function?

[Gemineye]

Yes i get a FilesystemException (An error occured). The base returns with false in the 3. row (PHP 8.1)
test.csv

[Kharhamel]

If the base function returns false then it is normal than the safe function return an exception. I don't know why you have an error but it doesn't come from safe.

[Gemineye]

Ok Thank you!

[pyrou]

@Kharhamel How are we supposed to read a csv using Safe ?
Because original fgetcsv return false on purpose at the end of the file, according to documentation example :

while (($data = fgetcsv($handle, 1000, ",")) !== FALSE) {
    // ...
}

I tried using feof,

while (!feof($handle)) {
   $data = \Safe\fgetcsv($handle, 1000, ",");
   // ...
}

But with a final "\n" in csv file, which is something definitely pretty common, feof still isn't false after last line fgetcsv, and so next fgetcsv throw a FilesystemException. I checked implementation, and at this point error_get_last() is null.

How can we safely detect we are at the end of the file to get out of the while loop before exception ?

[pyrou]

Also resolution proposed in #325 is absolutely nonsensical to me. What would be the point of using a Safe function if the ONLY way to use it, is about muting and canceling the benefit it provide ?

[pointybeard]

I hit this problem also and having to use the workaround is ridiculous. The expected behaviour of fgetcsv() is to return false when there is no data. Safe\fgetcsv() should respect this and only throw an exception when there is actually an error of some kind.

[Kharhamel]

But how do we know if there is an error without checking if the return is false?

[Kharhamel]

According to the doc (https://www.php.net/manual/en/function.fgetcsv.php):

A blank line in a CSV file will be returned as an array comprising a single null field, and will not be treated as an error.

So i guess this should be the proper way to do it.

[pyrou]

A blank line in a CSV file will be returned as an array comprising a single null field, and will not be treated as an error.

This only concerns blank line (like in the middle on the file) not trailing newline.

But how do we know if there is an error without checking if the return is false?

Checking feof() value is a good choice IMHO. A false value on \fgetcsv while \feof returns true is not an error. This is how \fgetcsv() currently behaves.

Here is a possible implementation that would satisfy either the original behavior of \fgetcsv() and the purpose of Safe :

function fgetcsv($stream, int $length = null, string $separator = ",", string $enclosure = "\"", string $escape = "\\"): ?array|false
{
    error_clear_last();
    $safeResult = \fgetcsv(...func_get_args());

    if ($safeResult === false && feof($stream) === false) {
        throw FilesystemException::createFromPhpError();
    }

    return $safeResult;
}

---

Alternatively a specific exception can also be thrown

function fgetcsv($stream, int $length = null, string $separator = ",", string $enclosure = "\"", string $escape = "\\"): ?array
{
    error_clear_last();
    $safeResult = \fgetcsv(...func_get_args());

    if ($safeResult === false) {
        if (feof($stream)) {
            throw FilesystemEOFException();
        }
        throw FilesystemException::createFromPhpError();
    }

    return $safeResult;
}

This way would allow us to catch and ignore FilesystemEOFException while still letting FilesystemException bubbling up if any. To reduce breaking change, FilesystemEOFException can extends FilesystemException. So you'll remain 100% backward compatible.

*EOFException is a naming coming from java land.

[wederfabricio]

I got the same problem here, I guess we should use the @pyrou solution to resolve this.

[Kharhamel]

@pyrou I think I prefer the first solution since it doesn't require users to learn a new behavior. Working on it.

[Kharhamel]

Do you think there are other functions that might have the same issue?

[pyrou]

Do you think there are other functions that might have the same issue?

fgets is a similar case. Btw php.net example for fgets confirms reading foef after getting false was the right implementation :)

[Kharhamel]

The fix was deployed in version 2.3, I will do a MR later for fgets

[shish]

closing as it seems fgetcsv is handled, and we have #189 for fgets