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

Remove ServerInfoAwareConnection#requiresQueryForServerVersion() as an implementation detail #4115

Merged

Conversation

morozov
Copy link
Member

@morozov morozov commented Jun 27, 2020

Q A
Type improvement
BC Break yes

The method is going to be deprecated in #4114 and was originally removed in #3807.

It's required that the method is removed in 3.0.x in order to replace certain deprecated driver classes (see #4100) with their new versions marked final.

…n implementation detail

Testing the implementations of this method requires partial mocking of the implementing class which makes it impossible to make them `final` (doctrine#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 (doctrine#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.”
@morozov morozov merged commit 37531f9 into doctrine:3.0.x Jun 27, 2020
@morozov morozov deleted the remove-requires-query-for-server-version branch June 27, 2020 07:49
@morozov morozov self-assigned this Jun 27, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 1, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants