comment_form_submit():

function comment_form_submit($form, &$form_state) {
  $node = node_load($form_state['values']['nid']);
  $comment = comment_form_submit_build_comment($form, $form_state);
  if (user_access('post comments') && (user_access('administer comments') || $node->comment == COMMENT_NODE_OPEN)) {
    // Save the anonymous user information to a cookie for reuse.
    if (!$comment->uid) {
      user_cookie_save(array_intersect_key($form_state['values'], array_flip(array('name', 'mail', 'homepage'))));
    }

    comment_save($comment);

When editing/moderating a comment from an anonymous user ($comment->uid == 0 && $GLOBALS['user']->uid != $comment->uid), then your author cookie is overwritten with the author information of the author of the comment you are editing.

CommentFileSizeAuthor
#13 1032024-reroll.patch708 bytesandypost
#4 1032024.patch711 bytesgrendzy
#1 1032024.patch685 bytesgrendzy

Comments

grendzy’s picture

grendzy commented
Status: Active » Needs review
StatusFileSize
new 1032024.patch685 bytes
dries’s picture

dries commented
Status: Needs review » Reviewed & tested by the community

This looks RTBC to me, although I haven't tested it. Leaving it RTBC for a bit.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Reviewed & tested by the community » Needs review 
+++ modules/comment/comment.module
@@ -2216,7 +2216,7 @@ function comment_form_submit($form, &$form_state) {
-    if (!$comment->uid) {
+    if (user_is_anonymous()) {

Looked at this patch yesterday already, but wasn't really sure whether we shouldn't extend the old condition instead of replacing it, to make it more explicit.

The current patch should definitely fix the bug, but then again, neither the old nor new condition is 100% self-descriptive. I think I'd prefer to use both conditions.

Powered by Dreditor.

grendzy’s picture

grendzy commented
StatusFileSize
new 1032024.patch711 bytes

ok.

sun’s picture

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

Thank you! :)

dawehner’s picture

dawehner
Primary language German
commented
Version: 7.x-dev » 8.x-dev
Issue tags: +Needs backport to D7
dries’s picture

dries commented
Status: Reviewed & tested by the community » Fixed

Committed to 7.x and 8.x. Thanks!

bfroehle’s picture

bfroehle commented
Issue tags: -Needs backport to D7

Untagging since it's been committed to 7.x

andypost’s picture

andypost
he/him
Primary language Russian
commented

@Dries has commited a patch #1 not #3

D8 http://drupalcode.org/project/drupal.git/commitdiff/12165276c537f8ecf13a...
D7 http://drupalcode.org/project/drupal.git/commitdiff/62b8fd96ffbf0b5477f3...

sun’s picture

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

No cookies, @Dries!

grendzy’s picture

grendzy commented

sun: can you explain a bit more? Are you asking for a new patch that addresses #3, or getting rid of the cookie storage entirely?

Personally I'd prefer this feature use localstorage but that's a different issue. :-)

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

@grendzy: @Dries committed the patch in #1, instead of the patch in #4.

Hence, we now need a patch that re-does #4 but based on latest HEAD... which in turn is a bit borderline to ask for, as it's only clarifying logic in the code (for humans/readers) without substantial logical difference... umph.

Alternatively, @Dries could reverse-apply #1, then apply #4 on top of it, and commit.

andypost’s picture

andypost
he/him
Primary language Russian
commented
Status: Needs work » Needs review
StatusFileSize
new 1032024-reroll.patch708 bytes

Suppose #1 should work because a setting cookie used only for anonymous so when comment is edited by user (UID > 0) this condition is enough.

Attaching a patch against current state of head. I think this gives more readability to code and could prevent troubles with user masquerading modules.

sun’s picture

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

Thanks!

dries’s picture

dries commented

I committed #1 because I didn't understand why #4 was needed. #1 seems more elegant to me.

What is the rationale for going with #4?

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

The rationale was in #3:

The current patch should definitely fix the bug, but then again, neither the old nor new condition is 100% self-descriptive.

The question is not elegance, but readability of code:

+++ modules/comment/comment.module
@@ -2216,7 +2216,7 @@ function comment_form_submit($form, &$form_state) {
     // Save the anonymous user information to a cookie for reuse.
     if (user_is_anonymous()) {
       user_cookie_save(array_intersect_key($form_state['values'], array_flip(array('name', 'mail', 'homepage'))));
     }

To which comment authors (not users) does this apply?

+++ modules/comment/comment.module
@@ -2216,7 +2216,7 @@ function comment_form_submit($form, &$form_state) {
     // Save the anonymous user information to a cookie for reuse.
     if (!$comment->uid) {
       user_cookie_save(array_intersect_key($form_state['values'], array_flip(array('name', 'mail', 'homepage'))));
     }

Does this only apply to anonymous users?

+++ modules/comment/comment.module
@@ -2216,7 +2216,7 @@ function comment_form_submit($form, &$form_state) {
     // Save the anonymous user information to a cookie for reuse.
-    if (!$comment->uid) {
+    if (user_is_anonymous()) {
       user_cookie_save(array_intersect_key($form_state['values'], array_flip(array('name', 'mail', 'homepage'))));
     }

Does it or does it not apply to comments whose author is changed to Anonymous?

--

However, feel free to ignore me. This issue, code, and affected functionality is way too unimportant for getting into fundamental debates about elegance vs. performance vs. readability/clarity vs. style.

Which doesn't mean this discussion wouldn't be überimportant to have, but yeah, there's only so much time. ;)

andypost’s picture

andypost
he/him
Primary language Russian
commented

I think that check for authorship is important because there could be rules/triggers/actions that could be run from Anonymous against authored comments. OTOH this clarifies a reason to save cookie

catch’s picture

catch
he/him
Primary language English
commented
Issue tags: +Needs backport to D7

Tagging for backport.

catch’s picture

catch
he/him
Primary language English
commented
Issue tags: -Needs backport to D7

#13: 1032024-reroll.patch queued for re-testing.

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs backport to D7

The last submitted patch, 1032024-reroll.patch, failed testing.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Toronto, Canada
commented

I seen no cookie in D8 now.
This is from Drupal\comment\CommentFormController::submit()

    // If the comment was posted by an anonymous user and no author name was
    // required, use "Anonymous" by default.
    if ($comment->is_anonymous && (!isset($comment->name->value) || $comment->name->value === '')) {
      $comment->name->value = $this->config('user.settings')->get('anonymous');
    }
andypost’s picture

andypost
he/him
Primary language Russian
commented
Version: 8.0.x-dev » 7.x-dev
Issue summary: View changes
Issue tags: -Needs backport to D7

Comment form does not store cookies anymore in d8

  • Dries committed 1216527 on 8.3.x 
    - Patch #1032024 by grendzy: comment_form_submit() overwrites author/...

  • Dries committed 1216527 on 8.3.x 
    - Patch #1032024 by grendzy: comment_form_submit() overwrites author/...

  • Dries committed 1216527 on 8.4.x 
    - Patch #1032024 by grendzy: comment_form_submit() overwrites author/...

  • Dries committed 1216527 on 8.4.x 
    - Patch #1032024 by grendzy: comment_form_submit() overwrites author/...

  • Dries committed 1216527 on 9.1.x 
    - Patch #1032024 by grendzy: comment_form_submit() overwrites author/...

Status: Needs work » Closed (outdated)

Automatically closed because Drupal 7 security and bugfix support has ended as of 5 January 2025. If the issue verifiably applies to later versions, please reopen with details and update the version.