Remove support for date and time types

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.

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.

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

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".

Oki-doki. Committed to CVS HEAD. Thanks.

Ah, the delights of code freeze :-)

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

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.

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.

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". :-(

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.

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.

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.

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.

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.

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.

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?

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.

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.

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.

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

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

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

OK, lets get this into Dries' queue then.

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.

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.

Weather module is broken without the reroll. :(

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 :)

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

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.

subscribing..

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.

@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

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.

@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:

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 ???

@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

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

Drupal works without this.

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......

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.

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

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.

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

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.

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.

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.

@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.

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?

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 <engine>_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.

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.

Tagging

link to faulty docs (via bellHead): http://api.drupal.org/api/drupal/includes--database--schema.inc/group/sc...

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.

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.

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

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.

The date module uses datetime...

So it isn't completly gone...

#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.

@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

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

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?

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

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)

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

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

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

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

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?

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?

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

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.

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

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

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.

@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.

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

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