Mysql 8 Support on Drupal 7

I installed Drupal 7 with MySQL 8 successfully with this patch.

Added server version check for MySQL 8.0.11 and an if condition based on that.

Removed the line
$sql = str_replace("{system}", "{`system`}", $sql);
introduced in the original patch by elijahoyekunle.

I do not know what that line is good for. Works for me without that line.

Reinserted the line

$sql = str_replace("{system}", "{`system`}", $sql);

and wrapped also that in a version check. It is indeed needed for running D7 with MySQL 8.

I have now tested this patch on MySQL 5.6.40 and MySQL 8.0.11. (Without that line, on 8.0.11, I get an error on Cache Clear All.)

A drupal.org forum page about Drupal 7 with MySQL 8 is here and a there is a link with an explanation.

That line is required because system became a reserved word in MySQL 8. The backticks allow it to be used as table name.

Consider the conditional setting of sql-mode the right thing to implement this issue. Would prefer a different implementation:
- initialize without
- extend when appropriate ( not 8 )
- append to init command.

In addition you might want to check the manipulation of information schema in the D8 patch. Several additions of `as .....`.

This fix got Drupal working, but Drush is still problematic. That's because of the drush_module_list function which calls:
drush_db_select('system', 'name', 'type=:type AND status=:status', array(':type' => 'module', ':status' => 1));
And this aliases the "system" table as "system," so the query ends up looking like:
SELECT system.name AS name FROM {system} system WHERE (type='module' AND status='1');
So the patch escapes the {system} in braces, but not the system alias.
I "solved" this by adding a line before the patch:
$sql = str_replace("{system} system", "{system}", $sql);
So now Drush works, and nothing else appears to be broken, but this probably isn't the way to do it.
Anybody got a more effective solution?

Each module needs to fix its own bugs. Suggest to check drush queue and raise an issue there.

You might try replacing 'system' with '`system`'

Not convinced the solution for reserved word system works properly. It shouldn't replace {system} with {`system`}, which only works with particular prefixes. It should translate {system} into `system` when table prefix is not a database prefix or no prefix provided.

The current patch will break other database drivers like PostgreSQL which do not use backticks to quote identifiers. Also this patch will break table prefixes.

I suggest we'll backport the patches in #2966523: MySQL 8 Support and #2986452: Database reserved keywords need to be quoted as per the ANSI standard once these are committed in D8.

Also using

replace {system} with {`system`}

will brake core simpletest
so it would rather work this way:

$sql = str_replace("{system}", "`{system}`", $sql);

Ported some part of the patch in #2966523: MySQL 8 Support to Drupal 7. This patch still needs to resolve escaping table names that are reserved keywords particularly the `system` table which doesn't exist in Drupal 8. Still work in progress...

Hi,

There is any update on this?

Is still planned to support MySQL 8 in Drupal 7 or it has been dismissed?

Thanks

Hello,

If anyone would like to use Drupal 7 with Digital Ocean's Mysql 8 DBaaS, this needs to be fixed.

Hi,

Tested with success as well on my side.

To increase our confidence in the test results for the above issue, these two should go in first so we can re-queue php 7.3 and php 5.3 tests.
#3047844: [Regression] Tests fail on PHP 5.3
#3025335: session_id() cannot be changed after session is started

To clarify some things:

- We will be supporting MySQL 8 in Drupal 7 — it is just not yet clear when.

There is several concerns with the patch and in the worst case we can commit it in stages:

- We should document that not only is NO_AUTO_CREATE_USER removed from MySQL 8, but also that it’s the new default since 5.7 (so no longer needed)

- We definitely need the tests ported from D8 as well

- We might want some more tests, too: If we say something is supported and old legacy D7 code breaks when switching due to an oversight, then we don’t actually support.

Main questions I have here are:

db_query() + reserved key words.
Expressions + keywords
Table prefixes and keywords

The information schema parts are fine and easy to commit though.

@Fabianx and I also reviewed this:

+    $insert_fields = array_map(function ($field) {
+      return $this->connection->escapeField($field);
+    }, $insert_fields);

...which is the same as (or very similar to) the D8 patch, but we don't generally use this syntax (anonymous function as the callback) in D7. It was introduced in PHP 5.3, so should work pretty much everywhere nowadays, but this would be the first usage of it to be introduced to D7 core.

What I don't get is how difficult we make this. Why can't we not simply use back ticks for every table and column?

Here's my much simpler patch to advance the discussion. This one doesn't require any fixes to Drush.

Why can't we not simply use back ticks for every table and column

If we were to have a full ORM, we surely can inspect each and every table/field/alias and quote them with backticks. What we have is a half-baked system, that queries might come from structured data (which we can quote properly), and raw SQL queries which we cannot properly quote.

For example, the following is a valid SQL query:

SELECT `where` FROM `where` WHERE `where` = 'where' 

Unless we parse the query, there is no bullet-proof way to decide which ones need backticks, which ones should not. That is why we have this solution to individually process tables, fields, and aliases. I personally think that we can improve the performance of the escaping step (for example, using a single strtr call instead of an array_reduce + lambda calls for each reserved keyword), but I don't think we can have a more simpler solution.

I don't think we need to parse:

1. We can quote tables in any query using '{..}' which they already should.
2. We can quote stuff build with fields() etc, in those cases we know exactly what the column is.

What we currently don't handle is arbitrary quoting of arbitrary SQL. That's fine. I don't think any patch will ever do that. We can leave that stuff alone to either use db_select() or add their own quotes.

I.e. only quote the stuff we know for sure can be quoted. Leave the rest alone.

I'm also in favor of quoting all fields/tables with backticks just so we can avoid iterating the rather big reserved keywords list and the future-proofing it provides with future reserved keywords MySQL might add.

Some related issues: #3020839: Drupal\Core\Database\Driver\mysql\Connection::quoteIdentifier in_array performance improvements, #2986452: Database reserved keywords need to be quoted as per the ANSI standard.

Any update on the status of this critical issue?

I'm not a strong enough programmer to help, but would love to start using MySQL 8. :)

Does this patch affect performance? Can I use it in production? mysql-8-2978575-24.patch

Realistically, I suppose a lot more tests need to be done and written before this lands in Drupal core because there can be a lot of edge cases considering `system` is a reserved keyword in MySQL 8, which Drupal uses extensively.

I use a patched Drupal 7 core in several of my web sites, none of them fortune 500, but quite important ones. So far, it seems to be working well. Drush 8 is not working properly, so you will need some tinkering around to get it working.

Other than that, I encountered MySQL server has gone away errors with PHP 7.4. Not related to Drupal in particular though. See the linked article on a fix.

I'm not a strong enough programmer to help, but would love to start using MySQL 8. :)

I'd say go for it. If you encounter any errors, post here, and the others will gladly work together for a fix, including myself.

Does this patch affect performance? Can I use it in production? mysql-8-2978575-24.patch

I'm a bit concerned the excessive array scanning for reserved keywords. See #3020839: Drupal\Core\Database\Driver\mysql\Connection::quoteIdentifier in_array performance improvements. I use a similar patch from the linked issue (I'm the author), but the issue did not receive any attention. PHP's opcache should provide some optimizations given the pseudo-static nature of the reserved keywords array. This will be a micro-optimization and it will be unlikely a bottleneck for performance.

This comment was regarding the patch in #14; not 24.

Thanks for your reply :)

I use a patched Drupal 7 core in several of my web sites, none of them fortune 500, but quite important ones. So far, it seems to be working well. Drush 8 is not working properly, so you will need some tinkering around to get it working.

Sorry, which mysql 8 patch are you using on your websites?

Sorry, which mysql 8 patch are you using on your websites?

Patch in #14.

@Ayesh Thank you!

FYI, patch #14 did not work for me, but patch #24 works...so far.

I'm running PHP 7.2.25 and MySQL 8.0.18.

@Ayesh "Drush 8 is not working properly, so you will need some tinkering around to get it working." Can you elaborate? We recently upgraded to drush 8 on php 7.3 because drush 7 fails. The only issue we encountered was the dependence on needing the php extension fileino for now, see #3085098: PHP Phar Stream Wrapper should be updated (PHP7.4 support, drop fileinfo dependency). Is there something else? So far drush 8.3.2 appears to be working fine for us. We are using MariaDB 10.

This is the error I'm getting from Drush (for drush up):

WD php: PDOException: SQLSTATE[42000]: Syntax error or access violation: 1064 You have an error in your SQL syntax; check [error]
the manual that corresponds to your MySQL server version for the right syntax to use near 'system
WHERE  (type='module' AND status='1')' at line 3: SELECT system.name AS name
FROM
{system} system
WHERE  (type=:type AND status=:status) ; Array
(
    [:type] => module
    [:status] => 1
)
 in drush_db_select() (line 99 of /root/.config/composer/vendor/drush/drush/includes/dbtng.inc).
PDOException: 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 'system
WHERE  (type='module' AND status='1')' at line 3: SELECT system.name AS name
FROM
{system} system
WHERE  (type=:type AND status=:status) ; Array
(
    [:type] => module
    [:status] => 1
)
 in drush_db_select() (line 99 of /root/.config/composer/vendor/drush/drush/includes/dbtng.inc).
Drush command terminated abnormally due to an unrecoverable error.                                                        [error]
Error: Array and string offset access syntax with curly braces is deprecated in
/root/.config/composer/vendor/drush/drush/includes/sitealias.inc, line 176

Have you confirmed that the php extension fileinfo is loaded, php -m

Yes it's loaded.

From the findings it looks like there are two issues; (1) related to use of mysql 8 with drupal 7 itself, and (2) related to the use of drush.

@ayesh did you try the patch in #24 that does not affect drush?

Although it does have a hunk to make drush work.

Patch in #24 applies without difficulty to the latest drupal 7.69 and appears to work fine with MariaDB 5.5.5-10.2.29 and drush 8.3.2 with php 7.3.12 does not give errors with "drush up".

I have tested Patch #24 and appears to work for me and with drush 8

Also make sure settings.php configuration has

