Simple & anonymous comment system, Movable Type style.

The misc field in the database looks like it is serialized. Serialized data in the database usually causes annoyances (what if you want to list all the people who have anonymously commented with identity, or search them). Breaking it out into separate columns should be preferred.

This data might even belong in a separate table: it would save space because these values would be blank for logged in users and it could avoid storing duplicate data for not logged in users who have the same identity (such as comments from the same anonymous session).

I wonder if we need the fields configurable/extensible -- it is in Drupal's interest to encourage people to create a normal account where you can define what fields to add and remove. What fields would you like to add?

I suggest to add the aforementioned fields (name, e-mail, URL) to the comment table, to normalize the data and to drop the exstensibility in favor of performance. We've put a lot of effort into optimizing the comment module as there are quite a few Drupal sites with large comment threads.

We could also look into ways to ease registration and to create a better synergy between both systems. I'm mentioning this, because I feel this needs to be discussed thoroughly. For one, we must make sure (registered) users can't (easily) be impersonated.

I haven't tried this patch but it looks good. Here are some suggestions:

1. I'd rename all instance of '...authorname...' to 'name', all instance of '...authoremail' to 'mail' and all instance of '...authorhomepage...' to 'homepage'. That is, the SQL tables, the variables and the forms.
2. I don't think the comment pages validate as XHTML. From the looks, you forgot to remove a trailing <div> in: $output .= t("by %a on %b", array("%a" => theme("comment_author", $comment), "%b" => format_date($comment->timestamp))) ."</div>\n";
3. I don't think these parentheses should be part of the translatable string: $output .= $name . t(" (identity not verified)");
4. Instead of trying to fix up broken URLs in _comment_format_url() I urge you to add some input checking. There are functions in includes/common.inc that you can use to validate URLs and e-mail addresses. Please do.

1. The way the username defaults to 'anonymous' is not correct. You should respect the 'anonymous username' setting by using variable_get("anonymous", "Anonymous").
2. Not sure I understand how the error reporting (not the error handling) works. Typically, we use drupal_set_message($message, 'error') to output an error message or display the error message with the form element through the form item's $description parameter.
3. As for the comment listing. The former is confusing, the latter somewhat sloppy. I'd display the author's name and omit his e-mail address and URL. Don't make the username a link or it will be confused with registered users.

Looks good to me but:

1. Change 'email' to 'mail' for sake of consistency with the other mail-fields. See the users table for example; we use 'mail' elsewhere.
2. Provide updates for database/database.mysql and database/updates.inc.
3. Try using single qoutes around constant strings. Preferred, not required.
4. There is no point creating tiny _form_comment_xxx() functions when they only have one call site.

I tried your patch and had to fix a number of bugs to get it semi-working. New patch attached.

Still, it is not working correctly yet.

1. The error reporting should not only happen after one clicked 'Post' but also after one clicked 'Preview'. When posting failed, the form should be presented along with an explanation of what's wrong. The form fields should not be emptied.
2. When I posted as anomyous user, my name and e-mail address are stored properly in the database, yet it doesn't get shown anywhere. I'm using the 'Xtemplate default' theme.
3. After posting a comment, we used to display a message that the comment has been posted. This is no longer the case. I don't think your patch broke this but I figured I would mention it nonetheless.

Getting there ...

Updates:

1. My anonymous username and e-mail address are shown when the comments are collapsed. They are not shown when the coments are unfolded.
2. I was surprised to see that my e-mail address was publicly shown. This was confusing because (1) I added the URL of my homepage and would expect that to take precedence and (2) I don't want my e-mail address to be shown. I'm not opposed to showing one's e-mail address but it should be communicated to the user.

This is a very important feature request to me!

I've been hacking away at an alternate version of this feature for a while, now...

I don't know if this can give you any ideas, but please check out: http://www.davidnunez.com/blog/title/Anonymous+Comments+Hack

