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

DBAL 4 compatibility #1707

Merged
merged 5 commits into from
Oct 30, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions .github/workflows/continuous-integration.yml
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,12 @@ jobs:
stability: "stable"
remove-orm: true

# DBAL 4
- php-version: "8.2"
dependencies: "highest"
stability: "dev"
remove-orm: true
dmaicher marked this conversation as resolved.
Show resolved Hide resolved

# Bleeding edge
- php-version: "8.2"
dependencies: "highest"
Expand Down
38 changes: 30 additions & 8 deletions ConnectionFactory.php
Original file line number Diff line number Diff line change
Expand Up @@ -5,19 +5,25 @@
use Doctrine\Common\EventManager;
use Doctrine\DBAL\Configuration;
use Doctrine\DBAL\Connection;
use Doctrine\DBAL\Connection\StaticServerVersionProvider;
use Doctrine\DBAL\ConnectionException;
use Doctrine\DBAL\DriverManager;
use Doctrine\DBAL\Exception;
use Doctrine\DBAL\Exception as DBALException;
use Doctrine\DBAL\Exception\DriverException;
use Doctrine\DBAL\Exception\DriverRequired;
use Doctrine\DBAL\Exception\InvalidWrapperClass;
use Doctrine\DBAL\Exception\MalformedDsnException;
use Doctrine\DBAL\Platforms\AbstractMySQLPlatform;
use Doctrine\DBAL\Platforms\AbstractPlatform;
use Doctrine\DBAL\Tools\DsnParser;
use Doctrine\DBAL\Types\Type;
use Doctrine\Deprecations\Deprecation;
use InvalidArgumentException;

use function array_merge;
use function class_exists;
use function is_subclass_of;
use function method_exists;
use function trigger_deprecation;

