Fatal error with ltrim() when using the EmptyEscapeParser

[on2]

When calling $csv->setEscape(''); I've noticed that when the last line of my read-only CSV created from path was empty the following error would occur:

Fatal error: Uncaught TypeError: ltrim() expects parameter 1 to be string, boolean given in [...]vendor/league/csv/src/Polyfill/EmptyEscapeParser.php on line 156

I believe this to be down to differences between the SplFileObject and League\Csv\Stream implementations. For the empty line fgets() on a SplFileObject returns '' but using the procedural style or League\Csv\Stream, false is returned:

$path = sys_get_temp_dir().'/test.csv';
file_put_contents($path, '"1","2"' . "\n");

$fh = new SplFileObject($path, 'r');
$fh->fgets();
var_dump($fh->fgets()); // '' is returned

$fh = fopen($path, 'r');
fgets($fh);
var_dump(fgets($fh)); // false is returned

$fh = League\Csv\Stream::createFromPath($path);
$fh->fgets();
var_dump($fh->fgets()); // false is returned

unlink($path);

I'm not sure this pull request contains the best way to rectify the issue but tests are passing and if nothing else this can start a discussion.

[nyamsprod]

I would make the conditions more explicit ...

if (false === self::$line) {
    return [null];
}

and that should be enough no ?

[on2]

I would agree but that seems to confuse PHPStan:

 ------ ------------------------------------------------------------------ 
  Line   src/Polyfill/EmptyEscapeParser.php                                
 ------ ------------------------------------------------------------------ 
  166    Strict comparison using !== between false and string will always  
         evaluate to true.                                                 
 ------ ------------------------------------------------------------------ 

See also: https://travis-ci.org/thephpleague/csv/jobs/594868542

It seems a bad idea to ignore the error completely but to ignore the error in just that file would require updating PHPStan which seems to open a can of worms (at least in my development environment).

This won't work with the current version of PHPStan. We need v0.11+:

	ignoreErrors:
		-
			message: '#Strict comparison using \!\=\= between false and string will always evaluate to true.#'
			path: src/Polyfill/EmptyEscapeParser.php

[nyamsprod]

we can not update because of the PHP7.0+ requirement but I have no issue ignoring this PHPstan notice, this is kind of a trade off. The day we drop PHP7.2- we will be able to upgrade/improve the ignore message IMHO.

[nyamsprod]

I think your patch is applied at the wrong lines 🤔 if you put the patch inside the while loop then PHPStan will not complained 👍 . You just need to patch the $buffer not so much the raw self::$line.

            $buffer = '';
            if (false !== self::$line) {
                $buffer = ltrim(self::$line, self::$trim_mask);
            }

[on2]

That's great - trade-off avoided!

[nyamsprod]

Is it possible to use php://temp or add a file to the data directory under so that tests are not dependent of the underlying OS

[on2]

I completely overlooked the data directory. I've rewritten the test to use the foo_readonly.csv file.

[nyamsprod]

I'll merge your patch. I may update the code but as it is it's fine by me ... thanks. Hopefully I'll make a release for that later this week. And I won't forget to mention your name in the CHANGELOG 🎉

[nyamsprod]

version 9.4.1 is released with your bugfix 🎉