<?php
'init_commands' => array (
        'isolation' => "SET SESSION transaction_isolation='READ-COMMITTED'",
.....
?>

MySQL 8 renamed tx_isolation to transaction_isolation

<?php
'init_commands' => array (
        'isolation' => "SET SESSION tx_isolation='READ-COMMITTED'",
.....
?>

#24 works great with Drush 8.1.x and MySQL 8.0.x & PHP 7.2.x and Drupal 7.69.
Will this release be sent with the latest Drupal 7 Core? It shows Drupal 7.70 target.

I have updated mysql 8.0.18 version with drupal 7.56... after update version i am facing mysql syntax error.I am applied path #24 after that my application issue has been resolved. thank you soomuch...

I am added
includes/database/mysql/database.inc
public function query($query, array $args = array(), $options = array()) {
$query = preg_replace('/{([^}]+)}/', '`\1`', $query);
// This to make Drush work
$query = str_replace(' system.', ' `system`.', $query);
$query = str_replace('`system` system', '`system` `system`', $query);
return parent::query($query, $args, $options);
}

And Setting.php i have added
'sql_mode' => "SET sql_mode = 'REAL_AS_FLOAT,PIPES_AS_CONCAT,ANSI_QUOTES,IGNORE_SPACE,STRICT_TRANS_TABLES,STRICT_ALL_TABLES,NO_ZERO_IN_DATE,NO_ZERO_DATE,ERROR_FOR_DIVISION_BY_ZERO'",
);
mysql 80.18 PORT :3308
within database connection. it's working fine for me.

@berenddeboer

Thanks for the patch #24. It work great with Drupal 7.69, MySQL 8.0.x and PHP 7.3.x.

Would it be fair to say this is RTBC or is there something else left to do?

From my experience with the patch in #24 and the comments of the other users I set this as RTBC.

I have improved patch #24 to also handle queries that specify the database along with the table, like in "{db.table}". Those must be quoted to `db`.`table` instead of `db.table`. This is required e.g. by migrate when a different source database is queried and probably always if a database connection other than the default one is used. Without it, the query fails.

The quoting is also only done if we're indeed on MySQL 8 as it's unnecessary for version 5.

I reintroduced the conditional addition of NO_AUTO_CREATE_USER from patch #14 as it has gone into Drupal 8, too (see #2966523: MySQL 8 Support).

Before this can be RTBC, I guess we need test functions, like stated in #21.

The test on PHP 7.3 & MySQL 8 fails as DrupalTestCase::insertAssert() wants to save a field "function" to the simpletest table which doesn't work as "function" is a reserved keyword. Seems like quoting table names is not enough and changes from patch #14 need to be incorporated.

This is a fairly critical for anyone using Ubuntu 19.04 since it is no longer being supported. Upgrading would result in Mysql 8 which is throwing bad errors :/

Patches from #24 and #50 don't work when a table prefix has been set.

To me it seems more logical to have the code that replaces the { and } should add the back ticks, i.e. override method DatabaseConnection::setPrefix() instead of changing DatabaseConnection_mysql::query() as these patches do. So, patch #14 looks better, though if we override DatabaseConnection::setPrefix() to always back tick a table name, regardless if it is a reserved keyword or not, that code would be less complicated, see attached patch.

This patch solved for me:
- The problem with NO_AUTO_CREATE_USER being removed
- The error introduced by the patches from #24 and #50, namely something like "Table semaphore does not exist in ..." (I am using a database prefix, thus the table semaphore indeed does not exist, it should be prepended with the prefix).

This patch should solve for those who do not use a prefix:
- Tables having the same name as a reserved keyword giving errors because the table name is not being quoted.

Other problems with MySQL8 (quoting fields that are a reserved keyword?) are not solved by this patch.

We seem to be split between people not liking the complexity of the #14 route even though it keeps us closer in-sync with D8, and people not liking the divergence and less complete solution of the #24 route.

So why not try to make both camps happy and do a backport of #2986452: Database reserved keywords need to be quoted as per the ANSI standard instead?

That said, #14 is working fine over here, with a very minor Drush patch (https://github.com/drush-ops/drush/pull/4313).

#53 works fine for me.
However drush 8 is not working with #53.

I appreciate if someone can extend #53 so drush 8 is working, thx.

#53 fails for me when I visit /admin/config

PDOException: 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 'system WHERE type = 'module'' at line 1: SELECT name, schema_version FROM {system} WHERE type = :type; Array ( [:type] => module ) in drupal_get_installed_schema_version() (line 155 of /var/www/d7.example.com/public_html/includes/install.inc).

The line it's failing on is clearly one case this patch was intended to fix, as it is accessing the {system} table:
$result = db_query("SELECT name, schema_version FROM {system} WHERE type = :type", array(':type' => 'module'));

I made the priority higher because this started failing for me when I upgraded Ubuntu from 19.04 to 19.10. The latter is already 6 months old, and Ubuntu is about to release 20.04, but one must upgrade from 19.04 to 19.10 first before moving to 20.04. That said, I anticipate quite a few users encountering this problem over the next few months after 20.04 comes out. Likewise, the effort to make D7 run with PHP 7.4 won't be of much use if MySql 8 can't be used at the same time; most systems won't have a default configuration that includes a new version of PHP along with an old version of MySql - they will either have both new or both old.

As a temporary fix, I hacked core and changed the above query to use {`system`}. That's not a good solution of course, but it will allow me to continue to test #53 to see if I encounter any other problems.

EDIT: Yeah, there are a lot of problems still. Here's another, when visiting /admin/config/developement/testing (Simpletest):

PDOException: 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 'system' at line 1: SELECT name, filename FROM {system}; Array ( ) in simpletest_test_get_all() (line 336 of /var/www/d7.example.com/public_html/modules/simpletest/simpletest.module).

Again, this is clearly a query that should have been fixed by the patch. Again I hacked core, and that simply got me to the next similar error. I don't think it makes sense to keep listing them here - just a few minutes of manual testing shows that #53 only works for some things, but not nearly enough.

EDIT: forget this one, continue with #59

The patch did not work correctly due to the order of setting isMySql8 and calling setPrefix(). I tested with a prefix that obscured that (Tip: as a workaround work with a table prefix :))

Attached a correction of the patch form #53. Note that this still does not contain a fix for fields named after reserved words, though that still seems to be allowed in the query mentioned in #56 (type is a reserved word!).

I triggered the testbot to test #59 against MySql 8, since that's the whole point of this issue....

Fails mysql 8

PDOException: 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 'function, line, file) VALUES ('1', 'ActionsConfigurationTestCase', 'fail', 'The ' at line 1 in /var/www/html/includes/database/database.inc:2227
Stack trace:
#0 /var/www/html/includes/database/database.inc(2227): PDOStatement->execute(Array)
#1 /var/www/html/includes/database/database.inc(697): DatabaseStatementBase->execute(Array, Array)
#2 /var/www/html/includes/database/mysql/database.inc(147): DatabaseConnection->query('INSERT INTO `si...', Array, Array)
#3 /var/www/html/includes/database/mysql/query.inc(36): DatabaseConnection_mysql->query('INSERT INTO `si...', Array, Array)
#4 /var/www/html/modules/simpletest/drupal_web_test_case.php(229): InsertQuery_mysql->execute()
#5 /var/www/html/modules/simpletest/drupal_web_test_case.php(523): DrupalTestCase::insertAssert('1', 'ActionsConfigur...', 'fail', 'The test did no...', 'Completion chec...', Array)

@joseph.olstad: Is that output from local testing? The testbot output in #59 shows that the failure is in drush_db_select(), since drush is used by the testbot to set up the test environment. The testbot never gets around to testing this patch because the environment setup fails.

But that does bring up once again that drush also needs to be fixed to support MySql 8, and that seems to be a prerequisite for getting this patch tested by the testbot. This has been mentioned in #10, #11, #30, #34, #38, and #55. Patch #24 and its derivatives make a special fix to accomodate drush, but as mentioned in #11 this should be fixed in drush instead - we can't be adding special cases for contributed modules into core, that should be fixed in the contributed module (drush).

ok so there's a fix for drush that hasn't gone in. Perhaps someone who knows more about this issue should perhaps follow up by creating a new issue in the drupalci_testbot queue with helpful explanation and instructions for the drupalci_testbot maintainer. Such as which drush patch to apply to the mysql 8 runner.

https://www.drupal.org/project/issues/drupalci_testbot

So , which patch does the drupalci_testbot drush require in order to be able to properly test against mysql 8?

I imagine this will cause some issues as the same version of drush might be used in other runners.

@joseph.olstad: Do you have a link to that drush fix? What I'm seeing in github is that this is a problem with Drush 8, and since Drush 8 is not supported there hasn't been much traction to fix this: https://github.com/drush-ops/drush/issues/3830

DrupalCI needs to use Drush 8 because Drush 9 doesn't work with Drupal 7.

ok looks like @MustangGB already created a pull request for Drush 8.x

I suggest that if the drush people do not respond in a timely manner, an option would be to fork drush 8 , OR for the drupalci_testbot project to patch their drush 8.x

here is the link to the pull request for drush 8.x
https://github.com/drush-ops/drush/pull/4313

**begin update**

I have commented on the upstream pull request and issue.

**end of update**

Some thoughts:

• This looks like a chicken and egg problem: drush depends on Drupal and can thus not be tested on MySQL8, Drupal testbot depends on Drush and thus patches cannot be tested on MySQL8
• As already indicated, my patch from #59 only solves the set sql mode problem and non-quoted tables. It does not tackle the problem of quoting field names that are reserved words. (However, so far it seems to be working fine for me).
• Many test failures are to be expected, especially in the tests that test query construction. How do we adapt these tests (the test should know if we are using mysql8 or we should always quote table names).

To move forward: would it be an idea to split this issue in multiple smaller issues:

• The set mode problem.
• Table name quoting in MySql8 (and correcting the query construction tests).
• Quoting field names in queries build using SelectQuery and other query building classes (where the fields are known).
• Quoting field names in queries written using db_query() and similar functions (and as far as they fail).
• Other ...?

Thank you for your input @fietserwin. I also agree that this issue has become more complicated than it should have.

