Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MySQL 8 compatibility #35

Open
wants to merge 10 commits into
base: 6.x
from

Conversation

Projects
None yet
2 participants
@f1mishutka
Copy link

f1mishutka commented Mar 7, 2019

'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

This comment has been minimized.

Copy link
Author

f1mishutka commented Mar 7, 2019

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.

Show resolved Hide resolved includes/database.inc Outdated
@dsnopek

This comment has been minimized.

Copy link
Contributor

dsnopek commented Mar 14, 2019

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

@dsnopek

This comment has been minimized.

Copy link
Contributor

dsnopek commented Mar 14, 2019

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

This comment has been minimized.

Copy link
Author

f1mishutka commented Mar 15, 2019

Thank you David!

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

@f1mishutka

This comment has been minimized.

Copy link
Author

f1mishutka commented Mar 27, 2019

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

This comment has been minimized.

Copy link
Contributor

dsnopek commented Mar 27, 2019

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

This comment has been minimized.

Copy link
Author

f1mishutka commented Mar 27, 2019

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

This comment has been minimized.

Copy link
Contributor

dsnopek commented Mar 29, 2019

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

This comment has been minimized.

Copy link
Author

f1mishutka commented Mar 29, 2019

Thank you, David!

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

Merge pull request #1 from d6lts/6.x
d6lts 6.50 changes
@f1mishutka

This comment has been minimized.

Copy link
Author

f1mishutka commented Apr 19, 2019

Just merged changes from 6.50 version.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.