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.

Sign up for GitHub

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

Jump to bottom

DBAL 4 compatibility #1707

Merged
merged 5 commits into from
Oct 30, 2023
Merged

DBAL 4 compatibility #1707

merged 5 commits into from
Oct 30, 2023

Conversation

dmaicher
Copy link
Contributor

@dmaicher dmaicher commented Oct 17, 2023 •
edited
Loading

This adds compatibility with DBAL 4 (ignoring ORM 3 for now).

@dmaicher dmaicher mentioned this pull request Oct 17, 2023
Merged
@stof stof mentioned this pull request Oct 17, 2023
Merged
@dmaicher dmaicher force-pushed the dbal_4 branch 6 times, most recently from e33d951 to 8ca42b4 Compare October 18, 2023 19:00
dmaicher
dmaicher commented Oct 18, 2023
View reviewed changes
ConnectionFactory.php
@@ -102,7 +108,11 @@ public function createConnection(array $params, ?Configuration $config = null, ?
$connection = DriverManager::getConnection($params, $config, $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

DoctrineBundle/ConnectionFactory.php

Lines 105 to 127 in b1ff934

$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?

dmaicher
dmaicher commented Oct 18, 2023
View reviewed changes
ConnectionFactory.php Outdated Show resolved Hide resolved
stof
stof reviewed Oct 19, 2023
View reviewed changes
ConnectionFactory.php
@@ -102,7 +108,11 @@ public function createConnection(array $params, ?Configuration $config = null, ?
$connection = DriverManager::getConnection($params, $config, $eventManager);
$params = $this->addDatabaseSuffix(array_merge($connection->getParams(), $overriddenOptions));
$driver = $connection->getDriver();
$platform = $driver->getDatabasePlatform();
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 ?

ConnectionFactory.php
@@ -102,7 +108,11 @@ public function createConnection(array $params, ?Configuration $config = null, ?
$connection = DriverManager::getConnection($params, $config, $eventManager);
$params = $this->addDatabaseSuffix(array_merge($connection->getParams(), $overriddenOptions));
$driver = $connection->getDriver();
$platform = $driver->getDatabasePlatform();
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.

ConnectionFactory.php Outdated Show resolved Hide resolved
ConnectionFactory.php Outdated Show resolved Hide resolved
ConnectionFactory.php Show resolved Hide resolved
@dmaicher dmaicher marked this pull request as ready for review October 23, 2023 08:39
@dmaicher dmaicher requested review from stof and derrabus October 23, 2023 08:39
@dmaicher dmaicher added this to the 2.11.0 milestone Oct 23, 2023
@dmaicher dmaicher mentioned this pull request Oct 29, 2023
Merged
@stof
Copy link
Member

stof commented Oct 30, 2023

@derrabus this one needs rebasing to fix conflicts.

Also, I'd like to release 2.11.0 to have the other new features available. Should I wait for this PR or delay it to 2.12 ?

@derrabus derrabus merged commit 888d37f into doctrine:2.11.x Oct 30, 2023
13 checks passed
@dmaicher dmaicher deleted the dbal_4 branch October 30, 2023 10:14
@derrabus
Copy link
Member

@stof I'd like to see if we can get ORM 3 support done for 2.11 as well. Can you hold back the release for one more day?

@stof
Copy link
Member

stof commented Oct 30, 2023

yes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ostrolucky ostrolucky ostrolucky left review comments

@derrabus derrabus derrabus approved these changes

@stof stof Awaiting requested review from stof

Assignees
No one assigned
Labels
Improvement
Projects
None yet
Milestone
2.11.0
Development

Successfully merging this pull request may close these issues.
4 participants
@dmaicher @stof @derrabus @ostrolucky