The email notification does not work. I guess the drupal_mail() function in Drupal 6.x has changed and guestbook 6.x-1.0-r1 still uses the old API. The message 'New guestbook entry' seems to be interpreted as "TO:"-adresses, not as the message body. (But since I'm not familiar with programming Drupal, this is just a wild guess.)

CommentFileSizeAuthor
#8 guestbook.module.diff1.06 KBbeedaddy
#10 guestbook.module.diff1.16 KBbeedaddy
#5 guestbook.module.zip7.52 KBbeedaddy
#2 guestbook.module.diff998 bytesbeedaddy

Comments

beedaddy’s picture

beedaddy commented

Sorry, I should have said: The message 'New guestbook entry' seems to be interpreted as "TO:"-adresses, not as the subject.

beedaddy’s picture

beedaddy commented
StatusFileSize
new guestbook.module.diff998 bytes

I hope I don't abuse this bug reporting feature. But I jumped in at the deep end and patched the module so that it suits my needs, i.e. that it now sends the notification message. I use only a site guestbook so I don't know if it works with user guestbooks. I try to attach the small patch to this posting.

Anonymous’s picture

Anonymous (not verified) commented

I have the same problem here. I'll check the patch and report the result.

Anonymous’s picture

Anonymous (not verified) commented
Status: Active » Needs review

As I couldn't find a simple way (CygWin is not simple) to apply the file to my installation on windows, it would be nice, if you could post the complete file here.

beedaddy’s picture

beedaddy commented
StatusFileSize
new guestbook.module.zip7.52 KB

Ok, I've attached my (zipped) adapted guestbook.module.

Anonymous’s picture

Anonymous (not verified) commented

Thanks for that. Seems to work fine. I can receive the mail and there is no error in the frontend, yet.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs review » Needs work

- Module function names need to begin with the module name, so notify_mail() will turn into a fatal error if that function is already defined by another module (f.e. the Notify module). So the first argument to drupal_mail() must be 'guestbook'.
- language_default() looks wrong here.
- For both previous issues along with possibly others, see http://api.drupal.org/api/function/drupal_mail for how drupal_mail() works.
- Always use single quotes unless double quotes are required.
- Please use at least diff -up to create patches. More information can be found on http://drupal.org/patch/create

beedaddy’s picture

beedaddy commented
Status: Needs review » Needs work
StatusFileSize
new guestbook.module.diff1.06 KB

Ok, as I said, I'm not familiar with Drupal programming. I've now attached a hopefully slightly better patch (but still with language_default(), sorry). Everything else should be done by a more experienced person. :-)

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

No problem. Looks better than the first. However:

- The preceding declaration of $from can be happily removed.
- Missing array initialization: $params = array();
- $params['account'] is not defined, should be user_load('uid' => $uid)
- language_default() needs to be replaced with user_preferred_language($params['account'])
- $key should not contain the module name, simply 'notification'
- The subject should be more readable, i.e. 'New guestbook entry at !site'

Please change the status of this issue accordingly when posting a new patch for review.

beedaddy’s picture

beedaddy commented
Status: Needs work » Needs review
StatusFileSize
new guestbook.module.diff1.16 KB

Thanks for your patient advice. There is an updated diff attached.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs work » Needs review

@mdatab: Good job!

@forschi: Now if you would grab yourself fresh copy of Guestbook, apply the patch, test notifications for site and user guestbooks, and report back if anything went wrong, then you guys would have successfully participated in Guestbook's development.

Please change the status to RTBC if this patch has been tested (and works), otherwise to CNW.

Anonymous’s picture

Anonymous (not verified) commented

As the only simple tool to apply a patch on windows is Eclipse and I don't want to install that for only applying one patch, could you please post (or mail me) the entire file?

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

@forschi: Please have a look at http://drupal.org/patch/apply , especially http://drupal.org/patch/apply#comment-784749

Anonymous’s picture

Anonymous (not verified) commented
Status: Needs review » Reviewed & tested by the community

The patch seems to work fine for the global site guestbook as well as for user guestbooks. There are some other topics, but most of them are not related to this topic (users can only access their guestbook, if they have permission to view user guestbooks, TinyMCE is not displayed for global guestbook).

Maybe it's possible to let the user/administrator customize the topic and the body of the e-mail, e.g. "New guestbook entry at !site from user !username"

@sun: thank you for that hint, but I already tried to use this tool. It stopped with an internal error and I'm not willing to fix bugs in that tool. The guestbook is more important ;)

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Reviewed & tested by the community » Fixed

Thanks, committed!

Anonymous’s picture

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

Automatically closed -- issue fixed for two weeks with no activity.