add backend support for cck date fields

sounds great. i hope someone has time to write a patch for it. ;)

A different approach. If you look at how the event module works it allows you to specify content types that are events. It generally handles the adding of the date fields which is what it normally uses to produce the calendar. One could take the same approach for reminders, you could add another flag to say which content type works as reminders and add fields as needed to the approriate content forms.

Has anyone done anything on this as of recent? I am considering the same thing. There's now also the date module. What seems like the best point of entry, getting signup to send reminders or another module like scheduled actions?

cheers,
Ian

tracking

+1 (and a Mars bar) for anyone who writes a patch for CCK integration ;)

any mind reviewing part 1? http://drupal.org/node/146248

FYI: see killes's efforts for this over at http://drupal.org/node/154580

I just committed http://drupal.org/node/154580, so full date.module support will now be rather trivial to add. Someone just has to copy signup_event_5.x-1.inc to signup_date.inc, and re-write it accordingly. Should be quite obvious from looking at the existing file to see what needs to be done.

Any volunteers want to assign this to themselves and take a stab at a first draft?

Thanks,
-Derek

p.s. I marked http://drupal.org/node/146248 duplicate with this, since there's no longer anything to gain by keeping reminders and auto-close in separate issues.

This may not be so easy after all. I haven't actually tried this out, but just looking at the code, it seems to make an assumption that you know what the field names and date types are just by knowing that the date module is enabled. Your sql() calls have no parameters, so there's no way to know what field is being manipulated. It works for the event module because that module has fixed names for its fields and tables, but to write sql for date module fields you need to know the type of field (iso or timestamp) and you also need to know the field name so you can figure out the database column names and what table the field is stored in.

And for even more complication, there will always only be one set of event module dates on a node, but there could be several different date module dates on the same node.

I don't have time to dig into this right now, but if anyone else does, I think the key is figuring out a way to let those functions know something about what field is being evaluated so they can do the right thing.

Thinking about this some more...

Maybe what is needed is when you set up a content type for the signup module, you can select the specific date field that should be used for the signup for that content type. Store that content type/field_name relationship somewhere -- in a variable or in a table.

Then we just need the signup module to provide a way to alter the sql by content type instead of using the same sql for all content types, and the field info can be retrieved using content_database_info($field) to write the correct sql for that content type.

I've always thought (e.g. http://drupal.org/node/146248#comment-559264) that when enabling a content type for signups that had date fields, you had to select which field was the start time via an admin setting. I had assumed it'd be something like a drop-down of all date fields on that content type for the global content admin page (admin/content/types/[type]), since that's the page signup admins are already used to visiting to decide if they want to signup-enable any given content type. However, if folks thought it was better to have it as something you can specify when editing the field itself inside CCK, please make a convincing argument, I'm open to other approaches. However, this assumption is why the signup_*_sql() hooks don't take any arguments -- I thought the implementations would just look at some variables to know what to do, instead. The whole point is to hide the implementation details of each particular date-related backend from the main signup code as much as possible.

However, now that I think about it more closely, it gets complicated for the "signup-enable a single node of this type" use-case. :( You can't just select the "enable for signups" radio (or whatever it's called). If it's got date fields, you'd also have to select *which* date field to use as the start time. Not sure what to do about that. I see a few options:

A) Punt. Say that particular combination of uses isn't exactly supported, and that if you signup-enable a single node that has date fields, signup.module won't think of it as having a start time, and from signup's perspective, it's an untimed event. If you want timed events with date fields, use the global content type admin page.

B) Let the admin select which date field is the start time for a given node type, even if that node type isn't signup-enabled by default. So, they could still globally indicate which field to use, even if signups are only enabled on a per-node basis. This would probably be a little counter-intuative and confusing, but it'd be somewhat consistent in its own way. ;) Worst case that someone signup-enables a single node, but there's no global setting pointing to the right date field, we fall back to option (A) and consider this an untimed event.

C) If signups are globally disabled on a given node type, and the admin is trying to signup enable a single node of that type, they get to select which date field is the start time for that particular node. This seems like the most complicated for the underlying code and DB schema, and would probably be too confusing in the UI as well.

Thoughts?
-Derek

Maybe something like this patch (completely untested). It provides a way to pass the content type to the *_sql() hooks, which I think is necessary for anything to work at all.

With the name of the content type and the name of the specific date field to track in the content type, you could create a signup_date.inc file to create the right sql for that field and content type.

@moshe #17 -- that's exactly what option (B) was. ;)

@KarenS #16: Wow, thanks. That's quite a patch. Sadly, it seems there's some stray whitespace in signup* which is making your patch bigger than it needs to be. ;) That aside, I suppose you're right that we need something like this -- without the content types getting passed around and dealt with, we'd be screwed. I'll have to take a closer look when I have more time, but that seems like a reasonable start. One obvious thing would be to cache the results in signup_types(), since that LEFT JOIN is going to be expensive. :(

But, anyway, thanks!

rerolled to keep up with HEAD. still haven't had a chance to look deeper yet. anyone manage to find any funding for this yet from somewhere?

Bumping prio, too. I'd like to get this in before the 5.x-2.4 release.

subscribing

I'd like to test this patch, but I'd like to know what version of date this patch is meant for. Is it the stable 5.17 or the 5.2 (Head) release? The date module page says that there are significant api changes in the 5.2, so if it works for both I'd be pretty surprised.

It's for the stable version, not HEAD.

And it's not a finished patch. It tries to change the way that the Signup module handles things so you can pass the content type and field name to the .inc file. What is still needed is to create the .inc file for the Date module that can create the right results from the content type and field name.

BTW, 5.x-2.x-dev is from HEAD, (which you can tell from looking at the 5.x-2.x-dev release node) and that's where all this pluggable date back-end stuff is happening. ;)

Is there an update on this? Has anyone done further work on making the signup module work with date.module version 5.2?

+1
subscribing
really useful to have it fixed for date module

If KarenS or dww want to get this patch working in a stable release soon, I may be able to sponsor this.

I've got a client project I'm doing that I would use this on with a budget at my discretion.

Contact me at preston at ptone d_t com with details of what would be needed.

-Preston

subscribing

subscribing

Here's my attempt at implementing this feature, based off of the patch KarenS provided above. It has been tested with cck date fields, but not so much with the Event module (not much has changed in regards to the Event/signup integration, so it still should work).

Here's a list of improvements that still can/need to be done:

