Add support for event 5.x-2.*

this is a great start, thanks. as i mentioned in IRC, my preference would be to abstract some of this code into a thin layer, and then have support for different backends (event 5.x-1.*, even 5.x-2.*, date.module, etc). i'd rather not force signup 5.x-2.* users to upgrade to event 5.x-2.* if they don't want to, but maybe that's a silly concern. certainly date.module integration is a concern, though, and i'd like the code to make this easier, if possible.

anyway, testing from people who use signup and event would be most appreciated.

Mostly this looks great, thanks! After a quick read of the patches, I have a few concerns:

A) It'd be possible (though, I suppose unlikely) for both event.module and date.module to be enabled on the same site, no? Not sure if it's worth coding for this possibility, but I wanted to raise it. Might not be *too* hard to get this right, would it?

B) This looks wrong:

-    $starttime = $node->event_start ? _event_date(variable_get('signup_date_string', 'D, M jS, g:i A'), $node->event_start, $offset) : t('[Untimed]');
+    $starttime = signup_format_date($node->event_start);

Signup works for nodes that don't have any date information at all. Seems like this change assumes there's always date info in some form, which is wrong.

C) Similar problem here:

+  if (SIGNUP_EVENT == 'EVENT_5.1' || SIGNUP_EVENT == 'EVENT_5.2') {
     $tokens[] = '%time';
   }

It's not just a question of if event.module is enabled, it's a question of if that *particular* node has event-data.

D) I don't like moving all of signup_cron() into the .inc files. That'll make a bunch of duplicated code. I'd rather have functions in the .inc files for the module-specific logic, but allow signup_cron() itself to live in signup.module and have the bulk of the code be shared. In fact, as a core-provided hook, doesn't signup_cron() *have* to be in signup.module to work at all?

E) This is confusing:

function signup_admin_form_header() {
  $has_event = module_exists('event');

I thought this .inc file is only included if we know event module is enabled, so why check again? Also, wouldn't it be nicer to have the bulk of the headers defined in the .module file, and only optionally append the other header(s) we want depending on which event module is enabled, instead of duplicating the bulk of the headers in each .inc file?

F) Similar problems here:

function signup_list_user_signups_sql() {
  $has_event = module_exists('event');

and here:

function _signup_event_completed($node) {
  if (module_exists('event')) {

G) signup_format_date() has a combination of the problems from B and E above. ;)

All that said, this is a fantastic start. I'll review more closely and test once these issues have been fixed.

Thanks!
-Derek

p.s. you might find the patch and my replies at http://drupal.org/node/146248 helpful.

0) Patch doesn't apply cleanly since I committed a fix for PgSQL compatibility when event.module is enabled. :( Sorry about that.

1) I'm really not a fan of moving hook_cron() into the .inc files. Certainly the entire block of code starting here:

  // Loop through the users, composing their customized message
  // and sending the email.

could be shared. As could all the stuff to close signups that are past their configured starting time auto-close setting (which should be rewritten to just use signup_close_signup(). It seems you didn't read http://drupal.org/node/146248#comment-255564 as I asked. ;) Really, there are only a small handful of queries in hook_cron() that are event/date specific, so IMHO, those belong in the .inc files and everything else belongs in signup.module.

2) All the php code at the top of the module, completely outside of any hooks or functions, is really bad. I've run into various problems with this in the past. This stuff either belongs in hook_init() or hook_menu(). Probably hook_menu() is best.

3) You added signup_admin_form_extra(), which is nice, but you left the entire header definition signup_admin_form_header() in the .inc files. I'd prefer to keep the bulk of the header shared in signup.module, and signup_admin_form_header() just returns the .inc-specific column we care about. I'm fine with saying that if signup_admin_form_header() exists and returns data, it should always go to the front of the header array.

4) This is still wrong:

-    $offset = $node->event_start ? event_get_offset($node->timezone, $node->event_start) : '';
-    $starttime = $node->event_start ? _event_date(variable_get('signup_date_string', 'D, M jS, g:i A'), $node->event_start, $offset) : t('[Untimed]');
+    $starttime = signup_format_date($node->event_start);

This assumes that event.module is enabled and that $node has an event_start attribute.

5) Same here:

+      $trans['%time'] = $event->event_start ? signup_format_date($event->event_start) : t('[Untimed]');

