SplFileObject Flags have no effect / empty lines cannot be ignored #99

Closed
paslandau opened this Issue May 15, 2015 · 8 comments

Comments

Projects
None yet
2 participants
@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 nyamsprod added the bug label May 18, 2015

@nyamsprod

This comment has been minimized.

Show comment
Hide comment
@nyamsprod

nyamsprod May 18, 2015

Member

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.

Member

nyamsprod commented May 18, 2015

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

This comment has been minimized.

Show comment
Hide comment
@nyamsprod

nyamsprod May 18, 2015

Member

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

Member

nyamsprod commented May 18, 2015

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

This comment has been minimized.

Show comment
Hide comment
@paslandau

paslandau May 19, 2015

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.

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 added a commit that referenced this issue May 20, 2015

@nyamsprod

This comment has been minimized.

Show comment
Hide comment
@nyamsprod

nyamsprod May 21, 2015

Member

version 7.1.1 is released which correct this bug

Member

nyamsprod commented May 21, 2015

version 7.1.1 is released which correct this bug

@nyamsprod nyamsprod closed this May 21, 2015

@paslandau

This comment has been minimized.

Show comment
Hide comment
@paslandau

paslandau May 21, 2015

Great, thx!

Great, thx!

@nyamsprod

This comment has been minimized.

Show comment
Hide comment
@nyamsprod

nyamsprod Oct 19, 2015

Member

@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 ?

Member

nyamsprod commented Oct 19, 2015

@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

This comment has been minimized.

Show comment
Hide comment
@paslandau

paslandau Oct 25, 2015

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

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

This comment has been minimized.

Show comment
Hide comment
@nyamsprod

nyamsprod Oct 25, 2015

Member

@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

Member

nyamsprod commented Oct 25, 2015

@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

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