Posted by Damien Tournoud on September 30, 2010 at 5:11pm
18 followers
| Project: | Drupal core |
| Version: | 7.x-dev |
| Component: | database system |
| Category: | bug report |
| Priority: | normal |
| Assigned: | LaurentAjdnik |
| Status: | closed (fixed) |
| Issue tags: | API change, Needs Documentation |
Issue Summary
We have some code in the three schema.inc implementations that incorrectly rely on the generic type instead of the engine-specific type.
This triggers notices when you try to create a column without a generic type and can lead to wrong behaviors in some cases.
Comments
#1
This is actually very simple to fix.
#2
The last submitted patch, 927828-schema-rely-generic-type.patch, failed testing.
#3
bot is not happy. install failed.
#4
Subscribe.
#5
sub
#6
Shouldn't this be a higher priority than 'normal'? We have lost something that worked in D6, the ability for a contrib module to add other database types, and this patch is needed to return to that state, assuming the patch is fixed and it works. If this is marked as 'normal' it may never get fixed.
#7
Subscribe.
I agree this is a critical issue. It is a regression from D6. Fixing it is not an API change, it is restoring the API to the way it was intended to work and used to work.
#8
Tagging
#9
Attempt to fix the patch for mySQL. The array of types has to be mysql-ized:
<?phpif (in_array($spec['mysql_type'], array('VARCHAR', 'CHAR', 'TINYTEXT', 'MEDIUMTEXT', 'LONGTEXT', 'TEXT')) && isset($spec['length'])) {
?>
#10
#11
Now with pgsql and sqlite support.
#12
This works for me, and code looks good as well. Adding a field with just a mysql_type (on a mysql database that is) works without problems/warnings and the field is created in the database.
Code used:
<?php
function testschema_schema() {
$schema['testschema'] = array(
'description' => 'Description',
'fields' => array(
'datefield' => array(
'mysql_type' => 'datetime',
'not null' => FALSE,
'default' => '2010-07-01 20:24',
),
),
);
return $schema;
}
?>
#13
<?php+ if (in_array($spec['mysql_type'], array('VARCHAR', 'CHAR', 'TINYTEXT', 'MEDIUMTEXT', 'LONGTEXT', 'TEXT')) && isset($spec['length'])) {
?>
Nice catch. But now this needs a drupal_strtoupper() or something.
Also, this needs a (quick) test.
#14
#15
$spec['mysql_type'] contains fixed values returned by getFieldMap().
<?php
public function getFieldTypeMap() {
static $map = array(
'varchar:normal' => 'VARCHAR',
'char:normal' => 'CHAR',
'text:tiny' => 'TINYTEXT',
'text:small' => 'TINYTEXT',
'text:medium' => 'MEDIUMTEXT',
'text:big' => 'LONGTEXT',
'text:normal' => 'TEXT',
// [...]
);
return $map;
}
?>
Is a drupal_strtoupper() really necessary ?
Do you mean drupal_strtoupper($spec['mysql_type']) ?
SQLite also uses uppercase data types, but pgSQL uses lowercase ones. Would it require a drupal_strtolower() ?
About testing: is there any way to tell the test robot to use SQLite or pgSQL instead of mySQL ?
#16
What a nasty little critter. Some really nice test (that hopefully discoveres more bugs here!) would be awesome.
#17
Need help: any volunteers for the tests ? :-|
#18
Working on tests, got some clarification from chx in IRC.
#19
Before patch (test fails):
Undefined index: type Notice schema.inc 195 DatabaseSchema_mysql->processField()Undefined index: type Notice schema.inc 135 DatabaseSchema_mysql->createFieldSql()
After patch nothing.
#20
Looks good, just need a review of test.
#21
The test would need a pgsql_type and sqlite_type as well, if we want to get testing working on a non-mysql environment. 'timestamp' works fine in postgresql, datetime for sqlite.
+++ includes/database/mysql/schema.inc 16 Oct 2010 07:54:49 -0000@@ -132,7 +132,7 @@ class DatabaseSchema_mysql extends Datab
- if (in_array($spec['type'], array('varchar', 'char', 'text')) && isset($spec['length'])) {
+ if (in_array($spec['mysql_type'], array('VARCHAR', 'CHAR', 'TINYTEXT', 'MEDIUMTEXT', 'LONGTEXT', 'TEXT')) && isset($spec['length'])) {
We still need to make this comparison case insensitive. A user could easily specify mysql_type to be one of these types in lowercase. (same goes for the other db drivers)
Powered by Dreditor.
#22
Related to #866340: Remove support for date and time types, just for reference/background.
#23
Here we go then.
I placed the case logic in
processField($field)so that it's done once for all, meaning for any further comparison.I added a comment about it.
The decision between lowercase and uppercase is done according to the official references of the corresponding DB engines.
- mysql => uppercase: http://dev.mysql.com/doc/refman/5.0/en/data-types.html
- postgresql =>lowercase: http://www.postgresql.org/docs/7.4/interactive/datatype.html
- sqlite => uppercase: http://www.sqlite.org/datatype3.html
Example for mysql:
<?php
protected function processField($field) {
if (!isset($field['size'])) {
$field['size'] = 'normal';
}
// Set the correct database-engine specific datatype.
// In case one is already provided, force it to uppercase.
if (isset($field['mysql_type'])) {
$field['mysql_type'] = drupal_strtoupper($field['mysql_type']);
} else {
$map = $this->getFieldTypeMap();
$field['mysql_type'] = $map[$field['type'] . ':' . $field['size']];
}
if ($field['type'] == 'serial') {
$field['auto_increment'] = TRUE;
}
return $field;
}
?>
#24
The last submitted patch, 927828-schema-rely-generic-type.v05.patch, failed testing.
#25
+++ includes/database/mysql/schema.inc 16 Oct 2010 07:54:49 -0000@@ -187,7 +187,10 @@
+ } else {
'else' should be on a new line
+++ includes/database/pgsql/schema.inc 16 Oct 2010 07:54:49 -0000@@ -181,8 +181,12 @@
+ } else {
same again
+++ includes/database/sqlite/schema.inc 16 Oct 2010 07:54:49 -0000@@ -116,8 +116,12 @@
+ } else {
and again
+++ modules/simpletest/tests/schema.test 16 Oct 2010 07:54:49 -0000@@ -112,6 +112,22 @@ class SchemaTestCase extends DrupalWebTe
+ 'timestamp' => array(
+ 'mysql_type' => 'timestamp',
+ 'default' => NULL,
+ ),
sqlite_type and pgsql_type are still missing from the test, making the tests fail on sqlite / postgresql
Powered by Dreditor.
#26
Why are we testing that it is *not* created? Shouldn't we be testing that it can be created, since that's the problem at hand?
#27
#28
The last submitted patch, 927828-schema-rely-generic-type.v06.patch, failed testing.
#29
#30
Subscribing.
#31
The tests were missing from the latest patches. This one adds the tests back and makes them test the right thing (as per #26).
Also, some fields (timestamp in this case) are NOT NULL by default, so we should definitely be able to explicitly mark a field as having NULL as possible value. This is also the reason the tests in #19 didn't fail.
This patch addresses that by adding NULL to the createFieldSql when the 'not null' property is explicitly set to FALSE, but maybe should be split out to a separate issue.
#32
nice
#33
Code and test look good to me.
#34
So, one thing that's not clear to me is whether or not this solves Date module's problem?
Confirmation on that, preferably with an issue summary, would be lovely.
#35
Just getting ready to do a Date module test on this patch. I'll report back.
#36
w00t!
My best summary:
Core defines only some common generic sql column types but is intended to allow other modules to define others to handle less-common type or those which don't have really portable sql standards. Date types are a good example of that, every database has a slightly different way of defining date columns and the sql to manipulate them.
The D6 version allowed you to add information the schema in addition to the 'type' field, like 'mysql_type' or 'pgsql_type' to use columns that are different from one database to another. In D6 this works fine. If you add those values to your schema, the designated columns are created in the database.
In D7 this does not currently work. This was discovered when the 'datetime' field was removed from the core schema.inc. It appears that we had places in the code that were only looking at the 'type' of the schema instead of the 'mysql_type' or 'pgsql_type'. In the case where the 'type' is something not defined by core, the table cannot be created.
This patch alters the core code so that it uses the 'mysql_type' and 'pgsql_type' if they are provided so modules can do things like define database types that core does not support.
I got this working in the Date module. The original suggestion to use schema_alter() does *not* work, but simply defining a schema that has the right information does work. So in the case of Date and other modules that want to create fields that use non-core data types, they just need to add the 'mysql_type' and 'pgsql_type' information to the field schema they define in hook_field_schema().
Works for me, marking this ready to commit.
#37
flipping fantastic!
#38
Plus this patch adds a test that the ability to create custom column types still works, which hopefully will help keep this from getting broken by future changes. I'll be happy if the Date module is not the canary in the coalmine on this issue any longer :)
#39
In an ideal world with an ideal database layer we wouldn't need this, right?
#40
Excellent! :) Well let's see if we can get a Date module that works with beta2!
Committed to HEAD. Great work!
#41
In an ideal world, Drupal would not use a SQL database.
#42
In an ideal world SQL databases wouldn't all be incompatible crap.
#43
Could we add this in an upgrade doc???
#44
Probably not a bad idea.
#45
First go at documentation is here
http://drupal.org/update/modules/6/7#db_specific_types
#46
That documentation makes it look like adding mysql_type is new in D7. It is not. What is new is that datetime now has to be defined that way because core is not doing it anymore. The related issue about removing datetime types has several places where the documentation needs to be updated to reflect the fact that core does not support datetime any more.Sorry, read the documentation more closely. You did describe it right :)
#47
I think we should do following snippet
function getFieldTypeMap() {
// Put :normal last so it gets preserved by array_flip. This makes
// it much easier for modules (such as schema.module) to map
// database types back into schema types.
// $map does not use drupal_static as its value never changes.
static $map = array(
'varchar:normal' => 'VARCHAR',
'char:normal' => 'CHAR',
'text:tiny' => 'TINYTEXT',
'text:small' => 'TINYTEXT',
'text:medium' => 'MEDIUMTEXT',
'text:big' => 'LONGTEXT',
'text:normal' => 'TEXT',
'serial:tiny' => 'TINYINT',
'serial:small' => 'SMALLINT',
'serial:medium' => 'MEDIUMINT',
'serial:big' => 'BIGINT',
'serial:normal' => 'INT',
'int:tiny' => 'TINYINT',
'int:small' => 'SMALLINT',
'int:medium' => 'MEDIUMINT',
'int:big' => 'BIGINT',
'int:normal' => 'INT',
'float:tiny' => 'FLOAT',
'float:small' => 'FLOAT',
'float:medium' => 'FLOAT',
'float:big' => 'DOUBLE',
'float:normal' => 'FLOAT',
'numeric:normal' => 'DECIMAL',
'blob:big' => 'LONGBLOB',
'blob:normal' => 'BLOB',
'datetime:normal' => 'DATETIME',
);
return $map;
}
Now when you add schema for example LONGTEXT than do the following
$schema['TABLE_NAME'] = array('fields' => array(
'data' => array('type' => 'text', 'size' => 'big', 'not null' => TRUE)
),
);
#48
It looks like the docs are done, so so is this issue.