drupal_write_record is not using backticks (`) for column names. This leads to SQL syntax errors when using reserved keywords as column names.
This behaviour was observed with 'loop' as column name with mysql-5.1.
Querying using backticks around the column name (eg. "SELECT `loop` …") works normally.
Proposed solution would be for drupal_write_record to use backticks for all column names.
As I do not know if the solution would have to be made in drupal_write_record itself or somewhere 'further down' in the database system, I set the priority to major, since it could turn up anywhere in that case.
If you do happen to know that this issue is limited to drupal_write_record, feel free to set the priority back to 'normal'.
Comment | File | Size | Author |
---|---|---|---|
#66 | core-mysql-backticks-1426084-66.patch | 20.63 KB | kriboogh |
Comments
Comment #1
xjmBackticks are not used because they are MySQL-only. Instead, you should not use ANSI reserved words as table or field names.
References:
http://drupal.org/node/141051
http://drupal.org/node/555514
Comment #2
bleen CreditAttribution: bleen commentedGiven that we have abstracted out the DB layer completely, why can't we do this for mysql independently of PostgreSQL (etc.)?
Comment #3
xjmRecategorizing based on my IRC conversation with @bleen18. I'm personally -1 on this, but I don't know the technical implications, so hopefully someone with more in-depth knowledge can evaluate the request.
Comment #4
bleen CreditAttribution: bleen commentedChew on this testbot...
Comment #6
bleen CreditAttribution: bleen commented... and that's why we pay testbot the big bucks. The patch in #4 creates queries like this: "UPDATE {`system`} SET schema_version..." which is clearly not good. Ill take a deeper look.
Comment #7
chrisrockwell CreditAttribution: chrisrockwell commentedHas there been any progress on this? Maybe I can help out as I need this with 7.x. I've inherited a MySQL table that is the new back-end for an Access front-end. The Access structure has spaces in field names all over the place.
Comment #8
bleen CreditAttribution: bleen commentedclrockwell: I got uber stuck on making this work ... you're welcome to take a swing
Comment #9
chrisrockwell CreditAttribution: chrisrockwell commentedI'm all backwards I think. I need this to work in D7 and my issue doesn't involve drupal_write_record, sorry. Nonetheless, maybe the following can help. Note that db_query() works fine with backticks, but $query->execute will not, which I'm still trying to track down why.
Here is the relevant module code:
I added the following code to database/mysql/database.inc
I reference in the comments where I got the code from. This also allows for spaces in the table name. If drupal_write_record prepares it's queries this way, it should solve your issue as well. I'm fairly new to really diving into Drupal so I haven't quite figured out how to submit a proper patch.
Edit: note that the above 'patch' overrides the functions in database/database.inc
Comment #10
chrisrockwell CreditAttribution: chrisrockwell commentedFirst try
Comment #11
chrisrockwell CreditAttribution: chrisrockwell commentedSorry, bad coding, didn't realize those tabs were in there.
Comment #12
chrisrockwell CreditAttribution: chrisrockwell commentedComment #14
nevergone CreditAttribution: nevergone commentedIn Drupal 7.15, the next query is failure:
$query = Database::getConnection('default', 'joomla_migrate')
->select('jos_content', 'jc')
->fields('jc', array('id', 'title', 'introtext', 'fulltext'));
because "fulltext" is MySQL reserved word.
The error message:
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 'fulltext FROM jos_content jc' at line 1: SELECT jc.id AS id, jc.title AS title, jc.introtext AS introtext, jc.fulltext AS fulltext FROM {jos_content} jc;
Comment #15
cafuego CreditAttribution: cafuego commentedI'm fiddling with a diver for the Drizzle database (based on MySQL) and that fails with a bunch of core modules using reserved keywords as column names. Thus, I've also been looking at escapeTable() and escapeField().
It looks as if the database layer uses escapeTable() but pretty much never uses escapeField(), which it should really run over each field name!
Anyway, having modified the base database.inc to add backticks to field names, it seems that the DB layer adds backticks around the whole `tablealias.fieldname` combination (usually used in conditions), which breaks queries completely (but I notice you've addressed that :-)
The backticks get added *inside* the { } table name placeholders, does this mean that with a database prefix the table would be turned into
prefix_`users`
or is the prefix adding function smart enough?In addition, core seems to contain a bunch of db_query() statements with hardcoded field names that cannot be escaped without rewriting their queries, so I've opened #1776016: Database layer does not escape table and field strings everywhere
Comment #16
nevergone CreditAttribution: nevergone commentedLittle workaround for #14
$query = Database::getConnection('default', 'joomla_migrate')
->select('jos_content', 'jc')
->fields('jc', array('id', 'title', 'introtext', 'created', 'modified'));
$query->addExpression('`fulltext`', 'full');
Comment #17
idacrobert CreditAttribution: idacrobert commentedThanks - had exact same problem here
Comment #17.0
idacrobert CreditAttribution: idacrobert commentedMinor typo in issue description fixed.
Comment #20
kriboogh CreditAttribution: kriboogh at Calibrate commentedApparently this is a 4year old issue.
Anyway, reviving this, as I recently had to make '-' dashes in a table names to work, we are migrating some external tables into drupal 8. Dashes are allowed in MySql as long as you backtick the name. Why backticking everything isn't done by default in the MySQL layer is beyond me. Attached is a patch that allows dashes. Maybe someone from core needs to pick this up again.
Obvious things always seem to hit you in the head with Drupal. I also had problems with table names being reserved keywords and not being backticked.
Comment #21
daffie CreditAttribution: daffie commentedLets see what the testbot thinks.
Comment #23
daffie CreditAttribution: daffie commentedThis issue is a feature request and those go to the developer branch of Drupal core and that is 8.3.
Comment #25
kriboogh CreditAttribution: kriboogh at Calibrate commentedAdded some extra checking for '.' dots in the table name. Also backticking fields and aliases.
Comment #26
daffie CreditAttribution: daffie commentedLets see what the testbot thinks.
Comment #29
kriboogh CreditAttribution: kriboogh at Calibrate commentedWhat do I need to change to this patch to make it work for the testbot?
Comment #30
daffie CreditAttribution: daffie commented@kriboogh: Which version of Drupal 8 are you using for creating your patches? Did you do follow these instructions: https://www.drupal.org/node/3060/git-instructions/8.3.x/nonmaintainer?
Comment #31
kriboogh CreditAttribution: kriboogh at Calibrate commentedOur project is currently on version 8.2.x, maybe that's the problem. I'll try to do it on 8.3 if I get the time.
(I'm using the phpstorm create patch menu to generate the file, but don't think that would be the issue)
Comment #32
kriboogh CreditAttribution: kriboogh at Calibrate commentedPatch against 8.3.x.
Comment #33
kriboogh CreditAttribution: kriboogh at Calibrate commentedTestbot go!
Comment #35
kriboogh CreditAttribution: kriboogh at Calibrate commentedNot sure what the problem is with the tests. I just installed a fresh drupal 8.3 install with this patch without any hickups...
Comment #36
daffie CreditAttribution: daffie commented@kriboogh: Your patch applied correctly. So that is good. The testbot ran for Drupal core with your patch applied all the tests and that resulted in 5315 fails. If you want to see what the testbot failures are you can click on the text "PHP 5.5 & MySQL 5.5 15,844 pass, 5,315 fail". What I see is that your patch results in SQL syntax errors:
Comment #37
chrisrockwell CreditAttribution: chrisrockwell commentedI want to look into this more, but at first glance I would think the issue is that db_query accepts {table_name} where as the other db_* do not (i.e. db_select('table_name')). So you may want to look into the preparation of a query and if, because we're putting backticks in there, {`table_name`} isn't escaped, resulting it in being used literally in the query e.g. "SELECT * FROM {`table_name`}...".
To make this easier for testing, you might enable simpletest and then pick a couple relevant database tests to run locally. The SchemaTest runs relatively quickly and was failing on this.
Hope that helps and I'll try to find time to come back and contribute some more.
Comment #38
kriboogh CreditAttribution: kriboogh at Calibrate commentedIt might be related to prefixing tables, on a normal setup I don't have any problems. My guess is the simpletest tables are also prefixed which is why they fail.
BTW, I'm unable to xdebug the exception, any idea how I enable this on simple tests? Tried XDEBUG cookies or altering the query string options in drupalGet, but none seem to work. The simple test run doesn't seem to be picked up by phpstorm.
Comment #39
kriboogh CreditAttribution: kriboogh at Calibrate commentedFixed table prefixes.
Comment #41
kriboogh CreditAttribution: kriboogh at Calibrate commentedFixed EXISTS condition empty field being back ticked.
Comment #43
kriboogh CreditAttribution: kriboogh at Calibrate commentedAlmost there, still a couple of tests failing.
Things still to look at:
- Some tests use placeholders to insert table names in a query. This results in `'tablename'` being generated when the arguments to a query are replaced.
- There are tests failing on `11` as a table name, not sure what the original purpose is for the test. Maybe the writer of the test can have a look.
- The failure in PreviewUI needs some more insight.
Comment #44
kriboogh CreditAttribution: kriboogh at Calibrate commentedSmall extra patch, the schema also uses tables for creating, lookup ect that aren't back ticked.
Comment #46
kriboogh CreditAttribution: kriboogh at Calibrate commentedSecond attempt
Comment #47
kriboogh CreditAttribution: kriboogh at Calibrate commentedComment #48
kriboogh CreditAttribution: kriboogh at Calibrate commentedComment #49
kriboogh CreditAttribution: kriboogh at Calibrate commentedRenamed the file to patch.
Comment #51
kriboogh CreditAttribution: kriboogh at Calibrate commentedFound another problem with {} replacement. We were running an custom scripts that executes a sql file onto the drupal database. This basically executes each line of the file through $connection->query($line, array()). In one of the insert statements a value contained some serialised json, which has curly braces in it. The braces were replaced by the table prefix functionality. I replaced the str_replace method with a preg_replace statement, which as a bonus also deals with backticks, so the changes to the mysql driver could be simplified as well.
Hopefully fixed some failed test too.
Attached is a patch agains 8.2.5.
Comment #53
nevergone CreditAttribution: nevergone commentedtypo in title
Comment #54
kriboogh CreditAttribution: kriboogh at Calibrate commentedRe-upload patch file
Comment #56
kriboogh CreditAttribution: kriboogh at Calibrate commentedFixed regex expression.
Comment #58
kriboogh CreditAttribution: kriboogh at Calibrate commentedFixed some failing tests.
Comment #60
kriboogh CreditAttribution: kriboogh at Calibrate commentedI keep getting PHPUnit_Framework_Exception: sh: : command not found errors on my local install, so tackling those last failed test is going to be difficult. If someone feels up to the task please do.
Comment #62
kriboogh CreditAttribution: kriboogh at Calibrate commentedFinally was able to pin point the remaining problem. Apparently this bug was already present in the code, but was never revealed because by chance the test succeeded. The problem was the extra condition being added in case a entity query is performed with a delta higher than 1 on a single value field. In this case the '1<>1' condition is added as a field to the query. This 'field' is then transformed into '11' due to the escapeField function in the connection object. Before the backticks, this didn't raised an error, as 11 = NULL returns false anyway. but with the backticks added `11`= NULL did. The correct way is to add a where clause in stead of a condition, so the field isn't parsed through escapeField.
Comment #64
kriboogh CreditAttribution: kriboogh at Calibrate commentedForgot to backtick a test case.
Comment #66
kriboogh CreditAttribution: kriboogh at Calibrate commentedSorry about the noise, test still failed due to braces being different.
Comment #67
mradcliffeA test failure in views_ui asserting a query string seems related.
Also it really helps to review changes in patches if you can create an interdiff between your patches.
For instance I'm not sure where this change came from and why.
Comment #68
kriboogh CreditAttribution: kriboogh at Calibrate commentedHmm, were did you get that change from? At some point I switched from 8.2 to 8.3 so maybe that snook in. I never touched that file pgsql/Connection.php for this backtick fix.
I will add the interdiffs in the future. Alhough it might take a while now as the project we are using this for is about to round up.
Comment #69
joachim CreditAttribution: joachim commentedMarking these as duplicates:
- #2186291: Standardise escaping of table, field and aliases in database layer -- has a patch which should be looked at to compare it to this one, and use the best bits of both
- #2767595: Support "-" in database table names
- #2845333: can't JOIN to a foreign database whose name contains a hyphen
This issue needs a summary update, as that still refers to D7. Also changing this to a bug, as the other issues all refer to this as a bug -- queries fail when they shouldn't, and without a proper explanation of why.
Comment #72
Lennard WesterveldHi,
Bump, is there any update on this matter? because i have issues with it with a contentEntity with a field in it named key.
For example:
INSERT INTO viewed (type, user, key, counter, created, changed) VALUES ('video', 1, '56dg211', 0, 1526384920, 1526384920);
This throws a error, because key is a function in MYSQL? This i literally a copy of the ContentEntity::create([])->save() query execution.
Escaping it with backtick and it will work again!
INSERT INTO viewed (`type`, `user`, `key`, `counter`, `created`, `changed`) VALUES ('video', 1, '56dg211', 0, 1526384920, 1526384920);
Comment #73
daffie CreditAttribution: daffie commented@Lennard Westerveld: The word "KEY" is a reserved keyword for MySQL. That is why it only works with backticks.
Comment #75
alexpottThis issue is superseded by #2986452: Database reserved keywords need to be quoted as per the ANSI standard which escapes columns for all database drivers. Going to mark as duplicate and credit people that worked on and review this issue on that one.