MySQL 8 compatibility

[f1mishutka]

'system' is reserved keyword in MySQL since version 8.0.0 so need to escape it to be able to select from system table.

There is a list of all reserved keywords, so escaping them all to make it more universal.

Changes are based on Drupal 7 and 8 patches for MySQL 8 support.

[f1mishutka]

I've managed to run complicated Drupal 6 site on MySQL 8.0.15 and PHP 7.3 using this patch.

So it seems to be ready to be merged for everyone.

P.S.: I'm new to GitHub workflows so I'll be thankful if someone will review my changes. There are not to much of them. Thanks.

[dsnopek]

Thanks for the PR! I've put some comments on the changes.

[dsnopek]

Thinking about this a little more: adding backticks is MySQL specific, so we'll need to make sure we're somehow adding the right marker for identifiers. In ANSI SQL, it's a double quote, and I actually have no idea what it is for Postgres, only that it's different than MySQL. :-)

Looking at how Drupal 8 is approaching this problem, this issue is proposing adding a new syntax with square brackets that'll be converted to the right identifier marker for the database:

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

I don't think that necessarily makes sense for Drupal 6, but just to point out that this is a kind of unsolved problem even for Drupal 8.

[f1mishutka]

Thank you David!

Will review your note in a couple days and make required changes.

[f1mishutka]

David, I've replied to all your notes. All required changes made to source code. Please let me know if you have any other thoughts on this.

Thank you!

[dsnopek]

Thanks!

I think the main remaining thing here is that backticks are a MySQL-specific thing, and we'd need to make sure that any code that'll run for any database (ex. drupal_write_record(), db_prefix_tables(), etc) use the right method to escape column names for the database that's in use.

[f1mishutka]

I think the main remaining thing here is that backticks are a MySQL-specific thing, and we'd need to make sure that any code that'll run for any database (ex. drupal_write_record(), db_prefix_tables(), etc) use the right method to escape column names for the database that's in use.

Both this methods check MySQL version before table names escaping. So there will no backticks added for MySQL 5.7 or any version of Postgres.

[dsnopek]

Ah, ok, great! Somehow I didn't notice that when I was looking at the diff before, but I see it now. :-) I'll try to do some testing of this when I get a chance. Thanks!

[f1mishutka]

Thank you, David!

Please let me know if you have any other thoughts or suggestions.

[f1mishutka]

Just merged changes from 6.50 version.

Any chances to merge this pull request to original repository? :)

[dsnopek]

Yes, I'd really like to merge this! I just need to find some time to properly test it. Hopefully, I'll get to it soon!

[f1mishutka]

Thank you, David.

Please let me know if I can help you somehow with testing.

P.S.: I use d6lts with this patch to run D6-based website of local cycling community( >6k users, >16k nodes). And it works fine since beginning of March 2019 on MySQL 8.0 and PHP 7.3. Performance increased at least twice comparing to MySQl 5.7 + PHP 5.3. Really happy about this migration! :)

[dsnopek]

I did a whole bunch of testing and review today! I also pushed some additional commits.

Here's the highlights of the code changes:

• Updated the code for Drupal coding standards, which was mostly comments and spaces
• Renamed db_is_version() to db_version_compare() for naming parity with version_compare()
• Replaced anonymous function with a foreach() loop for PHP 5.2 compatibility. (We've been trying to retain PHP 5.2 compatibility in order to not discourage people from applying the latest security updates. We can't do that forever, but so far, it's been easy enough to do.)
• Put the MySQL 8 settings changes that were previously only in 'mysqli' to be in 'mysql' too. I was able to get the plain 'mysql' database driver in PHP 5.6 to connect to MySQL 8 by: -- ... adding this line to to the '[mysqld]' section of my.conf: default-authentication-plugin=mysql_native_password -- ... and running this SQL statement to update the password of my database user: ALTER USER 'drupal6'@'%' IDENTIFIED WITH mysql_native_password BY 'drupal6';

I tested with CCK, Views and Webform. I didn't do very extensive tests with those modules, I just poked around and made some example content, views, forms, etc. It all seems to be working!

[f1mishutka]

David,

Thank you for great job done!

[f1mishutka]

David, I looked through all your commits: every change seems reasonable. So one more time thank you for you time!

Switched https://veloby.net (cycling community website I mentioned) to d6lts:6.x brach. Everything seems to work fine, and I consider this as "extensive tests" under real load :)

[neubreed]

Thanks for working on this! In my case I have multiple database connections so $db_url can be an array to define multiple database connections

So I amended db_is_version() to get the 'default' db url

function db_is_version($driver, $version, $operator = '>=') {
  global $db_url;

  $default_db_url = is_array($db_url) ? $db_url['default'] : $db_url;

  if(substr($default_db_url, 0, strlen($driver) ) === $driver) {
    if(version_compare(db_version(), $version, $operator)) {
      return true;
    }
  }

  return false;
}

[f1mishutka]

Hi @neubreed ,

Thank you for feedback!

So I amended db_is_version() to get the 'default' db url

This function was renamed to db_version_compare() and there is already check for an array in it. So as I see it should work properly with multiple connections specified.

[neubreed]

Hi @f1mishutka

Thanks for getting back to me .. initially I only patched just this file in my Drupal 6 install .. once I replaced my core with the full d6lts it works great.

Thanks a lot.