SplFileObject Flags have no effect / empty lines cannot be ignored

[paslandau]

I just updated from 7.0.1 to 7.1.0 and noticed that my internal tests failed due to a null value appended at the end of each CSV file that was imported. After some trial and error I figured that the SplFileObject falgs don't seem to have an effect any longer.

I'm missing the SplFileObject::SKIP_EMPTY flag in particular since it seemed to make sure that there's no null value at the end if the file terminates on a new line.

See the following test script:

<?php
use League\Csv\Reader;

include __DIR__."/vendor/autoload.php";

$path = __DIR__."/tmp.txt";
$str = "1st\n2nd\n";
$obj = new SplFileObject($path,"w+");
$obj->fwrite($str);
$obj = new SplFileObject($path,"r");
$reader = Reader::createFromFileObject($obj);

$flags = [
    "NONE" => 0,
    "READ_AHEAD" => SplFileObject::READ_AHEAD,
    "READ_AHEAD | DROP_NEW_LINE" => SplFileObject::READ_AHEAD | SplFileObject::DROP_NEW_LINE,
    "READ_AHEAD | SKIP_EMPTY" => SplFileObject::READ_AHEAD | SplFileObject::SKIP_EMPTY,
    "READ_AHEAD | DROP_NEW_LINE | SKIP_EMPTY" => SplFileObject::READ_AHEAD | SplFileObject::DROP_NEW_LINE | SplFileObject::SKIP_EMPTY,
    "DROP_NEW_LINE" => SplFileObject::DROP_NEW_LINE ,
    "DROP_NEW_LINE | SKIP_EMPTY" => SplFileObject::DROP_NEW_LINE | SplFileObject::SKIP_EMPTY,
    "SKIP_EMPTY" => SplFileObject::SKIP_EMPTY ,
];

foreach($flags as $flagName => $flag) {
    $reader->setFlags($flag);
    $lines = $reader->fetchAll();
    $vals = [];
    foreach($lines as $line){
        if($line == null){
            $vals[] = "<null>";
        }elseif(count($line)){
            $val = array_shift($line);
            if($val == null){
                $val = "<null>";
            }
            $vals[] = $val;
        }
    }
    echo count($lines). " lines [".implode(', ',$vals)."]\tfor ". $flagName."\n";
}

Output in 7.0.1

3 lines [1st, 2nd, <null>]  for NONE
3 lines [1st, 2nd, <null>]  for READ_AHEAD
3 lines [1st, 2nd, <null>]  for READ_AHEAD | DROP_NEW_LINE
2 lines [1st, 2nd]  for READ_AHEAD | SKIP_EMPTY
2 lines [1st, 2nd]  for READ_AHEAD | DROP_NEW_LINE | SKIP_EMPTY
3 lines [1st, 2nd, <null>]  for DROP_NEW_LINE
2 lines [1st, 2nd]  for DROP_NEW_LINE | SKIP_EMPTY
2 lines [1st, 2nd]  for SKIP_EMPTY

Please note the lines with SplFileObject::SKIP_EMPTY set: Those contain only 2 values (as expected).

Output in 7.1.0

3 lines [1st, 2nd, <null>]  for NONE
3 lines [1st, 2nd, <null>]  for READ_AHEAD
3 lines [1st, 2nd, <null>]  for READ_AHEAD | DROP_NEW_LINE
3 lines [1st, 2nd, <null>]  for READ_AHEAD | SKIP_EMPTY
3 lines [1st, 2nd, <null>]  for READ_AHEAD | DROP_NEW_LINE | SKIP_EMPTY
3 lines [1st, 2nd, <null>]  for DROP_NEW_LINE
3 lines [1st, 2nd, <null>]  for DROP_NEW_LINE | SKIP_EMPTY
3 lines [1st, 2nd, <null>]  for SKIP_EMPTY

Regardless of the flags, the output is always the same (3 lines, last one is null. I tried to figure out what was changed between those version but was unable to identify the cause - so this is my last desperate attempt to understand what's going on :)

[nyamsprod]

Hi @paslandau,

I've just looked at your code and after some digging I've found the bug ... I'll release a patch version tomorrow.

[nyamsprod]

I've push the bug fix in the master repo. So if you download the master branch you can test your code against a patch version. The 7.0.1 behaviour is the correct one. Tell me if all is good for you.
I'll try to incorporate your test into the CSV test suite.
By the way It would benefit everyone if you have other complementary tests to make a PR and add them to the package test suite

[paslandau]

Hey @nyamsprod,
just updated to the current dev-master and it works as expected. Thanks for the fix!

For the sake of completeness, here's the dev-master output of the script in the op.

3 lines [1st, 2nd, <null>]  for NONE
3 lines [1st, 2nd, <null>]  for READ_AHEAD
3 lines [1st, 2nd, <null>]  for READ_AHEAD | DROP_NEW_LINE
2 lines [1st, 2nd]  for READ_AHEAD | SKIP_EMPTY
2 lines [1st, 2nd]  for READ_AHEAD | DROP_NEW_LINE | SKIP_EMPTY
3 lines [1st, 2nd, <null>]  for DROP_NEW_LINE
2 lines [1st, 2nd]  for DROP_NEW_LINE | SKIP_EMPTY
2 lines [1st, 2nd]  for SKIP_EMPTY

The output is identical to v7.0.1.

My tests are currently part of my utility package for file based operations (tests are over here) and somewhat deeply integrated. But I'll check if parts of them can be extracted to extend the current league/csv test suite.

[nyamsprod]

version 7.1.1 is released which correct this bug

[paslandau]

Great, thx!

[nyamsprod]

@paslandau I have a question regarding League\Csv
Right now out of the box the reader uses SplFileIObject::DROP_NEW_LINE which is a bug
I want to switch this to SplFileObject::READ_AHEAD|SplFileObject::SKIP_EMPTY do you consider this as being a BC break or just a patch ?

[paslandau]

Hey @nyamsprod

Depends.. SemVer states that the "public API must not change within a patch/minor release". Did you specifiy the exact behaviour anywhere in the docs? If not, a patch might be okay - on the other hand, that might be a missing piece in the docs :)

I personally would bump up the major version because it might unexpectedly break existing code.

Cheers
Pascal

[nyamsprod]

@paslandau
About semver, no public API is changed so if I were to strictly follow semver then this is not a BC break 👍 . Moreover this change is really a bug fix has using SplFileIObject::DROP_NEW_LINE by default is a bug
I have publish a roadmap for v8.0 which takes into account this bug fix