Initial Drupal 7 port of signup module

Looking through the issue queue at NW and NR issues, I see #290305: Split out email functionality into separate submodule(s) and #29568: Flexible number and type of fields. The former is for the 2.x branch and seems large enough that it shouldn't hold up a 7.x release, and the latter is complex enough that I'm inclined to say, "Let's address this in 7.x" possibly in a 7.x-2.x branch.

I think 7.x-1.x should be a straight port.

How does that sound, @dww?

Sounds good. It'd be nice to look through the bugs in the queue, and see what actually holds up a 6.x-1.0 official release. Once that's out (with all the fixes committed to HEAD and DRUPAL-6--1), we can create the DRUPAL-6--2 branch and start doing a straight port of D6--1 to D7 in HEAD.

Also, Katherine Senzee (ksenzee) and David Rothstein did some D7 porting (and D7UX integration) as the example module for http://sf2010.drupal.org/conference/sessions/d7ux-how-integrate-core-dru... -- they threatened to send me a patch at some point, but it hasn't happened. ;) I just pinged them about this issue via email, hopefully one/both will reply here.

Cheers,
-Derek

We did the work at http://github.com/ksenzee/signup/tree/shiny and anyone who wants access to that can have it. I'm totally slammed at the moment but I don't want to wait to respond until I have time to clean this up, so I'm posting a patch here that I believe represents all the work we did. It applied to signup when we checked it out in March, and it worked on D7 HEAD as of just before Drupalcon.

The main problem with it is that we didn't even touch some of the submodules, and we didn't try to get any of the views integration working. (Views is in good enough shape now that that's a totally reasonable goal, but at the time it wasn't, afaik.) So in a lot of ways this is a rough first pass at an upgrade. Hopefully it's good enough to get people started though.

Sorry for the delay in getting this stuff posted.

I'm also attaching a version of the work split up into two different patches, which might be useful. We had two branches in the Git repository, one just to do basic functional upgrades and the other to add the D7UX integration on top of that. It may make sense to just start with the basic functional upgrade here, and save the D7UX stuff for a followup issue (we already got a live in-person review from @dww at DrupalCon pointing out that one of the D7UX changes we made, switching the URL of the "signup broadcast" link so it appears underneath an admin tab, might not be good for some use cases of the module :)

So, the first attached patch contains (I think) just functional changes (plus whatever stylistic changes Coder Upgrade added along for the ride), and the second is the patch that would be applied on top of that to make it hopefully look reasonable when combined with the rest of Drupal 7.

Yay, thanks folks!!!

I definitely think the D7UX changes should each happen in their own separate issues as followups once the initial porting is done. Some are pretty straight-forward, but others are more complicated. None of them should necessarily hold up an initial port getting committed to CVS, and all of them deserve their own issue so we can discuss the changes and how to best make the UX improvements.

Subscribing! Interested to learn if any more work has been done on the D7 port....

Just tweaking the branch here.

Is a updated dev version of D7 port available?
And which package you patched? It seams to be an older version.

This may be of use at some point during the 7 lifecycle: http://drupal.org/project/field_convert

Very interested in this, subscribing.

Subscribing

sub

subscribing

What is left to be done for the basic drupal 7 port? I might be able to contribute a little.

(and a stealthy subscribe)

A lot of work on Signup and API changes to Drupal 7 core have happened since #4.

I applied this patch to 6.x-1.x dev, and separately ran coder_upgrade on another copy of 6.x-1.x-dev and applied the hunks that would apply to the upgraded codebase.

I then diffed the two and compared the results.

It looks to me like overall the coder_upgrade version + no-ux patch from #4 gets closer to a D7 version, than just #4, though either approach needs more manual work.

Attached is off *that version* against 6.x-1.x

Ahem.

Could you also post a .zip or .tar.gz of the entire work in progress? I applied your patch to a checkout of the 6-1 CVS branch and it applied cleanly, but just to be sure...

Subscribing

subscribing!

I started working on a port just before you posted the patch. So this might look like duplicate work, but that wasn't on purpose...

I forked the master github repo from #3, then started backporting patches (manually :( ...) using the CVS messages for the 6--1 branch starting from June 30, 2010. (http://drupal.org/project/cvs/29351?nid=29351&page=3&nid=29351)

After that, I compared the results with your patch and made further improvements. Still lots of todo's left.

Github repo: https://github.com/woest/signup
commit history: https://github.com/woest/signup/commits/master?page=1

