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:

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

In D7, comment.module has:

  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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

JohnAlbin’s picture

Status: Active » Needs review
FileSize
537 bytes

And here's a patch.

franz’s picture

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

Anonymous’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
545 bytes

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.

sun’s picture

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.

oriol_e9g’s picture

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.

dixon_’s picture

Status: Needs work » Needs review
FileSize
3.22 KB

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.

rickmanelius’s picture

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.

oriol_e9g’s picture

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.

oriol_e9g’s picture

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.

oriol_e9g’s picture

Status: Needs work » Needs review
FileSize
2.65 KB

Reroll with tests in #6.

rickmanelius’s picture

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.

oriol_e9g’s picture

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.

rickmanelius’s picture

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...

sun’s picture

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.

oriol_e9g’s picture

Status: Needs work » Needs review
FileSize
2.86 KB

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

rickmanelius’s picture

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?

dixon_’s picture

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.

catch’s picture

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

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

webchick’s picture

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.

grendzy’s picture

Version: 7.x-dev » 8.x-dev
Status: Closed (fixed) » Needs review
FileSize
1.19 KB

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.

oriol_e9g’s picture

Can we add some tests for this cases?

sun’s picture

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.

oriol_e9g’s picture

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.

oriol_e9g’s picture

@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 (===).

oriol_e9g’s picture

Assigned: Unassigned » oriol_e9g
oriol_e9g’s picture

Status: Needs work » Needs review
FileSize
7.65 KB
6.14 KB

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.

oriol_e9g’s picture

Status: Needs work » Needs review
FileSize
8.34 KB
6.83 KB

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.

oriol_e9g’s picture

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) { ...
oriol_e9g’s picture

Assigned: oriol_e9g » Unassigned
FileSize
8.45 KB

Status: Needs review » Needs work

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

oriol_e9g’s picture

Status: Needs review » Needs work
FileSize
8.44 KB
oriol_e9g’s picture

Status: Needs work » Needs review
FileSize
6.97 KB
8.48 KB

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.

oriol_e9g’s picture

Status: Needs work » Needs review

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

oriol_e9g’s picture

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.

oriol_e9g’s picture

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

catch’s picture

Tagging this as a D7 stable release blocker.

Tor Arne Thune’s picture

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.

Tor Arne Thune’s picture

Status: Needs review » Reviewed & tested by the community
sun’s picture

Assigned: Unassigned » webchick
Status: Needs review » Reviewed & tested by the community
FileSize
8.26 KB

Cleaned up the code and comments.

webchick’s picture

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!

sun’s picture

Version: 8.x-dev » 7.x-dev
Status: Fixed » Reviewed & tested by the community
FileSize
875 bytes

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

Tested manually.

webchick’s picture

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. :)

grendzy’s picture

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.