• Support for datestamp cck field types (currently only supports date cck field types)
• Ability to order by date when viewing signup list in admin (not sure if this is possible, since there's the possibility of different date fields for each node)
• Better theming of date ranges
• Better documentation

Let me know what everyone thinks, and if anyone runs into any bugs. I'm working on getting this active on a client site, so if I run into any bugs, I'll be sure to update the patch.

Thanks,
Jon Duell

Changing status to patch review

Sorry, forgot to run my patch through coder to meet Drupal coding standards. Attached is a cleaned up version.

subscribing

subscribing

subscribing

This patch is incompatible with date v2. Simply replacing all instances of date_show_date with date_format_date in signup_date.inc worked for me.

subscribing

I am getting an error when I edit a node. I have this patch enabled.

Fatal error: Call to undefined function date_iso2unix() in /homepages/21/d245084341/htdocs/chssite/sites/all/modules/signup/signup_date.inc on line 95

On line 95, I think date_iso2unix needs to be replaced with strtotime().

@will_in_wi: Are you using date v2? Currently, this patch only works with date v1, though I'll be posting an updated patch soon.

I am using date v2. It seems to work with the modifications from comments 39 and 41.

Attached is an updated patch with support for date v2, including the changes listed by will_in_wi and a few other fixes (thanks will_in_wi, didn't realize at first you made both comments 39 and 41).

I applied this (signup_date_2.x.patch - comment #44) patch for Signup module with latest Date module (rc 2 from July, 10th)
but I got a Fatal Error after I submit signup:

Fatal error: Call to undefined function signup_format_date() in /home/users/somvprahe.sk/web/www/sites/all/modules/signup/signup.module on line 1432

Igorik
http://www.somvprahe.sk

Works for me. What version of signup did you patch?

hi!
It is Signup 5.x-2.4 2008-Feb-08 (the only one 2x there is)

thanks
Igorik
http://www.somvprahe.sk

Hmm, if the code can't find signup_format_date, then the signup_date_5.x-2.inc code isn't loading for you. Can you verify that the file exists?

Hi!

I am not sutre if I had that file like you wrote - signup_date_5.x-2.inc, propably no.
I applied patch again (on new signup module)
but I got this error:

Fatal error: Cannot access empty property in /home/users/somvprahe.sk/web/www/sites/all/modules/signup/signup_date_5.x-2.inc on line 113

Igorik

hmm, it looks like the node object wasn't passed to the signup_format_date function. What were you doing when that error occurred?

I tried to submit signup.

Hi, the problem is that I have no signup_date_5.x-2.inc file in signup module. (directory)
Where can I find it?
I use module - Signup 5.x-2.4 2008-Feb-08 61.52 KB

thanks
Igorik

I looked into latest dev version of signup module - signup 5.x-2.x-dev , Last updated: June 7, 2008 - 12:08
but there is no such file too.

Igorik

The file is in the latest patch I uploaded (Comment #44)

Hi

It is possible that I am doing some mistake, but that patch you wrote I apply (a few times, for new clean downloaded module),
files in signup module are patched, but the file signup_date_5.x-2.inc is not created.

other patches I applied on various modules run correctly, so I have no idea what can be problem.

thanks
Igorik

(subscribe)

[Sorry, I was out of town for a few weeks and just now catching up on things...]

It's not obvious from the trail of comments here what needs work about #44. @duellj: Is anything actually broken that you know of, or is all of this just user-error from people trying to test your patch? ;) Can you please comment (and if appropriate, set this back to "needs review"). If something needs work, can you summarize what it is and state your intentions (if any) to fix it yourself? I don't want to spend scarce time reviewing something that's known to be broken, nor do I want to duplicate effort fixing it if you're going to fix it yourself. However, if everything is peachy in your eyes and you want me to give this another look in its current form, please let me know. Thanks!

@dww

I just tested patching HEAD of signup with the patch at #44, and everything went through fine. Not sure what the problem that igorik is having with signup_date_5.x-2.inc not being created; it looks like maybe he's not patching it against HEAD?
It'd be great if you could take a look at this in its current form. Let me know any bugs or hangups you find.

Thanks,
Jon

I used #44 and patched signup-5.x-2.x-dev and got:

Fatal error: Call to undefined function _signup_event_completed() in /var/www/sites/all/modules/signup/signup.module on line 640

This happened when submitting a signup-enabled node.

What backend (if any) are you using (either Event or CCK Date)? It looks like the include file isn't loading for you. Can you verify that signup_date_5.x-2.inc was created during the patching process?

@duellj: The backend is Date and CCK. I patched it again with #44 and sure enough the signup_date_5.x-2.inc was created. I probably had the patch in a folder other than the signup folder and now have a signup_date_5.x-2.inc somewhere.

Signing up at a node now causes a different error.

Fatal error: Cannot access empty property in /var/www/sites/all/modules/signup/signup_date_5.x-2.inc on line 113

When reloading or viewing the node again, the signup button says "Cancel signup" so the member is indeed signed up.

ok, found the bug you encountered. When you don't select the cck date field for signup to use (Located in the signup settings in the Content Type admin section for your signup node type), then signup_date fails because it doesn't know which cck field to operate on.

Attached is a new patch that suppresses the errors if the cck date field isn't define for that node type. This probably isn't the most desired final solution. What shoud be? I see a couple solutions when cck Date is enabled but a cck Date field hasn't been defined for that node type:

• Show a warning message for node types that are signups and have at least one CCK date field asking the admin to define a signup CCK date field
• Automatically select the first cck Date field for the node type, maybe show a message saying that is happening
• If a node type hasn't defined a cck Date field, then regress to signup_event_none (not sure if this is possible, and might be a separate issue)

Any other ideas?

re #62: Good question. I'm definitely not in favor of automatically selecting the "first" date field -- the admin should have to intervene. Seems like you need a hybrid solution that's something like the following:

- use hook_requirements() to ensure that all node types with a CCK date field which are signup-enabled have a date field selected, and if not, generate an error for the site status report about it.

- treat signup-enabled node types that don't have a date field selected as untimed events for the purposes of signup.module.

I haven't played with the new code yet, but perhaps the UI to signup-enable a node type could also generate a warning if there are 1 or more CCK date fields and you signup-enable without selecting the date field you want to use for the signup-related start time. Basically, it'd be an early warning before you even get to the site status report error from hook_requirements().

Does that all make sense?

#62 is also broken if you use Date 5.x-1.x and run cron. There's an SQL error in the confirmation email query:

user warning: 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 'FROM do_test_signup s INNER JOIN do_test_node n ON n.nid = s.nid LEFT JOIN do_te' at line 1 query: _signup_cron_send_reminders SELECT n.title, n.nid, s.reminder_email, s.forwarding_email, n.type, FROM do_test_signup s INNER JOIN do_test_node n ON n.nid = s.nid LEFT JOIN do_test_content_ ON do_test_content_.nid = n.nid WHERE (s.send_reminder = 1 AND ('2008-07-30 19:09:35' + INTERVAL s.reminder_days_before DAY ) > ) in ...

Tracing this through the code, it's another problem where you assume every content type has a date. A mostly empty $content_type_field gets passed into signup_reminder_sql(), which results in a NULL being stuffed into the 'fields' member of the array it returns, which results in the extra comma at the end of the FROM clause in the SQL, which is an SQL syntax error. This makes it impossible to have signups on untimed events, once you enable the date.module. This is functionality I've tried hard to keep working up till now, and I don't want to break it for this.

So, we need even more changes to this patch:

IFF a node type *has* a CCK date field, and signup.module doesn't know which one to use, that should generate a requirement error on the status report, etc.

However, if a node type doesn't have a CCK date field at all, we need to properly ignore that, and treat it basically like signup_event_none.inc would.

FYI, see also #289326: Add event backend API for knowing if a node/node type is untimed...

Here's the patch that generates a requirement error on the status report page if a content type is signup enabled but no date field is selected. It also fixes the problem with #64 with cron and Date.

The other difficulties that arise when a CCK date field and signup doesn't know which one to use should be addressed by #289326: Add event backend API for knowing if a node/node type is untimed, correct?

* I am having trouble getting this to work. I am using Drupal 5.9, CCK 5.x-1.7, and Date 5.x-2.0-rc4. I signup enabled the page content type, then added a start_date field, and then did a "Save content type". However, on the status report I continue to get this message:
"Signup: page empty
Your signup enabled content type (page) has one or more Date fields, but no Date field is set to be used for signup date integration. Please select the appropriate Event Date field on the Content Type edit form"
And when I set the start_date field in a page to a minute in the future, the signup does not auto-close when cron runs after that time. Maybe I have set something up wrong?

* Suggestion: On the admin/content/types/page form, the label "Event date field:" should be "Signup date field".
* It seems possible that someone might signup enable a node, and have a date field, but still not want to use that date field as the Signup date field. Ex: A class is doing group projects on historical events. Weird edge case, but possible, so the drop down should have a option.

Fixed the issue with cron (started using date_sql api function to account for iso and database differences). Also added in the appropriate functions patched in #289326: Add event backend API for knowing if a node/node type is untimed.

@starbow:

Integrated your two suggestions listed in #67. As far as the first issue, that's part of the reason why I wanted to automatically select the first CCK date field (see #62). The reason why it's a bit confusing is because it's not really intuitive in the workflow. Here's what currently needs happens when a signup enabled content type is created:

-User creates a new content type
-the content type is set to be signup enabled
-user saves content type
-user adds CCK date field
-user then needs to go back to the main content type edit form, select the appropriate CCK date field, and save it again

It seems like most people will miss the last step, which is why we put the requirements message in there. But it still seems like it's not enough. Any suggestions?

thinking-out-loud...

Can we form_alter the 'add field' workflow and inject a checkbox or something on date fields about "use this field as the signup start time"? Might be too much of a can of worms.

At the very least, we should be able to add a drupal_set_message() whenever a date field is saved on a node type where signups are allowed or enabled that don't have a signup date field, with a link to the page and text something like "[content_type_name] allows users to signup but you have not selected a signup date field. Choosing a signup date field is required for [link to help text]certain features[/link] to work. You can configure this on the [link-to-admin/content/types/[foo]] page."

Oh, and for the record (starbow and I discussed this in person, but I wanted to leave it here for all to see)...

- I think it's important that we don't assume the "first" date field is the signup date field. The admin should consciously decide what field will be used for autoclose and reminder emails, since that's sort of a big deal.

- We also need to support node types that have dates that don't want to use signup date-driven stuff.

- I think it's reasonable to have warnings to nag admins that have signup-enabled node types which don't have a signup date field selected. We should remind people in the common case they need to do something else, even if it means we're nagging the people in the edge case who know what they're doing and are choosing the weird thing. We could consider a checkbox to hide the warning, if we wanted to be kind to the edge case folks, but I don't think that's essential.

I agree that the date field should not be automatically selected, I was just trying to think of ways to make the signup process easier for admins (or at least more intuitive). I'll look more into either altering the "add date field" form or setting a message after a date field is created (maybe both?).

Right, and I appreciate everyone's eye for usability with this stuff. This module is already way more complicated that it probably should be, and it's only getting worse. ;)

p.s. @duellj ever use IRC? If so, what's your nick? I'm dww in that world, too, and although I haven't been in #drupal for a while, I'm there now if you want to discuss in real time...

I'm fairly new to IRC, but on the advice of add1sun I'm starting to spend more time there. My nick is duellj as well.

#68 and #69 - adding the signup selection to the CCK field edit form is fine -- that's how the fieldgroup module does its group selection element. And that seems like it would be the most intuitive method of doing this. The logic for that element can test if there's already another datefield selected on that content type and if so add some text like 'The xxx field is currently selected as the signup field for this content type, do you want to choose this field instead?'.

That would take care of a lot of the problems.

And that form could default to 'yes' for the first date field it encounters if no other date field is already selected, which makes it semi-automatic but still controllable.

[edit] It should only do this when the field is first created. After that we wouldn't want to override a conscious decision not to use a signup.

One more thought -- you could display informational messages on the date field edit form about the status of the signup. Displaying them there might be as useful or even more useful that using hook_requirements since you'd be right where you need to be to do anything about it:

'You have enabled this content type for signup but have not selected a date field',

'You have more than one date field in this content type and have chosen field xxx for signup.'

'Signup is not enabled for this content type. Click here to enable it.'

Cool, thanks KarenS -- those are all great ideas.

I happen to know duellj will be offline until Tuesday, so if anyone else is inspired to incorporate any of this into the patch before then, it won't be duplication of effort. ;) Just reply here and assign it to yourself so others know it's in progress. Thanks.

Has anyone tackled integrating signup messages into cck date edit forms? If not, I've got some time to throw this together.

I integrate it, it works fine.
date and time of event is properly shown in mail (confirmation mail about signup)

What doesn't work is sending notify mail x days before event start, no that mail is send.

Igorik
http://www.somvprahe.sk

@igorik

Do you mean you integrated what was discussed in #74,#75, and #76 by KarenS? If so, could you post a patch?

Here's a patch that adds support for selecting the signup event date field from within the cck edit form (as discussed in #74, #75, and #76).

[edit] It should only do this when the field is first created. After that we wouldn't want to override a conscious decision not to use a signup.

Anybody know how to check if a cck form is on Add mode vs. Edit mode? This patch currently doesn't support only selecting when the field is first created.

I also fixed the problem with the mail notification and autoclosing.

Whoops, forgot to include the new files in the patch. Don't use the #81 patch, use this one instead.

Anybody know how to check if a cck form is on Add mode vs. Edit mode?

Hmm... in the form itself we do it by checking isset(), but you're not using a regular field here, it's a variable, and there's no way to know whether the variable was deliberately unset or was never set. So maybe that's not possible.

Maybe it's reasonable to assume that if signup is turned on for a content type they want *some* date field selected. If so, this patch would work OK. And we're on the edit form here so they can turn it off if they want.

Oops, another thing -- there's no test in the form_alter that we're on a date field, we want to test that $field['type'] = 'date' or 'datestamp' and just exit if it's not one of them.

I didn't see a message about signup being turned off and how to turn it on, do we want that to show up on any date field that does not have signup? [Edit] The message is there, it wasn't in the place I was looking for it.

@KarenS: We want to be able to support having a CCK node type that has the following properties:
- Signup-enabled
- Contains 1 or more date fields
- None of those date fields are the signup start time (for auto-closing and reminder emails)

For the purpose of signup, this would be an untimed event, it just happens to have date field(s). An example use case for this (seemingly crazy) configuration would be something like this: you've got a site where students can claim topics for a given class project. Each topic is some historical event with a date field. You signup-enable "class topic" nodes, and set the signup limit to 1. But, the date field has nothing to do with when signups should be closed or reminders emails. If it automatically said "you've got a date field, that must be the signup start time", all the topics would be closed on the first cron run after they were created.

Make sense?

Fixed #84.

@dww: So as it is now, this could be done by explicitly unchecking the Signup box on each of the date fields added to the CCK node type. The only problem is that they would have to uncheck the Signup box for the date field every time they edit it, since it is checked by default if no date fields have been selected. So the conflict is having someone add a date field to a signup-enabled CCK node and accidentally not selecting it for use with Signup; or having someone add a date field and not wanting it to be used with Signup and accidentally not unchecking it.

I suppose we could add an option in the edit form for the Content Type that sets the nodes as "Untimed", which would then dismiss all of the "warning" messages and not select any date fields automatically.

Subscribing.

Very interested in this feature and a port to d6

I haven't seen any activity on this lately. Is the patch in #86 good to use, do I patch it against the 5.x-2.x-dev version or the 5.x-2.4 version?

Thanks,
Shane

I tried to apply the patch in #86 to both the dev and 2.4 versions and both returned the following:

patching file signup.module
Hunk #2 FAILED at 90.
Hunk #3 succeeded at 100 (offset -11 lines).
Hunk #5 succeeded at 313 (offset -11 lines).
Hunk #7 succeeded at 408 (offset -11 lines).
Hunk #9 succeeded at 741 (offset -11 lines).
Hunk #11 succeeded at 1108 (offset -11 lines).
Hunk #13 succeeded at 1396 (offset -10 lines).
1 out of 13 hunks FAILED -- saving rejects to file signup.module.rej
patching file signup_date_5.x-1.inc
patching file signup_date_5.x-2.inc
patching file signup_event_5.x-1.inc
patching file signup_event_5.x-2.inc
patching file signup_event_none.inc

Do I need to apply other patches first, or can I just go straight to patch #86?

Thanks,
Shane

patch #86 was for dev a few months ago. It looks like dev is in active development, but if there's enough need I can reroll this for the most current version of dev.

I would like to see it rerolled... What are we waiting on to actually get this added to the dev? Seems to me that we've been patching this patch for quite a while and this has been a request for a long time. Lets get it added to dev and then work against patching it for the changes. To me, it does what it should be doing...

- Shane

p.s. Great work on this module and patch for everyone that has helped!

@shane_jordan: "Lets get it added to dev and then work against patching it for the changes."

Because I'm about to release 5.x-2.5, I'm not sure I'm going to include this patch in that release yet, so I can't just commit this in an unfinished state. See #293005: Create a signup 5.x-2.5 release for more.

@duellj: If you wanted to re-roll, that'd be great, just be aware that there are a few more patches that are going to land before I can turn my attention here. So, if you want to avoid needing to reroll twice, you could just wait a few days. Either way -- I'm just letting you know. It's possible none of the remaining patches will conflict, but I can't be sure.

@all: There are still some unresolved UI issues from #83-#86. So, setting this to CNW for both the reroll and the final UI efforts before this can go in.

btw, the patch hunk that fails from #86 on current HEAD is a totally bogus code formatting hunk that has nothing to do with the rest of the patch. ;) But, here's a trivial re-roll that applies cleanly to HEAD now. I'm too tired to really dig into this issue right now, so no comment on any of the other unresolved issues in here.

Adds signup_site_has_dates() to both the new *date*.inc files. See #289326: Add event backend API for knowing if a node/node type is untimed. This prevents fatal errors on the settings page, etc.

p.s. Sorry, wrong issue in that previous comment -- signup_site_has_dates() is from #213239: hide event specific features when module not available...

Ok, I just had a chance to play with this some more. I also just re-read this entire issue. In general, the current UI to select the right date field is very confusing. :( There are some other problems with the patch, too. Here are my current thoughts:

A) It doesn't appear that date 5.x-2.* has date_sql() anymore. There's only date_sql_concat() and date_sql_coalesce(). I haven't taken time to figure out these various date API functions. However, if you setup a test site with date 5.x-2.* and this patch, when you run cron, you get a fatal error about date_sql() being undefined. I tested with 5.x-1.* and both auto-close and reminder emails are working fine. I haven't yet tested this patch yet with event (either 5.x-1.* or 5.x-2.*).

B) The "Use as Signup date field" checkbox on the date add/edit field to seems like the wrong kind of form element, since it sort of implies there could be more than one.

C) The checkbox is also very hard to find, and it's not at all clear to me why it's considered a "widget setting". This has nothing to do with the field widget and its behavior. If anything, it's more of a data setting. However, it really feels like it belongs in its own fieldset, since it's not really a widget nor data setting.

D) I think it's confusing that "Signup options" lives at admin/content/types/gig but there's no way to specify what date field you want to use for signups in the same place. I understand that the usual workflow would be what duellj described in #68:

