Zen 6.x-2.x had a convenient class called "comment-by-anonymous" that was added to D7.

In Zen's template.php the code was:

<?php
 
if ($comment->uid == 0) {
   
// Comment is by an anonymous user.
   
$vars['classes_array'][] = 'comment-by-anonymous';
  }
?>

In D7, comment.module has:

<?php
 
if ($comment->uid === 0) {
   
$variables['classes_array'][] = 'comment-by-anonymous';
  }
?>

See the error? $comment->uid is often a string containing the UID of the user who submitted the comment. Which means when the uid is "0" it doesn't === 0, and, thus, the class is never added to the comment template.

Files: 
CommentFileSizeAuthor
#45 drupal.comment-classes.45.patch875 bytessun
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal.comment-classes.45.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots.
[ View ]
#43 drupal8.comment-classes.43.patch8.26 KBsun
PASSED: [[SimpleTest]]: [MySQL] 33,910 pass(es).
[ View ]
#39 comment-classes-1110650-39.patch8.49 KBoriol_e9g
PASSED: [[SimpleTest]]: [MySQL] 33,916 pass(es).
[ View ]
#35 comment-classes-1110650.patch8.48 KBoriol_e9g
PASSED: [[SimpleTest]]: [MySQL] 33,899 pass(es).
[ View ]
#35 comment-classes-onlytests-1110650.patch6.97 KBoriol_e9g
FAILED: [[SimpleTest]]: [MySQL] 33,903 pass(es), 8 fail(s), and 0 exception(es).
[ View ]
#34 comment-classes-1110650-34.patch8.44 KBoriol_e9g
FAILED: [[SimpleTest]]: [MySQL] 33,905 pass(es), 0 fail(s), and 3 exception(es).
[ View ]
#32 comment-classes-1110650-32.patch8.45 KBoriol_e9g
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/comment/comment.test.
[ View ]
#30 comment-classes-onlytests-1110650-30.patch6.83 KBoriol_e9g
FAILED: [[SimpleTest]]: [MySQL] 32,268 pass(es), 1 fail(s), and 0 exception(es).
[ View ]
#30 comment-classes-1110650-30.patch8.34 KBoriol_e9g
FAILED: [[SimpleTest]]: [MySQL] 32,255 pass(es), 1 fail(s), and 0 exception(es).
[ View ]
#28 comment-classes-onlytests-1110650-28.patch6.14 KBoriol_e9g
FAILED: [[SimpleTest]]: [MySQL] 32,268 pass(es), 1 fail(s), and 0 exception(es).
[ View ]
#28 comment-classes-1110650-28.patch7.65 KBoriol_e9g
FAILED: [[SimpleTest]]: [MySQL] 32,269 pass(es), 1 fail(s), and 0 exception(es).
[ View ]
#25 1110650-rollback-d8.patch2.9 KBoriol_e9g
#25 1110650-rollback-d7.patch2.86 KBoriol_e9g
#22 1110650.patch1.19 KBgrendzy
PASSED: [[SimpleTest]]: [MySQL] 33,313 pass(es).
[ View ]
#16 comment-anonymous-classes-1110650-16.patch2.86 KBoriol_e9g
PASSED: [[SimpleTest]]: [MySQL] 32,999 pass(es).
[ View ]
#11 comment-anonymous-classes-1110650-11.patch2.65 KBoriol_e9g
PASSED: [[SimpleTest]]: [MySQL] 33,020 pass(es).
[ View ]
#10 before-comment-patch.png55 KBoriol_e9g
#10 after-comment-patch.png58.41 KBoriol_e9g
#10 comment-anonymous-classes-1110650-10.patch544 bytesoriol_e9g
PASSED: [[SimpleTest]]: [MySQL] 32,997 pass(es).
[ View ]
#6 1110650-comment-classes-6.patch3.22 KBdixon_
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1110650-comment-classes-6.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots.
[ View ]
#3 1110650-03-comment-by-anonymous.patch545 byteslinclark
PASSED: [[SimpleTest]]: [MySQL] 33,633 pass(es).
[ View ]
#1 1110650-1-comment-by-anonymous.patch537 bytesJohnAlbin
PASSED: [[SimpleTest]]: [MySQL] 29,388 pass(es).
[ View ]

Comments

Status:Active» Needs review
StatusFileSize
new537 bytes
PASSED: [[SimpleTest]]: [MySQL] 29,388 pass(es).
[ View ]

And here's a patch.

http://drupal.org/node/306358#comment-1637480

Perhaps you could ask yourself why, since it was your patch that got committed. =)

Free-trolling aside, it was actually introducing by http://drupal.org/node/306358#comment-1012865

I don't see a reason to make this check strict with '===', +1 to this patch

Status:Needs review» Reviewed & tested by the community
StatusFileSize
new545 bytes
PASSED: [[SimpleTest]]: [MySQL] 33,633 pass(es).
[ View ]

This works for me. Tested without the patch and the class does not show, tested with the patch and the class does show.

John Albin's patch was created right around the git migration, so requires the -p0 flag. I'm going to attach a rerolled one just for the ease of the committers, but I didn't change anything that's actually in the patch, so I'm going to RTBC... hope that isn't too presumptuous.

Status:Reviewed & tested by the community» Needs work
Issue tags:+Quick fix, +Needs tests, +needs backport to D7

Heh. Looks good. Not sure whether this is important enough for having to be covered tests? I'd be fine with skipping them and moving on.

...euhm... or actually:

+++ b/modules/comment/comment.module
@@ -2285,7 +2285,7 @@ function template_preprocess_comment(&$variables) {
   // Gather comment classes.
-  if ($comment->uid === 0) {
+  if ($comment->uid == 0) {
     $variables['classes_array'][] = 'comment-by-anonymous';
   }
   else {

The else means that we're actually outputting completely off CSS classes right now.

So, in that case, we should add some very basic assertions for this. I'm sure we have an existing test case suitable to squeeze them in.

20 days to next Drupal core point release.

Issue tags:+Novice

We need tests and validate other comment classes like:

...
    if ($comment->uid === $variables['node']->uid) {
      $variables['classes_array'][] = 'comment-by-node-author';
    }
    if ($comment->uid === $variables['user']->uid) {
      $variables['classes_array'][] = 'comment-by-viewer';
    }
...

Tagged as Novice, if no one is interested with the work I will do in a few days.

Status:Needs work» Needs review
StatusFileSize
new3.22 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1110650-comment-classes-6.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots.
[ View ]

Here is a patch that contains a first set of tests. I haven't added tests for all HTML classes yet.

I also changed the other conditionals from === to == since I believe the same could happen with those checks, no?

I added most of the test to CommentInterfaceTest since I though that made most sense. But there are no tests for anonymous comments there, so the check for that class is in CommentAnonymous.

Just applied and confirmed that comment-by-anonymous. comment-by-anonymous, comment-by-node-author, and comment-by-viewer are all appearing in the correct areas. If there are any additional tests, let me know.

Status:Needs review» Needs work
Issue tags:+Quick fix, +Needs tests, +Novice, +needs backport to D7

The last submitted patch, 1110650-comment-classes-6.patch, failed testing.

StatusFileSize
new544 bytes
PASSED: [[SimpleTest]]: [MySQL] 32,997 pass(es).
[ View ]
new58.41 KB
new55 KB

I have created two users (oriol and user1) and tested all the classes in comments with oriol, user1 and anonymous user.

It's seems that comment-by-node-author and comment-by-node-viewer are working fine without patching and only comment-by-anonymous it's not working without patching, so, the other changes are not needed.

Status:Needs work» Needs review
StatusFileSize
new2.65 KB
PASSED: [[SimpleTest]]: [MySQL] 33,020 pass(es).
[ View ]

Reroll with tests in #6.

Status:Needs review» Reviewed & tested by the community

I can confirm that #6 works. All 3 classes (comment-by-anonymous, comment-by-node-author, and comment-by-viewer) are appearing in their proper locations.

If there are any other items to test, let me know.

Status:Reviewed & tested by the community» Needs review

@rickmanelius both work #6 and #11, and both pass the tests, and both adds the three classes in correct places.

Actually $comment->uid is the string "0" for anonymous users and an integer for authenticated users. To solve this we have 3 options:

1. Commit a rerolled #6 for flexibility (we accept always both, integer and string numbers).
2. Commit #11, the strict fix for this issue.
3. Investigate why anonymous UID it's and string an cast to integer.

I would think the patch in #11 would be good to commit as it fixes the specific issue listed in this ticket. And additional tickets could be opened, particularly for the questions posed in option 3. But I defer to others...

Status:Needs review» Needs work

+++ b/modules/comment/comment.test
@@ -331,6 +331,8 @@ class CommentInterfaceTest extends CommentHelperCase {
+    $by_viewer_class = $this->xpath('//div[contains(@class, "comment-by-viewer")]');
@@ -417,6 +419,13 @@ class CommentInterfaceTest extends CommentHelperCase {
+    $by_node_author_class = $this->xpath('//div[contains(@class, "comment-by-node-author")]');
@@ -963,6 +972,8 @@ class CommentAnonymous extends CommentHelperCase {
+    $anonymous_class = $this->xpath('//div[contains(@class, "comment-by-anonymous")]');

These xpaths should make sure that they are not mistakenly yield a false-positive -- as we have the comment IDs readily available, the xpath should use them as additional selector/conditions.

9 days to next Drupal core point release.

Status:Needs work» Needs review
StatusFileSize
new2.86 KB
PASSED: [[SimpleTest]]: [MySQL] 32,999 pass(es).
[ View ]

Changes from #11:
- xpath updated with the comment id.
- Logout/Login in comment 5 generation removed because it's not needed.

Novice question here :) how would one check the xpath changes in #15/#16? I can easily test if the classes are appearing and use a custom module to export the comment object/array using dpm... but would would I be looking for?

Status:Needs review» Reviewed & tested by the community

Patch fixes the problem and issues reviewed by sun in #15. Test passes. Code looks good, imo.

Version:8.x-dev» 7.x-dev

Committed to 8.x, moving to 7.x for webchick.

Status:Reviewed & tested by the community» Fixed

Looks good. Committed and pushed to 7.x. Thanks!

Status:Fixed» Closed (fixed)

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

Version:7.x-dev» 8.x-dev
Status:Closed (fixed)» Needs review
StatusFileSize
new1.19 KB
PASSED: [[SimpleTest]]: [MySQL] 33,313 pass(es).
[ View ]

Whoops, this commit broke class="comment-unpublished" for anonymous comments. After upgrading to Drupal 7.10, admins moderating anonymous comments will not get the unpublished class.

Can we add some tests for this cases?

Priority:Normal» Critical
Status:Needs review» Needs work
Issue tags:-Quick fix

mmm, bumping to critical.

I'd suggest to only roll the previous patch back if we don't manage to fix it properly until Jan 31st.

Also, this means we need way more and more granular tests here. The tests need to assert every single permutation of the expectations we have. AFAICS, that's:

$parameters = array(
  'comment_uid' => array(0, 2),
  'node_uid' => array(0, 2, 3),
  'user_uid' => array(0, 2, 3),
);
$permutations = $this->generatePermutations($parameters);

(whereas 2 refers to $this->web_user->uid, and 3 needs to be a new $this->other_user->uid)

Furthermore:

+++ b/core/modules/comment/comment.module
@@ -2143,19 +2143,19 @@ function template_preprocess_comment(&$variables) {
     if ($comment->uid === $variables['node']->uid) {
...
     if ($comment->uid === $variables['user']->uid) {

So the originally reported issue even continues within the very function and just some lines below...?

+++ b/core/modules/comment/comment.module
@@ -2143,19 +2143,19 @@ function template_preprocess_comment(&$variables) {
+  // Published class is not needed. It is either 'comment-preview' or 'comment-unpublished'.

Let's also make this comment wrap at 80 chars, while we're here.

StatusFileSize
new2.86 KB
new2.9 KB

Ok. First of all this is a rollback patch for D7 & D8. In the next days I will try to add a better tests coverage.

@sun $comment->uid === 0 sometimes fails because anonymous uid is not always casted to an integer and 0 (integer) is equal (==) than 0 (string) but not identical (===).

$comment->uid === $variables['node']->uid always works because the two values are casted to the same var type.

However, I think that in this case is safer use always equal (==) operator than identical (===).

Assigned:Unassigned» oriol_e9g

Status:Needs work» Needs review
StatusFileSize
new7.65 KB
FAILED: [[SimpleTest]]: [MySQL] 32,269 pass(es), 1 fail(s), and 0 exception(es).
[ View ]
new6.14 KB
FAILED: [[SimpleTest]]: [MySQL] 32,268 pass(es), 1 fail(s), and 0 exception(es).
[ View ]

First attend with a more granular tests suit. Now testing all possible permutations for comment-by-anonymous, comment-by-node-author, comment-by-viewer and comment-unpublished.

Only tests patch should fail for comment-unpublished tests, full patch should pass.

Status:Needs review» Needs work

The last submitted patch, comment-classes-1110650-28.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new8.34 KB
FAILED: [[SimpleTest]]: [MySQL] 32,255 pass(es), 1 fail(s), and 0 exception(es).
[ View ]
new6.83 KB
FAILED: [[SimpleTest]]: [MySQL] 32,268 pass(es), 1 fail(s), and 0 exception(es).
[ View ]

More, now tests for comment-by-anonymous, comment-by-node-author, comment-by-viewer, comment-unpublished and comment-new.

Comment-unpublished and comment-new tests should fail, full patch should pass.

In my localhost:
- Only tests patch: Test comment user interfaces. 2019 passes, 12 fails.
- Full patch: Test comment user interfaces. 2031 passes, 0 fails.

Status:Needs work» Needs review

Help? Testbot it's failing here...

+    $parameters = array(
+      'comment_uid' => array(0, $this->web_user->uid),
+      'node_uid' => array(0, $this->web_user->uid, $this->admin_user->uid),
+      'user' => array(drupal_anonymous_user(), $this->web_user, $this->admin_user),
+      'comment_status' => array(COMMENT_PUBLISHED, COMMENT_NOT_PUBLISHED),
+    );
+    $permutations = $this->generatePermutations($parameters);
+
+    foreach ($permutations as $case) { ...

Assigned:oriol_e9g» Unassigned
StatusFileSize
new8.45 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/comment/comment.test.
[ View ]

Status:Needs review» Needs work

The last submitted patch, comment-classes-1110650-32.patch, failed testing.

Status:Needs review» Needs work
StatusFileSize
new8.44 KB
FAILED: [[SimpleTest]]: [MySQL] 33,905 pass(es), 0 fail(s), and 3 exception(es).
[ View ]

Status:Needs work» Needs review
StatusFileSize
new6.97 KB
FAILED: [[SimpleTest]]: [MySQL] 33,903 pass(es), 8 fail(s), and 0 exception(es).
[ View ]
new8.48 KB
PASSED: [[SimpleTest]]: [MySQL] 33,899 pass(es).
[ View ]

I think that this is good :/ I was going crazy with a fatal error that couldn't reproduce.

Status:Needs review» Needs work

The last submitted patch, comment-classes-onlytests-1110650.patch, failed testing.

Status:Needs work» Needs review

Review! #35 prove the bug and the solution! xD

Issue tags:-Needs tests, -Novice

If nobody checks this patch in 2 days (D7.11 release date) I will downgrade the priority to normal.

Note that this is critical because we introduce a regression in D7.x, so, D7.10 works fine with html classes: comment-by-node-author, comment-by-viewer, comment-unpublished and comment-new. We fix and commit the comment-by-anonymous class in #20 but with the fix we have lost the comment-new and comment-unpublished classes for anonymous users, because they was using the bug to appear in D7.10.

To fix properly would be desirable commit the patch in #35 to avoid the regression in D7.11 or rollback the commit with the patch in #25, and fix properly in D7.12.

StatusFileSize
new8.49 KB
PASSED: [[SimpleTest]]: [MySQL] 33,916 pass(es).
[ View ]

Small comment typo fix, no code changes. Failing tests to prove the bug in #35.

Tagging this as a D7 stable release blocker.

Status:Reviewed & tested by the community» Needs review

Did a review and patch looks good. Tested this manually on D8 and an unpublished comment made by an anonymous user gets the classes comment-unpublished, comment-new and comment-by-anonymous.

Status:Needs review» Reviewed & tested by the community

Assigned:Unassigned» webchick
Status:Needs review» Reviewed & tested by the community
StatusFileSize
new8.26 KB
PASSED: [[SimpleTest]]: [MySQL] 33,910 pass(es).
[ View ]

Cleaned up the code and comments.

Priority:Critical» Normal
Status:Reviewed & tested by the community» Fixed

Thanks a lot for the really comprehensive tests in this issue.

The one part I had a question about was changing === to == in a few of these conditionals, but sun explained on IRC that this was done because these objects come out of the database with values like '1'. Bleh.

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

Version:8.x-dev» 7.x-dev
Status:Fixed» Reviewed & tested by the community
StatusFileSize
new875 bytes
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal.comment-classes.45.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots.
[ View ]

Follow-up fixes for D7 to unbreak broken HEAD. :)

Tested manually.

Status:Reviewed & tested by the community» Fixed

Great, thanks for the fast turnaround!

Committed and pushed to 7.x. Let's see if testbot likes us better now. :)

Nice job folks, thanks! While we're here, I want to crosslink #1006042: Links outside the overlay with existing fragments are broken also. Combined with this fix, it's actually possible to moderate comments from the dashboard overlay. :-)

Status:Fixed» Closed (fixed)
Issue tags:-needs backport to D7, -D7 stable release blocker

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