Removed ServerInfoAwareConnection#requiresQueryForServerVersion() as implementation detail

[morozov]

Q	A
Type	improvement
BC Break	yes

Testing the implementations of this method requires partial mocking of the implementing classes which makes it impossible to make them final (#3590, build #632116824).

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.

In the future, I'd like to get rid of the ServerAwareConnection interface at all make the platform instantiation part of the driver and driver connection interfaces:

interface Driver
{
    public function connect() : Connection;
    public function createPlatform(Connection $connection) : Platform;
}

interface Connection
{
    public function getServerVersion() : string;
}

Depending on whether the DBAL supports multiple versions of the given platform, the driver will or will not call getServerVersion() on the given connection and will always return a platform. This way, the wrapper logic will not have any conditions based on the implementation details of the underlying driver and the platform.

[beberlei]

@morozov the problem we tried to solve, with the check for server versions is not the time it takes to make the server version query, but that making the query opens up the connection, even though the current request might not even need the database.

Making the query breaks the laziness of the connection.

However since we recommend to set the server version as param now anyways it might make sense to just show this everywhere explicitly in the documentation.

[morozov]

but that making the query opens up the connection, even though the current request might not even need the database.

I don't see how this is relevant to the laziness. Whether this is a query or another driver API call, detecting the server version needs a connection establishing which makes the connection not lazy.

[morozov]

@beberlei could you clarify how exactly lazy the connection is meant to be? I tried the following code:

use Doctrine\DBAL\DriverManager;

$conn = DriverManager::getConnection([
    'driver' => 'pdo_mysql',
    'user'   => 'root',
    'dbname' => 'mysql',
]);

exit();

In this scenario, the code of PDOMySql\Driver::connect() or even MysqliConnection::requiresQueryForServerVersion() is not executed because there's no need for the server version.

We can test the requirement of the laziness by mocking a driver and making sure its connect() method is not invoked in certain scenarios where the laziness should be guaranteed.

[beberlei]

@morozov I only remember this popped up massively in the context of Symfony, so maybe the DoctrineBundle acquires the platform early for something and triggers the query.

[beberlei]

@morozov just checked with a callgraph that the main reason it happens is actually the ORM (v2.6), which loads the platform for entity persisters, proxy generators and other services, that can be created already without a query being executed.

[morozov]

Even if that's the case, as I said earlier, acquiring a specific version-aware platform requires a connection. It's irrelevant whether the version is detected via a query.

Should we proceed with the change or you want some research to be done?

UPD: this response was to #3807 (comment).

[beberlei]

@morozov I believe we should move forward, and I open a ticket on ORM for fetching the platform more lazily in some places (not in constructors), so that we can avoid accidentally loading the connection, when the platform is fetched but not used.

[morozov]

@beberlei please file the ticket since I'm not that well familiar with ORM and the specific components where the platform instantiation needs to be revisited.

[beberlei]

@morozov done here: doctrine/orm#7972