Port signup_status_invite to messaging framework
shenzhuxi - September 17, 2009 - 05:26
| Project: | Signup Status |
| Version: | 6.x-1.0-alpha1 |
| Component: | Code |
| Category: | feature request |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | needs work |
Jump to:
Description
By messaging framework, signup_status_invite can send invites with privatemsg, mail, sms and etc.
| Attachment | Size |
|---|---|
| signup_status_invite.patch | 6.5 KB |
#1
(Note: when you have a patch for maintainers to review: set it to "needs review" -- "to be ported" is for cases where some change is already done for one version of the code (e.g. the 6.x-1.* branch) and then the bug fix needs to be backported to earlier versions (e.g. the 5.x-1.* branch) or something).
Generally, my 2nd hand experience with the messaging framework is that it's huge and clunky and a pain. I'd be opposed to making it a required dependency. I'd consider optional integration, but only if it can be turned off. If you want this patch to be seriously considered, you need to:
A) Either make the call to messaging_message_send_user() conditional on if the messaging module is enabled, and/or add a setting to control if the messaging framework should be used.
B) Clean up the patch so it's not just commenting out some things and adding others -- remove lines that should be removed, don't just
comment them out.
C) Ensure that the new code follows the Drupal coding standards. For example, this does not:
+ if ($account = user_load(array('name' => $name)))+ {
If you were proposing to add a dependency on the messaging framework (which your patch currently does), it would also need to modify the .info file to define that dependency. However, I think such a dependency is a bad move, so it's actually a good thing your patch doesn't add it to the .info file. ;) But, for future reference, dependencies need to be defined in the .info file.
If you re-write this patch to address these problems, you can set the status to "needs review"...
Cheers,
-Derek
#2
Derek, thanks for review.
I think messaging framework is not huge comparing with most module. It just provides a middle layer for drupal to control message system and supports privatemsg, mail, sms and etc. with plugins. Do it simple and do it right. I port this module just by replacing drupal_mail function with messaging_message_send_user function without any pain. Defining the dependencies in the .info file will not be bad.
For usability considering, signup_status_invite is for sending invitation to register users. So the "To" field in the form shouldn't be email but username. That's more logical for users.
For B) C), thanks again for pointing out.