ezra-g, if you think this is worth pursuing (I for one prefer having the github repo, so one can actually track back all the various changes), please tell me and I'll continue working on it. If you also want to work on the github repo (or anyone else), give a shout and I'll add you to the github repo. The repo is only intended (at best) as a temporary solution untill the D7 port is good enough to commit to CVS.

EDIT: I wanted to attach an interdiff between the work in the repo and your patch, but I'm on a windows machine and can't do it quickly...

Is there any chance that this will soon become an official release?

FYI: Now that 6.x-1.0 is out, I'm syncing the end of DRUPAL-6--1 into HEAD so we can start actively doing the D7 port in HEAD. I'll reply here when HEAD is ready to accept D7 patches.

Okay, HEAD now has the same code as DRUPAL-6--1-0. Let the porting begin. ;)

subscribing

subscribing

subscribing

Sooo.. I worked on it some more, and it doesn't explode immediately on first use. However, still very much work in progress and doesn't function yet as it should. Could use a more skilled eye to take a look, and very much the reason I'm even posting the patch ;). As I said earlier, I prefer to work on this together in github (see my posts above), then commit one huge patch, and only have small patches left for committing here.Thoughts?

General todo's:
- db_rewrite_sql needs to be adressed.
- various other db calls not converted to DBTNG.
- views is still a mess
- .install is a mess
- mass eats kittens on a daily basis :)

Commit history + source @ https://github.com/woest/signup/commits/master , and/or check todo's in the code.

Blargh, old CVS headers cluttering the patch....

subscribe

By the way, I noticed when checking out from the CVS repo yesterday that the following files/folders were present:
- theme/no_views.inc
- inc/no_views.inc
- and a bunch of submodules that weren't there before (possibly part of the 6-2 branch???)

@Kiphaas7: Right, the DRUPAL-6--2 branch is a mess. Don't use that for anything. See this for why:

http://drupal.org/project/issues/signup?status=15&version=346809

Err, right :). But HEAD still has no_view.inc in /theme:
http://drupalcode.org/viewvc/drupal/contributions/modules/signup/theme/?...

And I don know if these modules should be part of HEAD either since they got committed with the 6-2 branch:
http://drupal.org/cvs?commit=438172
http://drupal.org/cvs?commit=438174
http://drupal.org/cvs?commit=438176
http://drupal.org/cvs?commit=438178
http://drupal.org/cvs?commit=438184

And lastly, I'm not sure what the contrib folder is:
http://drupalcode.org/viewvc/drupal/contributions/modules/signup/contrib...

Basically, I'm confused :). The first time I checked out without specifying a branch, and got the above result. So I'm guessing I need to explicitly checkout HEAD? Apparently, it checks out the MAIN branch by default...

subscribing

subscribing as well - excited!

I forked Kiphaas7's into git@github.com:jodyHamilton/signup.git, fixed a bunch of stuff, and sent you a pull request of the changes.

I just enabled the module and went around trying the basic functionality I need and fixed a bunch of pages/functionalities that were giving fatal errors. I now feel the basic features are working well enough to make this useable.

Thanks, Jody.

Any chance you could provide this as a diff against DRUPAL-6--1?

@ezra-g: I think you mean as a diff against HEAD. But hopefully that should be the same at this point.

Here's the patch against HEAD. I'd love to get this pushed so we can move on to smaller patches :)

Awesome - thanks. Will review soon.

subscribe

Looks great Jody! +1 on push and smaller patches.

hang on, slightly updated patch coming shortly

Ok, the new patch applies cleanly to HEAD and I fixed some things related to token.inc going into core.

The biggest TODOs I know about (lots of todos in the code) are updating the date field code to fields in core api, and testing and working on the token functionality. Additionally, the modules in /modules are untested and the modules in /contrib have not been ported.

I think there are also some DBTNG patches left, also one that isn't that straightforward to patch. In admin.signup_administration.inc: signup_admin_form_sql(). That entire function probably needs to be rewritten.

(kudos for your work btw, and I merged your changes in my own repo)

Fixed a node_save bug

When will this become an official download/dev-version of the module?

+1

Nevermind my comment in #44. I tried to make a start tackling the signup_admin_form_sql(), but found that it is too risky to try and fix it right now, in this giant patch where reviewing is tricky. Besides, it works in it's current form, although it has 1 critical bug: it doesn't check node_access. To fix that, because db_rewrite_sql() is no more, instead of db_query(), db_select() has to be used. That also means the following functions need to be rewritten in some way (if I forgot some, please tell me so I can add them, it's good to have a reference for later):