use const PHP_EOL;
Expand Down Expand Up @@ -63,6 +69,10 @@ public function __construct(array $typesConfig, ?DsnParser $dsnParser = null)
*/
public function createConnection(array $params, ?Configuration $config = null, ?EventManager $eventManager = null, array $mappingTypes = [])
{
if (! method_exists(Connection::class, 'getEventManager') && $eventManager !== null) {
throw new InvalidArgumentException('Passing an EventManager instance is not supported with DBAL > 3');
}

if (! $this->initialized) {
$this->initializeTypes();
}
Expand Down Expand Up @@ -92,17 +102,23 @@ public function createConnection(array $params, ?Configuration $config = null, ?

if (isset($params['wrapperClass'])) {
if (! is_subclass_of($params['wrapperClass'], Connection::class)) {
if (class_exists(InvalidWrapperClass::class)) {
throw InvalidWrapperClass::new($params['wrapperClass']);
}

throw DBALException::invalidWrapperClass($params['wrapperClass']);
}

$wrapperClass = $params['wrapperClass'];
$params['wrapperClass'] = null;
}

$connection = DriverManager::getConnection($params, $config, $eventManager);
$connection = DriverManager::getConnection(...array_merge([$params, $config], $eventManager ? [$eventManager] : []));
$params = $this->addDatabaseSuffix(array_merge($connection->getParams(), $overriddenOptions));
$driver = $connection->getDriver();
$platform = $driver->getDatabasePlatform();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so previously this skipped any server version and for mysql for example just returned MySQLPlatform.

We could use $connection->getDatabasePlatform() but it would mean we auto-detect the server version which could mean we actually connect to the database. We should avoid that here I guess.

Is there any other way to do this with DBAL 4 except for using a StaticServerVersionProvider with a fallback to an empty server version? 🙈 🤔

@derrabus maybe you have an idea

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so previously this skipped any server version and for mysql for example just returned MySQLPlatform.

Why though? For MySQL drivers especially, this sounds like a bad idea because we need the server version to determine whether we talk to a MySQL or a MariaDB.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We check if its a "Mysql like" platform (AbstractMySQLPlatform) to set the collation option 🤔

See

$platform = $driver->getDatabasePlatform();
if (! isset($params['charset'])) {
if ($platform instanceof AbstractMySQLPlatform) {
$params['charset'] = 'utf8mb4';
if (isset($params['defaultTableOptions']['collate'])) {
Deprecation::trigger(
'doctrine/doctrine-bundle',
'https://github.com/doctrine/dbal/issues/5214',
'The "collate" default table option is deprecated in favor of "collation" and will be removed in doctrine/doctrine-bundle 3.0. '
);
$params['defaultTableOptions']['collation'] = $params['defaultTableOptions']['collate'];
unset($params['defaultTableOptions']['collate']);
}
if (! isset($params['defaultTableOptions']['collation'])) {
$params['defaultTableOptions']['collation'] = 'utf8mb4_unicode_ci';
}
} else {
$params['charset'] = 'utf8';
}
}

Maybe this isn't even needed anymore on DBAL 4? See doctrine/dbal#5214, doctrine/dbal#5246

I believe AbstractMySQLPlatform only handles collation on 4.0, or?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm but we also set a default charset utf8mb4 for those platforms 🤔 We have to keep this I think?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not using $connection->getDatabasePlatform() instead, which would automatically use the version provider based on the configuration ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or better, using $this->getDatabasePlatform() to have the better error message telling you how to do if you want to be able to instantiate the connection in environments not able to connect yet.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried this but it would mean we might connect to the database to determine the platform version (if not specified via config). For example all over the testsuite we would try to connect to some mysql db.

Since this is the initial connection that is replaced further down with changed parameters: we should avoid connecting, or?

$platform = $driver->getDatabasePlatform(
...(class_exists(StaticServerVersionProvider::class) ? [new StaticServerVersionProvider($params['serverVersion'] ?? '')] : [])
);

if (! isset($params['charset'])) {
if ($platform instanceof AbstractMySQLPlatform) {
Expand Down Expand Up @@ -134,7 +150,7 @@ public function createConnection(array $params, ?Configuration $config = null, ?

$connection = new $wrapperClass($params, $driver, $config, $eventManager);
} else {
$connection = DriverManager::getConnection($params, $config, $eventManager);
$connection = DriverManager::getConnection(...array_merge([$params, $config], $eventManager ? [$eventManager] : []));
}

if (! empty($mappingTypes)) {
Expand Down Expand Up @@ -162,7 +178,9 @@ private function getDatabasePlatform(Connection $connection): AbstractPlatform
try {
return $connection->getDatabasePlatform();
} catch (DriverException $driverException) {
throw new DBALException(
$class = class_exists(DBALException::class) ? DBALException::class : ConnectionException::class;

throw new $class(
'An exception occurred while establishing a connection to figure out your platform version.' . PHP_EOL .
"You can circumvent this by setting a 'server_version' configuration value" . PHP_EOL . PHP_EOL .
'For further information have a look at:' . PHP_EOL .
Expand Down Expand Up @@ -226,7 +244,7 @@ private function addDatabaseSuffix(array $params): array
* URL extracted into individual parameter parts.
* @psalm-return Params
*
* @throws Exception
* @throws DBALException
*/
private function parseDatabaseUrl(array $params): array
{
Expand All @@ -237,7 +255,7 @@ private function parseDatabaseUrl(array $params): array
try {
$parsedParams = $this->dsnParser->parse($params['url']);
} catch (MalformedDsnException $e) {
throw new Exception('Malformed parameter "url".', 0, $e);
dmaicher marked this conversation as resolved.
Show resolved Hide resolved
throw new MalformedDsnException('Malformed parameter "url".', 0, $e);
}

if (isset($parsedParams['driver'])) {
Expand All @@ -251,7 +269,11 @@ private function parseDatabaseUrl(array $params): array
// If a schemeless connection URL is given, we require a default driver or default custom driver
// as connection parameter.
if (! isset($params['driverClass']) && ! isset($params['driver'])) {
throw Exception::driverRequired($params['url']);
if (class_exists(DriverRequired::class)) {
throw DriverRequired::new($params['url']);
}

throw DBALException::driverRequired($params['url']);
}

unset($params['url']);
Expand Down
3 changes: 2 additions & 1 deletion DependencyInjection/DoctrineExtension.php
Original file line number Diff line number Diff line change
Expand Up @@ -278,7 +278,8 @@ protected function loadDbalConnection($name, array $connection, ContainerBuilder
->setArguments([
$options,
new Reference(sprintf('doctrine.dbal.%s_connection.configuration', $name)),
new Reference(sprintf('doctrine.dbal.%s_connection.event_manager', $name)),
// event manager is only supported on DBAL < 4
method_exists(Connection::class, 'getEventManager') ? new Reference(sprintf('doctrine.dbal.%s_connection.event_manager', $name)) : null,
$connection['mapping_types'],
]);

Expand Down
2 changes: 1 addition & 1 deletion composer.json
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@
"require": {
"php": "^7.4 || ^8.0",
"doctrine/cache": "^1.11 || ^2.0",
"doctrine/dbal": "^3.7.0",
"doctrine/dbal": "^3.7.0 || ^4.0",
"doctrine/persistence": "^2.2 || ^3",
"doctrine/sql-formatter": "^1.0.1",
"symfony/cache": "^5.4 || ^6.0 || ^7.0",
Expand Down