The set mode problem.

I propose that NO_AUTO_CREATE_USER mode should not be used at all. It provides questionable security, and it is deprecated in MySQL 5.7 anyway. It will make out patch cleaner as well, because we do not need version-compare before adding the flag.

Table name quoting in MySql8 (and correcting the query construction tests).

I think this is where the most of out bikeshedding happens. Drupal 8's approach is more safer, as it has a known list of MySQL reserved keywords. It provides some sort of overhead compared to the regex approach in #24. It's worth noting that even if a table name is not a reserved keyword, there is absolutely no hard in quoting them with back-ticks either.

Quoting field names in queries build using SelectQuery and other query building classes (where the fields are known).

We can follow the D8 approach here, with the known list of keywords (although I personally would like to see use if isset() as opposed to in_array). Patch in #24 does not provide protection for this.

Quoting field names in queries written using db_query() and similar functions (and as far as they fail).

This is where I really like #24's regex approach. We can easily replace table names as long as they use {table} pattern, which is recommended way anyway.

We cannot fix db_query() with a 100% assurance, because it's arbitrary SQL. This is the case for Drupal 8 as well.

What I have is a hybrid of #14 and #24, that it tries to escape known database fields and tables, but also runs a a regex to catch the ones that did not make it (i.e came via db_query()). I will attach the Sparta comment below, but it is not to be taken seriously. It however works with pretty much every scenario I throw at it, even with Drush.

I've not had a chance to dig into the details of this one yet I'm afraid, but just wanted to mention that I think we'll need a very good reason to approach this differently to what's been committed to D8.

However it looks like #2986452: Database reserved keywords need to be quoted as per the ANSI standard has only gone into D9 and won't be backported to D8.9

So thanks for all the thorough comments and contributions so far... I think we'll need to approach this with the backport policy in mind and therefore thinking about how we can use the code (/ approach) that's already been committed to newer Drupal versions, and where possible avoid coming up with anything novel/different for D7.

Apologies if that's missing the point of recent comments; as mentioned I've not got into the details yet.

I think we'll need a very good reason to approach this differently to what's been committed to D8

There is no good reason, other than "too complicated".

The first pass backport of D8 is in #14, and I've not run into any problems with it, other than Drush which is a simple fix on their side.

the MustangGB MySQL 8 fix was merged into drush 8.x.
created #3128406: Drupal 7 MySQL 8 test runner needs a new version of drush 8.x

#67 has worked for me for latest drupal 7.69 with Mysql 8.0

With the release of Ubuntu 20.04 LTS which ships with MySQL 8, it would seem that the urgency to get a fix into core would perhaps be more pressing. I have a production server running D7 and CiviCRM that I'd like to get up to Ubuntu 20.04; however, I don't want to have to back install MySQL to make things work. Is there anything I can do to help move this issue forward? Peace - Anthony

To be as transparent as possible about prioritisation of issues for the 7.x branch, compatibility with newer PHP versions is a higher priority at the moment than this, because it's more likely that D7 sites find themselves forced to update to new versions of PHP 7 than to upgrade to MySQL 8.

This is reflected in the Drupal 7 roadmap and release schedule at:

https://www.drupal.org/core/drupal7-roadmap-release-schedule#release-sch...

...where PHP and MySQL EOL dates are included for context.

Latest Drush 8.3.3 release should resolve things over there, so D8 backport should be good to go.

If for some reason there's still an issue with the test runner using the newly tagged drush 8.3.3, please follow up here: #3128406: Drupal 7 MySQL 8 test runner needs a new version of drush 8.x

... it's more likely that D7 sites find themselves forced to update to new versions of PHP 7 than to upgrade to MySQL 8.

@mcruid: As I mentioned in #56,

... the effort to make D7 run with PHP 7.4 won't be of much use if MySql 8 can't be used at the same time; most systems won't have a default configuration that includes a new version of PHP along with an old version of MySql - they will either have both new or both old.

Specifically in the context of Ubuntu, where people who have been using the 18.04 LTS for the past two years will soon be upgrading to the new 20.04 LTS (the upgrade path for the LTS release means this will happen in July when 20.04.01 is released). Ubuntu switched to MySql 8 last year - EVERYONE who upgrades to 20.04 LTS will be using PHP 7.4 and MySql 8.

I don't know about other distributions and their timelines, but Ubuntu is a big one ...