The main difference is that I modified format_name() way up in "includes/common.inc" to look for anonymous user uid=0 and substitute in a name and url link if they existed... That helped simplify the number of changes I needed to make in other places.

My hack has been working fairly solidly, but it's not as generalized as it could be.

I absolutely believe that we need the option to have the user's name link to their url, their email address, both, or nothing at all (if you're concerned about people confusing anonymous members with registered members). This should be a four-way toggle in the Admin menu.

I like the approach you are taking here, so far. I'm looking forward to testing this out.

1. It is probably a good idea to patch format_name() and to route all 'name displaying' through format_name() so we don't have to update all themes. I'm not sure it is feasible but if it can be done cleanly, I'm all for it.
2. The comment validation needs work, especially now more things can go wrong. When the user's comment is incorrect or incomplete, an error message is shown but his comment is lost. If you don't know how to fix this behavior, we can work on that later.

I committed a slightly modified version of your patch to HEAD. Thanks. I'm marking this report 'active'.

I modified format_name() to behave differently in absence of $user->uid. Hopefully we can sort out the remaining issues (eg. lack of warning about what will happen with their e-mail address, lack of communication as to what are required fields, error handling of incorrect comment submissions, and post submission handling) in a timely manner.

There also seems to be another problem. Using tonight's CVS , if a user leaves an anonymous comment, their name is linked to the profile URL of the content author (such that clicking on the commenter's name would give you the author profile of the content).

#31 is irrelevant now - latest CVS fixed it.

Seeing something else though. I can't, for the life of me, get any comment to work, regardless of whether it's anonymous or authenticated. All the comments show up in the watchdog as being added, and I can "view details" no problem. However, when I click "view comment", they never show up in the page (regardless of threaded, a reply to an existing comment, or a new comment on the post). Finally, when an auth'd user "previews" his comment (which is required on my test installation), the name/info shows up as "Anonymous" in the preview form.

Pablo's comment_0.diff (#35) fixed my #32. Changing status to patch for commit.

When I apply this patch, the anonymous user names always show up as 'Anonymous' while the information is stored properly in the database (the link to the homepage is correct though). Without this patch, the correct usernames are shown. Looks like I'm having the opposite problem ... ?

Heh, heh. Yay! :)

With a clean CVS checkout today, authenticated user comments show up as "Anonymous", and anonymous comments show up correctly (with or without homepage links, and with the "(not verified)") addition. With the patch applied, everything shows up as I expect, including previews.

How can we help each other test and clarify further?

I've attached a patch that fixes the redirect issues mentioned on drupal-devel, reprinting here for onlookers: "After submitting a comment, you are not directed to the correct page. The user should see that his or her comment is being posted so it is better to redirect the user to 'node/view/x'. This bug is not introduced by Pablo's patch." The patch also fixes a similar incorrect redirect issue when the user is modifying their comment view settings (you know, the whole "do I want to see it threaded?" form).

I'm not sure if I'm using drupal_goto correctly here, especially since I'm "hardcoding" the "node/view" string. What happens when the user has used an alternate path (as per the path.module)? I suspect this will be transparent, but if the user has requested the alternate path, shouldn't they return to the alternate path? The only other places I could find that used drupal_goto with hardcoded values was the forum.module: 456: drupal_goto("node/view/$nid"); and user.module: 901: drupal_goto("user/edit");

I can reproduce the problem now - comments by authenticated users are indeed shown as anonymous. Applying the patch fixes this behavior but breaks the anonymous comments. Test against a hybrid comment thread that uses both comment styles and you'll see. A better patch is needed.

Code-wise I think it is better to test against $comment->uid than to test against $comment->name. At least it is slightly more consistent with the rest of the code.

The redirect patch looks like an improvement as the use of comment_node_url() appears to be broken. I suggest we remove the last instance of comment_node_url() too as well as the function itself.

