This is a preliminary patch to port signup to 5.0. Changes so far are as follows:

  • Change user_mail() to drupal_mail() - Done
  • Removed hook _settings() - Done
  • Change hook_view() and hook_nodeapi($op = 'view') - Done
  • Changes to node type settings form - Still in progress
  • Altered the behaviour of placeholders in t() calls - Done
  • Changed drupal_get_form() to take a $form_id, not a $form - Done
  • Changed - forms must be created in dedicated builder functions - Done
  • module_exist() is now module_exists() - Done
  • format_plural() @count change - Done
  • Change menu item and node links to use Sentence capitalization instead of lowercase - Almost Done

Please review as I might have missed out some things. Until then, setting this to code needs work. Will continue working on it when I wake up. Feel free to add on to this patch. Thanks.

CommentFileSizeAuthor
#17 code-style-result.txt6.86 KBstborchert
#15 signup_7.patch27.74 KBdarren oh
#12 signup_6.patch28.27 KBdarren oh
#10 signup_5.patch27.87 KBdarren oh
#2 signup_5.0_port_1.patch23.75 KBedmund.kwok
signup_5.0_port.patch20.44 KBedmund.kwok

Comments

dww’s picture

dww
we/he/they
commented

sweet, thanks!

you seem to be duplicating your patch from http://drupal.org/node/74242 in here... should we just mark that one duplicate with this and focus on 1 giant patch?

i eagerly await your updated patch...

thanks, again!
-derek

edmund.kwok’s picture

edmund.kwok commented
StatusFileSize
new signup_5.0_port_1.patch23.75 KB

You're welcome :) We're eating our own dog food anyway :p

Updated patch. Node settings form should work now and menus have been capitalized accordingly. Menu descriptions can be improved, and comments for the form builder function could be added; leaving this for the seasoned Drupal coders to handle.

Two issues still present:
1. Haven't figure out a way to assign unique form ids for canceling each users' signup in the signup overview page. Not sure how to make it possible with hook_forms.
2. Watchdog overview entries not showing event title, but it's displayed accordingly in the detailed watchdog entry page. Now using !event placeholder for t(); using @event/%event causes node title to be enclosed in em tags.

Yeah, I think we can remove the two individual porting task and focus on this big one instead. Setting this for review. Test it out people!

edmund.kwok’s picture

edmund.kwok commented
Status: Needs work » Needs review
genemcc’s picture

genemcc commented

tracking

stborchert’s picture

stborchert
he/him
Primary language German
commented

The patch seems to work fine (in combination with a .info file).
But there's a little error in signup.module: text replacements should be "!..." and not "%...".
For example in line 410: "...please %login to sign up..." has to be changed to "...please !login to sign up...". Otherwise the link to the login page isn't displayed correctly.

Found no more errors ...

dww’s picture

dww
we/he/they
commented
Status: Needs review » Needs work

i looked, and found some errors, too. hopefully i'll finally have a chance later this week to work on this, commit it, and make a 5.x-1.0 official release (since someone might finally be sponsoring my time on it).

stay tuned.

ray007’s picture

ray007 commented

Maybe I should write an own feature-request, but I think the question also fits here:

would it be possible to use the signup-procedure also for content-types with ckk-fields of type date or datestamp, and not depend on the event module?
reading the discussions about the future of event management in drupal, I got a feeling where everything could be headed ...

dww’s picture

dww
we/he/they
commented

@ray007: please search the signup issue queue first: http://drupal.org/project/issues/signup
you would have quickly found: http://drupal.org/node/86462

thanks,
-derek

ray007’s picture

ray007 commented

sorry, seems the search doesn't work with multiple keywords :-(

darren oh’s picture

darren oh
Primary language English
Location Lakeland, Florida
commented
StatusFileSize
new signup_5.patch27.87 KB

I'm trying to get this module to the point where I can try it out. I hunted down the remaining t() function errors. I'll look into the cancellation form IDs (comment 2) after I've learned how to use it.

laken’s picture

laken commented

tracking

darren oh’s picture

darren oh
Primary language English
Location Lakeland, Florida
commented
Assigned: Unassigned » darren oh
Status: Needs work » Needs review
StatusFileSize
new signup_6.patch28.27 KB

I think it's finished. Titles show in watchdog entries and each user on the signup overview page has a unique form id.

hunmonk’s picture

hunmonk commented

i passed along a code review to darrenoh. as soon as he posts the fixes, then we need to get some +1's on the functionality of the patch from a few people. then we'll look at committing and getting a DRUPAL-5 branch. :)

ray007’s picture

ray007 commented

Suggestion: make a DRUPAL-5 branch now, apply the patch and commit it.
Just because you have that branch doesn't mean it already has to be perfect.
Especially if you don't create a release.

And it makes it easier for people to get it and test it.

just my 2c ;-)

darren oh’s picture

darren oh
Primary language English
Location Lakeland, Florida
commented
StatusFileSize
new signup_7.patch27.74 KB

I fixed the issues hunmonk found with the patch. We will still have event titles cut off until Drupal core is fixed. See issue 116625 for more information.

hunmonk’s picture

hunmonk commented

can we get one person to apply this patch and verify all the basic functionality before i commit?

stborchert’s picture

stborchert
he/him
Primary language German
commented
StatusFileSize
new code-style-result.txt6.86 KB

Hi.
Darren asked me to review the last patch. Here are my results:

code review
no evident errors found
code-style.pl: see attachment

functional review
seems to work all fine except sending reminders on closed signups:
As long as signup is open reminders are send as expected. But when you close signup before there should be a reminder, no mail is sent.

Apart from this I would say the patch can be committed. I left status on "needs review" because of the reminder-issue and code style (minor issue).

greets,

Stefan

darren oh’s picture

darren oh
Primary language English
Location Lakeland, Florida
commented

Thanks for the review, Stefan. The two issues you found with the patch are in the original code and appear to be by design:

  • The recommendations of the code-style.pl script conflict with the Drupal coding standards.
  • There is a deliberate check on line 57 of signup.module (both DRUPAL-4-7 and HEAD) to see if signups are open before sending reminders. I'm opening issue 118648 to deal with this issue.
stborchert’s picture

stborchert
he/him
Primary language German
commented

Hi.

The recommendations of the code-style.pl script conflict with the Drupal coding standards.

Oops. I didn't check this.

Yesterday I found a small typo at line #687
* Prints the setttings page under admin/setttings/signup
You can never have enough t's ;-)

There is a deliberate check on line 57 of signup.module (both DRUPAL-4-7 and HEAD) to see if signups are open before sending reminders.

Ok, I've found it and tested it without s.completed = 0 AND. Setting the issue to "rtbc".

markfoodyburton’s picture

markfoodyburton commented

For what it's worth, I've been using Drupal for hours (!), the patch as you sent it seemed to work for me out of the box.
(I do have a question on the emails, that I suspect has nothing to do with version 5.0, but it might be worth just checking to make sure? (node/118794) )

darren oh’s picture

darren oh
Primary language English
Location Lakeland, Florida
commented
Status: Needs review » Fixed

Fixed in CVS commit 56031. There is now a DRUPAL-5 branch.

Anonymous’s picture

(not verified) commented
Status: Fixed » Closed (fixed)