6) Re comment #7, point "C" -- at least given your replies here and the new patch, you still seem to confuse "event.module enabled" with "this particular signup-enabled node has event data". Yes, I understand it used to be wrapped inside a if (module_exists('event')), but that's not the only test that used to be happening. Please acknowledge that you understand this point, and please write code that doesn't assume that just because event.module is enabled that all signup-enabled nodes have event data. ;)

7) Since we're going to be refactoring things like this, I think it'd be better to move towards a query-builder model for some of these things, instead of having each .inc file construct nearly identical queries. I just smell a maintainability hassle in the future when we try to modify any of the queries. :( For example, instead of a slight variation of this in each .inc:

function signup_admin_form_sql() {
  $type = $_SESSION['signup_status_filter'];
  if ($type == 'open') {
    $where = ' WHERE s.status = 1';
  }
  else if ($type == 'closed') {
    $where = ' WHERE s.status = 0';
  }
  else {
    $where = '';
  }

  // Construct SQL to get all the info for the requested signup nodes.
  $sql = "SELECT n.title, n.nid, s.status AS signup_status, e.event_start, e.timezone,
          COUNT(s_l.nid) AS signup_total,
          s.close_signup_limit AS signup_close_signup_limit
          FROM {signup} s INNER JOIN {node} n ON n.nid = s.nid
          LEFT JOIN {signup_log} s_l ON s.nid = s_l.nid LEFT JOIN {event} e ON e.nid = n.nid";
  $sql .= $where;
  $sql .= " GROUP BY n.nid, n.title, s.close_signup_limit, s.status, e.event_start, e.timezone";

  return $sql;
}

I think this would be better in signup.module:

function signup_admin_form_sql() {
  $fields = array();
  $group_by = array();
  $joins = array();
  $where = array();

  $type = $_SESSION['signup_status_filter'];
  if ($type == 'open') {
    $where[] = 's.status = 1';
  }
  else if ($type == 'closed') {
    $where[] = 's.status = 0';
  }

  $fields[] = 'n.title';
  $fields[] = 'n.nid';
  $fields[] = 's.status AS signup_status';
  $fields[] = 'COUNT(s_l.nid) AS signup_total';
  $fields[] = 's.close_signup_limit AS signup_close_signup_limit';
  if ($admin_sql_fields = signup_admin_sql_fields()) {
    $fields = array_merge($fields, $admin_sql_fields);
  }

  $group_by[] = 'n.title';
  $group_by[] = 'n.nid';
  $group_by[] = 'signup_status';
  $group_by[] = 'signup_close_signup_limit';
  if ($admin_sql_ group_by = signup_admin_sql_ group_by()) {
    $group_by = array_merge($group_by, $admin_sql_ group_by);
  }

  $joins[] = 'INNER JOIN {node} n ON n.nid = s.nid';
  $joins[] = 'LEFT JOIN {signup_log} s_l ON s.nid = s_l.nid';
  if ($admin_sql_joins = signup_admin_sql_joins()) {
    $joins = array_merge($joins, $admin_sql_joins);
  }

  if ($admin_sql_where = signup_admin_sql_where()) {
    $where = array_merge($where, $admin_sql_where);
  }

  $sql = 'SELECT '. explode(', ', $fields) . ' FROM {signup} s ';
  $sql .= explode(' ', $joins) .' ';
  if (!empty($where)) {
    $sql .= ' WHERE '. explode(' AND ', $where) .' ';
  }
  $sql .= ' GROUP BY '. explode(', ', $group_by);
  return $sql;
}

And then, the .inc files just have to have something like this:

function signup_admin_sql_fields() {
  $fields = array();
  $fields[] = 'e.event_start';
  $fields[] = 'e.timezone';
  return $fields;
}

function signup_admin_sql_group_by() {
  // All of the fields should go directly into GROUP BY, so just reuse the function.
  return signup_admin_sql_fields();
}

function signup_admin_sql_joins() {
  return array('LEFT JOIN {event} e ON e.nid = n.nid');
}

A similar query building approach for signup_cron() would solve most of the problems of trying to keep that code in singup.module, too. Yes, it's obviously a bigger shared function, but I think it's more clear, and will be easier to maintain in the long run than forking this code and having 3 slightly different copies in the .inc files... And, once you include all the duplicated queries in all the .inc files, I'm not sure this longer query builder is actually more total code for the module overall.

And, you can't complain it's more work, since I just wrote 90% of the code for you. ;)

Thanks, this is going to be a great improvement once it's in.
-Derek

I'll take a look as soon as I get a chance. Thanks!

This is looking really great...

