Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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
Comment #1
Damien Tournoud CreditAttribution: Damien Tournoud commentedThis is actually very simple to fix.
Comment #3
moshe weitzman CreditAttribution: moshe weitzman commentedbot is not happy. install failed.
Comment #4
KarenS CreditAttribution: KarenS commentedSubscribe.
Comment #5
jpstrikesback CreditAttribution: jpstrikesback commentedsub
Comment #6
KarenS CreditAttribution: KarenS commentedShouldn'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.
Comment #7
bjaspan CreditAttribution: bjaspan commentedSubscribe.
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.
Comment #8
LaurentAjdnik CreditAttribution: LaurentAjdnik commentedTagging
Comment #9
LaurentAjdnik CreditAttribution: LaurentAjdnik commentedAttempt to fix the patch for mySQL. The array of types has to be mysql-ized:
Comment #10
LaurentAjdnik CreditAttribution: LaurentAjdnik commentedComment #11
LaurentAjdnik CreditAttribution: LaurentAjdnik commentedNow with pgsql and sqlite support.
Comment #12
Stevel CreditAttribution: Stevel commentedThis 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:
Comment #13
Damien Tournoud CreditAttribution: Damien Tournoud commentedNice catch. But now this needs a drupal_strtoupper() or something.
Also, this needs a (quick) test.
Comment #14
Damien Tournoud CreditAttribution: Damien Tournoud commentedComment #15
LaurentAjdnik CreditAttribution: LaurentAjdnik commented$spec['mysql_type'] contains fixed values returned by getFieldMap().
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 ?
Comment #16
chx CreditAttribution: chx commentedWhat a nasty little critter. Some really nice test (that hopefully discoveres more bugs here!) would be awesome.
Comment #17
LaurentAjdnik CreditAttribution: LaurentAjdnik commentedNeed help: any volunteers for the tests ? :-|
Comment #18
boombatower CreditAttribution: boombatower commentedWorking on tests, got some clarification from chx in IRC.
Comment #19
boombatower CreditAttribution: boombatower commentedBefore patch (test fails):
After patch nothing.
Comment #20
boombatower CreditAttribution: boombatower commentedLooks good, just need a review of test.
Comment #21
Stevel CreditAttribution: Stevel commentedThe 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.
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.
Comment #22
greg.harveyRelated to #866340: Remove support for date and time types, just for reference/background.
Comment #23
LaurentAjdnik CreditAttribution: LaurentAjdnik commentedHere 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:
Comment #25
Stevel CreditAttribution: Stevel commented'else' should be on a new line
same again
and again
sqlite_type and pgsql_type are still missing from the test, making the tests fail on sqlite / postgresql
Powered by Dreditor.
Comment #26
KarenS CreditAttribution: KarenS commentedWhy 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?
Comment #27
LaurentAjdnik CreditAttribution: LaurentAjdnik commentedComment #29
LaurentAjdnik CreditAttribution: LaurentAjdnik commentedComment #30
effulgentsia CreditAttribution: effulgentsia commentedSubscribing.
Comment #31
Stevel CreditAttribution: Stevel commentedThe 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.
Comment #32
jpstrikesback CreditAttribution: jpstrikesback commentednice
Comment #33
moshe weitzman CreditAttribution: moshe weitzman commentedCode and test look good to me.
Comment #34
webchickSo, 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.
Comment #35
KarenS CreditAttribution: KarenS commentedJust getting ready to do a Date module test on this patch. I'll report back.
Comment #36
KarenS CreditAttribution: KarenS commentedw00t!
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.
Comment #37
jpstrikesback CreditAttribution: jpstrikesback commentedflipping fantastic!
Comment #38
KarenS CreditAttribution: KarenS commentedPlus 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 :)
Comment #39
Dries CreditAttribution: Dries commentedIn an ideal world with an ideal database layer we wouldn't need this, right?
Comment #40
webchickExcellent! :) Well let's see if we can get a Date module that works with beta2!
Committed to HEAD. Great work!
Comment #41
Damien Tournoud CreditAttribution: Damien Tournoud commentedIn an ideal world, Drupal would not use a SQL database.
Comment #42
Crell CreditAttribution: Crell commentedIn an ideal world SQL databases wouldn't all be incompatible crap.
Comment #43
aspilicious CreditAttribution: aspilicious commentedCould we add this in an upgrade doc???
Comment #44
webchickProbably not a bad idea.
Comment #45
larowlanFirst go at documentation is here
http://drupal.org/update/modules/6/7#db_specific_types
Comment #46
KarenS CreditAttribution: KarenS commentedThat 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 :)
Comment #47
sukh.singh CreditAttribution: sukh.singh commentedI think we should do following snippet
Now when you add schema for example LONGTEXT than do the following
Comment #48
Crell CreditAttribution: Crell commentedIt looks like the docs are done, so so is this issue.