Posted by edmund.kwok on December 9, 2006 at 5:51pm
9 followers
| Project: | Signup |
| Version: | 5.x-2.x-dev |
| Component: | Code |
| Category: | task |
| Priority: | normal |
| Assigned: | Darren Oh |
| Status: | closed (fixed) |
Issue Summary
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.
| Attachment | Size |
|---|---|
| signup_5.0_port.patch | 20.44 KB |
Comments
#1
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
#2
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!
#3
#4
tracking
#5
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 ...
#6
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.
#7
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 ...
#8
@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
#9
sorry, seems the search doesn't work with multiple keywords :-(
#10
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.
#11
tracking
#12
I think it's finished. Titles show in watchdog entries and each user on the signup overview page has a unique form id.
#13
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. :)
#14
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 ;-)
#15
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.
#16
can we get one person to apply this patch and verify all the basic functionality before i commit?
#17
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
#18
Thanks for the review, Stefan. The two issues you found with the patch are in the original code and appear to be by design:
#19
Hi.
Oops. I didn't check this.
Yesterday I found a small typo at line #687
* Prints the setttings page under admin/setttings/signupYou can never have enough t's ;-)
Ok, I've found it and tested it without
s.completed = 0 AND. Setting the issue to "rtbc".#20
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) )
#21
Fixed in CVS commit 56031. There is now a DRUPAL-5 branch.
#22