Port Guestbook to Drupal 7

Waiting for this as well...

This is going to be *fun*. Chainsaw massacre probably describes it best. :)

sun -- as noted here I'd like to propose deprecating user guestbooks in favor of Facebook-style Statuses and transforming this module into only a site-wide Guestbook. I think the transition of this module to D7 is as good a time as any to implement that change.

Subscribing.

Subscribing

subscribe

Ready port to D7: Guestbook 7.x-2.0-dev
http://drupal.org/node/1050818

#1050818: Guestbook 7.x-2.0-dev has been marked as duplicate.

This is not really the chainsaw massacre I had in mind, but well, it looks like people could use it. Happy to commit to HEAD and do the massacre only later on.

So please test, review, patch, whatever...

This patch seems to be working overall, which is nice. I havnt fiddled around with all the options but the "notification by email" option doesnt work.

When used it prints this error when you makes a post:

Fatal error: Cannot use object of type stdClass as array in C:\wamp\www\drupal\modules\user\user.module on line 2710

Also, when I clear the "notification email" field and save I get the following output:

Warning: Missing argument 3 for user_mail_tokens(), called in C:\wamp\www\drupal\sites\all\modules\guestbook\guestbook.module on line 708 and defined in user_mail_tokens() (line 2709 of C:\wamp\www\drupal\modules\user\user.module).

i get a strange behaviour with the patched version.

i set the entry-form to 'seperate site' and in this seperate site, the site title is 'Startseite' (which means 'Home'). I think this is the drupal standard title for a page, because there isn't any occurance of this word in the guestbook-translation.

I tried to set the title manually in line 78 in the guestbook.module file, but that didn't work.

I have the same error as moname when trying to activate the notification-by-email-option. But when I clear the notification email, I get this Notice:
Notice: Undefined index: anonwebsite in guestbook_form_entry_form_submit() (line 635 of /sites/all/modules/guestbook/guestbook.module).

Further question: As sun mentioned here http://drupal.org/node/173263#comment-3954524, there is kind of a moderation option implemented. But I can't find any option for this. Am I missing something or is this feature not yet available for the patched Drupal 7 version?

I'm really looking forward to a D7-Version! Many thanks in advance @ sun and the other maintainers!

an other warning:

Strict warning: Only variables should be passed by reference in theme_guestbook() (line 886 of /sites/all/modules/guestbook/guestbook.module).

this warning comes up on multiple frontend and backend sites of drupal (not only on guestbook related pages) since I activated the guestbook module.

Subscribing

patch from #8 doesn't work anymore with the latest version (1.35)
can we get a new patch, or in which way has the old patch to be modified.

realy cool and helpful module, but....

strange maintainer politics. wtf, dude? you can make a patch for 7.x, but why you cannot make a stable version?
go on, stop making a patches )))

Rerolled #8 against master. No other changes, yet.
EDIT: sigh, I guess this was against a 6.x-2.x checkout. Gonna try again, and not remove mollom this time.

Strict warning: Only variables should be passed by reference in theme_guestbook()

   // Form on separate page.
-  $output .= ($form_location == 'separate page' ? guestbook_form_entry($uid, 'link') : '');
+  if ($form_location == 'separate page') {
+    $output .= drupal_render(guestbook_form_entry($uid, 'link'));
+  }

   // Form and pager above entries.
-  $output .= ($form_location == 'above' ? guestbook_form_entry($uid) : '');
-  $output .= ($pager_position & GUESTBOOK_PAGER_ABOVE ? theme('pager', NULL, $limit, 0) : '');
+  if ($form_location == 'above') {
+    $output .= drupal_render(guestbook_form_entry($uid));
+  }
+  $output .= ($pager_position & GUESTBOOK_PAGER_ABOVE ? theme('pager') : '');

   // Form and pager below entries.
-  $output .= $pager_position & GUESTBOOK_PAGER_BELOW ? theme('pager', NULL, $limit, 0) : '';
-  $output .= $form_location == 'below' ? guestbook_form_entry($uid) : '';
+  $output .= $pager_position & GUESTBOOK_PAGER_BELOW ? theme('pager') : '';
+  if ($form_location == 'below') {
+    $output .= drupal_render(guestbook_form_entry($uid));
+  }

     // Display owner comment edit form.
-    $output .= guestbook_form_comment($uid, $entry);
+    $output .= drupal_render(guestbook_form_comment($uid, $entry));

drupal_render() must be passed a variable. Appropriate and safe seems using $build here.

Notice: Undefined index: anonwebsite in guestbook_form_entry_form_submit()

The entry form constructor does not always add the anonwebsite, anonemail and anonwebsite textfields. The notice must be coming from these changes:

