Problem/Motivation

Azure Database for MySQL server always reports the version as 5.7 even though it is MySQL 8. This causes problems for Drupal because of 5.7 we try to set the NO_AUTO_CREATE_USER option which does not exist on MySQL 8.

Proposed resolution

Regardless of the behaviour of Azure Database for MySQL which is highly irregular and incorrect we can still work around this issue in Drupal because it is completely unnnecessary to set NO_AUTO_CREATE_USER. Since MySQL 5.7 this has been the default behaviour and changing this behaviour via the sql mode will produce warnings - see https://dev.mysql.com/doc/refman/5.7/en/sql-mode.html#sqlmode_no_auto_cr...

Remaining tasks

User interface changes

None

API changes

None

Data model changes

None

Release notes snippet

None

CommentFileSizeAuthor
#74 interdiff_71-74.txt501 byteskishor_kolekar
#74 3089902-74.patch3.24 KBkishor_kolekar
#71 interdiff_53_71.txt663 bytesanmolgoyal74
#71 3089902-71.patch3.12 KBanmolgoyal74
#53 interdiff_46_52.txt755 bytesanmolgoyal74
#53 3089902-53.patch3.01 KBanmolgoyal74
#46 3089902-46.patch2.79 KBlongwave
#44 3089902-44.patch976 byteslongwave
#36 interdiff-32-35.txt2.46 KBTaran2L
#36 3089902-fix-azure-mysql-version_35.patch2.76 KBTaran2L
#32 3089902-fix-azure-mysql-version_32.patch1.25 KBTaran2L
#31 3089902-fix-azure-mysql-version.patch1.25 KBtuan.hmt
#16 fix_mysql8_azure-3089902-16.patch1.14 KBjmlsteele
#13 fix_mysql8_azure-3089902-11.patch930 bytesjmlsteele
#12 3089902_azure_mysql8.patch843 bytesjmlsteele

Issue fork drupal-3089902

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Aron Novak created an issue. See original summary.

Aron Novak’s picture

The remote connection was towards an Azure managed MySQL 8 server.

The local PHP version:
PHP 7.3.9-1+0~20190902.44+debian9~1.gbpf8534c (cli) (built: Sep 2 2019 13:31:30) ( NTS )
Copyright (c) 1997-2018 The PHP Group
Zend Engine v3.3.9, Copyright (c) 1998-2018 Zend Technologies
with Zend OPcache v7.3.9-1+0~20190902.44+debian9~1.gbpf8534c, Copyright (c) 1999-2018, by Zend Technologies
with blackfire v1.27.0~linux-x64-non_zts73, https://blackfire.io, by Blackfire

Aron Novak’s picture

Issue summary: View changes
cilefen’s picture

That should not happen This specific issue is discussed in #2966523: MySQL 8 Support. I suggest reading it over, commenting if necessary, and definitely re-activate this issue if necessary.

In order to get the most attention, drop a comment in #2966523: MySQL 8 Support mentioning this issue.

cilefen’s picture

Title: MySQL 8 connectivity » "SQLSTATE[42000]: Syntax error or access violation: 1231 Variable 'sql_mode' can't be set to the value of 'NO_AUTO_CREATE_USER' " with MySQL 8 on Drupal > 8.6

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.9 was released on November 6 and is the final full bugfix release for the Drupal 8.7.x series. Drupal 8.7.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.8.0 on December 4, 2019. (Drupal 8.8.0-beta1 is available for testing.)

Bug reports should be targeted against the 8.8.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.9.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

jaycollett’s picture

I can say we are running into the same issue with 8.8.1 and an Azure managed mysql 8 instance. Drupal refuses to install with the same error thrown.

hploeger’s picture

Running in the same problem also on azure managed mysql 8 instance.
I queried the server's version through multiple methods. Here are my findings.

