From b98abf0a6acfc85b3c90f99466102b603c2361dc Mon Sep 17 00:00:00 2001 From: Sergei Morozov Date: Fri, 26 Jun 2020 18:37:33 -0700 Subject: [PATCH] Remove ServerInfoAwareConnection#requiresQueryForServerVersion() as an implementation detail MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Testing the implementations of this method requires partial mocking of the implementing class which makes it impossible to make them `final` (#3590). Additionally, this API breaks the encapsulation of the driver layer: instead of exposing the fact of whether the connection will perform a query to detect the server version, the driver should just instantiate a platform corresponding to a connection. The rationale behind introducing this method (#487) is really questionable: > This is also required for drivers that cannot return the database server version without an additional query (performance reasons). 1. There's no evidence that an underlying driver that exposes the server version via its API doesn't make a request of any kind to the server. 2. For an application that works with any realistic database, a query like `SELECT VERSION()` wouldn't be a performance bottleneck. 3. Even if it was, it's always possible to specify the platform version upfront. Otherwise, the current logic of falling back to a default platform may cause undefined behavior of the application (we don't test the compatibility of the lowest level of the DBAL platform with all supported server versions). Remember, “If it doesn’t work, it doesn’t matter how fast it doesn’t work.” --- UPGRADE.md | 4 ++ src/Connection.php | 2 +- src/Driver/IBMDB2/DB2Connection.php | 8 ---- src/Driver/Mysqli/MysqliConnection.php | 8 ---- src/Driver/OCI8/OCI8Connection.php | 8 ---- src/Driver/PDOConnection.php | 8 ---- src/Driver/SQLSrv/SQLSrvConnection.php | 8 ---- src/Driver/ServerInfoAwareConnection.php | 7 ---- tests/ConnectionTest.php | 4 -- tests/Driver/IBMDB2/DB2ConnectionTest.php | 37 ------------------- tests/Driver/Mysqli/MysqliConnectionTest.php | 23 ------------ tests/Driver/OCI8/OCI8ConnectionTest.php | 37 ------------------- tests/Driver/SQLSrv/SQLSrvConnectionTest.php | 37 ------------------- .../Functional/Driver/PDO/ConnectionTest.php | 5 --- 14 files changed, 5 insertions(+), 191 deletions(-) delete mode 100644 tests/Driver/IBMDB2/DB2ConnectionTest.php delete mode 100644 tests/Driver/OCI8/OCI8ConnectionTest.php delete mode 100644 tests/Driver/SQLSrv/SQLSrvConnectionTest.php diff --git a/UPGRADE.md b/UPGRADE.md index 93fc425dc5d..a7ef3f1657b 100644 --- a/UPGRADE.md +++ b/UPGRADE.md @@ -1,5 +1,9 @@ # Upgrade to 3.0 +## BC BREAK: `ServerInfoAwareConnection::requiresQueryForServerVersion()` is removed. + +The `ServerInfoAwareConnection::requiresQueryForServerVersion()` method has been removed as an implementation detail which is the same for all supported drivers. + ## BC BREAK Changes in driver exceptions 1. The `Doctrine\DBAL\Driver\DriverException::getErrorCode()` method is removed. In order to obtain the driver error code, please use `::getCode()` or `::getSQLState()`. diff --git a/src/Connection.php b/src/Connection.php index 23a22d14dd1..4eda534887f 100644 --- a/src/Connection.php +++ b/src/Connection.php @@ -396,7 +396,7 @@ private function getServerVersion() $connection = $this->getWrappedConnection(); // Automatic platform version detection. - if ($connection instanceof ServerInfoAwareConnection && ! $connection->requiresQueryForServerVersion()) { + if ($connection instanceof ServerInfoAwareConnection) { return $connection->getServerVersion(); } diff --git a/src/Driver/IBMDB2/DB2Connection.php b/src/Driver/IBMDB2/DB2Connection.php index 035aa8415b1..3666ef605f3 100644 --- a/src/Driver/IBMDB2/DB2Connection.php +++ b/src/Driver/IBMDB2/DB2Connection.php @@ -73,14 +73,6 @@ public function getServerVersion() return $serverInfo->DBMS_VER; } - /** - * {@inheritdoc} - */ - public function requiresQueryForServerVersion() - { - return false; - } - public function prepare(string $sql): DriverStatement { $stmt = @db2_prepare($this->conn, $sql); diff --git a/src/Driver/Mysqli/MysqliConnection.php b/src/Driver/Mysqli/MysqliConnection.php index ee914b52a0f..be3adb93b51 100644 --- a/src/Driver/Mysqli/MysqliConnection.php +++ b/src/Driver/Mysqli/MysqliConnection.php @@ -96,14 +96,6 @@ public function getServerVersion() return $majorVersion . '.' . $minorVersion . '.' . $patchVersion; } - /** - * {@inheritdoc} - */ - public function requiresQueryForServerVersion() - { - return false; - } - public function prepare(string $sql): DriverStatement { return new Statement($this->conn, $sql); diff --git a/src/Driver/OCI8/OCI8Connection.php b/src/Driver/OCI8/OCI8Connection.php index 23140a39ee8..43769c8dcb9 100644 --- a/src/Driver/OCI8/OCI8Connection.php +++ b/src/Driver/OCI8/OCI8Connection.php @@ -97,14 +97,6 @@ public function getServerVersion() return $matches[1]; } - /** - * {@inheritdoc} - */ - public function requiresQueryForServerVersion() - { - return false; - } - public function prepare(string $sql): DriverStatement { return new Statement($this->dbh, $sql, $this->executionMode); diff --git a/src/Driver/PDOConnection.php b/src/Driver/PDOConnection.php index a08d3931198..6edfc9af7fb 100644 --- a/src/Driver/PDOConnection.php +++ b/src/Driver/PDOConnection.php @@ -112,14 +112,6 @@ public function lastInsertId($name = null) } } - /** - * {@inheritdoc} - */ - public function requiresQueryForServerVersion() - { - return false; - } - /** * Creates a wrapped statement */ diff --git a/src/Driver/SQLSrv/SQLSrvConnection.php b/src/Driver/SQLSrv/SQLSrvConnection.php index 1b15876e9cc..92fa36b24c5 100644 --- a/src/Driver/SQLSrv/SQLSrvConnection.php +++ b/src/Driver/SQLSrv/SQLSrvConnection.php @@ -66,14 +66,6 @@ public function getServerVersion() return $serverInfo['SQLServerVersion']; } - /** - * {@inheritdoc} - */ - public function requiresQueryForServerVersion() - { - return false; - } - public function prepare(string $sql): DriverStatement { return new Statement($this->conn, $sql, $this->lastInsertId); diff --git a/src/Driver/ServerInfoAwareConnection.php b/src/Driver/ServerInfoAwareConnection.php index 97e464b2400..18a9e113bd3 100644 --- a/src/Driver/ServerInfoAwareConnection.php +++ b/src/Driver/ServerInfoAwareConnection.php @@ -13,11 +13,4 @@ interface ServerInfoAwareConnection extends Connection * @return string */ public function getServerVersion(); - - /** - * Checks whether a query is required to retrieve the database server version. - * - * @return bool True if a query is required to retrieve the database server version, false otherwise. - */ - public function requiresQueryForServerVersion(); } diff --git a/tests/ConnectionTest.php b/tests/ConnectionTest.php index d29a4dcf1a0..a0146693381 100644 --- a/tests/ConnectionTest.php +++ b/tests/ConnectionTest.php @@ -661,10 +661,6 @@ public function testPlatformDetectionIsTriggerOnlyOnceOnRetrievingPlatform(): vo ->method('connect') ->will(self::returnValue($driverConnectionMock)); - $driverConnectionMock->expects(self::once()) - ->method('requiresQueryForServerVersion') - ->will(self::returnValue(false)); - $driverConnectionMock->expects(self::once()) ->method('getServerVersion') ->will(self::returnValue('6.6.6')); diff --git a/tests/Driver/IBMDB2/DB2ConnectionTest.php b/tests/Driver/IBMDB2/DB2ConnectionTest.php deleted file mode 100644 index 31f9d977689..00000000000 --- a/tests/Driver/IBMDB2/DB2ConnectionTest.php +++ /dev/null @@ -1,37 +0,0 @@ -markTestSkipped('ibm_db2 is not installed.'); - } - - parent::setUp(); - - $this->connectionMock = $this->getMockBuilder(DB2Connection::class) - ->disableOriginalConstructor() - ->getMockForAbstractClass(); - } - - public function testDoesNotRequireQueryForServerVersion(): void - { - self::assertFalse($this->connectionMock->requiresQueryForServerVersion()); - } -} diff --git a/tests/Driver/Mysqli/MysqliConnectionTest.php b/tests/Driver/Mysqli/MysqliConnectionTest.php index 5791fd5d859..a5e617eb47e 100644 --- a/tests/Driver/Mysqli/MysqliConnectionTest.php +++ b/tests/Driver/Mysqli/MysqliConnectionTest.php @@ -4,22 +4,12 @@ use Doctrine\DBAL\Driver\Mysqli\Driver; use Doctrine\DBAL\Driver\Mysqli\HostRequired; -use Doctrine\DBAL\Driver\Mysqli\MysqliConnection; -use Doctrine\DBAL\Platforms\MySqlPlatform; use Doctrine\DBAL\Tests\FunctionalTestCase; -use PHPUnit\Framework\MockObject\MockObject; use function extension_loaded; class MysqliConnectionTest extends FunctionalTestCase { - /** - * The mysqli driver connection mock under test. - * - * @var MysqliConnection|MockObject - */ - private $connectionMock; - protected function setUp(): void { if (! extension_loaded('mysqli')) { @@ -27,19 +17,6 @@ protected function setUp(): void } parent::setUp(); - - if (! $this->connection->getDatabasePlatform() instanceof MySqlPlatform) { - $this->markTestSkipped('MySQL only test.'); - } - - $this->connectionMock = $this->getMockBuilder(MysqliConnection::class) - ->disableOriginalConstructor() - ->getMockForAbstractClass(); - } - - public function testDoesNotRequireQueryForServerVersion(): void - { - self::assertFalse($this->connectionMock->requiresQueryForServerVersion()); } public function testHostnameIsRequiredForPersistentConnection(): void diff --git a/tests/Driver/OCI8/OCI8ConnectionTest.php b/tests/Driver/OCI8/OCI8ConnectionTest.php deleted file mode 100644 index ed4ed90166c..00000000000 --- a/tests/Driver/OCI8/OCI8ConnectionTest.php +++ /dev/null @@ -1,37 +0,0 @@ -markTestSkipped('oci8 is not installed.'); - } - - parent::setUp(); - - $this->connectionMock = $this->getMockBuilder(OCI8Connection::class) - ->disableOriginalConstructor() - ->getMockForAbstractClass(); - } - - public function testDoesNotRequireQueryForServerVersion(): void - { - self::assertFalse($this->connectionMock->requiresQueryForServerVersion()); - } -} diff --git a/tests/Driver/SQLSrv/SQLSrvConnectionTest.php b/tests/Driver/SQLSrv/SQLSrvConnectionTest.php deleted file mode 100644 index 2086b2677a0..00000000000 --- a/tests/Driver/SQLSrv/SQLSrvConnectionTest.php +++ /dev/null @@ -1,37 +0,0 @@ -markTestSkipped('sqlsrv is not installed.'); - } - - parent::setUp(); - - $this->connectionMock = $this->getMockBuilder(SQLSrvConnection::class) - ->disableOriginalConstructor() - ->getMockForAbstractClass(); - } - - public function testDoesNotRequireQueryForServerVersion(): void - { - self::assertFalse($this->connectionMock->requiresQueryForServerVersion()); - } -} diff --git a/tests/Functional/Driver/PDO/ConnectionTest.php b/tests/Functional/Driver/PDO/ConnectionTest.php index a9871ee9423..46b1298439f 100644 --- a/tests/Functional/Driver/PDO/ConnectionTest.php +++ b/tests/Functional/Driver/PDO/ConnectionTest.php @@ -45,11 +45,6 @@ protected function tearDown(): void parent::tearDown(); } - public function testDoesNotRequireQueryForServerVersion(): void - { - self::assertFalse($this->driverConnection->requiresQueryForServerVersion()); - } - public function testThrowsWrappedExceptionOnConstruct(): void { $this->expectException(Exception::class);