-      db_query("UPDATE {guestbook} SET anonname = '%s', anonemail = '%s', anonwebsite = '%s', message = '%s' WHERE id = %d", $form_state['values']['anonname'], $form_state['values']['anonemail'], $form_state['values']['anonwebsite'], $form_state['values']['message'], $form_state['values']['entry_id']);
+      db_update('guestbook')
+        ->fields(array(
+          'anonname' => $form_state['values']['anonname'],
+          'anonemail' => $form_state['values']['anonemail'],
+          'anonwebsite' => $form_state['values']['anonwebsite'],
+          'message' => $form_state['values']['message']['value'],
+        ))
+        ->condition('id', $form_state['values']['entry_id'])
+        ->execute();

-    db_query("INSERT INTO {guestbook} (anonname, anonemail, anonwebsite, author, recipient, message, comment, created)
-      VALUES('%s', '%s', '%s', %d, %d, '%s', '%s', %d)", $form_state['values']['anonname'], $form_state['values']['anonemail'], $form_state['values']['anonwebsite'], 0, $uid, $message, '', time());
+    $fields = array(
+      'anonname' => $form_state['values']['anonname'],
+      'anonemail' => $form_state['values']['anonemail'],
+      'anonwebsite' => $form_state['values']['anonwebsite'],

A straight check and assigning the empty string if not set seems like the appropriate fix for this patch.

EDIT: Or not. The undefined index was there in D6 already...

Fatal error: Cannot use object of type stdClass as array in C:\wamp\www\drupal\modules\user\user.module on line 2710
Turns out guestbook_mail() is still unported.

Thanks for reporting, reviewing, and testing! Committed to #8 resp. #17 to master.

@Eric_A: Can you provide a patch for the bugs you've found? :)

To show what I'm thinking a quick follow-up, completely untested...

+++ b/guestbook.module
@@ -697,11 +697,11 @@ function guestbook_form_entry_form_edit_submit($form, &$form_state) {
-  $variables = user_mail_tokens($params['account'], $language);
+  $variables = token_generate('site', array('name' => '!site'), array(), array('sanitize' => FALSE));
...
+      $message['subject'] = t('New guestbook entry at !site', $variables, array('langcode' => $language->language));

Doesn't look correct to me. See http://api.drupal.org/api/drupal/includes--token.inc/7 for a high-level description of Token API in D7.

We should also compare what user_mail() and system_mail() implementations are doing. Unfortunately, Token API didn't receive much attention since it went into core, so the API is a bit hard to use and has a lot of pitfalls.

+++ b/guestbook.module
@@ -697,11 +697,11 @@ function guestbook_form_entry_form_edit_submit($form, &$form_state) {
+      $message['body'][] = drupal_html_to_text($params['body']);

In D7, the mail body is supposed to contain HTML. The mail subsystem is responsible for converting into plain-text (if that applies).

Powered by Dreditor.

Thanks a bunch for quickly reviewing.

I chose token_generate() over token_replace() because we have a string literal and know exactly which tokens we have to replace. I've done some manual testing and it just works. There's certainly a documentation problem with token_generate(), since the system_tokens() implementation expects the tokens to be keyed by name (in accordance with what hook_tokens() says) not by the literal text of the token as it appeared in the source text.

EDIT: erhm, maybe the token_generate() $tokens param documentation talks about the return value and not about what you pass into the function... it should not...

I suppose there's a documentation problem in system.api.php, too, regarding the hook_mail() example... Fixed the patch.

On a sidenote, to everyone who is confused: the name of the token here is 'name'. In Drupal 7 we have site:name. Type "site", name "name".

Restoring support for a multilingual !site. That is, in D6 this was tied to variable_get() trickery and had nothing to do with injecting $language here. But it seems to make sense to just pass in $language here to the token system...

Here is another patch to remove the db_result() call in _guestbook_newentries() as this function doesn't exist anymore.

#27 looks like a case for using REQUEST_TIME instead of time(). http://drupal.org/update/modules/6/7#time

Should #27 use db_query() like node_get_recent() and comment_num_new() do?

EDIT:
@jurgenhaas: the original patch converted 3 SELECT queries to a db_select() (two of which use query extenders) and turned 4 into a D7 db_query(). Can we do a straight db_query() port of the count query at hand? Turning it into an extendable and alterable query could be discussed in a separate issue.

EDIT2:
So I'm suggesting:

$count = db_query("SELECT COUNT(created) FROM {guestbook} WHERE recipient = :recipient AND created > :created", array(':recipient' => $user->uid, ':created' => isset($user->guestbook_visited) ? $user->guestbook_visited : REQUEST_TIME))->fetchField();

Improved on #26. Better token code.

An April 22 snapshot of very raw D7 code is here: http://drupalcode.org/project/guestbook.git/snapshot/2cd89080b136008d3ce...
From this snapshot on the two patches from #27 and #30 are relevant. These patches complement one another and are fixing bugs that are in still in snapshot code. My patch from #30 should apply cleanly against the above snapshot. I have not tried to apply the patch from #27.

Subscribing.

Hello,
There are already 5 months since the release of Drupal 7. How long must we wait?

Subscribing !

@KirstenLangholz, @likewhoa, @Joenet, @gawi, @sun.
How would you feel about changing the mail subject to the D7-style 'New guestbook entry at [site:name]'. This will break a customized D6 translation (of 'New guestbook entry at !site'), but other things will improve. One of those is a clean-up/ simplification of the D7 patch, which might or might not speed up its acceptance :-).

Let me know and I'll roll a new version of #30 which will also incorporate #27, #28, #29.

@Eric_A: Yes, we should use the token system that's in core now. We can update the corresponding variable and attempt to automatically replace !site with [site:name]

please add official dev-version

After applying this patch against the master branch everything reported by the community in this issue is addressed.

There is more to do, but it would be sweet if this gets committed soon. When committed there will finally be a git snaphot available without known (fatal) errors in (user) guestbooks.

Committed #39 to master.

However, the token replacement in guestbook_mail() still needs work, I think.

token.inc describes the high-level usage of Token API. Our implementation should look fairly similar to system_mail() (minus the conversion in body).

I don't think we need to prepare or generate any tokens upfront - at least at this point in time, as we're only using site tokens. The call chain is:
token_replace() (without any context/arguments),
calls into token_generate(),
calls into system_tokens() (hook_tokens()), which unconditionally generates the global site variable tokens.

Hence, something along the lines of attached patch. Didn't test.

+1 on token integration.

I guess #40 is about supporting all available tokens in the mail subject, rather than just the one that is in the translatable source text.

If it was decided to support a hard coded (sub)set of tokens provided by required core modules I would be very much in favor of trying to come up with a source text containing all these tokens, rather than sticking with 'New guestbook entry at [site:name]'.
The patch in #40 supports all tokens provided by core and contrib (that don't need extra data) just like system_mail() does. Here it is clear that we're abusing the translation system, but it's still easier on the eyes when using a source text containing all replaceable core tokens.

Thoughts?

I don't see how "we're abusing the translation system"?

t() is cleanly separated from token_replace(); first translate, then perform token replacements. Also, token_replace() does not generate tokens, which do not appear in the text.

I've spent some more time with the new token system and I guess #40 is just fine.
If administrators abuse the translation system they can now do so with all generally available tokens, but that's general D7 behaviour I guess. There does not seem to be an API to white list tokens to be build and replaced.

Actually, I'm confused. When translating a string literal, you just know which tokens should be there in any translation. In this case: [site:name]. It would be different if we were dealing with a multilingual variable textfield.

Thanks for reporting, reviewing, and testing! Committed to 7.x-2.x.

A new development snapshot for 7.x-2.x will be available within the next 12 hours.

Also note that the master branch has been deleted.

Now we need some manual testing and confirmations or reports of remaining bugs in order to create a D7 release.

Subscribing

I think only 2 issues are blockers of stable release

- menu item to configure module #1237194: Put the Guestbook admin/config menu
- drupal 7 should apply text format but this upgrade path needs review #1192778: Implement explicit text format for messages and comments

I have an idea and concrete offer to make:

I'm positively impressed by the collaboration, peer-reviewing, and work on some issues in the queue.

If both of you, @Eric_A and @andypost, want to become co-maintainers so that both of you can actively continue that work and move the Guestbook project forward, then I would immediately grant both of you the necessary perms.

Note the "both." I could see that working smoothly. Whereas only one would have a good chance of things going not so smoothly.

Of course, I'd be available for discussing and reviewing larger and more complex change proposals, and architectural changes.

Let me know what you think. :)

Sounds like a plan to me!

Idea is good but I'm not sure that I can put a lot of time to maintain the issue queue

The module looks not very popular and I heva no ideas about a ways to move this forward but glad to help with reviews cos I'm using it on a few sites

I am waiting this module to be ported to Drupal 8...
Estou esperando esse módulo para o Drupal 8...

I don't think this project is very actively maintained 🙂.

If you want a more modern (entity based) guestbook for Drupal 7 and Drupal 8, I've written up a case study about using the Anonymous Publishing project to emulate the functionality of Guestbook.

Link to case study: http://heim.ifi.uio.no/gisle/staging2/drupalprimer/drupal/cs_guestbook.html

If somebody is willing to sponsor the work, I may consider making it available as a features export.