+++ b/includes/database/mysql/database.inc
@@ -86,21 +357,88 @@ public function __construct(array $connection_options = array()) {
+  public function prefixTables($sql) {
+    // Escape reserved mysql keywords used as table names.
+    $sql = array_reduce($this->reservedKeyWords, function ($original_sql, $keyword) {
+      return str_replace('{' . $keyword . '}', '`{' . $keyword . '}`', $original_sql);
+    }, $sql);
+    return parent::prefixTables($sql);
+  }
+

The problem with this piece of code is that it still can go wrong: If I have a table 'fill' and a prefix 'zero', this query:
select * from {fill}
will end up as:
select * from zerofill

(I understand that this is thus also a minor bug in D8/9.)

Furthermore, the array_reduce is over about 270 entries so with 100 queries on a page (not uncommon) we are talking 27,000 str_replace calls and 100,000+ string concatenations. In the light of 100 DB queries probably nothing but still.

Oh and that list is probably a running target that may change again in the future.

So, I would definitely go with just escaping all tables: easier, faster, more robust.

@fietserwin Nice forwarding looking suggestion. I believe this is a valid concern we need to take seriously. Speed and future maintenance cannot be hindered. Let's do this right. :)

#67 has worked for me upgrade to latest drupal 7.70 with Mysql 8.0.20 and PHP 7.4.3

I agree with 78 and 79 , so we need a D8 issue for this and this means it will require waiting for the D8/D9 fix to go in first according to policy. So please lets create the D8/D8 issue.

I believe the D8/D9 issue is the one I mentioned above, namely #2986452: Database reserved keywords need to be quoted as per the ANSI standard.

thanks @MustangGB, ok perfect so we're all set for doing the backport of that strategy.

#67 works for me, apart from queries that specify the database along with the table, like {db.table}. As I wrote in #50, those must be quoted to `db`.`table` instead of `db.table`. This is required e.g. by migrate when a different source database is queried and probably always if a database connection other than the default one is used. Without it, the query fails.

I have merged the changes to DatabaseConnection_mysql::query from my patch #50 into #67 to fix this. There might be a more efficient way than doing this with preg_replace_callback() though.

This seems like a use case that is not supported as with current behavior this will become prefixdb.table instead of db.prefixtable???

Please note that the patch in #67 is not idea because it tries to fix the problem with both approaches. I'm personally in favor of escaping all keywords in the queries when possible, which in theory should work faster due to lack of vigorous in_array calls.

It works for now, but ideally I think we should discuss on one correct approach and stick with it.

I had to apply again #67 patch to upgrade to latest drupal 7.71 with Mysql 8.0.20 and PHP 7.4.3

I had to install patch #67 after upgrading to Ubuntu 20.04 LTS with MySQL 8.0.20 and getting this message on Drupal 7.70:

PDOException: SQLSTATE[42000]: Syntax error or access violation: 1231 Variable 'sql_mode' can't be set to the value of 'NO_AUTO_CREATE_USER' in lock_may_be_available() (line 167 of /var/www/html/drupal/includes/lock.inc).

Thanks Ayesh, I was able to connect and all seems well even though there was one error applying the patch... Hunk #1 FAILED at 343.

$ more schema.inc.rej

--- schema.inc
+++ schema.inc
@@ -343,7 +343,7 @@ public function findTables($table_expression) {
     // couldn't use db_select() here because it would prefix
     // information_schema.tables and the query would fail.
     // Don't use {} around information_schema.tables table.
-    return $this->connection->query("SELECT table_name FROM information_schema.
tables WHERE " . (string) $condition, $condition->arguments())->fetchAllKeyed(0,
 0);
+    return $this->connection->query("SELECT table_name as table_name FROM infor
mation_schema.tables WHERE " . (string) $condition, $condition->arguments())->fe
tchAllKeyed(0, 0);
   }

   /**

Any idea how this error will affect my D7 site?

As Andrés said above, in order to upgrade from 7.70 to 7.71 running on PHP 7.4.3 and MySQL 8.0.20 this patch needs to be applied before running update.php. Otherwise, you will get this error message...

PDOException: SQLSTATE[42000]: Syntax error or access violation: 1231 Variable 'sql_mode' can't be set to the value of 'NO_AUTO_CREATE_USER' in drupal_get_installed_schema_version() (line 155 of /var/www/html/drupal/includes/install.inc).

I'm unable to update drupal to 7.71 due to MySQL 8 issues.

Which patch is recommended? Patch #67 doesn't seem to exist on this page anymore.

@ronlee wrote:

Which patch is recommended?

Please try the latest.

Patch #67 doesn't seem to exist on this page anymore.

Somehow patches got out of order. I've rearranged them.

Patch in #84 applies cleanly and lets me install 7.x-dev on Ubuntu 20.04 LTS (MySQL ver 8.0.20) without a hitch.

Proceeding to test with some of the extensions I use on major production sites.

Changing tags, 7.70 is history. Let us try to get this one into Drupal 7.72.

Any release date for 7.72?

I'm stuck in a weird situation where I had to migrate a Drupal 7 site to PHP 7.2/MySQL 8 before I could get the migration to Drupal 8 out of the way. Now I have a Drupal 7 database from MySQL 8 that is incompatible with the Migrate Upgrade module (and several others) and I cannot create a migration properly due to the error that the module thinks that the D7 database is actually a D8 database.

I tried the patches above and none of them seemed to match or work. I've taken to creating my own patches at this point in an attempt to fix the issue. But, before I get through this does anyone even know if it will work - upgrading/migrating a D7 database to a D8 site on MySQL 8.0.20?

If I create a patch that works with the latest release I will contribute the product of the experiment.

Thanks!

Again: Patch in #84 applies cleanly to the HEAD of 7.x-dev and works fine. I've now tested this combo on 14 different Drubal 7 sites and no glitches so far.

@saxmeister: You're facing two distinct problems:
1) D7 doesn't work with MySql 8.
2) Migrate doesn't work in D8 with MySql 8. See #3093565: Migrate from Drupal 7 fails with mysql 8.0.3 or later: syntax error on select s.* from system

The patch here can probably fix 1) but doesn't help with 2).

In my case, Patch #84 applied cleany to an existing site with Drupal 7.67 - using NO table prefixes
But applies not to an existing site (Drupal 7.70) - using table prefixes.

Error: PDOException: SQLSTATE[42S02]: Base table or view not found: 1146 Table 'juz.semaphore' doesn't exist: SELECT expire, value FROM `semaphore` WHERE name = :name; Array ( [:name] => variable_init ) in lock_may_be_available() (line 167 .... includes\lock.inc).

The table prefix ist false, in settings.php it is set as: 'prefix' => 'juz_'

Any ideas on this are very welcome. Thank! :-)

pyQlo, IMHO, It makes no sense to apply a patch to any other versions than the version of the dev-branch the patch was rolled against. What you try to do here is extremely unlikely to work.

This is about the dev-brach of Drupal 7. Don't try this on a production site running a tagged release.

Yes, I totally agree. Unfortunately a very popular german hoster updates to MySQL 8 next week (announcement was 3 days ago...) - so I have to react.... :-(
Of course I didn't try it in production environment, but in my local dev site.

Updated to Drupal 7.71 and got the same error using table prefixes.

pyQlo,
this works for me:
1. Use git to clone the project.
2. Check out HEAD of the 7.x-dev branch.
3. Apply the patch in #84. It should apply cleanly.
4. Install the patched version on the site running MySQL 8.

gisle, thanks - I'll try that!

pyQlo, IMHO, It makes no sense to apply a patch to any other versions than the version of the dev-branch the patch was rolled against. What you try to do here is extremely unlikely to work.

Normally that is the case for the active branch. For the D7 branch the files involved haven't been touched in 3 years. The patch applies to any release since then. That would be 7.65+ on the save side.

So, it is guaranteed to work as there have been no changes for so long.

Regarding table prefixes, are they pointing to a different database?
When the prefix uses a '.' it points to a different database. When '_' it refers tot the same database, just a different table name. Obviously, ending the prefix in a '.' is not going to solve the reserved words issue for system/groups as it only points to a different database. Having a prefix without ending in '.' should solve that, unless the new table name ends up being a reserved word.

Thanks mmjvb.

I can also report back, that Patch #84 applies cleanly for dev and installation (even with table prefixes) worked.
For testing purposes I changed an existing site (7.71 - the one I mentioned above with prefix "juz") to use no table prefixes and here too #84 worked and the site runs without errors.

I understood, that this thread focusses on dev-branch, but it would be awesome, if the patch could be used for existing sites that are using table prefixes.

mmjvb - there ist only one datebase and the prefix was 'prefix' => 'juz_'.
No '.' is used even when the error looks like:

PDOException: SQLSTATE[42S02]: Base table or view not found: 1146 Table 'juz.semaphore' doesn't exist: SELECT expire, value FROM `semaphore` WHERE name = :name; Array ( [:name] => variable_init ) in lock_may_be_available() (line 167 .... includes\lock.inc).

Does your database happen to be 'juz' ?
What does line 167 of lock.inc say? Does it say {semaphore} ?
Do you have a table juz_semaphore ?

You might be better of undoing the patch, override sql-mode in settings.php, use table prefix for tables that are considered reserved words without them.

Database is not called juz. Yes - 167 of lock.inc says {semaphore}. Yes, I have a table "juz_semaphore". Checked these points before.
The prefix was inserted during installation process years ago.
Yes, perhaps eliminating the prefix is the easiest way.

Thanks for the quick responses. I assumed that was the current state of things but wanted to make sure.

You would need the prefix for table system or use the patch.

// Quote the table name.
+      $quoted .= '`' . $matches[3] . '`';

Should have {} around it.
Like:
$quoted .= '`{' . $matches[3] . '}`';

Looks like things are implemented in the wrong order. No need for checking for db.table format and ignoring prefix. Don't know about D7, but expect the prefix to introduce the db.table format. Not expecting existing code having {db.table}.

So, it should be the other way around. Change {table} to `{table}`, replace prefix, fix db.table format.

mmjvb wrote:

Should have {} around it.
Like:

$quoted .= '`{' . $matches[3] . '}`';

Good catch. It's not DatabaseConnection_mysql::query()'s job to remove the curly brackets from the query (DatabaseConnection::prefixTables() does that). After changing that, I also had to remove DatabaseConnection_mysql::prefixTables() which escaped table names that use reserved keywords (otherwise you'd get errors about "incorrect table names") - they are already escaped/quoted in query().

@pyQlo: This should now work with prefixes.

I'm in a hurry, but I did a quick test and #109 worked in another site using table prefixes. I'll test in detail later.
Great - thanks!!

Fixed the regular expression in DatabaseConnection_mysql::query() so that tests should pass now.

Successfully tested with and without table prefixes.

Without a patch, my D7/MySql 8 site won't even run ...

I have applied #112 and after about 20 minutes of clicking around on my site I haven't encountered any errors.

(This is already far better than the previous patches, where I quickly encountered fatal errors ... see #56)

So far so good.

After Drupal core 7.72 upgrade I reapplied patch 2978575-67. All is well again it seems, but there was one error when applying the patch... Hunk #1 FAILED at 343. I am still not sure what this affects. I am not seeing any issues on the site.

$ more schema.inc.rej

--- schema.inc
+++ schema.inc
@@ -343,7 +343,7 @@ public function findTables($table_expression) {
     // couldn't use db_select() here because it would prefix
     // information_schema.tables and the query would fail.
     // Don't use {} around information_schema.tables table.
-    return $this->connection->query("SELECT table_name FROM information_schema.
tables WHERE " . (string) $condition, $condition->arguments())->fetchAllKeyed(0,
 0);
+    return $this->connection->query("SELECT table_name as table_name FROM infor
mation_schema.tables WHERE " . (string) $condition, $condition->arguments())->fe
tchAllKeyed(0, 0);
   }

   /**

Just for kicks, I tried the latest patch and same failure... Hunk #1 FAILED at 343. All seems well on the site, despite this error.

$ more schema.inc.rej

--- schema.inc
+++ schema.inc
@@ -343,7 +343,7 @@ abstract class DatabaseSchema implements QueryPlaceholderInt
erface {
     // couldn't use db_select() here because it would prefix
     // information_schema.tables and the query would fail.
     // Don't use {} around information_schema.tables table.
-    return $this->connection->query("SELECT table_name FROM information_schema.
tables WHERE " . (string) $condition, $condition->arguments())->fetchAllKeyed(0,
 0);
+    return $this->connection->query("SELECT table_name as table_name FROM infor
mation_schema.tables WHERE " . (string) $condition, $condition->arguments())->fe
tchAllKeyed(0, 0);
   }

   /**

Thanks for the patch!

@buddym: #67 has known problems, including problems that you yourself pointed out in #88.

Don't use #67!

Use #112 - that is the latest patch under consideration, and it fixes the known problems with #67. Let's try to move forward with this issue - does #112 work for you?

The hunk failed error was due to my inexperience applying patches. I was moving the patch into the mysql directory and applying with a simple patch command like 'patch < patch.file'.

The solution was to move the patch.file to the root drupal directory and use this patch command 'patch -p1 < patch.file'.

The latest patch #112 applied without any failures. Thanks!

Here is the same patch as #112, with a demonstration of how to patch a custom drupalci.yml for d7 core tests. (includes core/drupalci.yml). This is only necessary until we are able to upgrade the drush version in the containers.

Just FYI Drush 8.3.3 contains the fixes needed so that Drush will run with MySql 8 and php 7.4.

However the latest release is 8.3.5

https://github.com/drush-ops/drush/releases/tag/8.3.5

@Mixologic: Thanks! That's going to be really useful - now we can run tests and see if the patch works with all edge cases. That wasn't possible before. I don't think any of the patches over the past 4 months would have tested green (it was easy enough to manually find problems with them) so we've wasted a lot of time considering patches which weren't good enough.

Now with #112 we have a patch that fails only 1 out of 2053 cases - that gives me a lot of confidence that if we can fix that one tiny problem then this patch might be ready to commit.

What needs to be done now is to find and fix this 1 failure. ALL PATCHES posted here should now include the same drupalci.yml that is included with #118, and ALL PATCHES should run a test against MySql 8 - that's the whole point of this issue, to make sure this works with MySql 8!

Changing tags. 7.72 is out.

This is a bug report at this point, right?

D7 is unusable on MySql 8 and upgrading to MySql 8 breaks sites.

MySql 8 has been out for 2 years and is currently the default in some major OS distributions.

If D7 errors on PHP 7.4, that's always been a bug, even before PHP 7.4 was officially supported. The DB version should get the same consideration.

Major or Critical? I'm leaning towards critical ...

'`system` system' => '`system` `system`',
Should probably be
'`{system}` system' => '`{system}` `system`',
to fix that one problem left.

mmjvb wrote:

    '`system` system' => '`system` `system`',

Should probably be

    '`{system}` system' => '`{system}` `system`',

to fix that one problem left.

That indeed does the trick! I added that to #118.

But shouldn't the line above

      ' system.' => ' `system`.',

then also become

      ' {system}.' => ' `{system}`.',

? Works both ways...

@Ronino: This issue is large and complex - an interdiff would be greatly appreciated so we could see exactly what you changed without having to do a line-by-line comparison to your previous patch.

@TR: That one time I didn't create an interdiff ;-). I changed exactly what mmjvb described, Here we go...

Suspect ' system.' to refer to the `system` in the line needing the change. So, it is the table alias used in a column reference. It shouldn't be prefixed.

Indeed, an interdiff would be welcome. Regardless whether you just add a file or change a line. It helps to see what was exactly changed. While you are at it, could you provide the missing one of #118 as well?

@Ronino: Thanks for the interdiff - that makes it a lot easier to follow what's going on here.

@mmjvb: The difference between #112 and #118 is just the drupalci.yml file. Here is the interdiff.

My only remaining question about #124 is why do we still need to have special-case code for Drush? If it's only a problem with Drush, this sounds like something that should be fixed in Drush.

+    // Make Drush work.
+    $query = strtr($query, array(
+      ' system.' => ' `system`.',
+      '`{system}` system' => '`{system}` `system`',
+    )); 

Trusted that to be the case, but the interdiff helps, thanks

Looked into the pattern for 'db.table' and noticed it is not using \w, any reason for that? In addition I would have expected the same expression for db and table, but that doesn't work, any idea?

Finally, nobody addressed my concern of introducing 'db.table' by prefix. Any users using the prefix to refer to tables in different database that can confirm this works for them?

Finally, nobody addressed my concern of introducing 'db.table' by prefix. Any users using the prefix to refer to tables in different database that can confirm this works for them?

Is this not covered by existing test cases? Do we need a new test case to ensure this works?

Upgrading to D7.72, MySql 8.0.20-0ubuntu0.20.04.1, PHP 7.4.3, Apache/2.4.41 with patch #124 worked like a charm!! Thanks Ronino

Similarly, brand new Ubuntu 20.04LTS cloud server with D7.72, Mysql 8, PHP 7.4 - #124 also working for me.

I fed $query = '{foobar.baz}'; into the regex and got the expected `foobar.`.`{baz}`.

If I had any say left in these matters -- I am not sure I do -- I would strongly advocate for removing this regex and move the escaping into the prefixing code. Yes, it won't support database but that should be a followup -- are you sure you want to preg_replace_replace every fixed string query on every D7 page load out there just for the sake of the miniscule amount of queries hardwiring a database?? Perhaps it should be gated by a variable. Anyways, I would say it should be a followup so it doesn't hold up this any longer, it's been long enough.

To fix either chop the match db. to get rid of the trailing .
or revert pattern to make a reference to db in addition to db. as it was before.

Haven't found a test case that uses per table prefix, but it appears that simpletest uses database prefix pointing to separate db. Don't understand why it works, but when tests are not failing. So, no need for additional test as every test is already testing it.

That also sounds like dealing with the db.table is needed for testing to work. So, not optional for later.

Charlie ChX Negyesi wrote:

I fed $query = '{foobar.baz}'; into the regex and got the expected `foobar.`.`{baz}`.

Thanks! Here's a fix.

It seems like no test covers this...

Added a test to DatabaseQueryTestCase to make sure that "{db.table}" is correctly prefixed.

The php containers now include drush 8.3.5, so future patches can remove the /core/drupalci.yml file.

Thanks to everyone working on this topic! I'd really appreciate a quick 7.73 core update including this feature!

I don't understand why this wasn't tackled earlier, and still is not being considered as critical. My hosting provider announced a OS update on July 1st (from Debian 8 LTS to Debian 10) which also contains the update to MySQL 8. I still have some older projects running there for which an update to Drupal 8 is out of question. It's hard to explain to customers that there's a chance that there won't be a stable core update by then, and we'll probably have to apply some patch, which is still being developed, evaluated and subject to further change by the community. Things like this are exactly why I shifted more and more projects to WordPress in the last years.

hello, we have a drupal 7 page for our sports club. the friend who made the website back then is unfortunately no longer with us. Unfortunately I don't understand how I can get the Drupal website up and running again. Is there a way to download the patched files? I also do not find a folder called Core in the Drupal 7 installation.

@pepeve: I uploaded the patched files for you (Drupal 7.72)

Unzip "core.zip" and put the content as folder /core/ in the root-folder (next to the folders "includes", "misc", "modules", ...).
Unzip "database-folder-patched-124.zip" and replace the folder /includes/database/

Hope it helps ...

Removed drupalci.yml per #138, thanks @Mixologic

Removed the workaround for drush as it's no longer necessary now that https://github.com/drush-ops/drush/pull/4313 is in recent drush releases (h/t @MustangGB). I suppose this means anyone using earlier versions of drush will have problems, but I don't think we should keep the workaround in core for that reason.

Only other change I've made is that $reservedKeyWords needs to use the old school array() syntax (to cater for all the sites that run PHP 5.3 along with MySQL 8 ;)

Some other thoughts:

1) As mentioned a few times in this issue, D9 no longer uses the list of reserved words but instead quotes everything. Just to see what would happen, I commented out the checking of the reserved words so that identifiers are always quoted, e.g.:

    //if (in_array(strtolower($identifier), $this->reservedKeyWords, TRUE)) {
      // Quote the string for MySQL reserved keywords.
      $identifier = '"' . $identifier . '"';
    //}

...and nothing bad happened AFAICS. I didn't test this extensively though. What would be wrong with this approach?

2) We're completely dropping NO_AUTO_CREATE_USER in the recent patches, as proposed in #67 by @Ayesh, but that's not what happens in D8/9. The simplicity of omitting it is appealing though.

3) Thanks for adding the test in #137 @Ronino. I believe there are more tests in the D8 issue that we should backport too.

4) The concerns expressed in #134 are concerning me.

5) Less of a concern, but I mentioned in #22 that the anonymous function as a callback here:

+    $insert_fields = array_map(function ($field) {
+      return $this->connection->escapeField($field);
+    }, $insert_fields);

...would be the first usage of this pattern in D7 core. How easy would that be to refactor?

6) Nit: I'd like to see the keyword AS capitalised in the schema queries, e.g.:

return $this->connection->query("SELECT table_name as table_name FROM information_schema.tables WHERE " . (string) $condition, $condition->arguments())->fetchAllKeyed(0, 0);

...however, it looks like it's been committed lowercase to D8/9. I'll file a follow-up for that.

5) Less of a concern, but I mentioned in #22 that the anonymous function as a callback here:

+ $insert_fields = array_map(function ($field) {
+ return $this->connection->escapeField($field);
+ }, $insert_fields);

...would be the first usage of this pattern in D7 core. How easy would that be to refactor?

It should be trivially possible to refactor this into a standard class method, and refer it in the array_map() call.

For example:

$insert_fields = array_map(array($this, 'escapeCallback'));

protected function escapeCallback($field) {
  return $this->connection->escapeField($field);
}

If I recall correctly, we made some changes to D7 core that is not compatible with PHP 5.2, so I'd vote to keep the anonymous function as it makes the code much cleaner.

Thanks @Ayesh. I don't think the anonymous function's a problem, but noteworthy if it's the first in D7 core; will see if @Fabianx is happy with it.

There's some syntax in the new test that PHP 5.3 doesn't like; hopefully this tweak will fix that.

The MySQL 8 test is also failing - that looks like the error from drush again doesn't it?

Unrelated: DrupalCI probably should `drush --version` right after the existing `php -v` so we know exactly what's going on.

Yes, that'd be helpful.

PHP Fatal error:  Using $this when not in object context in /var/www/html/includes/database/mysql/query.inc on line 52

It looks like PHP 5.3 might not like the reference to $this inside the anonymous function? Perhaps we'll need to refactor that after all.

However, it looks like recent versions of drush8 don't work with PHP 5.3 (because of [] array syntax AFAICS) so I'm struggling to try and reproduce the error the testbot seems to be hitting doing the initially install, e.g.:

drupal-7.x# ./drush-8.3.5.phar si
PHP Parse error:  syntax error, unexpected '[' in phar:///path/to/drush-8.3.5.phar/includes/startup.inc on line 63

So yes, I'm not sure what version of drush the PHP 5.3 testbot is using, but it seems unlikely it'll work with the MySQL 8 patch without the drush workaround included.

In #138 Mixologic said:

The php containers now include drush 8.3.5

is it possible to have a different version of drush run for php 5.3 ? IMHO, we can still support php 5.3, but not support php 5.3 with mysql 8
For my clients still using php 5.3, they are very unlikely to upgrade to mysql 8 without upgrading php.

I think it's reasonable to support mysql 8 for all versions of php except php 5.3. The key would be to change the drupal ci to use the older version of drush for ONLY php 5.3 and use drush 8.3.5 for all other versions of php.

automated testing for php 5.3 should be limited to mysql 5.5, 5.6, 5.7
automated testing for other php versions should include testing for mysql 8

AFAICS the current patch breaks PHP 5.3 regardless of the MySQL version, so we'll need to dig into that some more.

Here's a patch which restores the drush workaround in \DatabaseConnection_mysql::query() conditionally.

Let's see if doing so gets the MySQL 8 test passing again.

I'll have a look at what's going on with that one failing test.

Another issue in the meantime though; with all of the recent patches (e.g. back to #137), it looks like drush site-install will work if there's no existing database (which I think is what happens when the testbot runs). However, if there's already a db in place, I'm getting this error:

$ drush si
You are about to DROP all tables in your 'drupal7x' database. Do you want to continue? (y/n): y
Failed to create database: ERROR 1064 (42000) at line 1: 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 'system, taxonomy_index, taxonomy_term_data,                   [error]
taxonomy_term_hierarchy, taxonomy_vo' at line 1

Running drush sql-drop seems to work fine, and drush site-install works okay after that.

It turns out that the "drush workaround" wasn't only fixing things for old versions of drush.

DatabaseTemporaryQueryTestCase does this:

    // Now try to run two db_query_temporary() in the same request.
    $table_name_system = db_query_temporary('SELECT status FROM {system}', array());
    $table_name_users = db_query_temporary('SELECT uid FROM {users}', array());

    $this->assertEqual($this->countTableRows($table_name_system), $this->countTableRows("system"), 'A temporary table was created successfully in this request.');

https://git.drupalcode.org/project/drupal/-/blob/7.72/modules/simpletest...

I believe that's the code that's hitting an error if we don't have the workaround (unconditionally) for the system table in \DatabaseConnection_mysql::query().

Specifically, it's this substitution which allows that test to run:

'`{system}` system' => '`{system}` `system`',

So we can either put that workaround back in (and not label it as being drush-specific), or figure out why this is necessary and see if it can be addressed another way.

It concerns me a little to have such a specific workaround; I'd imagine the problem could happen for other queries if there's an alias that is a reserved word which is not being quoted.

The test is failing when this happens in the assertion in \DatabaseTemporaryQueryTestCase::testTemporaryQuery

$this->countTableRows("system")

That does (comments added to show param values):

db_select($table_name)->countQuery()->execute()->fetchField(); // $table_name = 'system'

The db_select() call results in the \SelectQuery::__construct() being called with $table = 'system' and $alias = NULL (the default).

The last line of that constructor calls \SelectQuery::addJoin() which then copies $table to $alias if the latter is empty.

It would appear that alias is not escaped / quoted unless that happens via the "drush workaround" in \DatabaseConnection_mysql::query.

Without the workaround, this query is executed:

SELECT COUNT(*) AS expression
FROM 
(SELECT 1 AS expression
FROM 
`{system}` system) subquery

...and that unquoted alias causes the error in MySQL 8 as it's a reserved word.

We can reproduce the error using drush:

$ drush ev "var_dump(db_select('system')->countQuery()->execute()->fetchField());"
PDOException: 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 'system) subquery' at line 5 in                        [error]
/path/to/drupal-7.x/includes/database/database.inc:2227
Stack trace:
#0 /path/to/drupal-7.x/includes/database/database.inc(2227): PDOStatement->execute(Array)
#1 /path/to/drupal-7.x/includes/database/database.inc(697): DatabaseStatementBase->execute(Array, Array)
#2 /path/to/drupal-7.x/includes/database/mysql/database.inc(446): DatabaseConnection->query('SELECT COUNT(*)...', Array, Array)
#3 /path/to/drupal-7.x/includes/database/select.inc(1280): DatabaseConnection_mysql->query('SELECT COUNT(*)...', Array, Array)

I'm not entirely sure what the best solution to this would be; I don't think leaving it as a hacky workaround is very satisfactory.

One reason why is that the workaround is obviously specific to the one table name "system". The problem would be the same though if a site had another table with a name that conflicts with reserved words. For example:

mysql> CREATE TABLE `table` (`id` int);
Query OK, 0 rows affected (0.10 sec)

$ drush ev "var_dump(db_select('table')->countQuery()->execute()->fetchField());"
PDOException: 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 'table) subquery' at line 5 in                         [error]
/path/to/drupal-7.x/drupal-7.x/includes/database/database.inc:2227

Exactly the same problem with the table name being copied as the alias with no quoting.

The code looks very similar, FWIW, in D8's \Drupal\Core\Database\Query\Select::addJoin.

To solve, I see 2 options:
- Also always quote each and any alias that gets generated.
- Generate aliases that will never result in a reserved word, e.g. by letting it start with an underscore (seems to be allowed).

The "always quote each and any alias that gets generated" is part of what was done in #2986452: Database reserved keywords need to be quoted as per the ANSI standard IIUC, but that required such significant changes that it only went into D9 and wasn't backported to D8.

I'm not intimately familiar with every moving part of the DB API here, so I may well be missing something, but I'm not yet certain why the query causing the test fail is apparently not having its alias quoted already by \DatabaseConnection_mysql::escapeAlias(); it is a reserved word so if that method was being called, it would be quoted.

It may be because this particular way of constructing and executing a SelectQuery doesn't end up invoking \SelectQuery::__toString and therefore escapeAlias is not called. I'm not certain about that yet though.

I don't expect the approach in this patch to be the eventual solution, but I'd like to see whether with this all tests for MySQL 8 pass.

As suggested in #134 perhaps we could look at moving this into somewhere like \DatabaseConnection::setPrefix instead.

Oh, and I did also look at changing the alias being added in \SelectQuery::addJoin() when the table name is copied, but doing so broke a lot of queries as the field names being used no longer matched the alias. There's probably a better way of doing that if we wanted to take this approach though.

AFAICS this might be a problem in 8.9.x too:

mysql> SHOW VARIABLES LIKE "version";
+---------------+-----------------+
| Variable_name | Value           |
+---------------+-----------------+
| version       | 8.0.19-0ubuntu4 |
+---------------+-----------------+

mysql> CREATE TABLE `table` (`id` int);
Query OK, 0 rows affected (0.23 sec)

$ drush st --fields='Drupal version'
 Drupal version   :  8.9.2-dev

$ drush ev "var_dump(db_select('table')->countQuery()->execute()->fetchField());"
PDOException: 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 'table table) subquery' at line 5 in                   [error]
/path/to/drupal-8.x/core/lib/Drupal/Core/Database/Statement.php:59
Stack trace:
#0 /path/to/drupal-8.x/core/lib/Drupal/Core/Database/Statement.php(59): PDOStatement->execute(Array)
#1 /path/to/drupal-8.x/core/lib/Drupal/Core/Database/Connection.php(640): Drupal\Core\Database\Statement->execute(Array, Array)
#2 /path/to/drupal-8.x/core/lib/Drupal/Core/Database/Driver/mysql/Connection.php(357): Drupal\Core\Database\Connection->query('SELECT COUNT(*)...', Array, Array)
#3 /path/to/drupal-8.x/core/lib/Drupal/Core/Database/Query/Select.php(510): Drupal\Core\Database\Driver\mysql\Connection->query('SELECT COUNT(*)...', Array, Array)

[...snip...]

Next Drupal\Core\Database\DatabaseExceptionWrapper: 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 'table table)
subquery' at line 5: SELECT COUNT(*) AS expression
FROM
(SELECT 1 AS expression
FROM
{table} table) subquery; Array
(
)

So perhaps there's a difference in test coverage? (Trying hard to avoid any topical references about that here... ;)

We'll have to decide how we're going to approach this in 7.x

There seem to be a few things we'll need to address AFAICS; I'll try to summarise them in due course.

I've been contemplating implementing a different DB driver for MySQL 8, which I think may have some advantages for D7; it would hopefully avoid any regressions (performance or functionality) for sites that remain on MySQL 5.x

Filed #3155563: select query should quote aliases which are reserved words in MySQL against 8.9.x

I think I've made some progress in the 8.9.x issue, and hopefully this patch will get tests passing without any hacky workarounds to fix reserved word tablenames with unquoted aliases.

So I think it was definitely a bug in includes/database/select.inc's \SelectQuery::__toString where the alias was being passed through escapeTable rather than escapeAlias and therefore the mysql driver wasn't quoting the alias if it was a reserved word. I believe that's actually why drush was breaking; with the latest patch (#157) even older versions of drush seem to work okay. The D8 issue for that may need more work changing the type of test etc... but I think we should definitely keep the change in the overall MySQL 8 patch here. (See the interdiff in #157 for details).

I mentioned there being a few more issues I think we need to address; here are some of them:

Much as I can see the appeal in the simplicity of doing this:

     $connection_options['init_commands'] += array(
-      'sql_mode' => "SET sql_mode = 'REAL_AS_FLOAT,PIPES_AS_CONCAT,ANSI_QUOTES,IGNORE_SPACE,STRICT_TRANS_TABLES,STRICT_ALL_TABLES,NO_ZERO_IN_DATE,NO_ZERO_DATE,ERROR_FOR_DIVISION_BY_ZERO,NO_AUTO_CREATE_USER'",
+      'sql_mode' => "SET sql_mode = 'REAL_AS_FLOAT,PIPES_AS_CONCAT,ANSI_QUOTES,IGNORE_SPACE,STRICT_TRANS_TABLES,STRICT_ALL_TABLES,NO_ZERO_IN_DATE,NO_ZERO_DATE,ERROR_FOR_DIVISION_BY_ZERO'",
     );

...I think we ought to make that change conditional on the MySQL version. That's what D8 does, and it reduces the risk of unintended consequences of making this change unconditionally.

I have some concerns in general about the performance cost of the changes we're making here - e.g. running all queries through a regex to quote table names (see #134).

If there was no need to do this for MySQL versions before 8, I think we should avoid doing it for older versions if we can.

I tried to do some performance testing locally but it takes a really long time for the whole test suite to run for me; perhaps we can try a few A/B tests with the testbot infra to see if it looks like there is a noticeable performance degradation introduced by the recent patches. That would be interesting to know.

For that purpose, I'm attaching a noop patch which we can use to compare how long the test suite takes to run without the changes in e.g. #157.

Similarly, it looks like some of the changes being introduced break PHP 5.3, e.g.:

includes/database/mysql/query.inc
@@ -48,6 +48,10 @@ class InsertQuery_mysql extends InsertQuery {
     // Default fields are always placed first for consistency.
     $insert_fields = array_merge($this->defaultFields, $this->insertFields);
 
+    $insert_fields = array_map(function ($field) {
+      return $this->connection->escapeField($field);
+    }, $insert_fields);
+

...again if we don't need to do this for older versions of MySQL, I wonder if we can avoid the PHP 5.3 problem by making it conditional on the MySQL version. That may or may not work as some syntax issues will cause problems whether the code is actually executed or not.

There is #3155190: [meta] Should Drupal 7 drop support for older PHP versions? if anyone has thoughts on continued support for older PHP versions.

I did look at the option of having a completely separate driver for MySQL 8 (or perhaps MySQL > 5). It wasn't hard to get that working for core, but my initial experiments ran into problems with drush not being able to find the appropriate SQL class, which would be a deal breaker. It may be that it's possible to work around it, but I didn't carry on much beyond establishing that using a different driver name than mysql seemed to make drush (8) break.

All that said, I think it would be good to make any of the changes we're making inside the mysql driver conditional on the MySQL version. If we can do that cheaply, I think there may be benefits in terms of avoiding (performance and functionality) regressions with older versions of both PHP and MySQL (which D8/9 are not as concerned about as D7).

I would also like to see more tests.

Quick attempt to (re-)implement the approach of making changes in the mysql driver conditional on whether the db is actually MySQL 8 (or later).

One potential problem is that the getter method $this->connection->isMySQL8OrLater() only exists in the mysql driver's implementation of this class. I don't think practically that should be a problem, but my IDE at least is trying to warn me about this being potentially polymorphic.

Let's see if it actually works before worrying too much about that.

Also, naming things is hard :) I'm not especially keen on $this->mysql8OrLater and am certainly open to alternative suggestions.

Hmm, more speed lest haste. Let's try that again.

...and again without the [] array copy-pasta.

Interesting, the only test which is failing is the new one introduced with these patches.

      // "{table}" as well as "{db.table}" need to work.
      db_query("SELECT * FROM {" . $database . ".test} LIMIT 1")->fetchObject();
      $this->pass('Correctly apply prefixes if the table is specified along with the database.');

Now that we've made this conditional on the MySQL version:

  public function query($query, array $args = array(), $options = array()) {
    if ($this->mysql8OrLater) {
      // Match {table} as well as {db.table} and quote both DB and table with
      // backticks.
      $query = preg_replace_callback('/\{(([^.\}]+)\.)?(.*?)\}/', function ($matches) {

...that test isn't going to work with MySQL 5.x.

Is this testing the right thing? Being able to specify both {db.table} within the curly braces doesn't actually seem like part of adding MySQL 8 support to me, unless I'm missing something?

I appreciate that if we're quoting table names for MySQL 8 then that code has to work if the db has been specified too, but does this test failing with MySQL 5.x reflect something that's actually broken? If so, I'd think that would be a separate issue.

Adding a new patch to queue and run tests in Drupal infra.

- Refactored the regular expression to not use backslashes where not necessary.
- Made the database name match (group 2) a lazy qualifier for performance.
- Renamed `isMySQL8OrLater` to `needsReservedKeywordEscape` to make the intend clear. This follows other class properties available.
- Added a new `escapeFields` method to connection class that accepts an array of fields, and runs `escapeField` on it. This avoids the closure usage.
- Refactored table escaping to a separate method. It's currently a protected method, it can probably make it easier to run tests in unit-test style because it's database connection-independent.
- Updated the test run `SELECT.... {table}` and `SELECT... {database.table}` style queries, so the tests are more verbose.

Is this testing the right thing? Being able to specify both {db.table} within the curly braces doesn't actually seem like part of adding MySQL 8 support to me, unless I'm missing something?

I appreciate that if we're quoting table names for MySQL 8 then that code has to work if the db has been specified too, but does this test failing with MySQL 5.x reflect something that's actually broken? If so, I'd think that would be a separate issue

I think this brings an interesting point, as to if we should even bother with {db.table} style keywords. This is not the correct practices, and Drupal should be let to add the prefix, or the caller should take care to properly add database AND prefix. Right now, we let Drupal prefix a table when a database name is explicitly mentioned, which might not be the same database Drupal knows the prefix for.

Trying this again with {db.table} support removed.

mcdruid wrote in #165:

Is this testing the right thing? Being able to specify both {db.table} within the curly braces doesn't actually seem like part of adding MySQL 8 support to me, unless I'm missing something?

The reason this must be handled for MySQL 8 is that every table name is quoted and you cannot quote {db.table} to `db.table`, but rather need to make it `db`.`table`. This special handling is not necessary for MySQL 5 as there's no quotation.

I appreciate that if we're quoting table names for MySQL 8 then that code has to work if the db has been specified too, but does this test failing with MySQL 5.x reflect something that's actually broken? If so, I'd think that would be a separate issue.

Patches up to #157 worked fine with both MySQL 5 and 8.

Ayesh wrote in #169:

Trying this again with {db.table} support removed.

The migrate module relies on that to query a migration database that is different from the Drupal database. Specifying db.table is perfectly valid SQL syntax, worked with MySQL 5 and must work with MySQL 8, too.

Agree with the above mentioned in #173.

Do think changing db.table is easier after applying prefix instead of before. A matter of replacing . with `.` when there is a . within ``. The benefit would be when the per table prefix is used to refer to a table in different database it will be treated correctly.

@Ayesh thanks, I like a lot of your improvements, particularly needsReservedKeywordEscape :)

@Ronino, @mmjvb yup okay, I can see that if we're quoting all table names for MySQL that we don't want to end up with `db.table` (which IIUC might be why the most recent patches failed?)

I wasn't sure though why the new test failed for #162 when there should have been no quoting happening.

AFAICS in 7.x without any of the patches from this issue (and a MySQL 5 db), these all work:

• db_query("SELECT COUNT(1) FROM {drupal7x.users};")
• db_query("SELECT COUNT(1) FROM drupal7x.{users};")
• db_query("SELECT COUNT(1) FROM {drupal7x}.users;")

...and produce the same SQL queryString, namely: SELECT COUNT(1) FROM drupal7x.users;

However, if we use a prefix for the db tables db_query("SELECT COUNT(1) FROM drupal7x.{users};") works okay.

...but drush ev 'var_dump(db_query("SELECT COUNT(1) FROM {drupal7x.users};"));' doesn't.

That seems to be because Drupal tries to apply the prefix to the db name e.g. "SELECT COUNT(1) FROM drupal7x.foousers;" is the working SQL but {drupal7x.users} gives us:

PDOException: SQLSTATE[42S02]: Base table or view not found: 1146 Table 'foodrupal7x.users' doesn't exist

Whereas with the quoting code in place db_query("SELECT COUNT(1) FROM {drupal7x.users};") produces:

SELECT COUNT(1) FROM `drupal7x`.`foousers`;

So that'd be why the new test failed for {db.table} without the quoting for MySQL 8; when prefixes are being used (which they always are on the testbot) {db.table} doesn't work in D7 as it stands. I think the quoting is almost fixing that by accident (apologies if not and others were aware of exactly how this was working already!)

I think that's more what I meant by "Is this testing the right thing?". If e.g. migrate uses db.table syntax now (which makes sense) I'm guessing it doesn't wrap both in curly braces.

Thanks for the explanations @mcdruid, @Ronino, @mmjvb.

It's probably better if we establish some test strings, so the query escape method can be tested easily.

1. SELECT * FROM system -> SELECT * FROM system (we cannot do this with a regex query easily)
2. SELECT * FROM {system} -> SELECT * FROM `{system}` (parent class should add the prefix)
3. SELECT * FROM {db.system} -> SELECT * FROM `db`.`{system}` (Is this right? This seems to be the correct syntax for parent class to add prefix)
4. SELECT * FROM db.system -> SELECT * FROM db.system : Same as #1

Current patches until #166 appears to do this string transformation correctly: https://3v4l.org/YtN8T

Trying this again, with flexible {db.table} and {table}

178th time's the charm.
Rebased the patch from 157, where the last time we had MySQL 8 builds passing.

I think recent patches are missing this fix (from #157):

includes/database/select.inc
		@@ -1555,7 +1555,7 @@ class SelectQuery extends Query implements SelectQueryInterface {
		 
		       // Don't use the AS keyword for table aliases, as some
		       // databases don't support it (e.g., Oracle).
		-      $query .=  $table_string . ' ' . $this->connection->escapeTable($table['alias']);
		+      $query .=  $table_string . ' ' . $this->connection->escapeAlias($table['alias']);

...which is why we'd be seeing MySQL 8 fails like:

SELECT COUNT(*) AS expression
FROM 
(SELECT 1 AS expression
FROM 
`{system}` system) subquery;

@Ronino said in #173

The migrate module relies on that to query a migration database that is different from the Drupal database. Specifying db.table is perfectly valid SQL syntax, worked with MySQL 5 and must work with MySQL 8, too.

Yes db.table has to work, but {db.table} doesn't work in D7 if prefixes are in use, hence the reason that the new test for that is failing when we don't do quoting for MySQL 5 (i.e. don't change the existing behaviour for MySQL 5).

Does migrate - or anything else - actually use {db.table} where the db name is inside the curly braces? If it does then it seems to me that would not work with table prefixes. If not, I think we could simplify the quoting, possibly doing away with the regex entirely and going back to adding quotes as part of the prefixing code (e.g. #59).

AFAICS the only reason {db.table} is working when the regex-based quoting code is added is that we're removing the curly braces and replacing them only around the table name.

I've got a patch working which goes back to the approach in #59 whereby \DatabaseConnection_mysql::setPrefix is used to inject backticks into the prefix replacement patterns.

One slight annoyance is that this method is called before we've had a chance to set the needsReservedKeywordEscape property in the constructor, so I've moved the order of that around a little. An alternative to this would be to always put the backticks in without checking the MySQL version; that would mean we wouldn't have to change the order of the function calls in \DatabaseConnection::__construct which might be preferable.

This means we don't need to implement the query method in the mysql driver to run all queries through a regex.

So yes in this case {db.table} will not work, but it doesn't work now (at least it's not compatible with table prefixes) with MySQL 5.

I've updated the new tests to reflect what should and should not be working in terms of the db.table syntax.

@Ronino you've said a few times that {db.table} needs to work; can you provide some examples of where that pattern is used? If migrate does that, I think we could argue it's a bug in migrate as I don't think that'll work today if table prefixes are in place.

Before I forget, there's a comment in \DatabaseConnection_mysql::quoteIdentifier which I think comes from the D8 implementation, and I'm not sure it's actually relevant / accurate for D7? Let's check that. That method - incidentally - takes care of the db.table syntax if it's passed an identifier that includes a dot; perhaps that's sufficient?

I've also got a patch for drush 8 without which some commands don't work under MySQL 8; the patch wraps backticks around each table name when drush uses SHOW TABLES; in the listTables method of its mysql class. Otherwise you can't e.g drop all tables which means site-install only works if the db is empty to begin with. I'll submit that patch to drush and link the PR here. With any luck this doesn't cause a problem on the testbot as we start with an empty db.

Also, more tests would be good :)

https://github.com/drush-ops/drush/pull/4456 "Wrap mysql table names in backticks to avoid errors in MySQL 8"

I think this fixes the first failing test; we were ending up with table names like 'simpletest858168`foo` if the prefix is already wrapped in backticks, so this removes a backtick at the end of the prefix replacement pattern.

I am not certain how we end up with a prefix already wrapped in backticks though, and that would be good to know.

I think this should hopefully fix the next test fail; some schema queries such as those called by db_find_tables() don't want the backticks to be wrapped around table names, so this removes any that are there.

Restoring the getter for needsReservedKeywordEscape makes this hack a little cleaner, but technically we're back to the polymorphic problem.

Again, if we could avoid putting the backticks there in the first place that'd be better.

Looks like all the remaining failures are EntityFieldQuery related.

I'm not sure what's going on there yet, but FWIW here's an example of the query that was generated locally by the "Test sort entity entity_id property in descending order, with a field condition" test:

SELECT DISTINCT field_data_yilecucz_field_name0.entity_type AS entity_type, field_data_yilecucz_field_name0.entity_id AS entity_id, field_data_yilecucz_field_name0.revision_id AS revision_id, field_data_yilecucz_field_name0.bundle AS bundle
FROM 
`simpletest175103field_data_yilecucz_field_name` field_data_yilecucz_field_name0
INNER JOIN `simpletest175103test_entity_bundle_key` test_entity_bundle_key ON test_entity_bundle_key.ftid = field_data_yilecucz_field_name0.entity_id
WHERE  (field_data_yilecucz_field_name0.yilecucz_field_name_value > :db_condition_placeholder_0) AND (field_data_yilecucz_field_name0.deleted = :db_condition_placeholder_1) AND (field_data_yilecucz_field_name0.entity_type = :db_condition_placeholder_2) 
ORDER BY test_entity_bundle_key.ftid DESC

The error message is:

fail: [Other] Line 333 of modules/simpletest/tests/entity_query.test:
Exception thrown: SQLSTATE[HY000]: General error: 3065 Expression #1 of ORDER BY clause is not in SELECT list, references column 'jenkins_drupal_d7_78663.test_entity_bundle_key.ftid' which is not in SELECT list; this is incompatible with DISTINCT

So yes, test_entity_bundle_key.ftid is not in the SELECT list. I am not sure why this is happening yet though. The prefixing and quoting actually looks like it's working okay, but perhaps something's interfering with the logic when the EntityQuery is being assembled?

Oh dear, I am an idiot.

In #159 I took the $sql_mode code from D8 and the string is quite different to D7. I'm actually surprised it didn't break more things, but that's the reason that EntityQuery tests have been failing since then; specifically it's ONLY_FULL_GROUP_BY which has been causing the "Expression #1 of ORDER BY clause is not in SELECT list, references column $XYZ which is not in SELECT list; this is incompatible with DISTINCT" errors. Sorry about that.

So, with that fixed we should hopefully be back to looking at what I think are two options.

1) Use \DatabaseConnection_mysql::setPrefix to put backticks into the prefixReplace patterns so that when prefixing is applied to table names the curly braces are effectively replaced by backticks (plus any actual prefix).

This has the advantage that we're not running every single query through a regex; in fact we're not really adding any extra processing to the SQL.

It has meant making a few seemingly hacky changes to make tests pass (see interdiffs for #182 and #183). I would perhaps like to understand why we're having to do those workarounds better, but AFAICS they may be down to tests doing slightly unusual things with the prefixing anyway.

2) Go with a regex in \DatabaseConnection_mysql::query which searches for {table} in SQL and wraps that in backticks before the prefix step.

That approach doesn't seem to require the same workarounds to get tests passing, but it's potentially quite expensive. See #134

We need to establish whether we actually need to accommodate {db.table} in the regex; see discussion of that in e.g. #180

This patch uses option 1. I'll also get one ready that does 2.

...and here's a patch using option 2 outlined above.

This undoes the workaround to remove backticks from schema queries which seems to be necessary with the prefix approach.

It's also not necessary to change the order of calls in \DatabaseConnection::__construct (which is required if we override setPrefix for mysql).

This patch uses the simple preg_replace which only looks for {table} and not also {db.table}.

Thanks a lot @mcdruid for working on this. I would have never figured out the SQL mode changes which lead to the MySQL 8 failures.

I would personally, and strongly prefer the approach in #1 (with setPrefix) because it looks cleaner on the long run. It looks a bit like a hack to get those table prefixes applied (i.e using str_replace), but I think it aligns with how rest of the things work in core. It's also a one-time cost, so we definitely score some points in the performance front.

I also like the fact that we don't even need to override the quote() method.

---

In this patch, I worked a little bit on #185 patch (approach #1).

- Moved $this->needsReservedKeywordEscape = version_compare($mysql_server_version, '8', '>='); to constructor. This makes the property is available for all other purposes. I'm not 100% sure about this, but I think Drupal infra tests pass because it always contains the prefix.

- Replaced a subtr call with an string-array-access call (micro performance improvement)

- Removed a $info = $this->connection->getConnectionOptions(); because we no longer use this variable.

As Chx mentioned about the regular expression, I would like to propose that we take a look at our excessive in_array() calls. As of now, it is called for every quoteIdentifier, which can about as many, if not multiple times more, of the number of queries we run during a page load. The array contains ~260 items, and because they are reserved keywords, it is likely that the in_array() yields no results.

in_array() calls can be optimized easily with a flipped array lookup with isset(). in_array() is an O(n) operation while isset() is O(1). isset() is about 50-100 times faster: https://3v4l.org/2R4Dg

If there is a room to improve it, I like to propose that we make that optimization at the same time on similar grounds that we find the regex call concerning. I will attach a patch for reference. In my sites, I already use it with isset() tweaks (base patch #65).

Thanks @Ayesh, but I cross posted this with your patch. I need to stop for the day now so I'll repost this as-is.

Any of #187's changes would need to be re-applied to this patch; the change I'm making here is very simple and specific, but this is certainly not me rejecting anything from 187... I just don't have time to look at it and merge changes right now.

Original comment:

The reason we're having to do the extra checking for a prefix which has already been quoted in backticks (see interdiff in #182) is that \DrupalUnitTestCase::setUp does this:

    // Generate temporary prefixed database to ensure that tests have a clean starting point.
    $this->databasePrefix = Database::getConnection()->prefixTables('{simpletest' . mt_rand(1000, 1000000) . '}');

...so that prefix is wrapped in curly braces so that it itself will be prefixed :) As we're using \DatabaseConnection_mysql::setPrefix to put backticks into the prefixReplace patterns we end up with weird table names like 'simpletest858168`foo`.

That's very specific to DrupalUnitTestCase though, so if we want to avoid the added complexity in the mysql driver's setPrefix method, we could look to adjust the way that DrupalUnitTestCase is setting up the prefixed db for testing and try to avoid the situation where the prefix is wrapped in backticks before we try to add our own.

On the other hand, the setPrefix method doesn't get called many times, so a little extra logic in there to ensure that MySQL unit tests keep working isn't the end of the world.

This patch is - I think - a more correct way of avoiding problems with DrupalUnitTestCase; the previous approach of just removing a backtick at the end of the prefix didn't work if the site running the tests also uses a prefix.

Interdiff against 185 rather than 186 because this is an enhancement to approach 1 as outlined there; pretty much a different feature branch to 186.

Thanks @mcdruid.
I rebased the patch in #187 for #188.

Turns out moving the version_compare() call to construct failed the tests.

Patch in #190 is based on #185.

- Replaced a subtr call with an string-array-access call (micro performance improvement)

- Removed a $info = $this->connection->getConnectionOptions(); because we no longer use this variable.

Giving up :)
I vote for the one in #188 where all tests work for all PHP versions tested against.

Thanks @Ayesh. Yeah, I think setPrefix is called earlier than where we were setting $this->needsReservedKeywordEscape in the constructor, which is why I moved that assignment there.

We could check the PDO attribute directly within setPrefix and still move the assignment back into the constructor, which might be cleaner to read. I didn't want to do the assignment twice though.

Great that we're passing tests again. Remaining todos AFAICS:

• More tests - check what D8 added and see if we can do the same.
• Verify we don't need {db.table} to work.
• Check the comment in \DatabaseConnection_mysql::quoteIdentifier which I think comes from D8 and may not apply in D7.
• Check comments in general - should we add any more explanation of changes we're making?

I agree that it's not ideal that we're iterating through the array of reserved words, but that's what D8's doing. I don't see a massive performance impact from the timings of test runs, but haven't looked in too much detail. If we wanted to change that implementation (e.g. isset instead of in_array) that might be something to address in D8 first and backport.

Anything else?

Would be good to get some testing of #188 on some real sites before we mark this as RTBC.

Little bit of tidying up as mentioned in the last comment:

• Move assignment of $this->needsReservedKeywordEscape to the constructor, and check the MySQL version directly in setPrefix.
• Comments tidy up, including providing a D7 example of table.column being passed as an identifier to \DatabaseConnection_mysql::quoteIdentifier.
• Simplify the logic in setPrefix which removes any backticks already in the prefix.

More tests is still a todo.

Interdiff with #188 per #192

I'm using the latest patch #194 on MySQL 8.0.20 on a test site (with PHP 7.4), and everything looks to be working great. I agree that we might need more tests added though.

Submitting a patch with a new test containing 3 more assertions.

FWIW I've been running the patch from #141 on a production site for the last week with no issues. Would be happy to try a newer patch but I might need some help applying it.

The server is Ubuntu 20.04 so am running PHP 7.4.3 with MySQL 8.0.20-0ubuntu0.20.04.1

Would be great to get this into core.

I've moved the new tests into their own test case, and added a few more which involve a test table that uses reserved keywords that MySQL 8 doesn't like if they're not escaped.

It's slightly tricky (as we saw with the test fail in #196) to generate SQL that MySQL 5 tolerates but that would fail in MySQL 8 without the escaping.

I'm also not sure what other dbs (postgres / SQLite) are going to make of these tests; we could look at skipping the tests if the db driver is not mysql, which I think happens elsewhere.

Ah, I think I've come across a problem when working on improving the tests using the new test table.

$ drush ev "print (string) db_select('virtual')->fields('virtual', array('function'));"
SELECT virtual."function" AS "function"
FROM 
{virtual} "virtual"

Looks like the alias in the SELECT list is not being quoted / escaped there, and indeed it doesn't work:

$ drush ev "var_dump(db_select('virtual')->fields('virtual', array('function'))->execute());"
PDOException: 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 'virtual."function" AS "function"                      [error]
FROM 
`virtual` "virtual"' at line 1 in ...