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

Wrap pdo instances #3544

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion lib/Doctrine/DBAL/Connection.php
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
use Doctrine\DBAL\Driver\ResultStatement;
use Doctrine\DBAL\Driver\ServerInfoAwareConnection;
use Doctrine\DBAL\Driver\Statement as DriverStatement;
use Doctrine\DBAL\Driver\WrappedPDOConnection;
use Doctrine\DBAL\Exception\InvalidArgumentException;
use Doctrine\DBAL\Platforms\AbstractPlatform;
use Doctrine\DBAL\Query\Expression\ExpressionBuilder;
Expand Down Expand Up @@ -189,7 +190,7 @@ public function __construct(
$this->params = $params;

if (isset($params['pdo'])) {
$this->_conn = $params['pdo'];
$this->_conn = WrappedPDOConnection::fromInstance($params['pdo']);
Copy link
Member

Choose a reason for hiding this comment

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

It still would be a breaking change since it's no longer a PDO instance (won't match a type hint, can't call PDO-specific methods, etc).

Copy link
Member Author

Choose a reason for hiding this comment

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

Not according to our docblocks...

Copy link
Member Author

Choose a reason for hiding this comment

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

We never expect to handle raw PDO connection in the internals

Copy link
Member

Choose a reason for hiding this comment

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

True. I didn't notice it was about the internal API. But still, even if we can afford introducing this class, I don't think we should. Having two PDO connections which share most of the code is not a proper solution. I'd like to propose #3549 instead.

Copy link
Member Author

Choose a reason for hiding this comment

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

@morozov I'm fine with not adding this adapter 😄 I'd suggest to port the tests to your PR, just to ensure that the basic stuff works fine.

$this->isConnected = true;
unset($this->params['pdo']);
}
Expand Down
161 changes: 161 additions & 0 deletions lib/Doctrine/DBAL/Driver/WrappedPDOConnection.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,161 @@
<?php
declare(strict_types=1);

namespace Doctrine\DBAL\Driver;

use Doctrine\DBAL\ParameterType;
use PDO;
use function assert;
use function func_get_args;

final class WrappedPDOConnection implements Connection, ServerInfoAwareConnection
Copy link
Member

Choose a reason for hiding this comment

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

I'm not really happy naming-wise and am open for suggestions.

PDOConnectionAdapter?

{
/** @var PDO */
private $connection;

private function __construct(PDO $connection)
{
$this->connection = $connection;
}

public static function fromInstance(PDO $connection) : self
{
try {
$connection->setAttribute(PDO::ATTR_STATEMENT_CLASS, [PDOStatement::class, []]);
$connection->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);

return new self($connection);
} catch (\PDOException $exception) {
throw new PDOException($exception);
}
}

public function setAttribute(int $attribute, $value) : bool
{
return $this->connection->setAttribute($attribute, $value);
}

/**
* {@inheritdoc}
*/
public function prepare($prepareString)
{
try {
$stmt = $this->connection->prepare($prepareString);
assert($stmt instanceof PDOStatement);

return $stmt;
} catch (\PDOException $exception) {
throw new PDOException($exception);
}
}

/**
* {@inheritdoc}
*/
public function query()
{
$args = func_get_args();

try {
$stmt = $this->connection->query(...$args);
assert($stmt instanceof PDOStatement);

return $stmt;
} catch (\PDOException $exception) {
throw new PDOException($exception);
}
}

/**
* {@inheritdoc}
*/
public function quote($input, $type = ParameterType::STRING)
{
return $this->connection->quote($input, $type);
}

/**
* {@inheritdoc}
*/
public function exec($statement)
{
try {
return $this->connection->exec($statement);
} catch (\PDOException $exception) {
throw new PDOException($exception);
}
}

/**
* {@inheritdoc}
*/
public function lastInsertId($name = null)
{
try {
if ($name === null) {
return $this->connection->lastInsertId();
}

return $this->connection->lastInsertId($name);
} catch (\PDOException $exception) {
throw new PDOException($exception);
}
}

/**
* {@inheritdoc}
*/
public function beginTransaction()
{
return $this->connection->beginTransaction();
}

/**
* {@inheritdoc}
*/
public function commit()
{
return $this->connection->commit();
}

/**
* {@inheritdoc}
*/
public function rollBack()
{
return $this->connection->rollBack();
}

/**
* {@inheritdoc}
*/
public function errorCode()
{
return $this->connection->errorCode();
}

/**
* {@inheritdoc}
*/
public function errorInfo()
{
return $this->connection->errorInfo();
}

/**
* {@inheritdoc}
*/
public function getServerVersion()
{
return $this->connection->getAttribute(PDO::ATTR_SERVER_VERSION);
}

/**
* {@inheritdoc}
*/
public function requiresQueryForServerVersion()
{
return false;
}
}
3 changes: 2 additions & 1 deletion lib/Doctrine/DBAL/Portability/Connection.php
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
use Doctrine\DBAL\Cache\QueryCacheProfile;
use Doctrine\DBAL\ColumnCase;
use Doctrine\DBAL\Driver\PDOConnection;
use Doctrine\DBAL\Driver\WrappedPDOConnection;
use PDO;
use const CASE_LOWER;
use const CASE_UPPER;
Expand Down Expand Up @@ -66,7 +67,7 @@ public function connect()
}