Sadly, the patch from #11 doesn't apply cleanly anymore. Probably broken by a bug fix I committed: http://drupal.org/node/160862

Re: signup_event_5.1.inc from #12:

A) some code style bugs in signup_admin_form_extra()

B) this looks like a bug in the SQL for signup_list_user_signups_sql():
... WHERE s_l.uid = '%s' ' AND ...

C) code style in _signup_event_completed():
return true; (should be TRUE)

In terms of the patch against signup.module (at least the parts that apply):

D) This code looks like a separate bug fix, unrelated to event refactoring, no?

-    $signup_form_data = serialize(array_merge($signup_form['signup_form_data'],
 $extra));
+    $signup_form_data = serialize(array_merge((array) $signup_form['signup_form
_data'], $extra));

E) Same here:

-  $addresses = signup_get_email_addresses($form_values[nid]);
+  $addresses = signup_get_email_addresses($form_values['nid']);
   if (is_array($addresses)) {
     $from = $form_values['from'];
     $subject = $form_values['subject'];
-    $event = node_load($form_values[nid]);
+    $event = node_load($form_values['nid']);

(Just committed the fix for this: http://drupal.org/cvs?commit=74982)

F) signup_cron() looks great with the query builder stuff. However, either your code is buggy or the functions are really poorly named. All the SQL related to the reminder emails is from functions that are called "signup_cron_sql_xxx()". All the SQL related to auto-closing signups is from functions that are called "signup_reminder_sql_xxx()". I'd rather the reminder SQL was from functions called "signup_reminder_sql_xxx()", and the SQL related to closing signups was from functions called "signup_autoclose_sql_xxx()". "cron" is ambiguous, since many things can happen during signup_cron().

Anyway, this is definitely looking fantasic, once these final problems are cleaned up and we have a patch that applies cleanly.

Thanks again!
-Derek

Re: #14.D -- see http://drupal.org/node/162688

Patch for the .theme file. Adds ifdefs for event 5.2 and date. These need to be filled when the .in files for these are made.

admin/content/signup form is still fubar:

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 'Array GROUP BY Array ORDER BY n.title ASC LIMIT 0, 25' at line 1 query: pager_query SELECT Array FROM signup s Array GROUP BY Array ORDER BY n.title ASC LIMIT 0, 25 in [...]/includes/database.mysql.inc on line 172.

The most trivial of testing of your own patches would be nice before asking me to review this. ;)

Yeah, explode != implode...

still full of explode() vs. implode() bugs. again, please at least visit the admin/content/signup page before you set this back to needs review.

100% explode free.

This still needed work:

A) Things were broken if neither event nor date were enabled, so I added a signup_event_none.inc file.

B) signup_admin_form_extra() always assumed event data. It should just use signup_format_date().

C) The local variables in signup_cron() for the query builder stuff were still screwy (e.g. $signup_reminder_sql_joins for the auto-close query).

D) Renamed the .inc files to be more consistent with the release system version strings.

E) For now, we don't want to break sites that only have date module enabled, so until signup_date.inc exists, we include signup_event_none.inc.

F) There was a bug due to killes's test site having an older DB schema -- in 5.x-2.* {signup}.completed was renamed to {signup}.status.

G) Instead of manually testing for what event backend is in use inside theme_signup_user_schedule(), there's now a theme_signup_event_dates() function in the .inc files to handle the backend-specific stuff.

H) Some minor code formatting stuff for some array definitions.

I) Removed some extra spaces we were adding to the SQL in the query builders that weren't needed.

J) Removed the definition of SIGNUP_EVENT since it's not used anywhere. If we need it, we can always add it later.

However, killes has done enough lifting on this issue already, so I made all those changes during my final review/testing, and committed to HEAD: http://drupal.org/cvs?commit=75725.

Setting this back to active so that killes can write and post his signup_event_5.x-2.inc file. ;)

This rocks! Thanks!!!
-Derek

Here is a preliminary .inc file for event 5.2.

It needs some testing. It needs the latest event 5.2 from CVS.

Other than the code style problem in signup_admin_form_extra() with the array formatting, what needs work in here? Just testing? Looks like event 5.x-2.* is now natively supporting query builders, huh? ;)

Yeah, I thought that if I do that complicated SQL, I should support my fellow developers a bit. :p

Here's a new file which adresses a bug. Not sure what you mean about the formatting.

More testing is required and shoudl be forthcoming later today.