--------------------------------------
select version();
+-----------+
| version() |
+-----------+
| 8.0.15 |
+-----------+
--------------------------------------
SHOW VARIABLES LIKE "%version%";
+--------------------------+-----------------------+
| Variable_name | Value |
+--------------------------+-----------------------+
| immediate_server_version | 999999 |
| innodb_version | 8.0.15 |
| original_server_version | 999999 |
| version | 8.0.15 |
| version_comment | Source distribution |
| version_compile_machine | x86_64 |
| version_compile_os | Win64 |
| version_compile_zlib | 1.2.11 |
+--------------------------+-----------------------+
-----------------------------------------------------------------
status;
Server version: 5.6.42.0 Source distribution
-----------------------------------------------------------------
$link=mysqli_connect(...);
echo mysqli_get_server_info($link);
5.6.42.0

So "select version();" seems to be the most reliable option.
Hope that helps

jmlsteele’s picture

This appears to be a bug caused by Azure. For whatever reason PDO::ATTR_SERVER_VERSION returns '5.6.42.0'. If instead we get the version by running an SQL query (e.g. $version = $pdo->query('SELECT version()')->fetchColumn();) we get the actual version (in my case
'8.0.15')

It's a known limitation specifically with Azure: https://docs.microsoft.com/en-us/azure/mysql/concepts-limits#current-kno...

daffie’s picture

Status: Postponed (maintainer needs more info) » Active
Related issues: +#3109534: Raise the minimum MySQL version to 5.7.8 and MariaDB version to 10.2.7 in Drupal 9

Thank everybody for reporting this issue and helping the root cause of the problem.

For the upcoming Drupal 9.0 we are working testing that the MySQL and MariaDB versions are equal or higher then the minimum version that is required for Drupal 9.0. I have asked over there if the problem from this issue can be solved in that issue. See: #3109534-58: Raise the minimum MySQL version to 5.7.8 and MariaDB version to 10.2.7 in Drupal 9.

daffie’s picture

Status: Active » Needs work

@jmlsteele: In the method Drupal\Core\Database\Driver\mysql\Connection::open() is the construction with \PDO::ATTR_SERVER_VERSION also used.

jmlsteele’s picture

Hey daffie,
I've done a search in the entire 8.8.x-dev codebase and the only other reference to ATTR_SERVER_VERSION is in Drupal\Core\Database\Connection::version(). Since it's generic, we can't change it there, so I've added an additional method override to Drupal\Core\Database\Driver\mysql\Connection

Patch attached

jmlsteele’s picture

jmlsteele’s picture

Title: "SQLSTATE[42000]: Syntax error or access violation: 1231 Variable 'sql_mode' can't be set to the value of 'NO_AUTO_CREATE_USER' " with MySQL 8 on Drupal > 8.6 » "Azure Database for MySQL server" reports wrong database version
Status: Needs work » Needs review
Issue tags: +Azure

Updating the issue title to better reflect the issue at hand

daffie’s picture

