MySQL 8 Support

@blakemorgan: Please open and link an issue in https://www.drupal.org/project/drupalci to add MySQL 8 testing.

Any chances for backport 'NO_AUTO_CREATE_USER' removal to Drupal 8.5?

Added two more things to the patch so that KernelTests may pass:

• Changed SELECT table_name FROM information_schema.tables WHERE to SELECT table_name as table_name FROM information_schema.tables WHERE
• Changed SELECT column_comment FROM information_schema.columns WHERE to SELECT column_comment as column_comment FROM information_schema.columns WHERE
• Changed SELECT table_comment FROM information_schema.tables WHERE to SELECT table_comment as table_comment FROM information_schema.tables WHERE

@almaudoh any reason for this "as" I can't find anything about it in https://dev.mysql.com/doc/relnotes/mysql/8.0/en/news-8-0-0.html

@almaudoh any reason for this "as" I can't find anything about it in https://dev.mysql.com/doc/relnotes/mysql/8.0/en/news-8-0-0.html

For queries on INFORMATION_SCHEMA tables, comparisons of schema and table names could be case sensitive or insensitive, depending on the characteristics of the underlying file system and the lower_case_table_names system variable value. Furthermore, it was ineffective to provide a COLLATE clause to change the comparison properties because that clause was ignored. This has been changed so that COLLATE is no longer ignored and can be used to obtain the desired comparison properties. (Bug #11748044, Bug #34921)

So for the $results = $this->connection->query("SELECT table_name as table_name FROM information_schema.tables WHERE " . (string) $condition, $condition->arguments()); $results stdobject will not have table_name property, but rather will have TABLE_NAME so the test for $table->table_name will always fail.

I wanted to better understand NO_AUTO_CREATE_USER and what the effect of removing it would be.

Prevent the GRANT statement from automatically creating new user accounts if it would otherwise do so, unless authentication information is specified. The statement must specify a nonempty password using IDENTIFIED BY or an authentication plugin using IDENTIFIED WITH.

It was added in MySQL 5.5, and both enabled by default and deprecated in MySQL 5.7.

Doing a quick search through Drupal core I did not find any GRANT statements, so I don't think this change should affect Drupal.

Unless MySQL 8 is going to be the minimum requirement for D8.6, I object to unconditionally removing NO_AUTO_CREATE_USER from sql-mode. Removing it for MySQL 5.7 and below should have its own issue.

I added the version check conditional from the D7 patch in order to only remove NO_AUTO_CREATE_USER when connecting to MySQL 8+

We have a mysql 8 test bot. I've run the tests against #15. Looks like we have a long way to go :(

+++ b/core/lib/Drupal/Core/Database/Driver/mysql/Connection.php
@@ -179,9 +179,21 @@ public static function open(array &$connection_options = []) {
-    $connection_options['init_commands'] += [
-      'sql_mode' => "SET sql_mode = 'ANSI,STRICT_TRANS_TABLES,STRICT_ALL_TABLES,NO_ZERO_IN_DATE,NO_ZERO_DATE,ERROR_FOR_DIVISION_BY_ZERO,NO_AUTO_CREATE_USER,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', '>=')) {
+      $connection_options['init_commands'] += array(
+        'sql_mode' => "SET 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'",
+      );
+    }
+    else {
+      $connection_options['init_commands'] += [
+        'sql_mode' => "SET sql_mode = 'ANSI,STRICT_TRANS_TABLES,STRICT_ALL_TABLES,NO_ZERO_IN_DATE,NO_ZERO_DATE,ERROR_FOR_DIVISION_BY_ZERO,NO_AUTO_CREATE_USER,ONLY_FULL_GROUP_BY'",
+      ];
+    }

This big condition is quite hard to read, can we remove NO_AUTO_CREATE_USER from the initial list of sql modes and only add it conditionally if the mysql version is <8.0.11?

Just added a way to test using docker so people don't wreck their local environment (like I did :) )

The problem with ChainedFastBackendTest is caused by \Drupal\Core\Cache\DatabaseBackend::schemaDefinition()

        'cid' => [
          'description' => 'Primary Key: Unique cache ID.',
          'type' => 'varchar_ascii',
          'length' => 255,
          'not null' => TRUE,
          'default' => '',
          'binary' => TRUE,
        ],

If you comment out 'binary' => TRUE, tests against mysql 5.7 fail in exactly the same way.

So I guess how you define binary strings has changed.

On MYSQL 8 the create column definition looks like:

`cid` varchar(255) CHARACTER SET ascii COLLATE ascii_general_ci NOT NULL DEFAULT '' COMMENT 'Primary Key: Unique cache ID.',

And mysql 5.7 it looks like:

`cid` varchar(255) CHARACTER SET ascii COLLATE ascii_bin NOT NULL DEFAULT '' COMMENT 'Primary Key: Unique cache ID.',

With the patch attached it looks like this:

`cid` varchar(255) CHARACTER SET ascii COLLATE ascii_bin NOT NULL DEFAULT '' COMMENT 'Primary Key: Unique cache ID.',

It looks like in HEAD the instructions to be BINARY and COLLATE ascii_general_ci are contradicting and on MySQL 8 it takes the last and on 5.7 it takes the first. That was fun to work out.

A little bit more progress.

+++ b/core/lib/Drupal/Core/Database/Driver/mysql/Insert.php
@@ -44,6 +44,12 @@ public function __toString() {
+    // Some queries use reserved words like 'function' and these need to be
+    // escaped.
+    $insert_fields = array_map(function ($f) {
+      return '`' . $f . '`';
+    }, $insert_fields);
+

I don't like this fix. It's because of the simpletest table which has a column called function.

I think we might consider renaming the column.

Are we going to provide the upgrade path for existing site moving from MySQL 5.5/5.6/5.7 to 8? Is it possible to migrate from one MySQL version to another in these cases?

Last fix we can do. Now we need @mixologic to fix the new MySQL 8 container to have enough disk space since it appears that we run out half way through functional tests.

re #24 - fortunately upgrading databases is not our job. And yes MySQL provides an upgrade path from MySQL 5.7 to MySQL 8. We just need to be able to support MySQL 5.5, 5.6, 5.7 and 8.

One thought I have is that

+++ b/core/lib/Drupal/Core/Database/Driver/mysql/Connection.php
@@ -179,9 +179,18 @@ public static function open(array &$connection_options = []) {
+    $version_server = $pdo->getAttribute(\PDO::ATTR_SERVER_VERSION);
+    if (version_compare($version_server, '8.0.11', '<')) {

this check will be interesting for MariaDB which is already version 10.

So the latest MariaDB version string is 5.5.5-10.3.8-MariaDB-1:10.3.8+maria~jessie :D

So when doing version_compare($version_server, '8.0.11', '<') on that it'll use the 5.5.5 so all is fine there. The NO_AUTO_CREATE_USER will still be added. That said I'm not looking forward to the day the MySQL and MariaDB diverge properly.

Atm we can't increase

  /**
   * {@inheritdoc}
   */
  public function minimumVersion() {
    return '5.5.3';
  }

past 5.5.5 because that'd drop MariaDB support completely. lolz.

I've added #2985530: Add documentation note about MySQL minimum version and MariaDB to document the version string thing so we don't drop support for MariaDB by mistake.

Let's see if it is the binary logs that are causing the disk space problem.

So the latest MariaDB version string is 5.5.5-10.3.8-MariaDB-1:10.3.8+maria~jessie

Well that's a shame. According to https://mariadb.com/kb/en/library/mariadb-vs-mysql-compatibility/, MariaDB 10.2 and 10.3 are said to be more similar to MySQL 5.7 than 5.5.

Atm we can't increase past 5.5.5 because that'd drop MariaDB support completely.

In #2983639-4: [PP-1] Figure out a way to not join the workspace_association table for every duplicate base table join of an entity query, I pointed out that MySQL 5.5 is EOL in 5 months. And causing us problems in that issue. So figuring out if we want to and how we can raise MySQL requirements to 5.6 while still supporting MariaDB will soon become timely, but could be done in a separate issue.

Re-uploading because 22/25 hit one of those obscure drupal.org bugs that come up when multiple tabs and old forms conflict with things.

Looks like we have a random fail in Drupal\Tests\views\Kernel\Entity\RowEntityRenderersTest see https://www.drupal.org/pift-ci-job/1017101

out of curiosity, trying that one test many bunches of times.

Thinking some more about the new MySQL reserved words like function. I think we need to make \Drupal\Core\Database\Driver\mysql\Connection::escapeField() work more like \Drupal\Core\Database\Driver\pgsql\Connection::escapeField(). The patch attached will escape fields and aliases as necessary.

Let's see what breaks.

Turns out statically caching the quoted version is a bad idea because $this->escapedNames on the Connection class has both field and table names together.

I did that because originally the MySQL escapeField() did a more Postgres like escapeField() - using regular expressions etc... but I don't think that that is necessary. Interestingly I think the Postgres escaping could be simplified hugely by taking a similar approach and always escaping. There's not much point in checking for a reserved word list or case as it does now - it could just always quote and be done. Less complexity and more reliability.

Ah we also escape field names that are empty strings :)

1. +++ b/core/lib/Drupal/Core/Command/DbDumpCommand.php
   @@ -260,7 +260,7 @@ protected function getTableCollation(Connection $connection, $table, &$definitio
   -    $definition['mysql_character_set'] = str_replace('_general_ci', '', $data['Collation']);
   +    $definition['mysql_character_set'] = str_replace(['_general_ci', '_0900_ai_ci'], '', $data['Collation']);
   

   The previous code was not really correct, because if someone had a different collation than utf(mb4)_general_ci, the resulting character set would've been wrong.

   I suggest we do an explode() on _ and use the first element of the array.

2. +++ b/core/lib/Drupal/Core/Database/Driver/mysql/Connection.php
   @@ -179,9 +179,18 @@ public static function open(array &$connection_options = []) {
   +    $version_server = $pdo->getAttribute(\PDO::ATTR_SERVER_VERSION);
   

   Why not use $this->version()?

3. +++ b/core/lib/Drupal/Core/Database/Driver/mysql/Insert.php
   @@ -44,6 +44,10 @@ public function __toString() {
   +    $insert_fields = array_map(function ($f) {
   +      return $this->connection->escapeField($f);
   +    }, $insert_fields);
   

   We need to do the same in \Drupal\Core\Database\Driver\mysql\Upsert.

   Also, is there any reason for using $f instead of $field?

4. +++ b/core/tests/Drupal/KernelTests/Core/Database/InsertTest.php
   @@ -198,14 +198,20 @@ public function testInsertSelectAll() {
   +    $records = $result->fetch();
   

   Why use plural here when there is only one record?

Also, there is a comment in \Drupal\Core\Database\Driver\mysql\Connection::open() which is no longer accurate:

// Force MySQL to use the UTF-8 character set. Also set the collation, if a
// certain one has been set; otherwise, MySQL defaults to
// 'utf8mb4_general_ci' for utf8mb4.

Thanks for the review @amateescu.

1. Fixed.
2. Because the method we're in is static.
3. Yep - $f is copy&paste
4. Fixed

And addressed the comments too.

Just somethings I noticed whilst fixing the test failures in #36 with the patch in #40.

1. +++ b/core/lib/Drupal/Core/Entity/Query/Sql/Tables.php
   @@ -219,7 +219,7 @@ public function addField($field, $type, $langcode) {
   -              $this->sqlQuery->condition('1 <> 1');
   +              $this->sqlQuery->where('1 <> 1');
   

   In HEAD this does something very funny. It add a condition to the query like 11 = "" which as it turns out has the same effect as doing 1 <> 1.

2. +++ b/core/modules/user/src/Plugin/EntityReferenceSelection/UserSelection.php
   @@ -242,7 +242,7 @@ public function entityQueryAlter(SelectInterface $query) {
   -            ->where(str_replace('anonymous_name', ':anonymous_name', (string) $value_part), $value_part->arguments() + [':anonymous_name' => \Drupal::config('user.settings')->get('anonymous')])
   +            ->where(str_replace($query->escapeField('anonymous_name'), ':anonymous_name', (string) $value_part), $value_part->arguments() + [':anonymous_name' => \Drupal::config('user.settings')->get('anonymous')])
   

   This change should make this funkiness much more robust with alternate DB drivers.

$this->sqlQuery->where('1 <> 1');

I usually like to write "always false" queries with 1 = 0, I think it's a bit more easy to read, but I don't feel that strongly about it if you prefer 1 <> 1 :)

@amateescu I don't feel strongly about 1 = 0 or 1 <> 1 - i just left it as 1 <> 1 because that was the way it was written.

So the big question for me is do we want the extra strictness of properly quoting column and aliases on MySQL.

On one hand we get better compatibility across MySQL 8 and 5.x (and probably MariaDB) since we no longer need to fear any mysql-compatible implementation adding new reserved words like function. We also get stricter adherence to our own API (for example, the 1 <> 1 thing above) and our code will probably work better with contrib / custom drivers because we have to call the escaping functions instead of making assumptions based on MySQL's norms.

On the other hand it is highly likely that these changes will have an impact on contrib and custom code. Any test that makes assertions against the string version of a query is likely to break (not that this is a good test) and some code that works today like ->condition('1 <> 1') will break (however as stated above this is not really using the API as intended).

The middle ground would be add a dictionary of reserved words for MySQL to quote if the field or alias matches. This would preserve API oddities like ->condition('1 <> 1') but also allow us to deal with column names like function. What we'd lose from this approach is the stricter usage of core API which forces code like \Drupal\user\Plugin\EntityReferenceSelection\UserSelection::entityQueryAlter to escape fields as expected.

On the other hand it is highly likely that these changes will have an impact on contrib and custom code. Any test that makes assertions against the string version of a query is likely to break (not that this is a good test) and some code that works today like ->condition('1 <> 1') will break (however as stated above this is not really using the API as intended).

These two potential breakages seem OK in a minor release, but it's maybe not great to introduce that change just before an alpha, rather than earlier in the minor cycle when contrib would have had more time to catch up. It does seem quite unlikely people are writing code like that though, and a lot simpler than adding a dictionary.

The middle ground would be add a dictionary of reserved words for MySQL to quote if the field or alias matches.

I think that's a good middle ground, and it's exactly what the pgsql driver is doing as well: \Drupal\Core\Database\Driver\pgsql\Connection::doEscape

Discussed a bit more with @catch and @amateescu on slack. We agreed to go for the dictionary approach because quoting everywhere will be disruptive for 8.6.x and MySQL 8 support is important. We'll file a followup to discuss the advantages / disadvantages of quoting everywhere. I'll also file fixes for API misuses this issue already found.

Bumping to major bug as not supporting the latest stable for MySQL feels very buggy.

Opened #2986334: Add a way to enforce an empty result set for a database condition query for the ->condition('1 <> 1') API misuse found in this patch :)

So I've go the list of reserved words by selecting out of a MySQL 8 DB.

No reserved words were removed since 5.5 see:

• 5.7 -> 8.0 - https://dev.mysql.com/doc/refman/8.0/en/keywords.html#keywords-removed-i...
• 5.6 -> 5.7 - https://dev.mysql.com/doc/refman/5.7/en/keywords.html#keywords-removed-i...
• 5.5 -> 5.6 - https://dev.mysql.com/doc/refman/5.6/en/keywords.html#keywords-removed-i...

So this is the complete list of MySQL reserved words. Postgres formats the list all on one line. Imo 1 word per line is better because it makes it easier to review changes.

I've removed all the changes that were due to quoting always as these are unnecessary now and hopefully that means we can land this in 8.6.x

Also added test case for upserting with special column names.

Here's a related older issue about MySQL and reserved keywords #2560965: Reserved column names are broken on MySQL, the test lies

Oh and this will fix the column name part of #371: resolve ANSI SQL-92/99/2003 reserved words conflict in query statements

One thought is do we need to handle table names too? Someone might have a table called groups for example. (Another new reserved word in MySQL 8).

Crediting @bojanz for the work on #2560965: Reserved column names are broken on MySQL, the test lies which I've marked as a duplicate.

Handling tables is going to be awful - we have to somehow handle \Drupal\Core\Database\Connection::setPrefix() and per-table prefixes. Ugh. At least per-table prefixes are deprecated in Drupal 9 as per #2551549: Deprecate per-table prefixing

I've opened #2986452: Database reserved keywords need to be quoted as per the ANSI standard to discuss quoting of keywords in general. What a mess.

I don't think we need to worry about prefixes, escapeTable() is supposed to be called on the non-prefixed table name, so let's be consistent with the pgsql implementation and override escapeTable() as well.

We should probably rename quoteField() to doEscape(), also for consistency reasons. And replace $field with $string everywhere :)

@amateescu I'd rather discuss escapeTable() in a follow-up. If we make that change for mysql table prefixes for tables with reserved names will be broken here's why:

1. Say we have a table called function and the database prefix is set to "test12345678_".
2. Given a query like: select count(*) from {function}
3. The escapeTable() code would then do this to the string: select count(*) from {"function"}
4. The prefix replacement code would then do this to the string: select count(*) from test12345678_"function"
5. And everything would be broken.

Therefore doing the quoting in escapeTable() is dangerous.

I suspect that the current Postgres escapeTable() is broken for table prefixes too and if it every did actually quote something it'd be highly likely to break other things. We just don't have any tables in core called a reserved word. In my opinion we should proceed with #49 because it allows MySQL 8 to work without making anything worse and do a more comprehensive solution in #2986452: Database reserved keywords need to be quoted as per the ANSI standard for identifier escaping.

\Drupal\Core\Database\Driver\pgsql\Connection::doEscape()is really poorly named. From what I've seen, the db driver documentation use the language of quoting identifiers, why not use the same language? And by that argument I think I should update the patch to match that language.

I like the change in #59 a lot and based on #58 it is indeed better to handle escaping tables in a followup.

Opened #2986539: \Drupal\user\Plugin\EntityReferenceSelection\UserSelection::entityQueryAlter() should escape the fake condition column on replacement to handle an issue found by previous versions of this patch.

Rerolled.

Mistakenly reverted an unrelated change (it was related at one point in the patch's history).

Committing this now prior to the beta, we don't want to be eight+ months behind MySQL versions.

Let's continue in #2986452: Database reserved keywords need to be quoted as per the ANSI standard which is a more comprehensive fix.

Committed/pushed to 8.7.x and cherry-picked to 8.6.x. Thanks!

Great work everyone!

BTW does anyone have performance numbers to compare against 5.6 or 5.7?

#3089902: "Azure Database for MySQL server" reports wrong database version

@damienmckenna its increase speed 60 to 70 percent. Our content has too many paragraphs fields. On MySQL 5.7 it loads 28 to 31seconds after changing to MySQL 8 it is now loading at 11 to 14.4 sec.

Attempting to use migrate_upgrade with the source D7 DB on MySQL 8 I get the error "Source database does not contain a recognizable Drupal version" (as described at https://www.drupal.org/project/migrate_upgrade/issues/2637870#comment-13...). I haven't got to the bottom of this, but my strong suspicion is that the system table is not correctly escaped with backticks. I see some escaping of reserved keywords for tables in the pgsql connection class, but not mysql. The mysql connection class lists the version 8 reserved words, but does not seem to use them for escaping table names. I also see no sign of backtick escaping.

@Andy_Read see #2986452: Database reserved keywords need to be quoted as per the ANSI standard for a generic solution to escaping all the things.

+++ b/core/lib/Drupal/Core/Database/Driver/mysql/Connection.php
@@ -190,6 +471,49 @@ public static function open(array &$connection_options = []) {
+      $identifier = '"' . $identifier . '"';

Looks like this was the issue that introduced the decision to use a double quote rather than a backtick.

Meanwhile, for Drupal 7, #2978575: Mysql 8 Support on Drupal 7 chose to default to a backtick, but make that overridable in settings.php with $conf['mysql_identifier_quote_character'].

See #3218978: MySQL driver allows settings.php to remove ANSI_QUOTES from sql_mode, but doesn't work when it is for if/how we want to fix things for when the MySQL sql_mode doesn't include ANSI_QUOTES.