if no author is passed, comment module will try to use currently logged in user as the author. But, if we want to make comment Anonymous then we have to modify it in the db instead of on the UI.

FYI: in D6, on comment editing, if no author passed, then the comment is assigned to Anonymous.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

webchick’s picture

Issue tags: +Needs tests

Hm. That sounds like a regression. We should add tests for this.

Damien Tournoud’s picture

Version: 7.0-beta1 » 7.x-dev

Confirmed.

bleen’s picture

This patch fixes the original issue (no tests yet) however I don't know that this is necessarily the correct place for this code... not sure.

Also, while working on this I found that there are a couple of other things going on. Comments author's cannot be changed to Anonymous, nor can they be changed from Anonymous to a registered user. In fact, if a comment is authored by anon and an admin goes to edit the comment, the "Authored by" field is not an auto-complete field.

bleen’s picture

Title: Cannot override comment author back to Anonymous » Cannot override comment author to or from Anonymous
Status: Active » Needs review
FileSize
1.53 KB

patch

bleen’s picture

FileSize
1.68 KB

Last patch still had dsm()s

bleen’s picture

FileSize
3.35 KB

ok ... this patch is closer but definitely needs work (seeing what testbot has to say though, so needs review). It allows a comment author to be changed to anonymous but if you try and change from anonymous to a registered user, it still lists the post as "not verified"

my head hurts ... sleep

sun.core’s picture

bleen’s picture

#777372: Regression: anonymous comments cannot be edited on Drupal 7 was fixed and committed in April 2010 ... this wasnt even opened till Oct 2010 .. so everything that has been tested/done on this has been with a version of HEAD that includes the fix from the other issue

bleen’s picture

#6: comment.patch queued for re-testing.

sun’s picture

This should be tackled after #632382: "Your name" should not be prepopulated with Anonymous, which solves the first half of the puzzle already.

yngens’s picture

This issue had been solved for 6.x if anyone interested on http://drupal.org/node/129822#comment-3169748

catch’s picture

Status: Needs review » Needs work

Looks like this still needs work.

bleen’s picture

Status: Needs work » Needs review
FileSize
3.87 KB
5.75 KB

Every time I try and enable the Simpletest module with the lates version of HEAD I get a PDO error (http://drupalbin.com/17147) so I'm attaching two patches: one with tests and one without. I'm fairly certain the tests will fail because I have how no way to try running them yet but you never know...

bleen’s picture

FileSize
2.04 KB

... and here is a patch with just the tests :)

Status: Needs review » Needs work

The last submitted patch, comments-test.patch, failed testing.

bleen’s picture

Status: Needs work » Needs review
FileSize
5.75 KB

Ok ... this patch (which includes the new tests) passes all tests locally

sun’s picture