Status: Needs review » Needs work
  1. +++ b/core/lib/Drupal/Core/Database/Driver/mysql/Connection.php
    @@ -451,7 +451,7 @@ public static function open(array &$connection_options = []) {
    -    $version_server = $pdo->getAttribute(\PDO::ATTR_SERVER_VERSION);
    +    $version_server = $pdo->query('select version()')->fetchColumn();
    

    The question that I have is running the code: $pdo->getAttribute(\PDO::ATTR_SERVER_VERSION) answered by the PDO layer or is it a roundtrip to the database. AFAIK it is answered by the PDO layer. If we then changing it to $pdo->query('select version()')->fetchColumn() will make much slower. Having such a code change in Drupal\Core\Database\Driver\mysql\Connection::open() which is run on every request and for the most popular database, is not very acceptable.
    I see 2 possible solutions:

    1. The solution from the current patch get committed to core. Which make all Drupal requests for MySQL slower.
    2. AzureDB for MySQL gets its own custom database driver.
  2. +++ b/core/lib/Drupal/Core/Database/Driver/mysql/Connection.php
    @@ -663,6 +663,13 @@ protected function popCommittableTransactions() {
    +    return $this->query('select version()')->fetchColumn();
    

    Can we store the result of this query in a static variable and use it so that we only have to run the query once.

  3. +++ b/core/lib/Drupal/Core/Database/Driver/mysql/Connection.php
    @@ -663,6 +663,13 @@ protected function popCommittableTransactions() {
    +  ¶
    

    Nitpick: Those spaces need to be removed.

@jmlsteele: If you apply the patch, does it fix the problem for AzureDB for MySQL?

alexpott’s picture

Another option would be allow connection options to override the server version. It seems that messing with the server version is something a lot of MySQl like providers do. And we could put a warning on the requirements page if the server version from the query doesn't match the provider version.

alexpott’s picture

Also this feels like an upstream bug.

daffie’s picture

Thank you @alexpott for your input.

Lets do the by @alexpott suggested solution.

alexpott’s picture

Also this feels like an upstream bug.

This really does feel like an upstream bug. Either in PDO or Azure - it's not Drupal's fault that the information in \PDO::ATTR_SERVER_VERSION is incorrect. Working around upstream bugs tends to tie us in knots that takes years to untie so it's be great if the error could be reported upstream to either PHP or Microsoft. I'm guessing (and it is only a guess) that problem is in the facade that Microsoft are using to present a MySQL db to the world and for some reason there MySQL 8 version is reporting the wrong version.

I've spent 5 minutes looking and I have no idea where to find the Azure database bug tracker - but someone who has an account and is using Azure Databases should raise a ticket.

alexpott’s picture

I've checked the PHP source code to see if it was doing anything funky with the MySQL version string but it does not appear to be. So, as above, I recommend starting with Microsoft.

longwave’s picture

Seems like this is a "won't fix" on their side if https://social.msdn.microsoft.com/Forums/en-US/795cb2be-dce7-4805-b324-a... is anything to go by, looks like they have a proxy service in front that cannot know the correct version ahead of time.

The only way to try and get this fixed seems to be upvoting https://feedback.azure.com/forums/597982-azure-database-for-mysql/sugges... but as the original report is over two years old and nothing has changed, I suspect this won't get done very quickly.

longwave’s picture

At a push we could use a similar version/host sniffing technique to https://github.com/mysql-net/MySqlConnector/blob/148c5ea7d62f95bcd5e1202... but it seems quite ugly and prone to breakage in the future.

daffie’s picture

Also this feels like an upstream bug.

And according to @longwave is this bug a "won't fix".

Working around upstream bugs tends to tie us in knots that takes years to untie

That sounds to me as something we should not do in core. Lets move it to contrib. We are hopefully doing a contrib database driver for PostgreSQL with a DrupalCI testbot. If we have that, then doing a contrib database driver for AzureDB is not so difficult. When AzureDB fixes the bug the users can then use the by core supported driver again.

longwave’s picture

@alexpott's comment in #20 seems to be the cleanest way of dealing with this in core, if we support a 'version' key in the database connection options that overrides the server-supplied version, and we can add a note somewhere saying to set this for Azure?

xjm’s picture

Issue tags: +beta target

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.7 was released on June 3, 2020 and is the final full bugfix release for the Drupal 8.8.x series. Drupal 8.8.x will not receive any further development aside from security fixes. Sites should prepare to update to Drupal 8.9.0 or Drupal 9.0.0 for ongoing support.

Bug reports should be targeted against the 8.9.x-dev branch from now on, and new development or disruptive changes should be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

tuan.hmt’s picture

The same problem with Drupal 9.0.x, wrong Azure Mysql server version with PDO Constant, this a patch.

Taran2L’s picture

Taran2L’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 32: 3089902-fix-azure-mysql-version_32.patch, failed testing. View results

Taran2L’s picture

Tests require updates

Taran2L’s picture

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Pasqualle’s picture

Version: 9.2.x-dev » 9.1.x-dev
xjm’s picture

Version: 9.1.x-dev » 9.2.x-dev
Issue tags: -beta target

Cleaning up pre-9.0.0 beta targets and restoring the correct branch.

daffie’s picture

Status: Needs review » Needs work

The problem is that at the moment the call to $pdo->getAttribute(\PDO::ATTR_SERVER_VERSION); is being run every time a database connection to MySQL is opened. When we change it to $pdo->query('SELECT VERSION()')->fetchColumn(); then we will add an extra database query call every time a database connection is opened. This is not acceptable.
We do this because we do not want to add the option "NO_AUTO_CREATE_USER" to the SQL modes when we are connection to a MySQL database 8 or higher.
Only what does the SQL mode option "NO_AUTO_CREATE_USER"? It prevents the creation of a new user with the GRANT statement. That sounds great, only to me that is something that is beyond the responsibility of a Drupal website. Therefore for me the SQL mode option "NO_AUTO_CREATE_USER" can be removed. As a result of that, getting the database version when a new database connection is opened is no longer necessary. For more info about the SQL mode option "NO_AUTO_CREATE_USER", see: https://dev.mysql.com/doc/refman/5.7/en/sql-mode.html#sqlmode_no_auto_cr...
The function getServerVersion() is only used when installing a Drupal website and is therefore not in the critical path. Having an extra database call when a new Drupal website is created is acceptable for me.

For these changes is a new patch needed.

Taran2L’s picture

hi @daffie, thanks for your feedback.

I've checked MySQL docs and seems like 5.7 (which is the minimum supported by D9) already sets NO_AUTO_CREATE_USER by default: https://dev.mysql.com/doc/refman/5.7/en/server-system-variables.html#sys...

So, maybe we can remove setting it explicitly?

alexpott’s picture

NO_AUTO_CREATE_USER was added in #1136854: MySQL 5.5 breaks speedy testing - it was part of splitting out TRADITIONAL to each individual option. We made the change because of performance on MyISAM - but we no longer use MyISAM :) - TRADITIONAL was added in #344575: Force MySQL to run in ANSI compatability mode..

@daffie nice sleuthing in #40.

I think @Taran2L's idea of removing this is great.

daffie’s picture

@Taran2L: Could you create a new patch, then I will do the reviewing.

longwave’s picture

Status: Needs work » Needs review
FileSize
976 bytes

Great solution, this avoids querying the database at all to find the version.

daffie’s picture

Status: Needs review » Needs work

We also need to change the method getServerVersion() to use the SQL query SELECT VERSION(), because the Azure database cannot handle $this->connection->getAttribute(\PDO::ATTR_SERVER_VERSION).

Edit: See the patch from comment #36.

longwave’s picture

Ah yes, sorry - that does fix the original issue reported here!

Taran2L’s picture

Well, I gave it another round of thinking, and seems like the latest patch is disabling NO_AUTO_CREATE_USER for MySQL 5.7 and 8 before 8.0.11, no ?

daffie’s picture

Status: Needs review » Reviewed & tested by the community

The patch looks good to me.
Testing has changed, so that it is testing the fix.
@alexpott likes this solution!
We should not be using NO_AUTO_CREATE_USER. It is not part of what Drupal is about.
For me it is RTBC.

longwave’s picture

> Well, I gave it another round of thinking, and seems like the latest patch is disabling NO_AUTO_CREATE_USER for MySQL 5.7 and 8 before 8.0.11, no ?

NO_AUTO_CREATE_USER only affects GRANT statements, and in normal usage, Drupal should never execute a GRANT statement?

Taran2L’s picture

@longwave yes, you are right. Drupal core does not need GRANT OPTION privilege, this is stated for example here https://git.drupalcode.org/project/drupal/-/blob/9.2.x/core/INSTALL.mysq...

RTBC +1

alexpott’s picture

Issue summary: View changes

I fixed the issue summary.

Also for all MySQL versions supported by Drupal 8 & 9 the NO_AUTO_CREATE_USER is the default behaviour and you have to go out of your way to change it - see https://dev.mysql.com/doc/refman/5.7/en/sql-mode.html#sqlmode_no_auto_cr... - specifically

Assignments to sql_mode that change the NO_AUTO_CREATE_USER mode state produce a warning

I also think it is worth a follow-up to discuss changing the mode back to "ANSI, TRADITIONAL" the MyISAM considerations of #1136854: MySQL 5.5 breaks speedy testing feel completely moot at this point.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/lib/Drupal/Core/Database/Driver/mysql/Connection.php
@@ -198,12 +198,6 @@ public static function open(array &$connection_options = []) {
     $sql_mode = 'ANSI,STRICT_TRANS_TABLES,STRICT_ALL_TABLES,NO_ZERO_IN_DATE,NO_ZERO_DATE,ERROR_FOR_DIVISION_BY_ZERO,ONLY_FULL_GROUP_BY';
-    // NO_AUTO_CREATE_USER is removed in MySQL 8.0.11
-    // https://dev.mysql.com/doc/relnotes/mysql/8.0/en/news-8-0-11.html#mysqld-8-0-11-deprecation-removal
-    $version_server = $pdo->getAttribute(\PDO::ATTR_SERVER_VERSION);
-    if (version_compare($version_server, '8.0.11', '<')) {
-      $sql_mode .= ',NO_AUTO_CREATE_USER';
-    }
     $connection_options['init_commands'] += [
       'sql_mode' => "SET sql_mode = '$sql_mode'",
     ];

There's no need for the local variable anymore. It only existed to add NO_AUTO_CREATE_USER as necessary.

anmolgoyal74’s picture

Status: Needs work » Needs review
FileSize
3.01 KB
755 bytes

Removed the local variable.

Taran2L’s picture

There is a comment just above the patched part:

    // Set MySQL init_commands if not already defined.  Default Drupal's MySQL
    // behavior to conform more closely to SQL standards.  This allows Drupal
    // to run almost seamlessly on many different kinds of database systems.
    // These settings force MySQL to behave the same as postgresql, or sqlite
    // in regards to syntax interpretation and invalid data handling.  See
    // https://www.drupal.org/node/344575 for further discussion. Also, as MySQL
    // 5.5 changed the meaning of TRADITIONAL we need to spell out the modes one
    // by one.

I'm suggesting changing it to something like:

    // Set MySQL init_commands if not already defined. Default Drupal's MySQL
    // behavior to conform more closely to SQL standards. This allows Drupal
    // to run almost seamlessly on many different kinds of database systems.
    // These settings force MySQL to behave the same as PostgreSQL, or SQLite
    // in regards to syntax interpretation and invalid data handling.
alexpott’s picture

@Taran2L / re #54 let's open a follow-up to change back to "ANSI, TRADITIONAL" and we can fix up that comment then.

Taran2L’s picture

@alexpott Sounds reasonable, but the follow-up issue will be blocked by this one. Do you see it being committed in the nearest future?

alexpott’s picture

Status: Needs review » Needs work

@Taran2L once this gets back to rtbc I'll commit it.

+++ b/core/lib/Drupal/Core/Database/Driver/mysql/Connection.php
@@ -297,7 +290,7 @@ protected function getMariaDbVersionMatch(): ?string {
-    return $this->connection->getAttribute(\PDO::ATTR_SERVER_VERSION);
+    return $this->connection->query('SELECT VERSION()')->fetchColumn();

This change is not necessary to affect a fix for Azure so I suggest we revert it. Which makes the changes to ConnectionTest unnecessary.

Sorry for the back and forth on this issue it's just taken a while to get to the correct solution.

alexpott’s picture

+++ b/core/lib/Drupal/Core/Database/Driver/mysql/Connection.php
@@ -297,7 +290,7 @@ protected function getMariaDbVersionMatch(): ?string {
   protected function getServerVersion(): string {
-    return $this->connection->getAttribute(\PDO::ATTR_SERVER_VERSION);
+    return $this->connection->query('SELECT VERSION()')->fetchColumn();
   }

The reason I think we should revert this is that it causes quite a few extra queries due mariadb checking. So it someone was using version() in something other than the installer we might have some issues.

alexpott’s picture

Just checked MariaDB - https://mariadb.com/kb/en/sql-mode/#no_auto_create_user - it's been the default for ages so removing this for Maria is fine too.

Taran2L’s picture

@alexpott, well I'm not sure I follow.

This issue is about how certain cloud/host providers (like Azure) set up their DB instances so they are misreporting own version in \PDO::ATTR_SERVER_VERSION, but the correct version can be retrieved by running SELECT VERSION() query.
The issue is that the incorrectly reported version prevents D9 from installing in such a scenario. Patch in #53 does fix this.

This issue is not about the SQL mode. The follow-up one would be. The reason we are editing an SQL mode here is described in #40 by @daffie: currently Drupal checks the MySQL version each time when connection is being established. The proposed patch in #36 adds an extra query for every single page view, which is not acceptable. So we ended up with a patch that does not do that in #53

However, it does not make sense to fix this here without a follow-up fixing the SQL mode in general. Thus, it might be a good idea to commit a change to the version retrieval (like #36) without any changes to the SQL mode here, and then commit SQL mode change as a separate issue. Just for the sake of consistency.

alexpott’s picture

@Taran2L so does having NO_AUTO_CREATE_USER in the sql mode break on Azure Database for MySQL? Or is like is the only issue an incorrect version on system status report?

Taran2L’s picture

We are running D9 project in Azure using Azure Database for MySQL server.

Azure Portal UI reports: MySQL version: 5.7
\PDO::ATTR_SERVER_VERSION reports: 5.6.47.0
SELECT_VERSION() reports: 5.7.29-log

Taran2L’s picture

@alexpott it's an incorrect version of the system on the status report page (which is not the critical issue) and it's also not possible to install a D9 there at all (as well as run any hook updates), because system_requirements() and installer both check the DB version

UPDATE 1: I believe NO_AUTO_CREATE_USER is not related to the issue at all. It just an update to the MySQL version detection that is more reliable (and fixes Azure)

UPDATE 2: This is a known bug of the Azure platform: https://social.msdn.microsoft.com/Forums/en-US/795cb2be-dce7-4805-b324-a...

PHP's PDO MySQL driver uses handshake information to fill the PDO attributes (I've checked the PHP source)

alexpott’s picture

You have to wonder what Azure are going to do when it 5.6 is EOL'd - https://blog.cpanel.com/mysql-5-6-and-mariadb-10-1-end-of-life/#:~:text=....

I really really think Azure needs to do some upstream work reporting a version that many applications latest versions do not support is not a fault of the applications. And maybe we should go back to #27 and have a contrib driver for Azure. It can extend from the core driver and override the version getter.

Taran2L’s picture

@alexpott, Azure is not a Database Engine type, rather a service. Also, writing a contrib module that has 3 lines of code? Going the SELECT VERSION() path does not affect any other MySQL service providers. This is a part of the API:

Yes, the patch adds an extra query (versus reading the handshake information, that is being read anyway). But this query is only executed:

  1. Once before the installation to verify everything is fine with the database
  2. Each time before running the update
  3. Each time when checking the status report

I don't think it's a big issue, right?

Taran2L’s picture

By the way, it's not only Azure: see the #3156078: Proxysql with Mysql8 support. The very same issue: handshake info tells one thing, the reality is different

I guess I'm out of the comments and just leave it to @alexpott to digest and think of the solution

Thanks

alexpott’s picture

@Taran2L you are correct for core this is only being read during those situations. But contrib and custom code it's unknowable. For contrib we can do http://grep.xnddx.ru/search?text=-%3Eversion%28%29&filename= and we see some runtime usage.

I'm torn between making the change in #53 or doing something like what was done in laravel. I.e. something like:

  • We add a new version key to the connection options
  • In the installer allow this to be set. The field should have an explanation about proxies / azure etc.

Going to discussing with other committers.

longwave’s picture

FWIW none of those contrib cases look like they will be heavily used, a lot of them are false positives for unrelated methods.

I also can't see SELECT VERSION(); ever being a slow query, we're simply asking the server to return a constant.

I dislike the idea of allowing an override to be set in the installer, when we can actually query the database for the real version. There is the chance of users entering incorrect values in here and then breaking something else further down the line when the assumed version is wrong.

alexpott’s picture

Discussed with @catch. We agreed to proceed with the query here but use a static variable to prevent duplicate queries. FWIW with the change in #53 every call to ->version() will do the query twice because of MariaDB checking.

+++ b/core/lib/Drupal/Core/Database/Driver/mysql/Connection.php
@@ -297,7 +290,7 @@ protected function getMariaDbVersionMatch(): ?string {
   protected function getServerVersion(): string {
-    return $this->connection->getAttribute(\PDO::ATTR_SERVER_VERSION);
+    return $this->connection->query('SELECT VERSION()')->fetchColumn();
   }

Let's do something like:

  protected function getServerVersion(): string {
    static $server_version;
    if (!$server_version) {
      $server_version = $this->connection->query('SELECT VERSION()')->fetchColumn();
    }
    return $server_version;
  }

I don't think this should be a class property. A little inline static feels okay.

daffie’s picture

Issue tags: +Novice

The patch only needs one change. The method Drupal/Core/Database/Driver/mysql/Connection::getServerVersion() needs to have the following code:

  protected function getServerVersion(): string {
    static $server_version;
    if (!$server_version) {
      $server_version = $this->connection->query('SELECT VERSION()')->fetchColumn();
    }
    return $server_version;
  }
anmolgoyal74’s picture

Status: Needs work » Needs review
FileSize
3.12 KB
663 bytes

Addressed #70

Status: Needs review » Needs work

The last submitted patch, 71: 3089902-71.patch, failed testing. View results

alexpott’s picture

+++ b/core/tests/Drupal/Tests/Core/Database/Driver/mysql/ConnectionTest.php
@@ -13,6 +13,13 @@
  */
 class ConnectionTest extends UnitTestCase {

This test is failing due to the static,.

You can add the @runTestsInSeparateProcesses annotation. See \Drupal\Tests\Core\Update\UpdateRegistryTest for an example.

kishor_kolekar’s picture

Status: Needs work » Needs review
FileSize
3.24 KB
501 bytes

Addressed #73

daffie’s picture

Status: Needs review » Reviewed & tested by the community

All points of @alexpott have been addressed and the testbot returns green.
Back to RTBC.

alexpott’s picture

Title: "Azure Database for MySQL server" reports wrong database version » [backport] "Azure Database for MySQL server" reports wrong database version
Version: 9.2.x-dev » 9.1.x-dev

I think we should consider backporting this to 9.0.x at least and have a discussion about 8.9.x. 8.9.x is more complex because it supports earlier versions of MySQL.

Committed 4391921 and pushed to 9.2.x. Thanks!

  • alexpott committed 4391921 on 9.2.x
    Issue #3089902 by Taran2L, jmlsteele, anmolgoyal74, longwave,...
alexpott’s picture

Filed the follow-up to go back to a better SQL mode #3185231: Use combination SQL modes instead of explicit modes

alexpott’s picture

Version: 9.1.x-dev » 9.0.x-dev

Discussed with @catch and we agreed this is good for 9.1.x - so at least this will be fixed in 9.1.0.

  • alexpott committed d69366f on 9.1.x
    Issue #3089902 by Taran2L, jmlsteele, anmolgoyal74, longwave,...
neclimdul’s picture

+++ b/core/lib/Drupal/Core/Database/Driver/mysql/Connection.php
@@ -297,7 +290,11 @@ protected function getMariaDbVersionMatch(): ?string {
-    return $this->connection->getAttribute(\PDO::ATTR_SERVER_VERSION);
+    static $server_version;
+    if (!$server_version) {
+      $server_version = $this->connection->query('SELECT VERSION()')->fetchColumn();
+    }
+    return $server_version;

Isn't this broken if you have connections to multiple servers which could possibly have multiple versions? Why doesn't it use an object property?

alexpott’s picture

Title: [backport] "Azure Database for MySQL server" reports wrong database version » "Azure Database for MySQL server" reports wrong database version
Version: 9.0.x-dev » 9.1.x-dev
Status: Reviewed & tested by the community » Fixed

@neclimdul if you've got backends on multiple sql versions you're out of the purview of core.

This doesn't use a class property because we don't want anyone to really use this of this they should. This should never change under the hood of a request. This is fudge because the proxy SQLs servers are doing the wrong thing and not setting the backend server version correctly for PDO. It's a work around and a fudge because Microsoft and other vendors are going to change.

Ah I guess you're asking about if you have a multiple databases defined and they are on multiple versions - like for a migration. Good point. It sucks so much that we can't just use the PDO property and be done. Let's file a follow-up and add the class property but as a private - nothing outside of this class should be using it. Not even a class that extends this one.

Also this won't make it to 9.0.x so we can mark this as fixed.

alexpott’s picture

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

odgroot’s picture

Anno 2021, we still ran into this issue.
Note that there is a workaround, namely to use port 3308/3309 instead of 3306.
Source and explanation:
https://docs.microsoft.com/en-us/azure/mysql/concepts-supported-versions