Provide backtick escaping for MySQL in DB abstraction layer

Backticks 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

Given that we have abstracted out the DB layer completely, why can't we do this for mysql independently of PostgreSQL (etc.)?

Recategorizing 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.

Chew on this testbot...

... 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.

Has 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.

clrockwell: I got uber stuck on making this work ... you're welcome to take a swing

I'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:

function _exhibitor_search($search_text = '', $search_category = NULL, $exclude = NULL) {
  db_set_active('exhibitors');
  //$results = db_query("SELECT `Product Category` FROM Exhibitors WHERE `Product Category` = 20"); WORKS without patch

  $search_category = array('20','21'); // give a default for testing
  $search_fields = array('Company', 'Booth#', 'Product');
  if (!is_null($exclude)) {
    // a field needs to be excluded from the query
    if (!is_array($exclude)) : (array)$exclude; endif; // makes sure is_array for array_diff
    $search_fields = array_diff($search_fields, $exclude); // new query fields with exclude removed
  }
  $query = db_select('Exhibitors', 'e');
  //$query->fields = array('e', array('Company', 'City', 'State', 'Web Link', 'Booth#', 'Product', 'Product Category'));
  $db_or = db_or();
  foreach ($search_fields as $field) {
    $db_or->condition('`' . $field . '`', "%$searchtext%", 'LIKE'); 
  }
  // search category need an AND
  if (!is_null($search_category)) {
    $db_and = db_and();
    $db_and->condition(db_like('`Product Category`'), $search_category, 'IN');
    $db_and->condition($db_or);
  }
  $query->condition($db_and);
  die ((string)$query); // looks good after applied patch
		
  db_set_active();
}

I added the following code to database/mysql/database.inc

/**
   * Escapes a table name string.
   *
   * Force all table names to be strictly alphanumeric-plus-underscore.
   * For some database drivers, it may also wrap the table name in
   * database-specific escape characters.
   *
   * @return
   *   The sanitized table name string.
   */
   /* From http://drupal.org/node/1600670#comment-6096740 */
  public function escapeTable($table) {
    $escaped = preg_replace('/[^A-Za-z0-9_ ]+/', '', $table);

    // Check for beginning and ending backtick.
    if (preg_match('/^[`]/', $table) && preg_match('/[`]$/', $table)) {
      $escaped = '`' . $escaped . '`';
    }
    return $escaped;
  }

  /**
   * Escapes a field name string.
   *
   * Force all field names to be strictly alphanumeric-plus-underscore.
   * For some database drivers, it may also wrap the field name in
   * database-specific escape characters.
   *
   * @return
   *   The sanitized field name string.
   */
  public function escapeField($field) {
    $escaped = preg_replace('/[^A-Za-z0-9_ ]+/', '', $field);

    // Check for beginning and ending backtick.
    if (preg_match('/^[`]/', $field) && preg_match('/[`]$/', $field)) {
      $escaped = '`' . $escaped . '`';
    }
    return $escaped;
  }

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

First try

Sorry, bad coding, didn't realize those tabs were in there.

In 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;

I'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

Little 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');

Thanks - had exact same problem here

Minor typo in issue description fixed.

Apparently 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.

Lets see what the testbot thinks.

This issue is a feature request and those go to the developer branch of Drupal core and that is 8.3.

Added some extra checking for '.' dots in the table name. Also backticking fields and aliases.

Lets see what the testbot thinks.

What do I need to change to this patch to make it work for the testbot?

@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?

Our 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)

Patch against 8.3.x.

Testbot go!

Not sure what the problem is with the tests. I just installed a fresh drupal 8.3 install with this patch without any hickups...

@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:

exception: [Uncaught exception] Line 685 of core/lib/Drupal/Core/Database/Connection.php:
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 '`config`
WHERE (`collection` = '') AND (`name` = 'core.extension')' at line 2: SELECT 1 AS `expression`
FROM 
{`config`} `config`
WHERE (`collection` = :db_condition_placeholder_0) AND (`name` = :db_condition_placeholder_1); Array
(
    [:db_condition_placeholder_0] => 
    [:db_condition_placeholder_1] => core.extension
)

I 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.

It 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.

Fixed table prefixes.

Fixed EXISTS condition empty field being back ticked.

Almost 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.

Small extra patch, the schema also uses tables for creating, lookup ect that aren't back ticked.

Second attempt

Renamed the file to patch.

Found 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.

typo in title

Re-upload patch file

Fixed regex expression.

Fixed some failing tests.

I 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.

Finally 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.

Forgot to backtick a test case.

Sorry about the noise, test still failed due to braces being different.

A 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.

+++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Connection.php
@@ -204,11 +204,14 @@ public function query($query, array $args = array(), $options = array()) {
+    // To make the ILIKE operator work, we type-cast bytea fields into text.
+    // @todo This workaround only affects bytea fields, but the involved field
+    //   types involved in the query are unknown, so there is no way to
+    //   conditionally execute this for affected queries only.
+    return parent::prepareQuery(preg_replace('/ ([^ ]+) +(I*LIKE|NOT +I*LIKE) /i', ' ${1}::text ${2} ', $query));

For instance I'm not sure where this change came from and why.

Hmm, 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.

Marking 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.

Hi,

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);

@Lennard Westerveld: The word "KEY" is a reserved keyword for MySQL. That is why it only works with backticks.

This 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.