Fix default MySQL charset with doctrine/dbal > 2 #1481

alexander-schranz · 2022-03-02T18:50:22Z

The $driver variable can return Doctrine\DBAL\Logging\Driver which is not instance of AbstractMySQLDriver. So it need to be via the platform.

Sadly AbstractMySQLDriver is not available on prefer lowest so we need to check on the deprecated getName method on the platform.

fixes #1480

alexander-schranz · 2022-03-02T19:17:30Z

Tests/ConnectionFactoryTest.php


        $this->assertInstanceof(FakeConnection::class, $connection);
-        $this->assertSame('utf8', $connection->getParams()['charset']);
+        $this->assertSame('utf8mb4', $connection->getParams()['charset']);


the FakeDriver is actually a MysqlPlatform that is why this test changees now.

alexander-schranz · 2022-03-02T19:18:06Z

ConnectionFactory.php


            if (! isset($params['charset'])) {
-                if ($driver instanceof AbstractMySQLDriver) {
+                if ($driver->getDatabasePlatform()->getName() === 'mysql') {


This is sadly calling a deprecated method but I could not find another method which works on this range of supported doctrine/dbal versions.

Maybe by performing several instanceof checks?

greg0ire · 2022-03-02T19:18:34Z

Tests/ConnectionFactoryTest.php


        $this->assertInstanceof(FakeConnection::class, $connection);
-        $this->assertSame('utf8', $connection->getParams()['charset']);
+        $this->assertSame('utf8mb4', $connection->getParams()['charset']);


Are you sure this is OK? It does not look OK to change that default value like that (I know it's a bad default, but it might result in a lot of migrations for a lot of people).

The fake driver ist actually a mysqlplatform that is why it is changing :

DoctrineBundle/Tests/ConnectionFactoryTest.php

Line 156 in 6e578fd

return static::$platform ?? new MySQLPlatform();

Before the check was on the driver not on the platform.

🤔 in fact, I think it might be fine, since people using MySQL ended up with utf8mb4 before #1456, but maybe we should rename the test to testDefaultCharsetForMySQL, and duplicate it with testDefaultCharsetForNonMySQL to test the other codepath with utf8

alexander-schranz · 2022-03-02T19:32:14Z

@greg0ire any idea how we can get the testContainer working when we now access the getDatabasePlatform in the factory?

greg0ire · 2022-03-02T19:33:04Z

ConnectionFactory.php


            if (! isset($params['charset'])) {
-                if ($driver instanceof AbstractMySQLDriver) {
+                if ($driver->getDatabasePlatform() instanceof AbstractMySQLPlatform


The resulting Psalm error will have to be ignored through configuration I think.

I'm not familiar with psalm do I need to ignore the whole file or is there another way?

okay seems @psalm-suppress UndefinedClass does the job

greg0ire · 2022-03-02T19:35:20Z

@greg0ire any idea how we can get the testContainer working when we now access the getDatabasePlatform in the factory?

Let's improve the error message in the test first? "Failed asserting that false is true." is not very helpful

alexander-schranz · 2022-03-02T19:41:42Z

Improved the message by using assertStringContainsString instead of some strpos() > 0

alexander-schranz · 2022-03-02T19:51:31Z

Is it maybe to early to access the platform because the platform is validated later here:

DoctrineBundle/ConnectionFactory.php

Line 110 in eb7f3a2

$platform = $this->getDatabasePlatform($connection);

Which is the method the testContainer test is testing. I'm a little bit stuck now.

dmaicher · 2022-03-02T19:52:57Z

The $driver variable can return Doctrine\DBAL\Logging\Driver which is not instance of AbstractMySQLDriver. So it need to be via the platform.

Ah nice catch. So this is caused by wiring the logging middlewares

Tests/ConnectionFactoryTest.php

ostrolucky · 2022-03-02T22:00:24Z

should target 2.5.x branch

greg0ire · 2022-03-03T07:46:48Z

At that point I'm starting to think the way forward would be to make the middlewares opt-in, and throw if the charset is not specified when using them.
From my point of view, a lot of the code in ConnectionFactory should go as it seems like hacks to fix issues that should either be fixed in DBAL, or defaults that should come from the framework (typically from Symfony recipes). Recipes are IMO the correct place to set defaults, because you can change the defaults there without that being a BC-break.

So I think we should also deprecate not specifying the charset.

Maybe @ostrolucky has an opinion to share on this as well?

ostrolucky · 2022-03-03T08:36:05Z

I share same opinion

Co-authored-by: Grégoire Paris <postmaster@greg0ire.fr>

alexander-schranz · 2022-03-06T14:41:07Z

Thank you all 👍

alexander-schranz force-pushed the bugfix/fix-driver-check-utf8mb4 branch 2 times, most recently from 3c23123 to 5340e68 Compare March 2, 2022 19:14

alexander-schranz changed the base branch from 2.5.x to 2.6.x March 2, 2022 19:16

alexander-schranz force-pushed the bugfix/fix-driver-check-utf8mb4 branch from 5340e68 to b78ad6b Compare March 2, 2022 19:17

alexander-schranz commented Mar 2, 2022

View reviewed changes

greg0ire reviewed Mar 2, 2022

View reviewed changes

Tests/ConnectionFactoryTest.php Outdated Show resolved Hide resolved

alexander-schranz and others added 10 commits March 3, 2022 11:08

Fix driver check for utf8mb4

09247a9

Check for platform name

70dd185

Remove use statement

c9028c2

Handle fake driver as mysql platform

87ab806

Use multiple instance of check for platform

b7634fe

Improve exception

c20c2a4

Ignore file in psalm

8dddbfa

Ignore ConnectionFactory.php file

0bb611c

Try psalm suppress

8686170

Update Tests/ConnectionFactoryTest.php

fbdd14a

Co-authored-by: Grégoire Paris <postmaster@greg0ire.fr>

alexander-schranz force-pushed the bugfix/fix-driver-check-utf8mb4 branch from bf7ecc5 to fbdd14a Compare March 3, 2022 10:08

alexander-schranz changed the base branch from 2.6.x to 2.5.x March 3, 2022 10:08

ostrolucky force-pushed the bugfix/fix-driver-check-utf8mb4 branch from 2150f08 to efa6531 Compare March 5, 2022 10:18

cleanup

ee6f1ee

ostrolucky force-pushed the bugfix/fix-driver-check-utf8mb4 branch from efa6531 to ee6f1ee Compare March 5, 2022 10:23

ostrolucky changed the title ~~Fix driver check for utf8mb4~~ Fix default MySQL charset with doctrine/dbal > 2 Mar 5, 2022

ostrolucky merged commit 1e0d1d7 into doctrine:2.5.x Mar 5, 2022

ostrolucky added this to the 2.5.7 milestone Mar 5, 2022

github-actions bot mentioned this pull request Mar 5, 2022

Merge release 2.5.7 into 2.6.x #1483

Closed

alexander-schranz deleted the bugfix/fix-driver-check-utf8mb4 branch March 5, 2022 11:52

Uh oh!

Fix default MySQL charset with doctrine/dbal > 2 #1481

Fix default MySQL charset with doctrine/dbal > 2 #1481

Uh oh!

Conversation

alexander-schranz commented Mar 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexander-schranz Mar 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

greg0ire Mar 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexander-schranz commented Mar 2, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

greg0ire commented Mar 2, 2022

Uh oh!

alexander-schranz commented Mar 2, 2022

Uh oh!

alexander-schranz commented Mar 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dmaicher commented Mar 2, 2022

Uh oh!

Uh oh!

ostrolucky commented Mar 2, 2022

Uh oh!

greg0ire commented Mar 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ostrolucky commented Mar 3, 2022

Uh oh!

alexander-schranz commented Mar 6, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

alexander-schranz commented Mar 2, 2022 •

edited

Loading

alexander-schranz Mar 2, 2022 •

edited

Loading

greg0ire Mar 2, 2022 •

edited

Loading

alexander-schranz commented Mar 2, 2022 •

edited

Loading

greg0ire commented Mar 3, 2022 •

edited

Loading