1. signup.module
   • _signup_build_query()
2. inc/admin.signup_administration.inc
   • signup_admin_form_sql()
3. inc/cron.inc
   • _signup_cron_send_reminders()
   • _signup_cron_autoclose()
4. inc/scheduler.inc
   • signup_admin_sql()
   • signup_autoclose_sql()
   • signup_reminder_sql
5. inc/event.6x-2.inc
   • _signup_event_reminder_sql()
   • _signup_event_autoclose_sql()
   • _signup_event_admin_sql()
6. inc/date.inc
   • _signup_date_admin_sql()
   • _signup_date_reminder_sql()
   • _signup_date_autoclose_sql()

So probably best to make this a separate issue once the initial D7 commit has happened. Thoughts?

Subscribe

subscribe - would definately be awesome, this is one of the modules holding me back from upping to D7=(

@matt.chapman,

I second that, but I understand things take time. I wish I was a coding master I would make it happen.

Subscribe

Subscribe. I'm new--is this really the only way to subscribe to an issue? It seems like it clutters up the actually substance of the thread.

@bnl: Yes, sadly for now, that's it. See http://3281d.com/2009/03/27/death-to-subscribe-comments for more.

subscribe

subscribe

I am using my d7 version on a live client site. If there are specific features missing people are waiting for, or bugs found in the d7 version, that would be a lot more useful than 'subscribe'. As it is I'm not even sure what you're subscribing to.

People might be subscribing cause there still is no note of d7 on the modules page? I could update the modules front page notes to reference this and remove the part about d6 testing?

A sanctioned 7-x branch is what I believe we're watching for.

That, and/or a code review by one or more core committers. But, as with any open-source project, these people have other obligations as well, preventing them for doing it right now :).

Mind you, it sounds to me like the D7-port might get more love from non-maintainers as a real dev-build (possibly omitted from the module page). It would be quite hard to manage missing features/bugs in this issue thread alone. A dev-build might be a way for the Signup maintainers to outsource part of the code review.
And if the current port runs a live site, I would deem it ready to move on - but I'm no module maintainer.

subscribe

Found a small, but really annoying bug in includes/date.inc:

$field = content_fields($field_name, $content_type);

content_fields() does not exist anymore, instead one of the following functions should be used:
http://api.drupal.org/api/drupal/modules--field--field.info.inc/7

Haven't had time to fix it yet, but should be a trivial fix.

EDIT: Just noticed that the following text is on the frontpage:

A Drupal 7 port has started (a straight port of the 6.x-1.0 release), but is not yet merged into CVS and available as a development snapshot. When it's ready for testing, it will appear in the table below.

Since there is quite some discussion, could any of the maintainers give some sort of statement what "ready for testing" means in their perspective?

Some changes to date.inc, cron.inc and removed cvs headers. See my github for a full list of changes.

https://github.com/woest/signup/commits/master

And some minor nitpicks for the submodules. Surprisingly, they didn't need a lot of conversion code.

As it stands, the current patch is fully working for me, but still has some major issues codewise. The following four points could probably best be solved in this order, in separate issues once the initial commit has happened. Why not in this issue? Most of the following issues should be discussed, because they are not so straight forward to port.

1. Queries against the node table aren't run via node_access. This is a security hole by definition, but requires some extensive rewriting of various functions, as mentioned in #49.
2. The code for supporting the date module has been converted to Date 7.x and the new Field API, but is flaky at best. Didn't do much more investigation, because a lot of the code is also intertwined with the previous mentioned query issue.
3. Token code needs a testdrive, as mentioned by Jody Linn in #43.
4. Clean up the numerous small todo's in the code.

subscribe

@Kiphaas7: I cloned your version of the code from github so any patches that I have will be diffs against your code (so you can add it in as I'm still a bit new to git and things surrounding it). Just so you know, I am looking to use this module with a set of anonymous users so the functionality I am looking to see working may be slightly different from what you have and let me know if I am stepping out of bounds what what you are trying to accomplish. I will be taking a look at the issues you are hoping to fix up over the next couple of weeks.

1) Whenever I went to the advanced help section, I would get a SQL error message. The patch I am attaching clears up that issue.
2) For my purposes, I am using the display suite to arrange where various fields end up on the page. When I enable a display suite template, I am unable to see the signup form; however, when it is disabled, it does show up correctly. However, I am unable to change where the form shows up on the page (I see that it requires the cck module). My proposal would be to change it so that we implement hook_field_extra_fields and allow a site admin to be able to pick where the form gets shown on the page. Should be a more flexible option. I'll try and tackle this aspect and add a patch once I get to that point.
3) As an admin, I can move the signup form to the signups tab, and it will show up there correctly. When I log out and go to a node that has enabled signups, I see the signups tab. However, when I click on it, I get WSOD (without any errors showing up in the error log or in watchdog) - I haven't looked into the code on where the issue might lie on this, however.

