Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix Third parameter not allowed for PDO::FETCH_COLUMN #4173

Merged
merged 1 commit into from Sep 25, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
10 changes: 9 additions & 1 deletion lib/Doctrine/DBAL/Statement.php
Expand Up @@ -264,7 +264,15 @@ public function fetch($fetchMode = null, $cursorOrientation = PDO::FETCH_ORI_NEX
*/
public function fetchAll($fetchMode = null, $fetchArgument = null, $ctorArgs = null)
{
return $this->stmt->fetchAll($fetchMode, $fetchArgument, $ctorArgs);
if ($ctorArgs !== null) {
return $this->stmt->fetchAll($fetchMode, $fetchArgument, $ctorArgs);
}

if ($fetchArgument !== null) {
return $this->stmt->fetchAll($fetchMode, $fetchArgument);
}

return $this->stmt->fetchAll($fetchMode);
}

/**
Expand Down
70 changes: 70 additions & 0 deletions tests/Doctrine/Tests/DBAL/Functional/ExternalPDOInstanceTest.php
@@ -0,0 +1,70 @@
<?php

namespace Doctrine\Tests\DBAL\Functional;

use Doctrine\DBAL\Connection;
use Doctrine\DBAL\Driver\PDO\SQLite\Driver as PDOSqliteDriver;
use Doctrine\DBAL\FetchMode;
use Doctrine\DBAL\Schema\Table;
use Doctrine\Tests\DbalTestCase;
use Doctrine\Tests\TestUtil;
use PDO;

class ExternalPDOInstanceTest extends DbalTestCase
{
/** @var Connection */
protected $connection;

protected function setUp(): void
{
if (! TestUtil::getConnection()->getDriver() instanceof PDOSqliteDriver) {
$this->markTestSkipped('External PDO instance tests are only run on PDO SQLite for now');
}

$pdo = new PDO('sqlite::memory:');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, I've had to remove this check as I'm not extending DbalFunctionalTestCase anymore, so I don't have access to the currently configured driver.

What will happen here if a run the test suite with say sqlsrv configuration and don't have pdo_sqlite installed? All existing functional tests have proper checks for the installed extensions and current configuration. Why do you need it to not be a functional one?

May I'd need to re-introduce a condition before running these tests, either reading from the global config, or at least check that PDO SQLite is installed?

I'd write a proper functional test that:

  1. Skips if the currently used connection is not PDO.
  2. Extract PDO from the connection.
  3. Create a new connection with external PDO.
  4. Proceed with testing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, I forgot that PDOConnection extends PDO in 2.x, so I can indeed get the PDO instance from the existing Connection to create a new one. Done!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, while the tests do pass, they also pass on 2.10 now I'm afraid. This is because the PDO we extract from the Connection is a PDOConnection, not a raw PDO. So we're back to square one: there does not seem to be a way to run this test for all PDO drivers, without creating our own PDO instances, which requires duplicating the PDO creation logic from the driver (DSN etc.) in the tests.

This is what I called overkill in development time, for a quick fix that will be gone in 3.0 anyway.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is because the PDO we extract from the Connection is a PDOConnection, not a raw PDO.

Uh... you're right. This is why we're getting rid of this untestable inheritance. Please restore the previous version and squash.

This is what I called overkill in development time

I can't agree. Fixing something that is not supposed to be used may be overkill. Proper testing is not.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please restore the previous version and squash.

Done! And re-introduced the PDO SQLite driver check to only run tests on CI that target SQLite, and prevent it from running when SQLite might not be available.


$this->connection = new Connection(['pdo' => $pdo], new PDOSqliteDriver());

$table = new Table('stmt_fetch_all');
$table->addColumn('a', 'integer');
$table->addColumn('b', 'integer');

$this->connection->getSchemaManager()->createTable($table);

$this->connection->insert('stmt_fetch_all', [
'a' => 1,
'b' => 2,
]);
}

public function testFetchAllWithOneArgument(): void
{
$stmt = $this->connection->prepare('SELECT a, b FROM stmt_fetch_all');
$stmt->execute();

self::assertEquals([[1, 2]], $stmt->fetchAll(FetchMode::NUMERIC));
}

public function testFetchAllWithTwoArguments(): void
{
$stmt = $this->connection->prepare('SELECT a, b FROM stmt_fetch_all');
$stmt->execute();

self::assertEquals([2], $stmt->fetchAll(FetchMode::COLUMN, 1));
}

public function testFetchAllWithThreeArguments(): void
{
$stmt = $this->connection->prepare('SELECT a, b FROM stmt_fetch_all');
$stmt->execute();

[$obj] = $stmt->fetchAll(FetchMode::CUSTOM_OBJECT, StatementTestModel::class, ['foo', 'bar']);

$this->assertInstanceOf(StatementTestModel::class, $obj);

self::assertEquals(1, $obj->a);
self::assertEquals(2, $obj->b);
self::assertEquals('foo', $obj->x);
self::assertEquals('bar', $obj->y);
}
}
24 changes: 24 additions & 0 deletions tests/Doctrine/Tests/DBAL/Functional/StatementTestModel.php
@@ -0,0 +1,24 @@
<?php

namespace Doctrine\Tests\DBAL\Functional;

class StatementTestModel
{
public function __construct(string $x, string $y)
{
$this->x = $x;
$this->y = $y;
}

/** @var int */
public $a;

/** @var int */
public $b;

/** @var string */
public $x;

/** @var string */
public $y;
}