Support for native date and time columns (like date, datetime, time) have been introduced on Drupal 7, but I suggest we remove them. Those types are only useful if we provide the necessary set of portable functions, and we don't.

More over, it is now very clear those types have very different semantics across the database engines: the 'Extra type tests' only passes on MySQL. It fails on at least SQLite, SQL Server and Drizzle (Drizzle doesn't even support the 'TIME' type).

Because it is *impossible* to build a portable application using those types, I suggest we remove them completely. We don't need to give module developpers more chances then necessary to shot themselves on the foot.

Moreover, the main argument for supporting those types was to enable Drupal to query external or legacy databases. Those applications are *not portable*, so in that case you can and should use the engine-specific type for a column, instead of the portable type, like this:

<?php
'mydate' => array(
 
'mysql_type' => 'TIME',
 
'description' => 'The date.',
),
?>

or

<?php
'mydate' => array(
 
'pgsql_type' => 'time without time zone',
 
'description' => 'The date.',
),
?>
Files: 
CommentFileSizeAuthor
#74 2010-12-28-866340-2.patch2.01 KBmikl
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 2010-12-28-866340-2.patch.
[ View ]
#72 2010-12-28-866340-docs.patch2.01 KBmikl
PASSED: [[SimpleTest]]: [MySQL] 30,444 pass(es).
[ View ]
#42 schema_alter_ideas.patch1.03 KBjpstrikesback
FAILED: [[SimpleTest]]: [MySQL] 28,256 pass(es), 0 fail(s), and 12 exception(es).
[ View ]
#39 datetime.patch621 bytesGerhard Killesreiter
PASSED: [[SimpleTest]]: [MySQL] 30,411 pass(es).
[ View ]
#30 rollback_date.diff6.8 KBmoshe weitzman
PASSED: [[SimpleTest]]: [MySQL] 22,834 pass(es).
[ View ]
#27 rollback_date.diff6.48 KBmoshe weitzman
PASSED: [[SimpleTest]]: [MySQL] 39,982 pass(es).
[ View ]
#1 866340-remove-extra-types.patch6.93 KBDamien Tournoud
PASSED: [[SimpleTest]]: [MySQL] 22,181 pass(es).
[ View ]

Comments

Damien Tournoud’s picture

Status:Active» Needs review
StatusFileSize
new6.93 KB
PASSED: [[SimpleTest]]: [MySQL] 22,181 pass(es).
[ View ]

Here is a patch that removes support for those types completely.

Josh Waihi’s picture

Status:Needs review» Reviewed & tested by the community

I have to agree with Damian here. Databases handle date/time very differently, However most databases understand EPOCH, an INT all databases understand. In addition, there is the date module that can provide these columns if needed.

chx’s picture

WTH? We support mysql_type and pgsql_type? Even in Drupal 6? There was no document for this, I added http://drupal.org/node/146939/revisions/view/968432/1093616 this quickly.

Now that's done the patch makes sense.

Dries’s picture

I'm OK with this but would love to get a 'yay' from Crell.

Crell’s picture

I'm with chx on "OMGWTFBBQ", as I'd never even heard of mysql_type et al before. It looks like it's not as much a feature as a clever hack due to the bare arrays we pass around. Yuck, yuck, and double-yuck.

That said, it is now kinda documented (although it should be better documented, probably), and as Damien notes it's not like the current fields are really useful, so I'll give this a "yay" with the caveat of "one more thing in Schema API we need to make suck less sometime in Drupal 8".

Dries’s picture

Status:Reviewed & tested by the community» Fixed

Oki-doki. Committed to CVS HEAD. Thanks.

rfay’s picture

Issue tags:+API change

Ah, the delights of code freeze :-)

Crell’s picture

This isn't an API change from Drupal 6 as Drupal 6 never had these columns, did it?

rfay’s picture

My definition of an API change is "something that breaks existing code". Since lots of contrib code has been ported, this is a possible break.

