Problem/Motivation

  • The database connection array in settings.php can specify an ['init_commands']['sql_mode'] that overrides the default that's provided at the end of Drupal\Core\Database\Driver\mysql\Connection::open().
  • However, Drupal\Core\Database\Driver\mysql\Connection::$identifierQuotes is hard-coded to double quotes.
  • As a result, if the connection info array overrides the sql_mode to not include ANSI or ANSI_QUOTES, queries throw exceptions, because MySQL doesn't accept a double quote as an identifier quote.

Steps to reproduce

Proposed resolution

  • Add a constructor to Drupal\Core\Database\Driver\mysql\Connection to fall back to the non-ANSI backtick if the SQL mode doesn't include ANSI quotes.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

effulgentsia created an issue. See original summary.

effulgentsia’s picture

The last submitted patch, 2: sql_mode-test-only.patch, failed testing. View results

mcdruid’s picture

This is a great fallback / safety net.

However, should we consider simply changing the identifierQuotes to backticks by default? Perhaps there would need to be a deprecation dance (although not sure what that would look like in this case).

We went with backticks in D7 - having originally intended to backport the double quotes implementation - in order to avoid the risk of contrib or custom code conflicting with the ANSI_QUOTES requirement or similar (e.g. #3172388: backup_migrate resets sql_mode causing problems with D7's MySQL 8 support).

effulgentsia’s picture

Good question. Asked in #2966523-78: MySQL 8 Support.

effulgentsia’s picture

Meanwhile, I'm curious what else breaks without our other default sql_mode options. Here's a patch to find that out for all of our kernel and functional tests.

Status: Needs review » Needs work

The last submitted patch, 6: sql_mode-6.patch, failed testing. View results

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
2.17 KB
888 bytes

Only 18 failures in #6, that's fewer than I expected. I'll open separate issues for them.

In the meantime, back to the scope of this issue. This goes back to the patch in #2, but removes the call to str_contains(), since that's a PHP 8 only function.

This does not address #4 yet, since I'd like others to weigh in on that.

mcdruid’s picture

Looks like the double quotes are very hard-coded in D8:

https://git.drupalcode.org/project/drupal/-/blob/8.9.16/core/lib/Drupal/...

	+    if ($this->identifierQuotes === ['"', '"'] && strpos($connection_options['init_commands']['sql_mode'], 'ANSI') === FALSE) {

I don't see any mention of case (in)sensitivity in the MySQL docs, but a quick experiment suggests that the sql_mode value is not case sensitive.

Perhaps we should cater for the (perhaps somewhat unlikely) scenario of a custom sql_mode not being all uppercase?

Wim Leers’s picture

Priority: Normal » Major

Seems like this problem is even worse when database or tables names consists solely of digits:

Certain objects within MySQL, including database, table, index, column, alias, view, stored procedure, partition, tablespace, resource group and other object names are known as identifiers. This section describes the permissible syntax for identifiers in MySQL.

[…]

Identifiers may begin with a digit but unless quoted may not consist solely of digits.

[…]

The identifier quote character is the backtick (`):

[…]

If the ANSI_QUOTES SQL mode is enabled, it is also permissible to quote identifiers within double quotation marks:

https://dev.mysql.com/doc/refman/8.0/en/identifiers.html

IOW: On MySQL, you should always quote identifiers, either using " when ANSI_QUOTES is on, otherwise using `.

That makes this at least major IMHO, bordering on critical. Any site that turns off ANSI_QUOTES which uses a fully numerical database name will get a SQL error, and fail to work.


Worse, counter to the docs, I can confirm that even database names that just start with a digit reproduce this behavior:

  1. SELECT * FROM d8.users u INNER JOIN 101.foo f ON f.id = u.uid
    

    You have an error in your SQL syntax; check the manual that corresponds to your MariaDB server version for the right syntax to use near '101.foo f ON f.id = u.uid' at line 1

  2. SELECT * FROM d8.users u INNER JOIN 73829e0c4090444f85fbae2e92f8925a.foo f ON f.id = u.uid
    

    You have an error in your SQL syntax; check the manual that corresponds to your MariaDB server version for the right syntax to use near '73829e0c4090444f85fbae2e92f8925a.foo f ON f.id = u.uid' at line 1

  3. Turns out this is because there's an e (the letter "e") in the database name, which makes MySQL interpret it as a number in exponential notation! 🤪🤯🤯🤯🤯🤯🤯🤯🤯🤯🤯 Check yourself:
    SELECT * FROM d8.users u INNER JOIN 1e0.foo f ON f.id = u.uid
    

    You have an error in your SQL syntax; check the manual that corresponds to your MariaDB server version for the right syntax to use near '1e0.foo f ON f.id = u.uid' at line 1
    … yet 1a0 works fine!


To reproduce:

  1. create three database names above
  2. create an empty foo table like so in each:
    CREATE TABLE `foo` (
      `id` int(11) unsigned NOT NULL AUTO_INCREMENT,
      PRIMARY KEY (`id`)
    ) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4;
    

⚠️ Databases named using only digits (unlikely) or named with a hash (kinda likely) that happen to contain the letter "e" (unlikely) will fail hard! ⚠️

daffie’s picture

Status: Needs review » Needs work
  1. +++ b/core/lib/Drupal/Core/Database/Driver/mysql/Connection.php
    @@ -90,6 +90,20 @@ class Connection extends DatabaseConnection {
    +    if ($this->identifierQuotes === ['"', '"'] && strpos($connection_options['init_commands']['sql_mode'], 'ANSI') === FALSE) {
    

    The test for 'ANSI' only works because there are only 2 sql_modes that have the text 'ANSI' in it and they are the 2 we are testing for. Maybe we should add a comment for that.

  2. +++ b/core/tests/Drupal/KernelTests/Core/Database/SqlModeTest.php
    @@ -0,0 +1,31 @@
    +    $result = $this->connection->query('SELECT [update] FROM {select}')->fetchObject();
    +    $this->assertEquals('Update value 1', $result->update);
    

    Can we change this code to:

        $query = $this->connection->query('SELECT [update] FROM {select}');
        $this->assertEquals('Update value 1', $query->fetchObject()->update);
        if ($this->connection->databaseType() == 'mysql') {
          $this->assertStringContainsString('SELECT `update` FROM `', $query->getQueryString());
        }
    

    As I would very much like to test that backticks are being used for MySQL.

effulgentsia’s picture

effulgentsia’s picture

#12 addresses #9 and #11.

daffie’s picture

Status: Needs review » Needs work

Testbot is failing for PostgreSQL and SQLite. Maybe doing: $info['default']['init_commands']['sql_mode'] = "SET sql_mode = ''"; only for MySQL.

effulgentsia’s picture

daffie’s picture

Status: Needs review » Needs work

The testbot is still not happy.

effulgentsia’s picture

daffie’s picture

Wim Leers’s picture

Nice work! 😊

  • catch committed cff1307 on 9.3.x
    Issue #3218978 by effulgentsia, daffie, mcdruid, Wim Leers: MySQL driver...

  • catch committed f0c7bb8 on 9.2.x
    Issue #3218978 by effulgentsia, daffie, mcdruid, Wim Leers: MySQL driver...
catch’s picture

Version: 9.3.x-dev » 9.2.x-dev
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 9.3.x and cherry-picked to 9.2.x, thanks!

Status: Fixed » Closed (fixed)

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

teodyseguin’s picture

I was still able to reproduce this one. After upgrading to 9.2.9 it started to throw an error message

SQLSTATE[42000]: Syntax error or access violation: 1064 You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near '"users_field_data" SET "access"='1638221696' WHERE "uid" = '1'' at line 1: UPDATE "users_field_data" SET "access"=:db_update_placeholder_0 WHERE "uid" = :db_condition_placeholder_0; Array ( [:db_update_placeholder_0] => 1638221696 [:db_condition_placeholder_0] => 1 ) in Drupal\user\UserStorage->updateLastAccessTimestamp()

Site is broken because of this.

I got this while running in a lando setup and the version of mysql server is (mysql Ver 14.14 Distrib 5.7.29).

teodyseguin’s picture

FileSize
927 bytes
mcdruid’s picture

@teodyseguin what do you have your sql_mode set to when your site has problems? (It's typically a key/value pair under init_commands in the $databases configuration in settings.php)

teodyseguin’s picture

Hi @mcdruid.. I don't have the init_commands being set the from the settings.php