From 8d015895d96368121aaa3fcb80da1220519304f1 Mon Sep 17 00:00:00 2001 From: Sergei Morozov Date: Thu, 2 Jan 2020 18:19:02 -0800 Subject: [PATCH] Removed ServerInfoAwareConnection#requiresQueryForServerVersion() as 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.” In addition to the above, the only driver that doesn't support the platform version detection via the underlying driver API is `sqsql` which is barely supported. --- UPGRADE.md | 4 ++ lib/Doctrine/DBAL/Connection.php | 2 +- .../DBAL/Driver/IBMDB2/DB2Connection.php | 8 ---- .../DBAL/Driver/Mysqli/MysqliConnection.php | 8 ---- .../DBAL/Driver/OCI8/OCI8Connection.php | 8 ---- lib/Doctrine/DBAL/Driver/PDOConnection.php | 8 ---- .../SQLAnywhere/SQLAnywhereConnection.php | 8 ---- .../DBAL/Driver/SQLSrv/SQLSrvConnection.php | 8 ---- .../DBAL/Driver/ServerInfoAwareConnection.php | 7 ---- tests/Doctrine/Tests/DBAL/ConnectionTest.php | 4 -- .../DBAL/Driver/IBMDB2/DB2ConnectionTest.php | 38 ------------------- .../Driver/Mysqli/MysqliConnectionTest.php | 21 ++-------- .../DBAL/Driver/OCI8/OCI8ConnectionTest.php | 38 ------------------- .../SQLAnywhere/SQLAnywhereConnectionTest.php | 38 ------------------- .../Driver/SQLSrv/SQLSrvConnectionTest.php | 38 ------------------- .../Functional/Driver/PDOConnectionTest.php | 5 --- 16 files changed, 8 insertions(+), 235 deletions(-) delete mode 100644 tests/Doctrine/Tests/DBAL/Driver/IBMDB2/DB2ConnectionTest.php delete mode 100644 tests/Doctrine/Tests/DBAL/Driver/OCI8/OCI8ConnectionTest.php delete mode 100644 tests/Doctrine/Tests/DBAL/Driver/SQLAnywhere/SQLAnywhereConnectionTest.php delete mode 100644 tests/Doctrine/Tests/DBAL/Driver/SQLSrv/SQLSrvConnectionTest.php diff --git a/UPGRADE.md b/UPGRADE.md index f34f3ddca77..242db866e6d 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 almost all supported drivers. + ## BC BREAK: PingableConnection and ServerInfoAwareConnection interfaces now extends Connection All implementations of the `PingableConnection` and `ServerInfoAwareConnection` interfaces have to implement the methods defined in the `Connection` interface as well. diff --git a/lib/Doctrine/DBAL/Connection.php b/lib/Doctrine/DBAL/Connection.php index d890e2bee7a..370978deed6 100644 --- a/lib/Doctrine/DBAL/Connection.php +++ b/lib/Doctrine/DBAL/Connection.php @@ -393,7 +393,7 @@ private function getServerVersion() : ?string $connection = $this->getWrappedConnection(); // Automatic platform version detection. - if ($connection instanceof ServerInfoAwareConnection && ! $connection->requiresQueryForServerVersion()) { + if ($connection instanceof ServerInfoAwareConnection) { return $connection->getServerVersion(); } diff --git a/lib/Doctrine/DBAL/Driver/IBMDB2/DB2Connection.php b/lib/Doctrine/DBAL/Driver/IBMDB2/DB2Connection.php index 4960dc13b72..c1cb60774a4 100644 --- a/lib/Doctrine/DBAL/Driver/IBMDB2/DB2Connection.php +++ b/lib/Doctrine/DBAL/Driver/IBMDB2/DB2Connection.php @@ -59,14 +59,6 @@ public function getServerVersion() : string return $serverInfo->DBMS_VER; } - /** - * {@inheritdoc} - */ - public function requiresQueryForServerVersion() : bool - { - return false; - } - /** * {@inheritdoc} */ diff --git a/lib/Doctrine/DBAL/Driver/Mysqli/MysqliConnection.php b/lib/Doctrine/DBAL/Driver/Mysqli/MysqliConnection.php index c00590be8e9..f01fc6aef9d 100644 --- a/lib/Doctrine/DBAL/Driver/Mysqli/MysqliConnection.php +++ b/lib/Doctrine/DBAL/Driver/Mysqli/MysqliConnection.php @@ -117,14 +117,6 @@ public function getServerVersion() : string return $majorVersion . '.' . $minorVersion . '.' . $patchVersion; } - /** - * {@inheritdoc} - */ - public function requiresQueryForServerVersion() : bool - { - return false; - } - /** * {@inheritdoc} */ diff --git a/lib/Doctrine/DBAL/Driver/OCI8/OCI8Connection.php b/lib/Doctrine/DBAL/Driver/OCI8/OCI8Connection.php index c2eac6d92a5..d0723f1a044 100644 --- a/lib/Doctrine/DBAL/Driver/OCI8/OCI8Connection.php +++ b/lib/Doctrine/DBAL/Driver/OCI8/OCI8Connection.php @@ -85,14 +85,6 @@ public function getServerVersion() : string return $matches[1]; } - /** - * {@inheritdoc} - */ - public function requiresQueryForServerVersion() : bool - { - return false; - } - /** * {@inheritdoc} */ diff --git a/lib/Doctrine/DBAL/Driver/PDOConnection.php b/lib/Doctrine/DBAL/Driver/PDOConnection.php index d81594b20bf..ed0f5d63eb0 100644 --- a/lib/Doctrine/DBAL/Driver/PDOConnection.php +++ b/lib/Doctrine/DBAL/Driver/PDOConnection.php @@ -105,14 +105,6 @@ public function lastInsertId(?string $name = null) : string } } - /** - * {@inheritdoc} - */ - public function requiresQueryForServerVersion() : bool - { - return false; - } - /** * Creates a wrapped statement */ diff --git a/lib/Doctrine/DBAL/Driver/SQLAnywhere/SQLAnywhereConnection.php b/lib/Doctrine/DBAL/Driver/SQLAnywhere/SQLAnywhereConnection.php index 4cb8162bed9..7a79afe025c 100644 --- a/lib/Doctrine/DBAL/Driver/SQLAnywhere/SQLAnywhereConnection.php +++ b/lib/Doctrine/DBAL/Driver/SQLAnywhere/SQLAnywhereConnection.php @@ -144,14 +144,6 @@ public function quote(string $input) : string return "'" . sasql_escape_string($this->connection, $input) . "'"; } - /** - * {@inheritdoc} - */ - public function requiresQueryForServerVersion() : bool - { - return true; - } - /** * {@inheritdoc} * diff --git a/lib/Doctrine/DBAL/Driver/SQLSrv/SQLSrvConnection.php b/lib/Doctrine/DBAL/Driver/SQLSrv/SQLSrvConnection.php index b7713592274..b2eaae74550 100644 --- a/lib/Doctrine/DBAL/Driver/SQLSrv/SQLSrvConnection.php +++ b/lib/Doctrine/DBAL/Driver/SQLSrv/SQLSrvConnection.php @@ -59,14 +59,6 @@ public function getServerVersion() : string return $serverInfo['SQLServerVersion']; } - /** - * {@inheritdoc} - */ - public function requiresQueryForServerVersion() : bool - { - return false; - } - /** * {@inheritDoc} */ diff --git a/lib/Doctrine/DBAL/Driver/ServerInfoAwareConnection.php b/lib/Doctrine/DBAL/Driver/ServerInfoAwareConnection.php index c33e9b50c8f..c5d63b099f4 100644 --- a/lib/Doctrine/DBAL/Driver/ServerInfoAwareConnection.php +++ b/lib/Doctrine/DBAL/Driver/ServerInfoAwareConnection.php @@ -13,11 +13,4 @@ interface ServerInfoAwareConnection extends Connection * Returns the version number of the database server connected to. */ public function getServerVersion() : string; - - /** - * 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() : bool; } diff --git a/tests/Doctrine/Tests/DBAL/ConnectionTest.php b/tests/Doctrine/Tests/DBAL/ConnectionTest.php index 8815b8b1473..e72fa94ea89 100644 --- a/tests/Doctrine/Tests/DBAL/ConnectionTest.php +++ b/tests/Doctrine/Tests/DBAL/ConnectionTest.php @@ -687,10 +687,6 @@ public function testPlatformDetectionIsTriggerOnlyOnceOnRetrievingPlatform() : v ->method('connect') ->will($this->returnValue($driverConnectionMock)); - $driverConnectionMock->expects($this->once()) - ->method('requiresQueryForServerVersion') - ->will($this->returnValue(false)); - $driverConnectionMock->expects($this->once()) ->method('getServerVersion') ->will($this->returnValue('6.6.6')); diff --git a/tests/Doctrine/Tests/DBAL/Driver/IBMDB2/DB2ConnectionTest.php b/tests/Doctrine/Tests/DBAL/Driver/IBMDB2/DB2ConnectionTest.php deleted file mode 100644 index db4270d5652..00000000000 --- a/tests/Doctrine/Tests/DBAL/Driver/IBMDB2/DB2ConnectionTest.php +++ /dev/null @@ -1,38 +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/Doctrine/Tests/DBAL/Driver/Mysqli/MysqliConnectionTest.php b/tests/Doctrine/Tests/DBAL/Driver/Mysqli/MysqliConnectionTest.php index fe9716d43b7..513d46bdab5 100644 --- a/tests/Doctrine/Tests/DBAL/Driver/Mysqli/MysqliConnectionTest.php +++ b/tests/Doctrine/Tests/DBAL/Driver/Mysqli/MysqliConnectionTest.php @@ -8,20 +8,12 @@ use Doctrine\DBAL\Driver\Mysqli\MysqliConnection; use Doctrine\DBAL\Platforms\MySqlPlatform; use Doctrine\Tests\DbalFunctionalTestCase; -use PHPUnit\Framework\MockObject\MockObject; use function extension_loaded; use function restore_error_handler; use function set_error_handler; class MysqliConnectionTest extends DbalFunctionalTestCase { - /** - * The mysqli driver connection mock under test. - * - * @var MysqliConnection|MockObject - */ - private $connectionMock; - protected function setUp() : void { if (! extension_loaded('mysqli')) { @@ -30,18 +22,11 @@ protected function setUp() : void parent::setUp(); - if (! $this->connection->getDatabasePlatform() instanceof MySqlPlatform) { - $this->markTestSkipped('MySQL only test.'); + if ($this->connection->getDatabasePlatform() instanceof MySqlPlatform) { + return; } - $this->connectionMock = $this->getMockBuilder(MysqliConnection::class) - ->disableOriginalConstructor() - ->getMockForAbstractClass(); - } - - public function testDoesNotRequireQueryForServerVersion() : void - { - self::assertFalse($this->connectionMock->requiresQueryForServerVersion()); + $this->markTestSkipped('MySQL only test.'); } public function testRestoresErrorHandlerOnException() : void diff --git a/tests/Doctrine/Tests/DBAL/Driver/OCI8/OCI8ConnectionTest.php b/tests/Doctrine/Tests/DBAL/Driver/OCI8/OCI8ConnectionTest.php deleted file mode 100644 index 0c54cc88c6c..00000000000 --- a/tests/Doctrine/Tests/DBAL/Driver/OCI8/OCI8ConnectionTest.php +++ /dev/null @@ -1,38 +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/Doctrine/Tests/DBAL/Driver/SQLAnywhere/SQLAnywhereConnectionTest.php b/tests/Doctrine/Tests/DBAL/Driver/SQLAnywhere/SQLAnywhereConnectionTest.php deleted file mode 100644 index d78a95fa6da..00000000000 --- a/tests/Doctrine/Tests/DBAL/Driver/SQLAnywhere/SQLAnywhereConnectionTest.php +++ /dev/null @@ -1,38 +0,0 @@ -markTestSkipped('sqlanywhere is not installed.'); - } - - parent::setUp(); - - $this->connectionMock = $this->getMockBuilder(SQLAnywhereConnection::class) - ->disableOriginalConstructor() - ->getMockForAbstractClass(); - } - - public function testRequiresQueryForServerVersion() : void - { - self::assertTrue($this->connectionMock->requiresQueryForServerVersion()); - } -} diff --git a/tests/Doctrine/Tests/DBAL/Driver/SQLSrv/SQLSrvConnectionTest.php b/tests/Doctrine/Tests/DBAL/Driver/SQLSrv/SQLSrvConnectionTest.php deleted file mode 100644 index 4225e65c2dd..00000000000 --- a/tests/Doctrine/Tests/DBAL/Driver/SQLSrv/SQLSrvConnectionTest.php +++ /dev/null @@ -1,38 +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/Doctrine/Tests/DBAL/Functional/Driver/PDOConnectionTest.php b/tests/Doctrine/Tests/DBAL/Functional/Driver/PDOConnectionTest.php index e6f41a0a0ae..069a87b15a2 100644 --- a/tests/Doctrine/Tests/DBAL/Functional/Driver/PDOConnectionTest.php +++ b/tests/Doctrine/Tests/DBAL/Functional/Driver/PDOConnectionTest.php @@ -46,11 +46,6 @@ protected function tearDown() : void parent::tearDown(); } - public function testDoesNotRequireQueryForServerVersion() : void - { - self::assertFalse($this->driverConnection->requiresQueryForServerVersion()); - } - public function testThrowsWrappedExceptionOnConstruct() : void { $this->expectException(PDOException::class);