Status: Needs review » Needs work
+++ modules/comment/comment.module	1 Jan 2011 19:34:43 -0000
@@ -1931,11 +1931,9 @@ function comment_form($form, &$form_stat
+      '#description' => !empty($comment->cid) ? t('Leave this field blank to make this an anonymous comment.') : '',

Please use the same #description that is used for $form['author']['name'] in node_form().

The additional !empty($comment->cid) conditional is superfluous, since this code path is only reached with $is_admin, which already contains that condition.

+++ modules/comment/comment.module	1 Jan 2011 19:34:43 -0000
@@ -2123,21 +2121,22 @@ function comment_form_validate($form, &$
   }
-
-  // Validate anonymous comment author fields (if given).
-  if ($form_state['values']['is_anonymous']) {
...
+  else {
+    // Validate anonymous comment author fields (if given).
+    if ($form_state['values']['is_anonymous']) {

Minor: We can simply change the if into an elseif instead of indenting the entire code block.

Not visible in the diff context lines is the leading if (!empty($form_state['values']['cid'])) condition.

This means that the check for registered usernames is only performed for new comments from anonymous users. While that sounds about right, we need to double-check the impact of this change on the functional behavior when the edit own comments permission is granted to anonymous users.

We need to make sure that we do not re-introduce #845774: Regression: Anonymous users can post comments in the name of registered users

+++ modules/comment/comment.module	1 Jan 2011 19:34:43 -0000
@@ -2217,6 +2216,18 @@ function comment_form_submit($form, &$fo
+    // If the name field has been emptied then make this an anonymous comment.
+    if (empty($comment->name)) {
+      $comment->uid = 0;
+      $comment->name = variable_get('anonymous', t('Anonymous'));
+    }
+
+    if ($comment->cid) {
+      // Verify the name in case it is being changed from being anonymous.
+      $user = user_load_by_name($comment->name);
+      $comment->uid = $user ? $user->uid : 0;
+    }

This should already happen earlier, in comment_form_validate(), so form validation and submit handlers of other modules can rely on the values.

I do not understand why these are two separate code blocks/conditions (and the comments don't explain either). It looks like only the second one is actually needed, whereas the condition as well as assignment of 'name' of the first should be merged in.

Lastly, never use $user, always use $account.

Powered by Dreditor.

bleen’s picture

Status: Needs work » Needs review
FileSize
4.82 KB

This should address Sun's review in #17

Status: Needs review » Needs work

The last submitted patch, comments.patch, failed testing.

bleen’s picture

huh?! ... /me goes to see how this could possibly effect the sessions tests

bleen’s picture

Status: Needs work » Needs review

#18: comments.patch queued for re-testing.

yngens’s picture

I believe one of these two nodes should be marked as duplicate to concentrate all the efforts to solve the issue at one place:

#936844: Cannot override comment author to or from Anonymous Posted by skyredwang on October 9, 2010 at 8:04am and has 21 comments to the moment
#129822: allow admins to assign anonymous comments to registered users Posted by ax on March 21, 2007 at 8:22am and has 38 comments to the moment

sun’s picture

Status: Needs review » Reviewed & tested by the community

Thanks, @bleen18! This looks sufficiently documented, tested, and ready to fly for me now.

wojtha’s picture

Version: 7.x-dev » 8.x-dev
Status: Reviewed & tested by the community » Needs work
Issue tags: -Needs tests

Now the patch needs to be rerolled against D8 in the first place...

sun’s picture

Status: Needs work » Needs review

#18: comments.patch queued for re-testing.

wojtha’s picture

Status: Needs review » Reviewed & tested by the community

Returning to RTBC state by sun (#23) since patch is ok for D8 too.

catch’s picture

Issue tags: +Needs backport to D7

Tagging for backport.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

This looks good, and the tests are perfect and very readable. Committed to 7.x and 8.x. Yay! Thanks all.

skyredwang’s picture

Status: Fixed » Needs work

The latest commit has a new bug: A comment cannot be assigned to an anonymous author with custom name on comment.create(), but this works on comment.update().

To reproduce, create a comment and fill a random string as name (not a registered username), the error will come out. On the other hand, create a comment, and leave the name blank, save, then edit the comment again, and fill a random string as name, then it works!

sun’s picture

If someone can reproduce what @skyredwang reported, then this issue must be bumped to 'critical' and block a 7.1 release (add the 'Release blocker' issue tag). I don't really grok what @skyredwang reported.

bleen’s picture

Issue tags: +Release blocker

I can reproduce, but i dont know if Ill have time this week to figure out why... Ill take a look next week if no one gets to it first

webchick’s picture

We're going to need a fix (and expended test coverage) for this in the next 24 hours or so, else I'm going to have to roll this patch back for 7.1.

bleen’s picture

Status: Needs work » Fixed
Issue tags: -Release blocker

Ok .. I took a few minutes tonight to look into this and there isn't actually a bug here. The situation that skyredwang describes in #29 is actually a conscious decision that we made. Webchcick and I discussed in IRC and she agrees ....

Take the following (common) situation: a user comments as an anonymous user (lazy or forgot pw...) and then wants to re-associate their comment later with the appropriate user so it appears correctly in tracker (etc...). That is precisely the behavior we are allowing in #29's example. We have no way of knowing if a user with "edit comments" perms (admin 99% of the time) is changing the name to a valid username intentionally (to correctly attribute it) or unintentionally (not knowing there is already a user named "bleen").

This is the same behavior we have in d6 (I'm 99% sure) and IMO its correct.

Skyredwang - if you want to make the case to change this behavior I'd say you should open a new D8 issue and reference this issue for background...

skyredwang’s picture

I have a use case: with the current logical flow: when migrating a site (D5/D6/Joomla/whatever) with comments to a D7 site, the migration has to create all these comments with no names, then update the same comments and provide the missing names. Does this use case happen often? I don't know.

sun’s picture

@skyredwang: A migration would programmatically create and update comments. The code in question here only affects form submissions.

Status: Fixed » Closed (fixed)
Issue tags: -Needs backport to D7

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