-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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 VersionAwarePlatformDriver and ServerInfoAwareConnection #4764
Remove VersionAwarePlatformDriver and ServerInfoAwareConnection #4764
Conversation
08ad421
to
288f5d6
Compare
@@ -807,25 +826,6 @@ public function testThrowsExceptionWhenInValidPlatformSpecified(): void | |||
new Connection($connectionParams, $driver); | |||
} | |||
|
|||
public function testRethrowsOriginalExceptionOnDeterminingPlatformWhenConnectingToNonExistentDatabase(): void |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wasted more than an hour trying to rework this test before I realized that the underlying implementation is pointless (originally implemented in #2671).
It only allows detection of the server version but doesn't allow anything else. If the configured database doesn't exist, the user will have to use the admin connection anyway (like the test suites do):
$connection = DriverManager::getConnection([
'driver' => 'mysqli',
'host' => '127.0.0.1',
'user' => 'root',
'dbname' => 'non_existing_database',
]);
echo get_class($connection->getDatabasePlatform()), PHP_EOL;
// Doctrine\DBAL\Platforms\MySQL80Platform
try {
$connection->executeStatement('CREATE DATABASE non_existing_database');
} catch (Exception $e) {
echo $e->getMessage(), PHP_EOL;
// An exception occurred in the driver: Unknown database 'non_existing_database'
}
It works like above on all supported DBAL versions.
Let's leave this test removed for now. I'll gather more details to deprecate this feature and subsequently remove it. In the miraculous case, if it's still needed, we'll replace it with an integration test.
should the main Connection class indeed implement a public method performing the version guessing (needing to connect) even when the static version is provided ? The current architecture means that Connection (the main public API of the package) implements Maybe a |
Yeah, I thought of this as the next logical step. It could have been done right away but I honestly don't remember why I didn't do that. My thinking was that the wrapper connection is quite a god object, and it should delegate certain responsibilities to specially crafted components rather than implement all the logic itself. I agree that the ability of the wrapper connection to detect the server version by connecting should not be exposed to the consumers. Actually, one of the reasons to use the wrapper connection was the fact that it can connect lazily. Probably, the |
See #4751.
A new
ServerVersionProvider
interface has been introduced to provide the server version to the driver if required to instantiate the platform.From the wrapper connection laziness, three scenarios are possible:
Driver::getDatabasePlatform()
callsConnection::getServerVersion()
, which in turn callsDriver::connect()
, so the connection is established to instantiate the platform.StaticServerVersionProvider::getServerVersion()
provides the configured server version, andDriver::connect()
doesn't get invoked.ServerVersionProvider::getServerVersion()
, andDriver::connect()
doesn't get invoked either.The above behavior is the same as in DBAL 3.x but doesn't require runtime type checks and assertions.