That is my report for now. I would just like to say thank you to you and the others (David, Ezra, Jody) for all the fantastic work you have put in to try and get signup for 7 out :)

Just to give an update regarding (2), I *kinda* have it working. The code to get that piece working was

/**
 * Implementation of hook_field_extra_fields().
 */
function signup_field_extra_fields() {
  $types = node_type_get_types();
  $extras = array();
  foreach ($types as $type => $object) {
    $extras['node'][$type]['display']['signup'] = array(
      'label' => t('Signup form'),
      'description' => t('Display the signup form to a user on the page'),
      'weight' => 10,
    );
  }
  return $extras;
}

We could add the other field listing the users in there as well if we wanted. And by using this, we can get rid of the controls from the main signup settings page for whether or not the signup form gets rendered on the content page.

Thoughts?

I've made a fork of the excellent work done by others here at https://github.com/btmash/signup to try and start adding my own fixes. While I try to figure out how to create a diff patch for here, I wanted to ask if we're strictly aiming for a port here or will the module become a candidate for a rewrite down the line. I ask this because the more I look at the code, what is being stored, etc, the more I see it taking advantage of new functionality in drupal (using the batch api for broadcast messages, queue for scheduled email blasts...largest part of the writeup I can see is that signup and it's usage of fields seem to make it ripe for being implemented as an entity.). However, I can also see that would be a LARGE undertaking and would push back the release for a 7.x version of this module by a lot (given the current lack of a 7.x branch in contrib, makes it that much harder to figure out what is the eventual plan for this module).

subscribe

Thanks so much everyone for your work here. I've committed the latest work, which should be a combination of the patches in #66 and #68 to a 7.x branch:
http://drupalcode.org/project/signup.git/commit/8842bd6

@dww - Could you either grant me permission to make a new release or create a dev snapshot release for the 7.x branch?

In the meantime, you can grab the 7.x version from.
http://drupal.org/node/29351/git-instructions/7.x-1.x

(given the current lack of a 7.x branch in contrib, makes it that much harder to figure out what is the eventual plan for this module

@BTMash - The plan (discussed somewhat in comments #1 and #2) is to do a "straight port" of 6.x-1.x (without using entity storage for Signup data) and to focus on new features and potentially switching to store signups with the entity system at a later point in a 7.x-2.x branch.

Let's keep working on the port in new issues.

Thanks again!

Thanks for the clarifications, ezra (I need to learn to read a little more) :) I have a bunch of patches up on github and will create the issues and corresponding patches for all them.

Great work, everyone!

Awesome - I look forward to your issues :D!

yay! Awesome!

@ezra: You can now make releases. Please just start with a 7.x-1.x-dev and perhaps a 7.x-1.0-unstable1 for now (unless you think the port is worthy of an alpha already, I haven't tried it). Maybe just start with a -dev and let's open a new issue to discuss the first alpha.

Thanks everyone!
-Derek

Thanks!

The 7.x dev branch snapshot is now available at http://drupal.org/node/1110728 .

Great, thanks ezra-g! One minor thing though: could you update the homepage to reflect the status of the D7 port?

Would you like to continue basic D7 porting issues here or elsewhere?

I see this in signup.module, meaning that a lot of field work has not yet been done.

        if (module_exists('content')) {
          // Due to a bug in CCK (http://drupal.org/node/363456), we need
          // to call this function twice to ensure we get the real value.
          // The bug is present when you first enable the setting to
          // display in the node instead of a separate tab, or when you
          // first upgrade to the version that contains this code.
          content_extra_field_weight($node->type, 'signup_node_info');
          $weight = content_extra_field_weight($node->type, 'signup_node_info');
        }
        else {
          $weight = variable_get('signup_info_node_weight_' . $node->type, 10);
        }
        $node->content['signup'] = array(
          '#markup' => $signup_info,
          '#weight' => $weight,
        );

Unfortunately I wasn't able to get signup working at all.

New issues, please. Thanks.