-User creates a new content type
-the content type is set to be signup enabled
-user saves content type
-user adds CCK date field
-user then needs to go back to the main content type edit form, select the appropriate CCK date field, and save it again

It seems like most people will miss the last step

However, this is a case where I think it'd be best to let admins specify this information in multiple places (just like you can control fieldgroups at both admin/content/types/[type]/fields ("Manage fields"), and at admin/content/types/[type]/fields/[field_name] (the "configure" link when editing a field or the 2nd page after you add a new field). I think admins should be able to define this both right next to the existing signup settings in the workflow fieldset at admin/content/types/[type] and when adding/editing any date field.

As a solution to (B), (C), and (D), here's my current proposal:

1. Have a "Signup date field" form element that's a select box with something like "Not specified" as the default option, the name(s) of any date field(s) of the given node type, and "None" as the other options.
2. Put this form element directly under the "Signup options:" radio buttons in the workflow fieldset at admin/content/types/[type]
3. Also put this form element in a new "Signup settings" fieldset on all date field add or edit forms at admin/content/types/[type]/fields/[field_name]
4. If a node type has signups allowed or enabled, and it has date fields, generate a drupal_set_message() warning on admin/content/types/[type]/* if the "Signup date field" setting is "Not specified".
5. In the edge case previously discussed where you have date fields and you don't want them used for signup, you can just select the "None" option for "Signup date field" and the warnings will go away.
6. MAYBE, we'd also let folks specify the field to use for signup functionality at admin/content/types/[type]/fields, probably as a new column in that table that would be a set of radio buttons next to each date field. But, it's not clear how to chose "none" there (without adding another row to the table, which is weird), and it might be a little hard to get all this working. So, I'd be happy to leave this part out.

I can't think of any cases where this would go wrong. There's nothing automatically happening without the admin's intervention. They get warnings if the configure something they might not have intended if they miss the setting. The way to change this setting is obvious from the various places you'd want to select it. If they know they're doing the weird case and they mean it, they can easily shut up the warnings.

What do y'all think? It's too late and I'm too tired right now to code this up, but I wanted to write it down to get feedback on it.

I'm not going to have a ton of time this week to work on this, anyway, so if someone else wanted to take a stab at coding this together, that'd be lovely.

Given how much work is left to do in here, I think I'm going to ship 5.x-2.5 without this patch, and make this my #1 top priority for 5.x-2.6 (which will probably be out in 1-2 weeks, not months).

In response to (A) in #97, I have fixed this on my system but i'm not 100% how to make a new patch for it. Here is what I did though:

Added this handler to the functions that were using sql_date():
$handler = new date_sql_handler();

changed sql_date to:
$handler->sql_extract();

The sql_extract only takes two parameters, so I removed the DATE_ISO from the end of each sql_date function. I'm going to attempt matching a patch and see if I can get it posted.

I tried to use the patch in #95 and my system just sat there, it didn't do anything. I ended up canceling it.

- Shane

I really don't know how to make the patch like the one that has been being posted. I did get the patch to apply from #95, I just didn't have my command right. I am posted my attempt at a patch, please tell me what I am doing wrong. I untarred dev, made a copy of it called signupNew. Then made my changed to signupNew and ran the following command: diff -urp signup signupNew > 86462_signup_date_2.x.99.patch

- Shane

@dww #97:

Everything sounds great. It makes sense to both provide the signup options for the date fields in a different fieldset for clarity, and in multiple places for ease of configuration. I'm not sure #6 is needed right now; it would be cool to set a global signup-enabled date field, but I think it's more important to get the basic configuration working.

I'd be happy to take on getting this working, if no one else has gotten started.

btw, patch #95 doesn't include the signup_date*.inc files, so we missed the changes you made to those files.

@shane_jordon:

To apply a patch, you need to run the following command:

patch < patch_file.patch

you probably left out the "<", which causes patch to hang.

Thanks for the patch, but it seems like you left out the bulk of the changes you made by not including the signup_date*.inc files. Try running diff with the "-N" switch to include the new files.

Here is the new patch with the -N switch, let me know if it doesn't work.

If the global signup enabled ate field is an option, I think it would need to be overridden based on a content_type field setting. From some of the posts i've read, it looks as though this is the approach that will be taken.

- Shane

Whoops, sorry about #95 leaving off the new files. Here it is again. Otherwise, I haven't touched anything further based on my proposal in #97 or the suggestion of shane_jordan in #98 about replacing date_sql() with that handler stuff.

@duellj -- if you're around this afternoon, i'll be in IRC (#drupal-signup, #drupal-dev and #drupal). it'd be nice to coordinate in real time so we don't waste each other's time on this.

If the signup date select box is generated with a list of existing date fields, will it also be have the current one being generated in that select list? I also think this could be a required field, since you would either have to choose a date field or choose none. This would prevent any errors of not making a choice. All checks would still have to exist, because if you have existing content types with dates and signup enabled, then these would break until the admin when in and assigned a date/choose none. I'm still really learning module development, but if there is anything I can help out with, just let me know. I can easily test any modifications to see how they work. I'm using date 5.x-2.

- Shane

@shane_jordan: Good questions. Yes, the selector that appears on the field form itself would have to include the in-progress date field being added to be useful. I suppose it could be a required field, but I'm not sure that's necessary given the other checks. But yeah, we should definitely consider this. Thanks for the offer to help. At this point, probably just testing patches once they're marked "needs review" would be the most helpful thing. Thanks!

Patch applies with some line corrections (head is a target moving very fast in the last days ;-)) and has some issues:
Fatal error: Call to undefined function dvm()...
After removing it its working without errors (except for "date_sql() not found").

Some notes:
- on adding a new date field "Use as Signup date field" is chceked initially though it was checked on an other field recently. It should be tested before if an other field is already set as the signup date field and then un-/checked depending on the previous test. Otherwise one could miss this and accidently leave this checked. This one will be handled better after including the proposals from #97.

- The first time I added two date fields to a new content type the setting signup_form_field_ wasn't set. I have to go to the date field I want to be the one and save it again (without doing any modifications). Maybe signup_date_content_admin_form_submit() doesn't do its checks correctly?

- it would be great if someone could insert the handler from #98. otherwise its not really testable.

-========-
Re #97
1-5: +1
6: I think we do not need a selection on this page but an indicator which field is currently used for signups would be great (an asterix or something else).

 Stefan

The patch in #101 contains the handler in #98. I wasn't sure how to make a patch at first, so I just posted the code. But the patch in #101 should be valid and it is fairly recent.

- Shane

Ok, after adding the changes from #101 to #103 I could test the functionality.
Results: autoclose isn't working because _signup_cron_autoclose() couldn't find any signup before the current date/time. This is because $handler->sql_extract() outputs a date formatted like '2008-10-17T14:00:00' and this is never smaller than '2008-10-17 14:00:01' (the format used by the date comparison in line 38 of signup_date_5.x-2.x.inc).
I replaced the date format with 'Y-m-d\TH:i:s' so autoclose and reminders are working.

Updated patch from #103 to current head and fixed the date format issue.

This leaves #97 1-5 still open.

 Stefan

@stBorchert: Thanks for taking a look at this and folding in solutions for some of the known issues. Sorry you spent time working with the current UI and finding problems with it -- that was the point of my comment #97. Also, apologies for the stray dvm() left in there from debugging (it's a devel.module function). ;) No time to review your latest patch right now, but I'll take a look ASAP.

In fact, I'm not going to have much time to work on this until next week, so if anyone wants to start with patch #108 and take a stab at implementing points 1-5 of my proposal in comment #97, that'd be lovely. I agree we should either forget about point 6 entirely, or at least postpone it for a followup issue. If no one implements this by early next week, I'll crank out a patch.

Thanks all.

13 of the functions in signup_date_* were identical in the 5.x-1.* and 5.x-2.* versions. That's nasty code duplication. :( So, I moved those out into signup_date.inc (151 lines and counting), and we now include that in addition to the appropriate version-specific signup_date_5.x-*.inc file.

Otherwise, I still haven't fixed 1-5 from comment #97. I was busy getting 5.x-2.5 out and working on some other stuff (project* work for d.o). Anyway, I'll hopefully have a chance to work on this after I've slept. If anyone else wants to run with it in the meantime, please feel free.

Ok, here it is.
This handles 1-5 from #97 (maybe the select widget could be more generic...).

Does the patch in #111 already include the changes from the patch in #110? I just noticed that there was only about a 1 hour turn around time between the two patches, so I wanted to verify. Once I receive verification, I will begin testing.

- Shane

Jup, I included Dereks last changes. I was testing my patch as the new patch arrived so I had to modify it a little :-)

 Stefan

Super cool... you even put in the JS to hide the setting if signups are disabled. ;) Yay. A brief look through the patch and light testing yields only relatively minor problems:

A) _signup_get_date_field_options() belongs in singup_date.inc

B) I wouldn't mind if all the extra code added to signup_alter_node_type_form() also lived in a helper function in signup_date.inc that was conditionally invoked, instead of lugging all that code around in signup.module itself, even for sites not using CCK date.

C) Not clear why this class name uses both - and _ : "signup-date_field-setting" Seems like "signup-date-field-setting" would be better.

D) You add this div twice: <div class="signup-node-default-state-radios">

E) [Broken from a previous patch, didn't notice until now] By putting the code to generate that drupal_set_message() warning users about a misconfiguration directly into the form builder function via form_alter(), it means we generate that message during various stages of form building, and it gets displayed once the form is submitted, even if by submitting the form you fix the problem. Try it yourself -- start with a clean slate, add a date field (but don't select a signup field), and signup-enable the node type. If you then select a signup field and press submit, you see the warning one more time, but if you reload the page, it goes away. So, we need a better way to generate that message. Not entirely sure what that should be without spending a little more time investigating.

F) Is this comment true at the top of signup_date_5x-*.inc: "// TODO: Add support for datestamp fields"? Don't they work already? Isn't that a TODO from event 5.x-2.*?

G) signup_date_form_alter() is still adding the new form element into the 'widget' array of the CCK field form. Now it's in a nested fieldset, but that's not what I had in mind. We need a whole new fieldset, outside of the existing ones provided by CCK. Just remove the ['widget'] parts of those FAPI arrays and we should be set. Might have to tweak the weight to see where we like it to appear.

H) Code style: + } else {

I) This help text could use some help: "Select the date field of this content type to use with signup. Select "%none" to do not use a date field." ;)

J) Not clear why we need this at all:

-          $node->signup_total = db_result(db_query("SELECT COUNT(*) FROM {signup_log} WHERE nid = %d", $node->nid));
+          if (!($node->signup_total = db_result(db_query("SELECT COUNT(*) FROM {signup_log} WHERE nid = %d", $node->nid)))) {
+            $node->signup_total = 0;
+          }

K) Code style: + '#value' => $signup_event->signup_total?$signup_event->signup_total:0,

L) Code style: if(...) (should be if (...)).

That's about all I could find. The basic functionality to select this signup date field seems to be working great. So, other than fixing this list (should be very easy) what we really need are people to test this with no event modules, event 5.x-1.*, event 5.x-2.*, date 5.x-1.*, date 5.x-2.*. Thanks! In particular, we need to test reminder emails and auto-closing events that already started.

Thanks, again!

It looks as though everything is working fine (using date 5.x-2.*). The one thing that I see as an issue is with timezones. I set my date field up to use the date timezone (America/New York). When I look at the signup administration page, it is not using that timezone. This causes all of the dates on this page to display the time 4 hours more than what is actually is. Example, I created the node with date of 10/23/2008 10:00am and in the signup administration page, it displays as 10/23/2008 2:00pm. This causes issues with closing the node/sending reminders. That node really won't get closed until 1:00pm according to signup, but my date/time has already passed.

When creating a date field, I have five options for timezones:
- Site
- Date
- User
- UTC
- No conversion

I figure that this issue has been there, just hasn't been noticed. There is an offset stored in the cck table for the field.

- Shane

you even put in the JS to hide the setting if signups are disabled

Sure, I love those tiny blinky functionality :-)

re A) updated

re C) hm, seems to be a typo and then copy and paste error :-). fixed

re D) fixed

re E) yeah, noticed this, too. Left this open for someone else ;-)

re F) Did a quick check and it seem to work.

re G) ok, moved this out of "Widget". Unfortunately all fieldset (and the submit button) seem to have "weight = 0" so this fieldset is inserted between "Widget settings" and "Data settings". I would like to see it at the bottom of the page right above the submit button.

re H) fixed (also in signup_date_5.x-*.inc)

re I) Uh. Sounds strange. What about "Select the date field of this content type to use with signup. Select "%none" to not use a date field with signup at all."

re J) Don't know either. It came in in one of the previous patches...

re K) wasn't me :-). fixed

re L) fixed

Happy testing!

 Stefan

It actually seems as though the date timezone gets messed up in views too. I personally don't like timezones.... it seems like they are a constant battle and no one wins.

For E, maybe have the default for the select list be 'none' unless a date field is selected. This would mean you would always have a selection and by default it is turned off.

- Shane

Re: #116 -- thanks! Here's a new patch with some replies:

A) Thanks. ;)

B) Fixed. Also cleaned up how you were specifying #submit for those forms. The node-type form automatically saves workflow values for you, so we don't need a #submit for that form at all. We only need it for when we edit the date field itself, and then #submit works differently than how you had it. By using 'signup_date_field_[type]' we get the magic from the node-type form for free, and I think that's a more self-documenting name for the variables, anyway. While I was at it, I made it so we always use 0 to mean 'Not specified' instead of the mix of 0 and FALSE from previous patches. I also changed the code for how the labels are generated so that the special values look different (<None> and <Not specified>), and so that <None> is always last.

C, D) Thanks.

E) I haven't looked into it yet, still broken for now, but I wanted to upload the fruits of my labor this morning.

F) Thanks.

G) Then we just need to specify the weight for the $form['submit'] element ourselves. Fixed in attached patch.

H) Thanks.

I) I'll take another look, that help text certainly sounds better than before, but we might be able to make it even more clear.

J) The code came from #44 with no motivation. I've ripped it back out, since it is unrelated to CCK support. If that code is needed, it belongs in another issue, potentially to be backported, etc. I've attached it here as "86462_signup_total_fork.patch" in case someone wants to run with it in another issue.

K) Thanks -- I moved your fix into the fork patch. ;)

L) Thanks.

So, that leaves us with:

E) By putting the code to generate that drupal_set_message() warning users about a misconfiguration directly into the form builder function via form_alter(), it means we generate that message during various stages of form building, and it gets displayed once the form is submitted, even if by submitting the form you fix the problem...

I) This help text could use some help ...

and the new thing shane_jordan raises in #115 and #117 about timezones. Let's call that "M". ;) I'm tempted to move that to a followup issue, since this one is already very long and confusing, but perhaps KarenS can shed some light on the subject...

I'm going to try to spend some time on this, but this thread is so long and confusing I can't tell where things stand. I picked up the patches from #118 (I assume I need both? Why are there two?)

Then, once have the right code, what are the outstanding questions to be answered or problems to be fixed?

Sorry to ask for help instead of figuring it out, but I tried following the thread down and have gone in circles several times trying to tell myself 'ignore this, it's already taken care of', 'remember this, it's still a problem', etc etc :)

Hey KarenS, I think #115 with the timezones is the one that dww was referring to for you to review. The issue is that you can set various timezones on a cck date field, but it doesn't always look like that timezone is adhered to through the various modules. For instance, I set the timezone to Date's timezone, it works for nodes and the calendar module displaying the proper time, but does not display the proper time in a view or in the signup administration page. Maybe we I am just missing a setting or something and possibly you could shed some light on it.

Thanks,
Shane

@KarenS: Thanks, for coming back here. Sorry the issue is long and confusing. The only thing we need your expert advice on is why timezones are apparently screwy (see comments #115 and #117). You just need signup HEAD (or DRUPAL-5--2-5, nothing has changed since then), and http://drupal.org/files/issues/86462_signup_cck_date.118.patch from #118. The other patch doesn't belong here at all, I just posted it so that the (unrelated) code wasn't completely lost. While you're at it, your thoughts on the (hopefully final) UI we've come up with for selecting the field to use for signup date functionality would be welcome, but the main thing we need from you is timezone fu. ;) Thanks!!!

I agree that the timezone issue should be moved to a separate issue. The patch works fine when you set the timezone on the date field to 'No conversion'. Then, it closes at the right time, sends reminder e-mails fine, displays the proper date/time in signup administration, and works fine with views. All of my testing is with date 5.x-2.*.

- Shane

This fixes (E), the trouble with doing the checking on the signup date field configuration for node types using the form builder.

I spent a while trying to get this working with help from chx and eaton in IRC by various forms of FAPI treachery, using a #process handler on an empty form element, etc, etc. It was a huge mess, and I still couldn't get it working in all cases. Plus, given where you get redirected after various form submissions, in practice, I found it was easy to enter a bad configuration and not get a warning about it, since you usually land back at /admin/content/types or /admin/content/types/[type]/fields, neither of which have the form that'd be displaying the warnings.

So, I've come up with a (if I may say so) rather slick solution to the whole mess that doesn't involve any FAPI trickery at all: just use hook_help(). Since hook_help() is only ever invoked once on a page load, as opposed to the form builder which is invoked at multiple stages of form processing, once you submit a given configuration, the next page reload can just do all the checks from hook_help() and tell you if your current configuration is valid. Furthermore, it was very easy to make it so that at /admin/content/types we run our checks on *all* content types and warn about *any* problems right from there. And, from /admin/content/types/[type]/* we warn you about that specific type.

I think the error reporting is better and the code is much cleaner with this approach. I really see no downside to doing it this way.

I also fixed (I) the help text for the signup date field. I added a private helper method to generate the form element so that it was consistently shared between the two places the form element is used, and then fixed the description in that one place.

In various testing, I also cleaned up some of the 'none' vs. 0 code for the date field. PHP continues to amaze me with its brain-dead handling of the == operator. :( So, more testing would be welcome. Also, I still haven't tested the actual reminder or autoclose functionality with CCK date fields, nor have I tested that we didn't break event support in here.

I'm setting this to CNR. I agree we should just move timezones to a new issue... Any final complaints, reviews or testing results before I commit this puppy? Thanks!

Just adds 'warning' to those drupal_set_message() calls for better visual cues that something is potentially wrong.

Note to reviewers/testers: What we most need right now is for someone to try out this patch and ensure that reminder emails and the auto-close functionality still works with the following 4 configurations:

1. event 5.x-1.*
2. event 5.x-2.*
3. CCK date 5.x-1.*
4. CCK date 5.x-2.*

Note that if you have event and CCK date fields on the same test site, only the event nodes will have time-based signup functionality, and none of the UI for selecting CCK date fields for signup features will be present. (Actually, it'd be nice for someone to confirm that, too). ;)

If anyone tests any of these cases thoroughly, please report your findings here so that others don't duplicate the same testing. Once all 4 configurations are clear, I'll commit. And yes, let's just assume that you always set your CCK date fields to have no special timezone handling as per comment #122, and we can sort that out in a followup issue.

Thanks, all!

Actually we can simplify this even more. This is a new feature and Date 5.1 is no longer listed as a recommended version, the recommended version is 5.2, so as far as I'm concerned we can drop support for Date 5.1. That should mean we can combine date.inc and the 5.2 file into a single file and delete the 5.1 file and any reference to 5.1, and there is no need to test 5.1.

@KarenS: hrm, interesting. My only concern is that all the code already exists, and it's going to take more work now to undo the 5.x-1.* support in this patch than to just ensure it works. ;)

Also, is the date API going to change between date 5.x-2.* and 6.x-2.*?

Tested with event 5.x-1.* and found that signup_content_type_fields() was missing from signup_event_5.x-1.inc. Once I fixed that, reminder emails and auto-close are working fine. I'll try event 5.x-2.* next...

A few comments (using Date 5.2):

1. Not being able to have event even enabled is going to make it very difficult to upgrade a site from Event to CCK + Date. Is there a reason I missed why this can't be done?
2. The Date Format selector at admin/settings/signup should show what the format is set to. Something like "Small (01-01-2008)" should work. Perhaps there is something in Date API to do it automatically?
3. What's the functional difference between "None" and "Not Specified" in the date select field on the content type screen?

Otherwise, installation and testing with a single node and one signup seems to work fine.

Thanks for the testing and feedback. To answer the points from comment #129:

1. It's just how the event backend stuff was setup in the first place -- it assumed all or nothing. And, we assumed that most sites either run one or the other, not both. If you run both, we assume you want signups on the event nodes, and are using CCK date fields for other things. Keep in mind, if you have both while you're upgrading a site, it just means that the CCK + Date nodes won't have time-based signup functionality until you turn off event.module. I don't think that's the end of the world. However, if someone was really motivated, now that the whole API for the event backends is node-type specific, we could reorganize all this code to not just rely on functions being defined in only one of the backends, but instead make them basically little hooks that can be implemented by multiple modules simultaneously, and you invoke the right hook for each node type, etc. Definitely out of scope for this issue. ;)

2. Completely unrelated to date API at all. That setting is from #132142: Add setting to use site-wide date formatting strings instead of our own weird setting -- feel free to reopen that or just add a new feature request issue that references it.

3. Functionally, "Not specified" means the admin hasn't touched the setting and hasn't thought about it yet. So if they do something potentially unexpected (e.g. signup enable a node type and add a date field, but don't select that date field for signup functionality) we generate warnings. "None" means you thought about it and decided you definitely don't want signup date functionality on this node type, even though you enabled it for signups. See comments #70 and #85 for more. We could potentially explain that "None" will disable any warnings about the signup date field configuration in the help text, I guess.

I tested on event using the end of the DRUPAL-5--2 branch and found two minor problems. However, when I removed this patch, the problems were still there, so I just submitted those as a new bug report: #327417: Timezone problems with autoclose in event 5.x-2.*. Since it's not a regression, it's not worth holding up this patch for that bug -- so I'm still leaving this as needs review.

Are we going to move the timezone issue to another issue instead of continueing it here (which I recommend). The system is working properly if you choose the no timezone option. I think the timezone issue may be something larger than the signup. If this is the case, then I feel as though the patch is working for date 5.x-2. I plan on running with signup 5.x-2.5 and this patch this weekend to migrate my events over to date/calendar. I have approximately 200 events in the system from now till december which will need converted. If the plan is to have a 5.x-2.6 release very soon, then I may be able to hold off, any timeframe? I know that it was once stated to release 5.x-2.5 and then do a release soon after that would include this patch.

- Shane

Works for me in Date 5.2 and I think I fixed the timezone problem. I can't seem to get out a patch that has new files in it, so I've attached the working date include file.

I'm not crazy about the fact that it won't work with both Date and Event enabled, but it's better to get something committed than nothing, so maybe a work-around can be found for that later. I would add a message somewhere about that to explain why nothing is working since it's not intuitive that it wouldn't and it's confusing.

I'm not supporting Date 5.1 any more and don't even have a test bed for it any more, and if anyone finds that the 5.1 version is broken I'm probably not going to do anything with it, so I still suggest leaving that out. Sorry you put all that work into it, but I couldn't drop 5.1 support officially until I had 5.2 working reliably, which just happened recently.

And the 6.2 API is the same as the 5.2 API, so the same code should work in D6.

I attached the wrong file, trying again.

This is the patch from #128 and the signup_date_5.x-2.inc from #135 rolled into one.

@KarenS: thanks for fixing the timezone stuff for date 5.x-2.*, yay!

Given that the date 5.x-1.* code already works in here and that some sites are still using that version, I don't see any great reason to undo that work at this point. Since you're not maintaining date 5.x-1.* anymore, the chances that something will change that would require more work in here are pretty low, right? ;)

Rerolled #136 after #327726: Reorganize signup directory layout. This needs testing again, to make sure I didn't break anything... However, I feel much better about this new directory layout for this patch, #321531: Provide signup form as a panel, and the D6 port going forward.

Tricky... ;) I was just retesting this with CCK, and started seeing duplicate messages about autoclosing the event, and was getting double reminder emails. I tracked it down to the fact that {content_type_foo} had multiple records for my date field since there were now multiple revisions of my node. So, the autoclose and reminder SQL was generating a separate hit for each revision of the node, which is what caused the duplicates.

The reminder email was already JOINing on {node} to get the title, so that was a trivial fix so that when we LEFT JOIN on {content_type_foo} we JOIN on table.vid = n.vid instead of using .nid.

The autoclose was slightly more work, since we need to add the JOIN on {node} in the first place. Otherwise, it's the same fix.

See attached patch -- works fine on date 5.x-1. I've gotta upgrade my test site to date 5.x-2 and test it there to be sure, but I think this should do it.

- Fixed a few other places where things were not expecting 'none' as a possible return value from signup_date_field()
- Fixed a similar nid vs. vid bug for signup_admin_sql() in includes/date.inc

Pretty sure this is now RTBC. Any other testing would be welcome. Thanks!

(edited typo: s/we're/were not/)

@dww, yup, zero chance I'll be making any changes to the 5.1 version :) I didn't realize anyone was already using it, so that's fine.

If I'd noticed the join on nid I would have mentioned that you need to join the vid, but I didn't notice and you already figured that out. All the CCK stuff joins to the vid.

I don't have any time to test this now, but hopefully someone else will.

I will work on testing this tomorrow with 5.x-2. I will post my results back.

- Shane

Committed to HEAD, yay! ;) http://drupal.org/cvs?commit=149995

If people find other problems or have feature requests related to this, please submit them as new issues.

For example, I just created #328181: Allow both event and CCK date to work simultaneously about KarenS's concern with date + event both enabled.

Thanks again to everyone who participated in this!

FYI: If anyone comes looking in here in the future, these patches for the actual CCK date support were totally wrong, and required major reworking over at #336255: CCK datestamp fields totally broken and fields without timezone handling mostly broken. ;) These only supported date + datetime fields, not datestamps at all, and only for certain timezone configurations.

A lot of the other changes in here were also reworked in followup patches:
#328181: Allow both event and CCK date to work simultaneously
#329668: Improve warning messages when CCK date fields are misconfigured
#330121: Shouldn't warn about CCK date configuration on event nodes
#330821: Test/fix CCK date 6.x-2.* integration