Theme function for signup_build_signup_data

signup_update_5202() adds these 2 new columns to the schema on existing installations, but you didn't change signup_install() in your patch to include these columns on new installs -- so, even if we were going with this patch, it'd need work.

however, i'm not sure this is the right approach. i'd *much* rather just add a theme function for how we print out the signup info, and sites that wish to customize it can override that. that's much more in the keeping with how drupal does things, and it avoids adding a bunch of additional code to signup.module for the handful of sites that want to tweak this stuff. personally, i don't use any of these email features, so this patch would just be bloat, additional complexity in the admin UI, and extra space in the DB, for customization i'd never use.

granted, it's not quite as flexible in the theme layer, since that would be site-wide, not per-node like you're doing here, but honestly, how often do you need to change the header for the signup info per node? also, keep in mind, the message itself is per node, so there's no good reason to have a special per-node field for "the thing right above %info"... you can just put that per-node stuff directly into the reminder or confirmation email body already, and just rip out the "SIGNUP INFO" part for your theme function if you always want to handle that in your per-node message text.

so, if you want to roll a new patch that doesn't mess with the DB schema at all, and just adds a new theme function for this, i'd happy review that and consider applying it.

thanks,
-derek

Sorry I've been slow to deal with the signup patch queue. Sadly, this no longer applies. Moreover, it conflicts *a lot* with the patch over at http://drupal.org/node/79331. I don't want to waste your time, so we should make sure we're on the same page about the order of these patches so that we can work on them 1 at a time to prevent needless re-rolling. I'd prefer:

http://drupal.org/node/137911 (signup_no_views.inc)
http://drupal.org/node/79331 (general signup UI patch)
http://drupal.org/node/137609 (this issue)

thanks.

We should wait until the patch lands for http://drupal.org/node/47913 before we try to re-roll this, to avoid conflicts.

#178441: Custom signup data doesn't include labels in emails, and has no theme functions is a bug that I just marked duplicate with this.

#47913: More themeability is too much of a catch-all issue to be useful as it currently stands. I think we should just fix this one right now (I'll work on a patch tomorrow morning), and get this in before both 5.x-2.5 and 5.x-1.1. I think a lot of the problems that #47913 are trying to solve are being addressed at #243035: How to list all signed up users in a view?? anyway...

p.s. Maybe I'll fix #213750: present checkbox 1/0 as yes/no while I'm at it, since it's directly related.

How's this? Moderately tested in 5.x-2.x (HEAD). Backported but untested for 5.x-1.x (DRUPAL-5).

Since stBorchert added a patch for #213750: present checkbox 1/0 as yes/no, I'm leaving that change out of this issue, and we can handle that separately.

Tee hee, glad I tested it with DRUPAL-5, since the previous patch had a few syntax errors. ;) Try this one, instead.

Even better still:
- uses call_user_func(__FUNCTION__, ...) when doing the recursion to avoid the overhead of all of theme() to route back to whatever theme function we're in.
- removes the bogus empty div we were adding for the 'table' case that was generating empty rows when you have a nested signup user form.
- more carefully and consistently handles the divs, always adds an id, etc, in the 'output' case.

I think the whole 'table' case of this function is weird and should be handled differently, but should be for a followup issue, really...

Re: "I've split up the function theme_signup_custom_data() into separate functions"

Cool, yeah, I was thinking of the same thing last night after I logged off, glad you did that. ;) I renamed them a little (so they all start with "signup_custom_data" and then have "_table", "_rows" or "_email" appended on the end of the function name). I also cleaned up the formatting of the comments and added a warning about the recursion to each one that calls itself. And, I finally fixed the code that was recursively generating the table rows to not have to rely on the silly static copy of $rows, but to just do the more obvious thing of merging the array from the recursive call vs. appending rows for the non-recursive case.

Re: "Beyond this I added theme_signup_custom_table_data()"

That's what I meant in #16 with "I think the whole 'table' case of this function is weird and should be handled differently, but should be for a followup issue, really...". ;) But ok, we can just leave it in this monster cleanup patch at this point. Note, it's stupid that _signup_print_current_signup() and _signup_node_output() aren't theme functions, either, but that's *REALLY* for another patch (#47913: More themeability). Please do *NOT* re-roll this one for that. ;)

I'll work on backporting this to the DRUPAL-5 branch next, but I wanted to post this version for (hopefully final) review and comment.

Thanks,
-Derek

After a brief discussion in #drupal IRC, people agree it's sort of silly to keep a separate .theme function like this. I'm going to move those functions back into the .module file in the near future (we already weren't being consistent, some theme functions already live in .module). So, this patch adds all the new ones directly to the .module file and avoids the complications of trying to include signup.theme when necessary. This also includes the DRUPAL-5 backport for 5.x-1.x. Pretty sure this is RTBC at this point, but I'll see if anyone else wants to comment quickly. ;)

Committed to both HEAD and DRUPAL-5. Thanks, stBorchert, for all your help on this one. Signup's finally gonna be themeable in the near future. ;)