if (isset($params['fetch_case']) && $this->portability & self::PORTABILITY_FIX_CASE) {
if ($this->_conn instanceof PDOConnection) {
if ($this->_conn instanceof PDOConnection || $this->_conn instanceof WrappedPDOConnection) {
SenseException marked this conversation as resolved.
Show resolved Hide resolved
// make use of c-level support for case handling
$this->_conn->setAttribute(PDO::ATTR_CASE, $params['fetch_case']);
} else {
Expand Down
12 changes: 6 additions & 6 deletions tests/Doctrine/Tests/DBAL/ConnectionTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@
use Doctrine\DBAL\DBALException;
use Doctrine\DBAL\Driver;
use Doctrine\DBAL\Driver\Connection as DriverConnection;
use Doctrine\DBAL\Driver\Statement;
use Doctrine\DBAL\DriverManager;
use Doctrine\DBAL\Events;
use Doctrine\DBAL\Exception\InvalidArgumentException;
Expand All @@ -26,6 +25,7 @@
use Doctrine\Tests\Mocks\ServerInfoAwareConnectionMock;
use Doctrine\Tests\Mocks\VersionAwarePlatformDriverMock;
use Exception;
use PDO;
use PHPUnit\Framework\MockObject\MockObject;
use stdClass;
use function call_user_func_array;
Expand Down Expand Up @@ -656,7 +656,7 @@ public function testFetchAll()

public function testConnectionDoesNotMaintainTwoReferencesToExternalPDO()
{
$params['pdo'] = new stdClass();
$params['pdo'] = new PDO('sqlite::memory:');

$driverMock = $this->createMock(Driver::class);

Expand All @@ -667,7 +667,7 @@ public function testConnectionDoesNotMaintainTwoReferencesToExternalPDO()

public function testPassingExternalPDOMeansConnectionIsConnected()
{
$params['pdo'] = new stdClass();
$params['pdo'] = new PDO('sqlite::memory:');

$driverMock = $this->createMock(Driver::class);

Expand All @@ -680,7 +680,7 @@ public function testCallingDeleteWithNoDeletionCriteriaResultsInInvalidArgumentE
{
/** @var Driver $driver */
$driver = $this->createMock(Driver::class);
$pdoMock = $this->createMock(\Doctrine\DBAL\Driver\Connection::class);
$pdoMock = $this->createMock(PDO::class);

// should never execute queries with invalid arguments
$pdoMock->expects($this->never())->method('exec');
Expand Down Expand Up @@ -709,9 +709,9 @@ public function dataCallConnectOnce()
public function testCallConnectOnce($method, $params)
{
$driverMock = $this->createMock(Driver::class);
$pdoMock = $this->createMock(Connection::class);
$pdoMock = $this->createMock(PDO::class);
$platformMock = $this->createMock(AbstractPlatform::class);
$stmtMock = $this->createMock(Statement::class);
$stmtMock = $this->createMock(Driver\PDOStatement::class);

$pdoMock->expects($this->any())
->method('prepare')
Expand Down
5 changes: 2 additions & 3 deletions tests/Doctrine/Tests/DBAL/DriverManagerTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -38,11 +38,10 @@ public function testInvalidPdoInstance()
*/
public function testValidPdoInstance()
{
$conn = DriverManager::getConnection([
'pdo' => new PDO('sqlite::memory:'),
]);
$conn = DriverManager::getConnection(['pdo' => new PDO('sqlite::memory:')]);

self::assertEquals('sqlite', $conn->getDatabasePlatform()->getName());
self::assertInstanceOf(Driver\WrappedPDOConnection::class, $conn->getWrappedConnection());
}

/**
Expand Down
71 changes: 71 additions & 0 deletions tests/Doctrine/Tests/DBAL/Functional/WrappedPDOConnectionTest.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
<?php
declare(strict_types=1);

namespace Doctrine\Tests\DBAL\Functional;

use Doctrine\DBAL\DriverManager;
use PDO;
use PHPUnit\Framework\TestCase;

/**
* @group GH-3487
*/
final class WrappedPDOConnectionTest extends TestCase
{
/** @var PDO */
private $pdo;

/**
* @before
*/
public function createInMemoryConnection() : void
{
$this->pdo = new PDO('sqlite::memory:');
}

/**
* @test
*/
public function queriesAndPreparedStatementsShouldWork() : void
{
$connection = DriverManager::getConnection(['pdo' => $this->pdo]);

self::assertTrue($connection->ping());

$connection->query('CREATE TABLE testing (id INTEGER NOT NULL PRIMARY KEY)');
$connection->query('INSERT INTO testing VALUES (1), (2), (3)');

$statement = $connection->prepare('SELECT id FROM testing WHERE id >= ?');
$statement->execute([2]);

self::assertSame([['id' => '2'], ['id' => '3']], $statement->fetchAll(PDO::FETCH_ASSOC));
}

/**
* @test
*/
public function autoIncrementIdsShouldBeFetched() : void
{
$connection = DriverManager::getConnection(['pdo' => $this->pdo]);

$connection->query('CREATE TABLE testing (id INTEGER NOT NULL PRIMARY KEY AUTOINCREMENT, name VARCHAR(10) NOT NULL)');
$connection->query('INSERT INTO testing (name) VALUES ("Testing")');

self::assertSame('1', $connection->lastInsertId());
}

/**
* @test
*/
public function transactionControlShouldHappenNormally() : void
{
$connection = DriverManager::getConnection(['pdo' => $this->pdo]);
$connection->query('CREATE TABLE testing (id INTEGER NOT NULL PRIMARY KEY)');

$connection->beginTransaction();
$connection->query('INSERT INTO testing VALUES (1), (2), (3)');
$connection->rollBack();

self::assertSame([], $connection->fetchAll('SELECT * FROM testing'));
}
}