Discovered SplFileObject::fgetcsv() bugs implemented a workaround

[jorgecolonconsulting]

discovered SplFileObject::fgetcsv() bugs implemented a workaround in the meantime, removed superfluous $line, more unit tests, updated README with examples

To care in code formatting. Hopefully I caught everything ahead of time.

After some more tests I discovered a few different SplFileObject::fgetcsv() bugs. I guess not a lot of people use it as they probably would've found the bugs a long time ago. As far as I can see PHP 5.5.15 and 5.4.31 are affected.

Here's my test case to show the bugs

<?php

class CSV {
    protected $delimiter = ',';
    protected $enclosure = '"';

    public function __construct($path, $mode = 'r+')
    {
        $this->handle = new \SplFileObject($path, $mode);
        $this->handle->setFlags(\SplFileObject::DROP_NEW_LINE);
    }

    public function getFile()
    {
        return $this->handle;
    }

    public function getRow()
    {
        if ($this->isEof() === true) {
            return false;
        }

        $row = $this->handle->fgetcsv($this->delimiter, $this->enclosure);

        return $row;
    }

    public function getAll()
    {
        $data = array();
        while ($row = $this->getRow()) {
            $data[] = $row;
        }
        return $data;
    }

    public function advanceTo($line)
    {
        $this->handle->seek($line);
    }

    public function getCurrentLine()
    {
        return $this->handle->current();
    }

    public function getLineNumber()
    {
        return $this->handle->key();
    }

    public function rewind(){
        $this->handle->rewind();
    }

    /**
     * @return bool
     */
    protected function isEof()
    {
        return $this->handle->eof();
    }
}

/**
 * Class SplFileObjectTest
 *
 * Test to show unwanted behavior when \SplFileObject::READ_AHEAD is set.
 *
 */
class SplFileObjectTest extends PHPUnit_Framework_TestCase {

    protected $headers = array('column1', 'column2', 'column3');

    protected $dataRow1 = array(
        '1column2value',
        '1column3value',
        '1column4value',
    );

    /**
     * @var CSV
     */
    protected $csv;

    public function setUp()
    {
        $this->csv = new CSV(__DIR__ . '/read.csv');
    }

    /**
     * No calls to current(), seek(), or rewind()
     */
    public function testCaseThatWorks()
    {
        $rows = $this->csv->getAll();
        $this->assertCount(6, $rows);
        $this->assertEquals($this->headers, $rows[0]);
    }

    public function testWithoutCurrentCall()
    {
        $row = $this->csv->getRow();

        $this->assertEquals($this->headers, $row);
    }

    public function testSeek()
    {
        $this->csv->advanceTo(0);

        $row = $this->csv->getRow();

        $this->assertEquals($this->headers, $row);
        $this->assertEquals(0, $this->csv->getLineNumber());

        $row = $this->csv->getRow();
        $this->assertEquals(1, $this->csv->getLineNumber());

        $this->assertEquals($this->dataRow1, $row);

        $this->csv->advanceTo(1);

        $row = $this->csv->getRow();

        // for some reason it's getting line 3 instead of line 2
        $this->assertEquals($this->dataRow1, $row);
        $this->assertEquals(1, $this->csv->getLineNumber());
    }

    public function testRewind()
    {
        $this->assertEquals(0, $this->csv->getLineNumber());
        $this->csv->rewind();

        $row = $this->csv->getRow();
        $this->assertEquals(0, $this->csv->getLineNumber());

        $this->assertEquals($this->headers, $row);
    }

    public function testCurrent()
    {
        $this->assertEquals(0, $this->csv->getLineNumber());
        // if we make a call to SplFileObject::current() for some reason it advances one line?
        // if we don't call it before the SplFileObject::fgetcsv() call, we get the right row
        $this->csv->getCurrentLine();

        $row = $this->csv->getRow();
        $this->assertEquals(0, $this->csv->getLineNumber());

        $this->assertEquals($this->headers, $row);
    }

    public function testGetRowsOneByOne()
    {
        $this->csv->getRow();
        $this->csv->getRow();
        $this->csv->getRow();
        $this->csv->getRow();
        $this->csv->getRow();
        $row = $this->csv->getRow();

        $expected = array(
            0 => '5column2value',
            1 => '5column3value',
            2 => '5column4value',
        );

        $this->assertEquals($expected, $row);
        $this->assertEquals(5, $this->csv->getLineNumber());
    }
}

[jorgecolonconsulting]

@jwage This is an emergency fix since the last one has some logic bugs thanks to bugs with SplFileObject::fgetcsv().

[jorgecolonconsulting]

@jwage just a gentle bump.

The quality score went down. Apparently covering several real use cases is frowned upon. :/ I'm assuming it's more of an analyzer issue than a real issue with the code so the cyclomatic complexity is more subjective. Open to ideas if it can be improved, but to me it's very clear.

[jwage]

👍

[jorgecolonconsulting]

👍 awesome. I'll push as soon as I can.