db_select('my_table', 'a')
    ->fields('a', array('e-mail'))
    ->execute()
    ->fetchObject());

produces

PDOException: SQLSTATE[42S22]: Column not found: 1054 Unknown column 'a.email' in 'field list': SELECT a.email AS email FROM {my_table} a; Array ( ) in my_function().

I am using MySQL 5.1.41

CommentFileSizeAuthor
#10 use_destruct.patch4.13 KBberdir

Comments

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

- is not a valid character for a column *unless* you use identifier escaping (`e-mail`).

Not sure what do do here...

Crell’s picture

Crell commented

- should not be a valid character for a column to begin with. We have an escapeField() method that we could throw on all field names to do DB-specific quoting/escaping, but other than that I'd call this "by design".

The query builders are never going to support the complete universe of all of SQL. The 95% that is used 95% of the time is the target.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

I think the problem here is that it is not possible with db_query() either, simply because our db drivers don't provide a method to escape a column/identifier.

Meaning, you can't do something like this:

db_query('SELECT ' . db_quote_identifier('e-mail') . ' FROM {my_table}')

So what about providing such a method but not use it internally?

Crell’s picture

Crell commented

Whoever decided that using a serialized string (SQL) as a query format was a good idea needs to be drawn and quartered. Sweet leapin' Jebus!

Before we do that let's step back and ask what the full set of potential quotable/escapable things are:
- Values (placeholders do this for us)
- Tables
- Fields

And all of the above used as part of an expression. Any I'm missing?

tutumlum’s picture

tutumlum commented 
function mymodule_schema() {
  $schema['mymodule_table'] = array(
    'fields' => array(
      'e-mail' => array(
        'type' => 'varchar',
        'length' => '128',
        'not null' => FALSE,
      ),
    ),
  );  
  return $schema;
}

works without any error and produces a table with field e-mail

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

Yes, the schema api supports it, especially because we want to support moving away from those field names :)

Because of that, I would suggest the following (If this is an existing module with an existing e-mail field: Write an update function and rename that field to email. That is the best way to avoid any issues in the future or with other databases. Just one example here: Mysql on Linux/Unix is always case sensitive with or without quotes, oracle uses only upper case characters unless you quote the identifier and PostgreSQL uses lower case characters unless you quote them. So when you create a quoted identifier in oracle (example: "mail"), then SELECT mail FROM ... will give you en error, stating that mail does not exist :) You can read more on http://www.alberton.info/dbms_identifiers_and_case_sensitivity.html

Given that, I would suggest that this is a feature request, allowing to quote identifiers in a db-agnostic way with a function db_quote_identifier() (or table/field, but I guess all databases use the same character for both).

Opinions?

damien tournoud’s picture

damien tournoud commented

@Berdir: we are using PDO::ATTR_CASE => PDO::CASE_LOWER which should avoid some case-sensitivity issues.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

@DamZ: Possible, but unless I'm wrong, I think that attribute only affects returned column names in result sets. Haven't found anything explicit, but http://usphp.com/manual/en/function.PDO-setAttribute.php only talks about returned in the description of the default value: Leave column names as returned by the database driver.

Crell’s picture

Crell commented

Any response to #4?

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Active » Needs review
StatusFileSize
new use_destruct.patch4.13 KB

Untested patch to switch to __destruct().

While doing that, I've noticed that there is a bug which would cause the cleanup to run multiple times, that would need to be fixed anyway (we check on $this->shutdownRegistered but set $shutdownRegistered).

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs review » Active

Erm, wrong issue, ignore that :)

chx’s picture

chx commented
Status: Active » Reviewed & tested by the community

Nice!

chx’s picture

chx commented
Status: Reviewed & tested by the community » Active

Opsie, wrong issue from me too :D

Crell’s picture

Crell commented
Status: Active » Needs review
berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs review » Active

There is no patch here, the one above does not belong into this issue.

chx’s picture

chx commented
Version: 7.x-dev » 8.x-dev

There are enough , way bigger problems in Drupal 7. We can ponder this during Drupal 8. For the time being, if you can't alter the table, run an SQL VIEW over it and ... that's it.

chx’s picture

chx commented

There are quite a number of rules to consider given #4. It'll be a hell supporting all.

damien tournoud’s picture

damien tournoud commented
Status: Active » Closed (duplicate)

This is nothing more then a duplicate of #371: resolve ANSI SQL-92/99/2003 reserved words conflict in query statements.