Posted by hswong3i on November 23, 2008 at 2:38am
| Project: | Drupal core |
| Version: | 7.x-dev |
| Component: | database system |
| Category: | feature request |
| Priority: | critical |
| Assigned: | hswong3i |
| Status: | closed (fixed) |
Issue Summary
ANSI-92 suggest database should return table/column/constraint name as upper case, but most open source database allow the mix use with lower case (lossy mode). On the other hand, some high-end database engine will force upper case (strict ANSI standard) for all table/column/constraint name, unless escape with escape characters.
PDO provide an ability for forcing return table/column/constraint name with upper/natural/lower case. This patch will force all database connection with PDO::CASE_LOWER so keep SQL compatibility for all database engine.
| Attachment | Size | Status | Test result | Operations |
|---|---|---|---|---|
| dbtng-PDO_CASE_LOWER-1227407521.patch | 823 bytes | Idle | Passed: 103 passes, 0 fails, 0 exceptions | View details |
Comments
#1
I'm not entirely sure I follow. SQL is supposed to be case-insensitive, per the spec. MySQL does it a bit wrong and is case-sensitive for table names if and only if it's running on a case-sensitive file system. By just always using lowercase table and field names in all code, everything just works fine.
So what do we need here? Remember we could be dealing with foreign databases as well, which may not be as controlled as Drupal database.
#2
This is a conventions that we used for many year. Seems there is no conflict with foreign database as well. From http://drupal.org/node/2497:
On the other hand, PDO::CASE_LOWER will only affect column name when fetching data. It have no duty for RAW SQL input. So standardize fetch mode with lower-case only can simplify our code implementation. Other system can still dealing with Drupal's database without any problem.
#3
I think this looks reasonable. Not Crell?
#4
If I'm understanding the issue correctly and it just does case-folding on fetch, the only place we'd still have an issue is if we run into a 3rd party database table with columns foo and Foo, which would then get corrupted when loading. If we're OK with that limitation (I am, since making a DB like that is a moronic idea to begin with) then I'm OK with this patch.
We should make sure to document the case handling in the handbook, too.
#5
IMHO we should keep focus on ourselves before consider for 3rd party integration. We are able to change ourselves, but there is no way to enforce other party to follow our standard. In case we need to connect with those moronic system, just let people work for it and find a suitable solution :D
+1 for documentation, or we can even give an additional check case in Coder module, too.
#6
OK, here's my statement on it. +1 to forcing lower-case, but let's do it in a way that allows it to be over-ridden by a specific driver. That way, if someone really needs to connect to a 3rd party database where they do care about case they can clone their own custom driver and override the case handling, then connect to the external database with that driver instead.
That should be an easy change here and give us the most flexibility.
#7
Here is the revamp. I am not so sure why SQLite looks a bit different from MySQL and PostgreSQL, but I clone the handling for it. Please correct me if that's not work :S
#8
The last submitted patch failed testing.
#9
Silly mistake...
#10
Committed to CVS HEAD.
#11
Thanks :D
#12
This broke sqlite support. Note that the $connection_options array is no longer passed to parent::__construct().
- parent::__construct('sqlite:'. $connection_options['database'], '', '', $connection_options);+ parent::__construct('sqlite:'. $connection_options['database'], '', '', array(
+ // Force column names to lower case.
+ PDO::ATTR_CASE => PDO::CASE_LOWER,
+ ));
#13
In #7, hswong3i said "I am not so sure why SQLite looks a bit different from MySQL and PostgreSQL, but I clone the handling for it. Please correct me if that's not work :S". Well, it was right, he wasn't sure.
Dries, could you give us more time to review those patches more carefully from now on?
Dave fix is sane.
#14
Yes, I am not sure, so I ask. Seem this break SQLite because it need a manual hack with DatabaseStatement_sqlite which is now skipped :S
BTW, another question is: why PostgreSQL don't need this handling for
$connection_options['transactions']?public function __construct(Array $connection_options = array()) {
$connection_options += array(
'transactions' => TRUE,
);
$this->transactionSupport = $connection_options['transactions'];
$dsn = 'pgsql:host=' . $connection_options['host'] . ' dbname=' . $connection_options['database'];
if (!empty($connection_options['port'])) {
$dsn .= ' port=' . $connection_options['port'];
}
parent::__construct($dsn, $connection_options['username'], $connection_options['password'], array(
// Convert numeric values to strings when fetching.
PDO::ATTR_STRINGIFY_FETCHES => TRUE,
// Force column names to lower case.
PDO::ATTR_CASE => PDO::CASE_LOWER,
));
}
In case, should PostgreSQL look like this way?
public function __construct(Array $connection_options = array()) {
// Convert numeric values to strings when fetching.
$connection_options[PDO::ATTR_STRINGIFY_FETCHES] = TRUE;
// Force column names to lower case.
$connection_options[PDO::ATTR_CASES] = PDO::CASE_LOWER;
$this->transactionSupport = isset($connection_options['transactions']) ? $connection_options['transactions'] : TRUE;
$dsn = 'pgsql:host=' . $connection_options['host'] . ' dbname=' . $connection_options['database'];
if (!empty($connection_options['port'])) {
$dsn .= ' port=' . $connection_options['port'];
}
parent::__construct($dsn, $connection_options['username'], $connection_options['password'], $connection_options);
}
#15
That would make sense to do, I'm testing it right now.
#16
In response to #14, it will fail for PostgreSQL. I literally had to code the following to get it to work in the pgsql driver:
<?php
$connection_options += array(
'transactions' => TRUE,
// Convert numeric values to strings when fetching.
PDO::ATTR_STRINGIFY_FETCHES => TRUE,
// Force column names to lower case.
PDO::ATTR_CASE => PDO::CASE_LOWER,
);
$this->transactionSupport = $connection_options['transactions'];
$dsn = 'pgsql:host=' . $connection_options['host'] . ' dbname=' . $connection_options['database'];
if (!empty($connection_options['port'])) {
$dsn .= ' port=' . $connection_options['port'];
}
$options = $connection_options;
unset($options['transactions']);
unset($options['host']);
unset($options['database']);
unset($options['port']);
unset($options['username']);
unset($options['password']);
unset($options['driver']);
parent::__construct($dsn, $connection_options['username'], $connection_options['password'], $options);
?>
#17
We should just bring the sqlite driver in line with the mysql/postgresql driver. This also brings the default port for postgresql in line with the mysql driver.
#18
Fair enough.
The confusion comes from the fact that: $connection_options (our Drupal-specific options) != $driver_options (PDO driver options).
#19
Nevermind...#17 will break postgresql. Revised patch that just fixes the sqlite driver and brings it in line with the mysql and pgsql drivers' __construct functions.
#20
Another quick rewrite. Standardizing the drivers __construct function will be a separate issue. This just simply fixes the HEAD sqlite breakage.
#21
Ok, this is the good one, thanks Dave.
#22
I give some similar tries as Dave Reid and coming with same conclusion as #19.
Maybe we can implement as this way? Move away
$connection_options['statement_class']as driver specific$this->statementClass, and clearly document that SQLite need to usePDOStatementas default, with semi-private PDOPrepare()? With this model we can always keep PDO's $driver_options as different with Drupal's private $connection_options, they don't need to mix together.Patch tested with both MySQL, PostgreSQL and SQLite, all pass.
#23
The last submitted patch failed testing.
#24
How about we focus on the patch in #20 to fix the HEAD broken-ness and create a separate issue for synchronizing the __contstruct functions
#25
Sorry for the delay of this issue, but this patch is now working for me. I am now running simpletest with both MySQL and SQLite, seems functioning. Hope test bot can catch if there is any error message.
#26
Committed to CVS HEAD. Thanks, and sorry if I pulled the trigger on this one too fast.
#27
Ok, let's do this properly, then.
#28
Fixed spelling.
#29
@Damien Tournoud: IMHO this issue is now get fix with its original target. Should clean up belongs to another new issue?
#30
@hswong3i: you made the mess here, let's fix it here.
#31
I am not responsible for the mess of this issue. This is your doing...
#32
It is a good clean up, indeed. Committed to CVS HEAD.
#33
Automatically closed -- issue fixed for two weeks with no activity.
#34
This change broke third-party integration for no good reason: #1171866: Enforced fetching of fields/columns in lowercase breaks third-party integration
#35
Forcing lowercase field names completely breaks connections to any other data... can this at least be optional?
#36
Please see the issue referenced by Sun for follow up.