Fixes #10 : Fix incorrect DbSelect::count() when setted \PDO::ATTR_CASE => \PDO::CASE_LOWER in driver options

[samsonasik]

Signed-off-by: Abdul Malik Ikhsan samsonasik@gmail.com

Q	A
Bugfix	yes

Description

Fix incorrect DbSelect::count() when setted \PDO::ATTR_CASE => \PDO::CASE_LOWER in driver options

[samsonasik]

need to update mock driver in test

[samsonasik]

The easiest way probably create a separate class that uses small 'c' as column, or just change the ROW_COUNT_COLUMN_NAME constant value to small 'c'

[froschdesign]

I think the solution is too complicated because the select query for counting contains only one column.

Some problems:

• PDO attribute is checked two times
• a new class constant is added to the class
• a concrete database adapter is added to the class

[froschdesign]

A possible unit test:

public function testCountQueryWithLowerColumnNameShouldReturnValidResult()
{
    $this->dbSelect = new DbSelect($this->mockSelect, $this->mockSql);
    $this->mockResult
        ->expects($this->once())
        ->method('current')
        ->will($this->returnValue(['c' => 7]));

    $count = $this->dbSelect->count();
    $this->assertEquals(7, $count);
}

[samsonasik]

I simplified check with add "c" column in columns list, so if no isset "C" column, we can fallback to "c" column. @froschdesign I applied your test.

[samsonasik]

travis build green 🎉

[samsonasik]

travis build green 🎉

[weierophinney]

I'm worried about how portable this is: is it going to work across all drivers PDO supports? what about non-PDO drivers?

I'm wondering if we either need (a) a flag to enable this behavior, with documentation about when you'd enable that flag, and/or (b) a separate adapter that does this. Right now, since we mock the connections, we may be getting a false sense of security that this approach works, when it may only work in select cases.

[samsonasik]

@froschdesign @weierophinney merge-able now ? Thank you.

[weierophinney]

What happens when the lowercase version is not present? This is likely to raise a warning.

I'd argue this should be done as follows:

$this->rowCount = $this->locateRowCount($row);

with the following helper method:

/* Not sure if `array` is correct here, or if this could be an `ArrayAccess` instance */
private function locateRowCount(array $row): int
{
    if (array_key_exists(self::ROW_COUNT_COLUMN_NAME, $row)) {
        return (int) $row[self::ROW_COUNT_COLUMN_NAME];
    }

    $lowerCaseColumnName = strtolower(self::ROW_COUNT_COLUMN_NAME);
    if (array_key_exists($lowerCaseColumnName, $row)) {
        return (int) $row[$lowerCaseColumnName];
    }

    throw new LogicException('Unable to determine row count; missing row count column in result');
}

This will satisfy #10, while also making the code less brittle.

[samsonasik]

@weierophinney implemented