Skip to content
This repository has been archived by the owner on Feb 15, 2023. It is now read-only.

Fix incorrect use of serverVersion for MariaDB #7

Merged
merged 1 commit into from
Apr 4, 2018

Conversation

tristanbes
Copy link
Contributor

This small change fixes 2 critical things:

1/ Incorrect use of serverVersion for MariaDB, which must be prefixed with mariadb-, see: doctrine/dbal#3083 && symfony/symfony-docs#9547 (and doctrine/dbal#2985 (comment) for the discussion)

2/ doctrine/dbal now supports MariaDB >= 10.2.7. I added the .12 as a minor version, but we don't need to update it when platform.sh will use .13 or .14 etc.... as long as it's superior as 10.2.7 (see: https://github.com/doctrine/dbal/blob/f76bf5ef631cec551a86c2291fc749534febebf1/lib/Doctrine/DBAL/Driver/AbstractMySQLDriver.php#L136)

This small change fixes 2 things:

1/ Incorrect use of `serverVersion` for MariaDB, which *must* be prefixed with `mariadb-`, see: doctrine/dbal#3083 && symfony/symfony-docs#9547 (and doctrine/dbal#2985 (comment) for the discussion)

2/ `doctrine/dbal` now supports `MariaDB >= 10.2.7`. I added the `.12` as a minor version, but we don't need to update it when platform.sh will use .13 or .14 etc.... as long as it's superior as `10.2.7` (see: https://github.com/doctrine/dbal/blob/f76bf5ef631cec551a86c2291fc749534febebf1/lib/Doctrine/DBAL/Driver/AbstractMySQLDriver.php#L136)
@tristanbes
Copy link
Contributor Author

Also.
Is there a way to detect which database the user choose ? Because right now, looking at the file, if the user choose MySQL as a database, it will detect MariaDB10.2 and it's NOT a good thing.

@damz
Copy link
Contributor

damz commented Apr 4, 2018

👍 Sounds like a reasonable first step. We have a plan to expose the server version in the relationship data, we can update this mapping when we do.

@damz damz merged commit a996606 into platformsh:master Apr 4, 2018
@tristanbes
Copy link
Contributor Author

Mhhhh @damz , I don't know if this is mergeable without detecting first which platform/version is setup on the platform.sh first why ?

Because until your merge, the incorrect version would result in using MySQL57Platform (aka MySQL with 5.7 capabilities).

But now, since this patch, it will treat all connection as MariaDB 10.2.x and then if you're using MySQL it may cause problems (like running migrations or other stuff).

@damz
Copy link
Contributor

damz commented Apr 4, 2018

@tristanbes It is already documented that this bridge assumes MariaDB 10.2.

@damz
Copy link
Contributor

damz commented Apr 4, 2018

See note in README.md:

(Note: Due to a bug in Doctrine, the code currently assumes MariaDB 10.2 as the service version. If that Doctrine bug is ever resolved this hard-coding can be removed.)

@tristanbes
Copy link
Contributor Author

ok then, let's hope user will read this note until the correct version/platform gets exposed in env variable

@damz
Copy link
Contributor

damz commented Apr 4, 2018

(And we currently only support MariaDB anyway. So the only variability here would be the major version.)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants