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
Comment | File | Size | Author |
---|---|---|---|
#74 | interdiff_71-74.txt | 501 bytes | kishor_kolekar |
#74 | 3089902-74.patch | 3.24 KB | kishor_kolekar |
#71 | interdiff_53_71.txt | 663 bytes | anmolgoyal74 |
#71 | 3089902-71.patch | 3.12 KB | anmolgoyal74 |
#53 | interdiff_46_52.txt | 755 bytes | anmolgoyal74 |
Issue fork drupal-3089902
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:
Comments
Comment #2
Aron NovakThe 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
Comment #3
Aron NovakComment #4
cilefen CreditAttribution: cilefen as a volunteer commentedThat 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.
Comment #5
cilefen CreditAttribution: cilefen as a volunteer commentedComment #7
jaycollett CreditAttribution: jaycollett commentedI 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.
Comment #10
hploeger CreditAttribution: hploeger commentedRunning 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
Comment #11
jmlsteele CreditAttribution: jmlsteele as a volunteer commentedThis 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...
Comment #12
jmlsteele CreditAttribution: jmlsteele as a volunteer commentedComment #13
jmlsteele CreditAttribution: jmlsteele as a volunteer commentedComment #14
daffie CreditAttribution: daffie commentedThank 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.
Comment #15
daffie CreditAttribution: daffie commented@jmlsteele: In the method Drupal\Core\Database\Driver\mysql\Connection::open() is the construction with
\PDO::ATTR_SERVER_VERSION
also used.Comment #16
jmlsteele CreditAttribution: jmlsteele as a volunteer commentedHey 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
Comment #17
jmlsteele CreditAttribution: jmlsteele as a volunteer commentedComment #18
jmlsteele CreditAttribution: jmlsteele as a volunteer commentedUpdating the issue title to better reflect the issue at hand
Comment #19
daffie CreditAttribution: daffie commentedThe 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:
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.
Nitpick: Those spaces need to be removed.
@jmlsteele: If you apply the patch, does it fix the problem for AzureDB for MySQL?
Comment #20
alexpottAnother 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.
Comment #21
alexpottAlso this feels like an upstream bug.
Comment #22
daffie CreditAttribution: daffie commentedThank you @alexpott for your input.
Lets do the by @alexpott suggested solution.
Comment #23
alexpottThis 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.
Comment #24
alexpottI'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.
Comment #25
longwaveSeems 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.
Comment #26
longwaveAt 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.
Comment #27
daffie CreditAttribution: daffie commentedAnd according to @longwave is this bug a "won't fix".
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.
Comment #28
longwave@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?
Comment #29
xjmComment #31
tuan.hmt CreditAttribution: tuan.hmt commentedThe same problem with Drupal 9.0.x, wrong Azure Mysql server version with PDO Constant, this a patch.
Comment #32
Taran2LLet's try all caps for MariaDB
Comment #33
Taran2LComment #35
Taran2LTests require updates
Comment #36
Taran2LUpdated patch with amended test
Comment #38
PasqualleComment #39
xjmCleaning up pre-9.0.0 beta targets and restoring the correct branch.
Comment #40
daffie CreditAttribution: daffie commentedThe 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.
Comment #41
Taran2Lhi @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?
Comment #42
alexpottNO_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.
Comment #43
daffie CreditAttribution: daffie commented@Taran2L: Could you create a new patch, then I will do the reviewing.
Comment #44
longwaveGreat solution, this avoids querying the database at all to find the version.
Comment #45
daffie CreditAttribution: daffie commentedWe also need to change the method
getServerVersion()
to use the SQL querySELECT VERSION()
, because the Azure database cannot handle$this->connection->getAttribute(\PDO::ATTR_SERVER_VERSION)
.Edit: See the patch from comment #36.
Comment #46
longwaveAh yes, sorry - that does fix the original issue reported here!
Comment #47
Taran2LWell, 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 ?Comment #48
daffie CreditAttribution: daffie commentedThe 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.
Comment #49
longwave> 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?
Comment #50
Taran2L@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
Comment #51
alexpottI 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
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.Comment #52
alexpottThere's no need for the local variable anymore. It only existed to add NO_AUTO_CREATE_USER as necessary.
Comment #53
anmolgoyal74 CreditAttribution: anmolgoyal74 at OpenSense Labs for DrupalFit commentedRemoved the local variable.
Comment #54
Taran2LThere is a comment just above the patched part:
I'm suggesting changing it to something like:
Comment #55
alexpott@Taran2L / re #54 let's open a follow-up to change back to
"ANSI, TRADITIONAL"
and we can fix up that comment then.Comment #56
Taran2L@alexpott Sounds reasonable, but the follow-up issue will be blocked by this one. Do you see it being committed in the nearest future?
Comment #57
alexpott@Taran2L once this gets back to rtbc I'll commit it.
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.
Comment #58
alexpottThe 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.
Comment #59
alexpottJust 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.
Comment #60
Taran2L@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 runningSELECT 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.
Comment #61
alexpott@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?
Comment #62
Taran2LWe 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
Comment #63
Taran2L@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 versionUPDATE 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)
Comment #64
alexpottYou 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.
Comment #65
Taran2L@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:
I don't think it's a big issue, right?
Comment #66
Taran2LBy 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
Comment #67
alexpott@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:
Going to discussing with other committers.
Comment #68
longwaveFWIW 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.
Comment #69
alexpottDiscussed 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.
Let's do something like:
I don't think this should be a class property. A little inline static feels okay.
Comment #70
daffie CreditAttribution: daffie commentedThe patch only needs one change. The method
Drupal/Core/Database/Driver/mysql/Connection::getServerVersion()
needs to have the following code:Comment #71
anmolgoyal74 CreditAttribution: anmolgoyal74 at OpenSense Labs for DrupalFit commentedAddressed #70
Comment #73
alexpottThis test is failing due to the static,.
You can add the
@runTestsInSeparateProcesses
annotation. See \Drupal\Tests\Core\Update\UpdateRegistryTest for an example.Comment #74
kishor_kolekar CreditAttribution: kishor_kolekar as a volunteer and at QED42 for Drupal India Association commentedAddressed #73
Comment #75
daffie CreditAttribution: daffie commentedAll points of @alexpott have been addressed and the testbot returns green.
Back to RTBC.
Comment #76
alexpottI 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!
Comment #78
alexpottFiled the follow-up to go back to a better SQL mode #3185231: Use combination SQL modes instead of explicit modes
Comment #79
alexpottDiscussed with @catch and we agreed this is good for 9.1.x - so at least this will be fixed in 9.1.0.
Comment #81
neclimdulIsn't this broken if you have connections to multiple servers which could possibly have multiple versions? Why doesn't it use an object property?
Comment #82
alexpott@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.
Comment #83
alexpottOpened #3186999: Make the version a private class property
Comment #85
odgroot CreditAttribution: odgroot commentedAnno 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