Christina suggested we should redirect the poster to his or her comment so he or she can see the comment being posted. Ideally, we would be redirecting to node/view/<em>xxx</em>#comment-<em>yyy</em>. Looking at the code, this might need a bit more work so maybe that should be postponed until we start working on the 'Preview' and 'Submit' code (to improve the interaction design of the error handling).

The current comment redirect works with remembering the previous page in the session. But if the user navigates on multiple pages, then the session will contain false information, and the redirect will be odd. This is also an issue in 4.4. The reason for having non hard-coded redirects is that the book page or the forum page has different URLs for the nodes, and if we redirect the user to the node/view page, then it will not be the book or forum node view page, which the user might started to add the comment on.

I am myself fine with hard coded redirects to node/view, as my site has unique URLs for content (or at least it is not supposed to display differently with different URLs). Some people using the book module might find this approach broken. But the current comment redirect method is also broken anyway.

Re: #40. Still can't duplicate what you're seeing, unfortunately. My testbed server (on a roving DSL IP address, so fear the reset) appears to be displaying fine: http://68.237.140.171:8888/?q=node/view/1.

Goba: quite true. One possible way of fixing this would be to include the proper redirect location as a hidden value in the form itself (much like we pass nid, pid, and cid in theme_comment_form). It's a similar approach to comment_node_url, only the form values tell us where to go, as opposed to the query string. There doesn't seem to be any other magical way, within the code itself, to determine the proper URL to redirect to, since all URLs and params received at time of posting are comment/reply related, and not content type related (I'm ignoring costly determinations like $node->type, which would be another possibility).

Would that work for all circumstances? If I have time today, I'll whip out a patch for it. The big, big, big, downside to this approach is templates that have themed _comment_form will need to be updated to include the new hidden field.

Morbus: the hidden field approach was suggested before, but as far as I remember, it was said to be unfriendly to the caching mechnanisms, so if someone uses page caching, the behaviour will still be odd. It can only be made cache friendly, if the previous URL can be stored in a GET param, since the request URI is used to index the cache. But that is quite ugly.

I am not convinced that separate comment submission pages should be cached, but I don't know a good way to exclude them from caching. Having the hidden field on node pages with the comment form displayed would not be a problem, as the hidden field would contain the URL of the node page itself. This is only a problem for pages generated exclusively for comment submission.

Sigh, in fiddling, I fixed the problem Goba mentioned, but there was still an unsolved bug - the "add new comment" link for nodes that had no comments didn't play nicely with my hopes that the proper node type would be passed in the URL at some point (viewing a node THEN replying/adding, everything worked dandy). So, ignore my #45; it's not a valid solution. What is the cost of adding a "get node type" SQL, solely for the redirect?

Gah - didn't see Goba's reply before I added my own. My failed approach was based on the GET - a new 'type' hidden field was added to the form based on arg(0), and that was used to generate the final URL for drupal_goto. I've attached the (unfinished, unworking, not for commit) patch that demonstrates that technique. As I mentioned, however, it fails for "add new comment" links on, for example, the main page. Since the "add new comment" link went directly to comment/reply/3, arg(0) was never set properly (compared to viewing the node first, THEN adding/replying).

I'll next experiment with grabbing the $nid type from the database. That wouldn't be based on anything exterior (thus, ignoring caching methods), nor would it require the xtemplate to change (as per the attached patch). The downside to this one, however, is the extra cost of a SQL statement.

When you submit a comment, you know the $nid, no?

Right. So the approach of the in-progress patch is to get $nid's $type, and base the URL around "$type/view/$nid". In this approach, I'm worried about my ignorance of the rest of Drupal's content types. What is the type of a project node? Of a forum node? Of all the different contributed node modules? Is basing the URL on the $nid $type stored in the DB a folly? Still experimenting.

$type/view/$nid will not work. There is no such URL as story/view/$nid for example. It would basically be fine to redirect to node/view/$nid if the nodes would not have multiple possible views. A node can be in a book (regardless of the type of the node), and the book view of the node lives under a different URL then node/view/$nid. Now if you approach the comment form from the book view, you expect the system to get you back there. That cannot be computed from the node type, since there are multiple URLs for one node, that is the problem.

Dries wrote: I can reproduce the problem now - comments by authenticated users are indeed shown as anonymous. Applying the patch fixes this behavior but breaks the anonymous comments. Test against a hybrid comment thread that uses both comment styles and you'll see. A better patch is needed.

I repro'd this by going from 4.4.1 to HEAD this morning, and I see what you mean.

But: Pablom's patch in #35 fixed this for me, and didn't break anonymous comments... at least not with this morning's checkout. Checked against all comment threading styles using HEAD's xtemplate theme's default style. Sorry if I'm missing something here, but it looks like it works (this aspect of it anyway) to me.

FWIW, it's: http://441tohead.lineofsight.org/ user/pass: admin

Goba - thanks for your intellect.

Giving up on the SQL/type idea, I went back to the closest "solution" I had: the hidden form field mentioned in #44 through #47. The problem with that one was that there was no way to know what /type/ of node was being commented on if the user clicked on a "add new comment" link from a main page or similar equivalent - we went directly to comment/reply/$nid, as opposed to something like book/reply/$nid.

What the attached patch does is, in the absense of a GET hint, we default to "node". For the most part, this seems to work correctly from a template/visual standpoint (for Goba's specific example of a book template compared to a generic node template, I could find no evidence of the default book templates offering "add new comment" from an index/contents page without "book" in the URL. Regardless, a book page promoted to the main page of the site isn't much of a problem, as the user would be in the generic node template on the main page, and wouldn't know they're not seeing the book template after a comment redirect).

The downside of all this is that we've added one new hidden form field to three forms (moderation, comment preferences, and the comment form), and thus, customized templates that don't have those fields will default to the same erroneous behavior that prompted this patch (which isn't any worse than what they're already used to, however).

Some further notes about the patch: Dries, even though I've removed all mention of comment_node_url from the case statement, I didn't remove the function - there are two other places in the code that use it, and I didn't investigate them enough to see if they have the same sort of problem with redirects. Finally, this patch quickly (and probably dirtily) implements Christina's #comment-yyy suggestion, first by modifying code_post to return the comment ID. The case statement now checks to see if comment_post returns an array (in which case, an error occurred); any other return value is assumed to be the $cid, and we use that as a fragment to drupal_goto.

Dries: would you commit a patch that turned "name", "mail" and "homepage" to "anon_name", "anon_mail", and "anon_url", as per jseng's "Drupal for Bloggers" patch? As per my comments, I think they're a much stronger choice of field names.

I'll give your patch some thought: maybe our best bet is to get rid of the multiple views (i.e. nuke book/view/nid). It is handy but confusing. Not sure it is legitimate to remove.

An alternative - but probably not easy to implement with the menu system - might be to change the URL scheme from comment/reply/nid/cid to something like book/view/nid/cid/reply or book/reply/nid/cid.

Personally, I'm not really in favor of the anon_ prefix.

Hmm, sounds like our database schema is wrong, if that is how nodes are being treated.

That is, our database insists that there is one and only one node type associated with a node (i.e. node.nid => node.type). If I understand Goba's response to Morbus, a single node (node.nid) can actually belong to several types -- for example, a story and a book.

If this is true, then I think we will continue to have these sorts of problems which are difficult to solve in a clean fashion until the database scheme is changed to match how it is used -- and code is changed to use the database in a consistent, API-like manner.

Hacking clever new features in by overloading database field meanings will almost always guarantee significant structurally intransigent problems in the future.

The book is some kind of cummulative stuff, which can contain any type of node, so it is quite versatile in what it can contain. A node still has one type, but as it can be part of more than one vocabulary, it can also be part of a book vocabulary, which means that it gets another URL to access it. It does not mean it has multiple types, it is just classified into different vocabularies in the taxonomy.

I committed patch #35 by Pablo to HEAD until others can reproduce my problem.