KarenS’s picture

Ah, also Drupal 6 schema API *does* have support for datetime fields, so this will be a huge WTF for any module or custom code that used it. There was also a way to extend Schema API to provide time fields in Drupal 6, which was used by some contributed modules.

I think this should get some review from developers that maintain modules using date data to see what this is going to do to the process of migrating their modules. Killing it after the API has been frozen for months in a one day old issue without any opportunity for discussion seems pretty harsh.

Crell’s picture

I skimmed through Date in CVS quickly before commenting here and I didn't see it used anywhere. That's why I was willing to pull it out. Can you point out where it's used in Date?

If this is a feature regression from D6 then I probably retract my "yay". :-(

KarenS’s picture

On line 302, http://drupalcode.org/viewvc/drupal/contributions/modules/date/date/date..., the type of field is 'datetime'.

I know there are other modules using it too, don't know any easy way to tell which ones. Event does for sure. Event also uses a time field.

Also note that the Schema API documentation for D6 refers to a datetime type (http://api.drupal.org/api/group/schemaapi/6). In fact, looking back at the evolution of Schema API, a datetime type has existed since 2007 (http://groups.drupal.org/node/3801).

I know that it is also used in custom code, by people using the Date API or just implementing it themselves.

Damien Tournoud’s picture

Those types are not portable, we should stop pretend they are. This is what this issue is about.

You just have to take a look at the code source for the Date module (for example, the date_api_sql.inc file, which is riddled with database-engine specific constructs) to be convinced.

This said, *nothing* prevents the Date module from using those types if it wants, with the engine-specific keys [engine]_type. It can even extends the Schema API to provide those types for other modules that want to shoot themselves in the foot (it will simply have to use hook_schema_alter() to provide the [engine]_type keys).

As a consequence, I see no reason to restore the lie that are those date/type types.

moshe weitzman’s picture

IMO, we should put back the removed types and mark them as mysql_type. This shows off our support for engine specific types (I never knew about it either - thats 3 core maintainers so far). If we can mark the test as mysql-only, we should do that. If thats not possible, drop the test. After that, we are no longer "pretending" as Damien dramatically says.

KarenS’s picture

I should also point out that there are people using the datetime field with postgres, they have submitted numerous patches to get/keep Date API working in postgres.

Dates are definitely engine-specific, there has never been any sort of 'universal' standard for how to handle them. But using the native date fields where possible has lots of advantages if you need to manipulate date data. I like Moshe's idea, leave it in but make it clear it may not be supported for all engines.

I am willing to move this to Date if there is consensus about that, but then Date will become a dependency for modules and code that don't require it now, which I'm not sure would make anyone happy, especially for something that has been in core for 3 years or so.

FYI, Geo is also using engine-specific database typing, for a geometry field. That's entirely in contrib, of course, just pointing out that we have more than one example of engine-specific database types.

Crell’s picture

Status:Fixed» Needs work

I'm re-opening this. I'm not comfortable removing a feature that was in D6 and at least sort of worked at this late stage. We need to roll this back and, possibly, implement Moshe's suggestion from #14.

Normalizing the creation of the field is at least something, even if we can't (and don't claim to) have a universal solution for working with the field. I suppose that's no worse than D6.

andypost’s picture

Status:Needs work» Fixed

Having datetime types without functions to format and display is useless.

Core uses unixtime so when someone needs to work with db-specific data anyway writes a custom queries in custom module.

Modules date, event, calendar is a good example of different approaches to work with dates.

Crell’s picture

Status:Fixed» Needs work

This is a regression from D6 at the moment, which in our current release state we should not allow without a damned good reason. (I didn't realize it was a regression earlier, which is why I OKed it originally.) This needs to be rolled back.

Damien Tournoud’s picture

No need to say that I strongly disagree with reverting that.

(1) The only type that was in D6 was datetime, the others were not supported
(2) This is a seldom used feature
(3) This is not really portable: we don't provide any of the required function to work with those types and the columns themselves have slightly different semantics across database engines
(4) We support engine-specific types so anyone can still use it for legacy reasons
(5) KarenS already agreed to bring this functionality (with all the associated required functions) to the Date module

webchick’s picture

Mmm. I think "agreed" is a strong word. I read that more as "If there's no other recourse." She also outlined that modules that did not previously need a dependency on Date API would need to add one, which is yet another API change...

This change is incredibly late in the cycle, and causes both a D6 and "first 28 months of D7" regression. It's a no-brainer to roll it back, IMO.

Damien, is what Moshe describes in #14 a compromise for you?

KarenS’s picture

ATM the D7 Date port is totally busted. Would one of the people who wants this to go into Date like to create a patch to move this functionality there so I can at least see if it and how it works. I need the patch to do anything at all until he question of rolling this back is resolved.

Damien Tournoud’s picture

We cannot easily mark a test as MySQL-only.

Marking the types as 'mysql_type' is basically the same as removing them. As I explained above, those types are *not portable* but *nothing prevents you from using them*. You just have to use the engine-specific types ('mysql_type', 'pgsql_type', etc.), instead of the portable type.

Here is the patch to date_admin.inc:

<?php
- $db_columns['value'] = array('type' => 'datetime', 'not null' => FALSE, 'sortable' => TRUE, 'views' => TRUE);
+
$db_columns['value'] = array('mysql_type' => 'datetime', 'pgsql_type' => 'timestamp without time zone', 'not null' => FALSE, 'sortable' => TRUE, 'views' => TRUE);
?>

This patch feels like a net gain (it acknowledges that those types are not portable, and that you should not use them to build a portable application), without putting any cost on anyone. The Date module will (unless someone does the job of writing all the necessary functions) only work on MySQL and PostgreSQL *anyway*.

Damien Tournoud’s picture

@KarenS, you have your patch to date_admin.inc. I'm not going to make the Date module portable across all the database engines we support. The native date types are *your problem*. I made clear a few times already that using them was a bad idea.

KarenS’s picture

The patch actually won't work without adding a 'type' as well as 'mysql_type' and 'pgsql_type', which then leads to a question of the right thing to use for type.

I could use 'varchar', but that will then might add an undesired 'length' attribute to my field. (createFieldSql($name, $spec) looks like it will add that column). I can't tell where that function actually gets invoked to tell where it might impact me.

I could get around that by calling the type 'datetime', an undefined type, which kind of works, but no idea what might happen if some other module creates a field type using that name or if there are other implications to using an type not defined by core. I could try making it a 'blob' type to avoid any potential problems with createFieldSql().

But if we are removing this type we also need to clean up the following, since core is not defining datetime:
<?php
function db_type_placeholder($type) {
switch ($type) {
case 'varchar':
case 'char':
case 'text':
case 'datetime':
return '\'%s\'';
...
}

And that fix will leave a type called 'datetime' without a placeholder, which I can only get around if I use 'varchar' or some other known type as the type. And if I use the 'blob' type the placeholder will be wrong, since datetime fields need to be '%s' not '%b'.

I'm playing with 'varchar' which looks like the closest fit to see what I will run into, but this all needs to be tested. I just want to point out that this change has more implications that the above patch indicates.

Damien Tournoud’s picture

The patch actually won't work without adding a 'type' as well as 'mysql_type' and 'pgsql_type', which then leads to a question of the right thing to use for type.

I could use 'varchar', but that will then might add an undesired 'length' attribute to my field. (createFieldSql($name, $spec) looks like it will add that column). I can't tell where that function actually gets invoked to tell where it might impact me.

An alternative, as I suggested in #13 is for Date to define this type, and add the 'mysql_type' and 'pgsql_type' entries in a hook_schema_alter().

<?php
function date_schema_alter(&$schema) {
  foreach (
$schema as &$table) {
    foreach (
$table['fields'] as &$field) {
      if (
$field['type'] == 'datetime') {
       
$field += array('mysql_type' => 'datetime', 'pgsql_type' => 'timestamp without time zone');
      }
    }
  }
}
?>

Obviously, you would also have to make sure that your module implement hook_requirements() so that it doesn't install on an unsupported database.

But if we are removing this type we also need to clean up the following, since core is not defining datetime:

<?php
function db_type_placeholder($type) {}
?>

db_type_placeholder() is not part of Drupal 7.

KarenS’s picture

Since no one has reversed the change, I went ahead and added the schema_alter to the Date module. Someone needs to get this documented on the module update page, both that the datetime type is no longer provided by core and how to add this support to a custom module, and also remove the current documentation that says that the new date and time types have been added. And there are probably other places where documentation needs to be altered, like http://api.drupal.org/api/group/schemaapi/7.

Also since Date is doing the schema_alter, if any other module tries to do a schema_alter for the same name one of us will overwrite the other, which is why it was nice to have it in core. I also added support in Date API for the date and time types. Again, if any other module tries to do that we will have problems.

I just want to repeat that it was way too late to make this regressive API change. If you wanted to debate this it should have been done before the freeze.

moshe weitzman’s picture

Status:Needs work» Needs review
StatusFileSize
new6.48 KB
PASSED: [[SimpleTest]]: [MySQL] 39,982 pass(es).
[ View ]

OK, here is a straight rollback. From there, we can decide what to do.

Crell’s picture

webchick informs me that Dries is considering what to do here, so we'll have to wait for his ruling.

Status:Needs review» Needs work

The last submitted patch, rollback_date.diff, failed testing.

moshe weitzman’s picture

Status:Needs work» Needs review
StatusFileSize
new6.8 KB
PASSED: [[SimpleTest]]: [MySQL] 22,834 pass(es).
[ View ]

oops. no with --no-prefix on git diff.

moshe weitzman’s picture

Status:Needs review» Reviewed & tested by the community

OK, lets get this into Dries' queue then.

Dries’s picture

Personally, I'm with DamZ here. I'd like to leave those types out, but I also want to think about it a bit more.

KarenS’s picture

Support for this is in Date API now. I think the question is whether other modules that want to define these types will be willing to defer to Date on this and either require the Date API as a dependency or define date types with different names if they want to do it themselves.

What will NOT work is for multiple modules to try to do this, using the same type names (three modules defining different ideas of what 'datetime' is would be disastrous). Or else we should clearly establish an expectation that any module that defines custom storage types will namespace the type names to avoid that problem. (Note: Will the system support underscores in storage type names if we go that route?)

And if core is going to remove support for a previously-supported type we should maybe add a 'custom storage type' test to ensure that later patches and changes don't interfere with the ability of other modules to create custom storage types.

I am willing to go either way, with that caveat.

aspilicious’s picture

Weather module is broken without the reroll. :(

jpstrikesback’s picture

Currently a schema_alter() can't put a 'mysql_type' or otherwise in at the time of creating a table...for fun I created a little create_table_alter() hook called in db_create_table() to make a 'mysql_type', that's weak I know, but it got me a 'mysql_type'/'pgsql_type'...

I still don't understand the harm of leaving these in the per database schema.inc's (since they're per DB), but I'll reread everything and try :)

Dries’s picture

Status:Reviewed & tested by the community» Fixed

I still believe this was 'fixed'. Not planning to roll back based on the provided information.

David_Rothstein’s picture

Status:Fixed» Needs work
Issue tags:+Needs Documentation

At a minimum it looks like this needs documentation, as described in #26 (for example, http://drupal.org/node/224333#schema_date_time is wrong, plus the other things Karen mentioned).

Also, I think Moshe's patch was intended to be the beginning of a discussion, not the end of it.... There are other options discussed above besides a complete rollback.

tutumlum’s picture

subscribing..

Gerhard Killesreiter’s picture

Priority:Normal» Critical
StatusFileSize
new621 bytes
PASSED: [[SimpleTest]]: [MySQL] 30,411 pass(es).
[ View ]

It is pretty much unacceptable for a PHP application to ship without support for datetime SQL types.

In D6 you could work around this lack of support by simply ignoring schema and defining your datetime column anyway.

You can not do this with current Drupal 7: it yells at you for using an undefined schema type.

I needed to patch schema.inc to make it shut up (attached patch is an illustration and not intended to be committed).

At the very least we need either:

1) a hook to extend schema.

2) a way for a module to override the current database driver's function getFieldTypeMap in schema.inc.

moshe weitzman’s picture

@killes - i'm not happy about this removal either, but you managed to not follow all the discussion above where we learned about how modules can extend schema api and even create mysql specific column types. that’s what karen has already done with date api. you could consider depending on that module and you will get your datetime column type back ... if you really want to roll this back, i proposed a way to get it back for mysql only in #14

Gerhard Killesreiter’s picture

Priority:Critical» Normal

Yes, In fact that seems to mostly work (found a buglet which Damien will hopefully deal with). I was coming here since KarenS seems to have updated her code but not her project page which still says that date id broken due to this change, so I assumed this to not work.

I still think this ought to be in core.

jpstrikesback’s picture

Priority:Normal» Critical
StatusFileSize
new1.03 KB
FAILED: [[SimpleTest]]: [MySQL] 28,256 pass(es), 0 fail(s), and 12 exception(es).
[ View ]

@Gerhard thankyou...re: 2) see #35

If we can't have those Types, surely we can at least have some usable db_this_alter() functions...attached is a rough idea, which I used like so:

<?php
function date_api_db_create_table_alter(&$table) {
  foreach (
$table['fields'] as &$field) {
    if (
$field['type'] == 'datetime') {
     
$field += array('mysql_type' => 'DATETIME', 'pgsql_type' => 'timestamp without time zone');
    }
    elseif (
$field['type'] == 'date') {
     
$field += array('mysql_type' => 'DATE', 'pgsql_type' => 'date');
    }
    elseif (
$field['type'] == 'time') {
     
$field += array('mysql_type' => 'TIME', 'pgsql_type' => 'time without time zone');
    }
  }
}
?>

oh and http://drupal.org/node/200953 ???

jpstrikesback’s picture

@Moshe, Gerhard see #35, schema_alter cannot add the mysql types at table creation from my tests...

EDIT: here is a Aug 30th IRC conversation (thread/method mentioned is this one/#25):

11:29:04 AM jpstrikesback: DamZ: I'm talking about using schema_alter to add mysql_type => 'thistype' to a db field being created, it just doesn't seem to add it in, I suspect since the field hasn't been created yet, tho that seems nuts since it's called for field creation right?
11:29:32 AM jpstrikesback: db field
11:32:02 AM jpstrikesback: DamZ: i.e. is using hook_schema_alter() to add a 'mysql_type' a chicken/egg scenario? or is there something else wrong with the method you suggested in the above thread?
11:42:41 AM DamZ: jpstrikesback: I guess we are inconsistent in the way we call hook_schema_alter()
11:46:03 AM DamZ: chx: what do you think about that? ^ we currently have no way to alter the schema of a table being created, because drupal_install_schema() uses the unprocessed schema and db_create_table() has no alter hook

MustangGB’s picture

Date module/api is planned to be integrated into D8 core, yes?
So dependency on Date in D7 would be fine for me
Gives time to get the api and portable functions sorted

moshe weitzman’s picture

Priority:Critical» Normal

Drupal works without this.

jpstrikesback’s picture

Pardon that status change, it was accidental (#42 - back to critical) :P

I didn't even realize that happened till just now - I had started writing that post and left it half done and came back and put a patch in there for fun and then by that time......

KarenS’s picture

OK, something has busted again. I had this working in Date back in comment #26 but the fix I used then no longer works. We now get error messages as reported in #925526: Cannot add datetime field to content. I have no idea what has changed and would appreciate help figuring it out.

The messages are:

Notice: Undefined index: datetime:normal in DatabaseSchema_mysql->processField() (line 192 of /Library/WebServer/clients/reliefweb/includes/database/mysql/schema.inc).

There was a problem creating field asdf: 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 'DEFAULT NULL, PRIMARY KEY (`etid`, `entity_id`, `deleted`, `delta`, `language`)' at line 9.

webchick’s picture

Anyone with git bisect able to help Karen sort this out?

KarenS’s picture

Field SQL Storage now fails if the type is not defined. If I use the patch in #39 and remove my schema_alter to define the datetime type, the field can be created. This means that there is no way to define any sql type other than what core defines and create a field from it.

There is no way for Date module to create a datetime field with core in its current state.

jpstrikesback’s picture

@webchick & @KarenS thanks, I guess I wasn't succinct enough ( in #35, #43)

rfay’s picture

With the last August 4 commit of D7 and the last August 4 commit of date, I get the same errors I do when trying to add a datetime with current HEAD of both. I may be confused or just getting something wrong. Is it possible that this never did work for datetimes?

Error message
Notice: Undefined index: datetime:normal in DatabaseSchema_mysql->processField() (line 192 of /home/rfay/workspace/d7git/includes/database/mysql/schema.inc).
Status message
There was a problem creating field somedt2: 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 'DEFAULT NULL, PRIMARY KEY (`etid`, `entity_id`, `deleted`, `delta`, `language`)' at line 9.
KarenS’s picture

I should be more clear. When you create a field field_create_field() runs, which in turn tries to execute field_sql_storage_field_storage_create_field(). That function, field_sql_storage_field_storage_create_field() in turn calls field_sql_storage_schema() and then tries to do db_create_table() on the result. Something in that process fails if I've defined the datetime type in schema_alter(), but works if the datetime type is defined by core in schema.inc.

The easy fix is to apply the patch in #39. But that only fixes datetime for mysql. The current state of affairs is a serious regression. In D6 any module could define a custom type and db queries worked without errors and you could create CCK fields with those types.

KarenS’s picture

And this did work at one time for datetime fields, but I've been unable to find a way to get back to a working setup.

KarenS’s picture

@jpstrikesback -- NOW I get what you are saying, yes, maybe that is what is needed is a new alter hook. But that also means (I think) that creating a new type takes quite a bit more effort -- you need a schema_alter and a create_table_alter and who knows what else.

Damien Tournoud’s picture

Damien Tournoud’s picture

The second issue I linked above is meant to take care of the notices (and fatal errors) you get when trying to create a field without a portable 'type'.

KarenS’s picture

Status:Closed (fixed)» Needs work

Also, as noted in #26 and #37, there is lots of documentation around this change that has not been updated. We either need to leave this open or start a follow up issue for that.

This issue has a lot of information in it, the follow up issues should link back to this one. Marking it closed (fixed) with no links back to this just made the whole unresolved topic disappear. Wouldn't it be better to mark it fixed (not closed), once all the follow up issues have been created to give people who are tracking this a way to see what is going on?

bjaspan’s picture

I didn't know about this issue until today. I don't have anything to add about datetime types but can provide some historical information on Schema API. Yes, it has always supported &lt;engine&gt;_type properties on field specifications. Without it, we would be forever limited to a least-common-denominator set of database column types. With it, the idea is any module can define a column using any db-specific column type it wants, with the provision that it won't work on other database engines. If that is no longer possible, that is indeed a bug.

I apologize if this was not documented. Looking now, I see it is mentioned in the comments at http://drupal.org/node/159605#comment-1674678, and in http://drupal.org/node/146939 but only b/c chx added that in response to this issue.

greg.harvey’s picture

I added a reference to this issue in #927828: Some schema code incorrectly rely on the generic type instead of the engine-specific type, for completeness as suggested by #57 ... the other issue already had a reference.

LaurentAjdnik’s picture

Tagging

mfb’s picture

jhodgdon’s picture

It looks like this is in serious need of documentation on the 6/7 update page, so changing the tag accordingly... I'm not clear on what needs to be written though?

If other Handbook pages need to be written/updated, the best thing to do would be to file an issue in the Documentation project.

mikl’s picture

Okay, I am really at a lack of words here. Just as I thought I had gotten rid of the need for ugly hacks like `mysql_type` in #200953: Schema API lacks the 'time' and 'date' type, I find this.

I am so tired of Drupal taking the lowest common denominator path with things like these. I know many devs hate SQL and would rather go playing in OOP-lah-lah land, but would you please stop ruining everything for the rest of us? INT is not a suitable replacement for the datatime column type.

mikl’s picture

Priority:Normal» Critical

Not to mention that this change broke a lot of contrib-code.

Damien Tournoud’s picture

Priority:Critical» Normal

This issue remains open for documentation only. Please open a separate issue if you want to discuss this further.

mikl’s picture

Priority:Normal» Critical

Elaborating, first of all, I am rather disappointed that Damien did not refer to the discussion in #200953: Schema API lacks the 'time' and 'date' type that caused DATE and TIME to be added to the schema types. Suppressing the debate that lead to earlier changes when reverting them seems rather dishonest to me.

And to add insult to injury, this patch also removes DATETIME support, which outside the Drupal world is the standard format for storing date and time, and in many ways better and more flexible than ugly hacks involving converting date and time to a number of seconds since some random date.

The main argument here is that these data types are not supported on databases used on an infinitesimal number of Drupal sites. At least 99.9% of all Drupal sites use either PostgreSQL or MySQL, and if a contrib developer decides that is good enough, who are the Drupal core devs to decide how he has to design his app?

We need to make working with Drupal easier, not harder, so please stop getting in the way of supporting standard database features with your ideological ivory tower standards. Lowest common denominator is a poor way to do APIs, and having to jump through all sorts of hoops to use DATETIME with your schema API is a good way of scaring away developers used to such things from other tools.

aspilicious’s picture

The date module uses datetime...

So it isn't completly gone...

Damien Tournoud’s picture

Priority:Critical» Normal

You are picturing this the wrong way:

  • The Date module provides those types and a decent set of API functions around them in contrib.
  • You have access to 'mysql_type', 'pgsql_type', 'sqlite_type' and friends if you want to use types that are only portable on a subset of the databases that Drupal support.

There is nothing to do here and your arguments in #66 are completely moot. We are not actively preventing you to decide you don't care about portability or are happy with the inconsistent implementations (and general uselessness, as I already explained in length) of those types.

By removing those types from the default set of types Drupal Core supports, we are just recognizing the fact that we cannot support them in a portable way. I see nothing wrong with that.

mikl’s picture

#68: I beg to differ. There is no documented way to use any field types besides those officially blessed by the SQL-dissing core developers. At the very least, the database system should to accept any input to the type-parameter and just use it as-is if it does not have an entry in the type map.

Damien Tournoud’s picture

The special keys 'mysql_type', 'pgsql_type', 'sqlite_type' and friends are the documented way to use types that are only portable on a subset of the databases that Drupal support.

jpstrikesback’s picture

@mikl here is the links to relavent docs and discussions:

Docs:
http://drupal.org/update/modules/6/7#db_specific_types

More background to that:
http://drupal.org/node/927828#comment-3609590

mikl’s picture

Status:Needs work» Needs review
StatusFileSize
new2.01 KB
PASSED: [[SimpleTest]]: [MySQL] 30,444 pass(es).
[ View ]

Well, it seems the anti-SQL boat has sailed, so here’s a patch for the API docs describing the current state of things.

jhodgdon’s picture

Status:Needs review» Needs work

Your patch needs to be reformatted without the spaces that are at the ends of some of the lines.

Also, in Drupal documentation, we do lists like this:

a, b, or c

(i.e., you need a comma before the "or").

Aside from those minor formatting issues, the patch is OK from a formatting/writing perspective. Can Damien or someone else review it for accuracy?

mikl’s picture

Status:Needs work» Needs review
StatusFileSize
new2.01 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 2010-12-28-866340-2.patch.
[ View ]

Here’s an updated patch with the corrections pointed out in #73.

Damien Tournoud’s picture

Status:Needs review» Reviewed & tested by the community

Yep, nice one. I never knew we had documentation there :)

hook_schema() should probably point there. Anyway, this one is accurate and badly needed.

jhodgdon’s picture

Damien: see #986546: Schema API page and hook_schema() doc need reorganization
I'm trying to get the schema docs to make some kind of sense and point to each other (review would be welcome)

webchick’s picture

Status:Reviewed & tested by the community» Fixed

Ok, since the hook_schema() thing is being handled elsewhere, committed #74 to HEAD. Thanks!

jhodgdon’s picture

Status:Fixed» Needs work

Setting back to Needs Work since I think this still needs a mention in the update guide, doesn't it?

juanca2626’s picture

Status:Needs work» Needs review
Issue tags:-API change, -Needs Update Documentation

#74: 2010-12-28-866340-2.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:+API change, +Needs Update Documentation

The last submitted patch, 2010-12-28-866340-2.patch, failed testing.

jhodgdon’s picture

juanca2626 - this patch has been applied. The issue is still open because this needs documentation, as per #78.

geerlingguy’s picture

Subscribe. I'm confused (after reading through the upgrade guide, I'm a bit confused as to what's supported, and what's not supported...). It seems, with 7.0, that I can't use 'type' => 'datetime', for a field in my module's schema.

Is the recommended method, then, to use a unix timestamp in an 'int' field?

jhodgdon’s picture

Status:Needs work» Postponed (maintainer needs more info)

OK. I've just read through this entire issue and I'm totally lost. I need an issue summary, because this still apparently needs some documentation in the 6/7 module update guide, and maybe elsewhere, and I cannot for the life of me figure out what the documentation should say.

Could someone who understands this issue and the current state of these fields please comment with the following information:

a) What needs to be documented in the 6/7 module update guide (i.e., what changed that affects module developers, and what should they do to modify their modules when upgrading from 6 to 7)?

