Wrong MySQL platform is being returned by the TracingDriverForV32

[SanderVerkuil]

Environment

What version are you running? Etc.

• php: PHP 8.2.7
• symfony: 5.4
• sentry/sentry-symfony: 4.9.1
• doctrine/dbal: 3.6.3
• doctrine/orm: 2.15.2

Steps to Reproduce

1. Run bin/console doctrine:schema:update --complete --dump-sql.

Expected Result

The command should succeed

Actual Result

An exception occurred that the platform does not support the json type.

---

When building the connection, doctrine is fetching the DatabasePlatform. This calls the detectDatabasePlatform method, which chooses whether it will call the method createDatabasePlatformForVersion or the method getDatabasePlatform.

The version is fetched with the method getDatabasePlatformVersion(). If the driver is not an instance of the VersionAwarePlatformDriver interface, it always returns null, which then calls the getDatabasePlatform method, which returns the default MySqlPlatform, which does not support the json type.

It should have returned the MySQL80Platform in my case.

The fix appears to be as simple as implementing the VersionAwarePlatformDriver on the TracingDriverForV32 class, but in this comment the choice was made to not implement the deprecated interface, which makes sense.

Though I do not fully understand why the VersionAwarePlatformDriver was deprecated, as the comment itself says that All drivers will have to be aware of the server version in the next major release., which leads me to believe that as of the next major version of the doctrine/dbal, the createDatabasePlatformForVersion will be moved to the Driver interface... This makes sense, but then the TracingDriverForV32 should still implement the VersionAwarePlatformDriver right? At least until the next major version?

[cleptric]

We fixed this in #731, with version 4.9.1 being released today that contains the change. TracingDriverForV32 does contain createDatabasePlatformForVersion now, but we did not implement the interface to fix triggering deprecation warnings (See #579).

So to confirm, you already updated to 4.9.1, and the issue persists?

[cleptric]

Ok, so I understand we should also add getDatabasePlatform, correct?

[SanderVerkuil]

Yes, I've updated to 4.9.1 and the issue persists. The reason is that the Connection class calls either the createDatabasePlatformForVersion or the getDatabasePlatform method. As the version is set to null, because the driver is not an instance of the VersionAwarePlatformDriver, the method getDatabasePlatform is called, which returns the MySQLPlatform, instead of the MySQL80Platform.

[SanderVerkuil]

The TracingDriverForV3 does implement the VersionAwarePlatformDriver, which ensures that the correct method is called, but the TracingDriverForV32 is not called.

This might be an issue in the doctrine/dbal though, that the VersionAwarePlatformDriver should not have been made deprecated just yet, as it is still required for the correct functioning of the system.

The getDatabasePlatform is already present, but that returns the MySQLPlatform according the the AbstractMySQLDriver. The only correct way I see is to let the TracingDriverForV32 implement the VersionAwarePlatformDriver, which will trigger deprecations again, but I think that those deprecations just aren't correct 🤷

[ste93cry]

I tried looking at the changes that have been made in the DBAL in the next future major version (4.0) and as far as I see, the way this deprecation has been handled is a real mess:

• The VersionAwarePlatformDriver interface was marked as deprecated in Deprecate VersionAwarePlatformDriver and ServerInfoAwareConnection doctrine/dbal#4751. However, the check that ensures that the interface gets implemented was left unaltered, giving no way to avoid triggering the deprecation. I would have expected it to change to something that also takes into account the existence of the method itself 🤔
• The VersionAwarePlatformDriver interface has been removed in Remove VersionAwarePlatformDriver and ServerInfoAwareConnection doctrine/dbal#4764. However, the original method createDatabasePlatformForVersion() has been removed as well, altering the existing getDatabasePlatform() method instead and making it version-aware.

To me, there is something missing for a deprecation-free and clean upgrade path from version 3.x to version 4.0 of the DBAL. But, there is also another thing to say: this deprecation is actually not triggered by the Doctrine package; as I said for similar other deprecations, this is PHPUnit Bridge not minding its own business and triggering notices all around as soon as it sees a @deprecation tag. Maybe we should revert all the changes and just tell everyone to suppress this deprecation using the baseline.

@morozov since you are the original author of the changes in the DBAL, can you give us some hints on how you envisioned the upgrade path?

[cleptric]

Maybe we should revert all the changes and just tell everyone to suppress this deprecation using the baseline.

I would like to go this route for now. I rather have some deprecation warnings than a broken SDK. I'll get the PR ready.

[cleptric]

@ste93cry Mind copying your comment over to #579? I reopened the issue.

@SanderVerkuil This issue was fixed in 4.9.2, sorry for the trouble!

[ruudk]

Maybe we should revert all the changes and just tell everyone to suppress this deprecation using the baseline.

@ste93cry FWIW, the baseline doesn't work when you run tests in random order.

[SanderVerkuil]

@cleptric Yep, it has indeed been fixed! Thanks for the quick iteration and fast feedback ❤️

[morozov]

@ste93cry if you believe that there is an upgrade issue that might/should be addressed in the DBAL, please file an issue with doctrine/dbal and provide the relevant details there.