Whoops. :( We seriously broke the module for sites without event.module enabled at all. See http://drupal.org/node/165791. I think the cleanest solution involves changing the API for these .inc files. See my comment there.

No more progress can be made in here until my patch at http://drupal.org/node/165791 is reviewed, tested, and committed. http://drupal.org/cvs?commit=75725 was rather spectacularly broken. ;)

Ok, http://drupal.org/node/165791 is now committed to HEAD. Please see the latest copy of signup_event_5.x-1.inc to see what the new API for these .inc files is (I think it's simpler and clearer this way -- sorry for the rushed job on the first iteration of the interface).

New version attached.

Cool, this is mostly looking good. I don't have time to fully test this, but here are the only problems I see via visual inspection of the patch:

A) 'group_by' => array(event_where(), 'e.timezone'), seems like a cut/paste bug.

B) date('Y-m-d H:i:s', time() + (variable_get('signup_close_early', 1) * 3600)); is duplicated in a few places, might be nice to put that in a private helper in your .inc so it's easily shared?

C) Minor code style/indentation problem at the end of a few functions, where the ");" is indented 2 spaces too far.

(A) seems like the only real bug, but (B) and (C) would be nice to fix, too.

Thanks!
-Derek

A) is not a cut and paste error. I've compared it to the prior version and it was there too. But it was buggy there too. :-D

Testing of the new version is commencing.

B) I've only found two places so this IMO doesn't warrant a function for it. I'd rather prefer to have the same number of function in both versions of the file for evnt 5.1 and 5.2.

C) Debatable, but I'll upload a new version after the testing is complete.

Working now.

Why this:

    'fields' => array(event_select(), 'e.timezone'),
    'group_by' => array('event_start', 'e.timezone'),

instead of this:

    'fields' => array(event_select(), 'e.timezone'),
    'group_by' => array(event_select(), 'e.timezone'),

?

(Side note: why doesn't event_select() include timezone?) ;)

Thanks,
-Derek

Or, to make it explicit we're sharing the same value for fields and group_by, how about this:

function signup_admin_sql() {
  $sql = array(
    'fields' => array(event_select(), 'e.timezone'),
    'joins' => array(event_join('n', 'LEFT')),
  );
  $sql['group_by'] => $sql['fields'];
  return $sql;
}

? Just a thought...

event_select() returns as string that looks like "complicated stuff + - bla foo AS event_start" This is not suitable for use in a group by statement.

Cool, thanks for the clarification. I didn't test, but it looks like it should work, and you said you tested which is good enough for me. Committed to HEAD. woot! ;)

If anyone's following this issue and interested in date.module support, that's going to happen over at http://drupal.org/node/86462 -- please leave this issue alone...

Thanks,
-Derek

Since I was thinking about making a 5.x-2.2 official release of signup, now that this was "done", I decided to test it after all. Sadly, running cron.php results in fatal SQL errors:

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 '17:44:23 + INTERVAL 's.reminder_days_before days' ) > e.event_start - INTERVAL I' at line 1 query: _signup_cron_send_reminders SELECT n.title, n.nid, s.reminder_email, s.forwarding_email, e.event_start AS event_start, e.timezone FROM signup s INNER JOIN node n ON n.nid = s.nid INNER JOIN event e ON n.nid = e.nid INNER JOIN event_timezones tz ON e.timezone = tz.timezone WHERE s.send_reminder = 1 AND (2007-08-17 17:44:23 + INTERVAL 's.reminder_days_before days' ) > e.event_start - INTERVAL IF(tz.is_dst, tz.offset_dst, tz.offset) HOUR_SECOND

Argh.

Killes, can you please fix this ASAP? I'm basically out of Drupal land starting Saturday evening around 5pm GMT-700 for over 2 weeks, so if you want this to happen before I leave, you need to act fast.

p.s. My test site is on a machine with MySQL 4.1 -- there's no mention anywhere on the event project page or the 5.x-2.x-dev release node specifying it depends on MySQL 5.* or something. If that's the case, consider this a bug in your own documentation. However, assuming you expect this to work, it's a bug in the .inc file... I'm using a fresh checkout from event HEAD as of right now.

Apparently, cron.php escaped our testing, sorry for the trouble.

Great, the patch looks reasonable and seems to work. ;) Committed to HEAD. I just made a 5.x-2.2 official release now that 5.x-2.* event support finally seems to be working. http://drupal.org/node/168553

Thanks,
-Derek

p.s. This will be my last activity in the signup issue queue until after 2007-09-04, since I'll be out of town and completely away from computers until then...

(setting back for posterity)