b) Is there any other documentation that is still incorrect and needs to be updated, in relation to these changes?

rfay’s picture

I suspect the documentation also dearly needs "How do I store date/time data now that these types have been removed".

KarenS’s picture

The internal documentation is all fixed now I think. The update guide has http://drupal.org/update/modules/6/7#db_specific_types, which includes an example of how to define a type not defined by core.

I'm trying to think of what other documentation might be needed. Maybe that is enough.

jhodgdon’s picture

Thanks! It looks like at least the update guide is in good shape. Do we need to add something to the Schema API section of the d.o online doc?
http://drupal.org/developing/api/schema

jhodgdon’s picture

Status:Postponed (maintainer needs more info)» Needs work

It looks like someone should take some of the text from http://drupal.org/update/modules/6/7#db_specific_types and add it to the bottom of the Data Types page in the Schema API docs:
http://drupal.org/node/159605

ardas’s picture

Guys,

Please keep the DateTime and Timestamp support on a Schema level. At least people who needs it would be able to support it. Lets the Date form field will be varchar by default but we still need an ability to develop our own field using more advanced date handling approach.

andypost’s picture

@ardas This issue still open for Doc team. There's 3 weeks for D8 in #501428: Date and time field type in core before feature-freeze.

d.novikov’s picture

This issue is somewhat related, no?
http://drupal.org/node/1360656

m v v’s picture

Status:Needs work» Needs review
Issue tags:-Needs Documentation, -API change

#27: rollback_date.diff queued for re-testing.