Posted by JohnAlbin on March 30, 2011 at 12:59pm
11 followers
| Project: | Drupal core |
| Version: | 7.x-dev |
| Component: | comment.module |
| Category: | bug report |
| Priority: | normal |
| Assigned: | webchick |
| Status: | closed (fixed) |
| Issue tags: | D7 stable release blocker, needs backport to D7 |
Issue Summary
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.
Comments
#1
And here's a patch.
#2
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
#3
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.
#4
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.
#5
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.
#6
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
CommentInterfaceTestsince I though that made most sense. But there are no tests for anonymous comments there, so the check for that class is inCommentAnonymous.#7
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.
#8
#6: 1110650-comment-classes-6.patch queued for re-testing.
#9
The last submitted patch, 1110650-comment-classes-6.patch, failed testing.
#10
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.
#11
Reroll with tests in #6.
#12
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.
#13
@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.
#14
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...
#15
+++ 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.
#16
Changes from #11:
- xpath updated with the comment id.
- Logout/Login in comment 5 generation removed because it's not needed.
#17
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?
#18
Patch fixes the problem and issues reviewed by sun in #15. Test passes. Code looks good, imo.
#19
Committed to 8.x, moving to 7.x for webchick.
#20
Looks good. Committed and pushed to 7.x. Thanks!
#21
Automatically closed -- issue fixed for 2 weeks with no activity.
#22
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.#23
Can we add some tests for this cases?
#24
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.
#25
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.
#26
@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 (===).
#27
#28
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.
#29
The last submitted patch, comment-classes-1110650-28.patch, failed testing.
#30
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.
#31
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) { ...
#32
#33
The last submitted patch, comment-classes-1110650-32.patch, failed testing.
#34
#35
I think that this is good :/ I was going crazy with a fatal error that couldn't reproduce.
#36
The last submitted patch, comment-classes-onlytests-1110650.patch, failed testing.
#37
Review! #35 prove the bug and the solution! xD
#38
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.
#39
Small comment typo fix, no code changes. Failing tests to prove the bug in #35.
#40
Tagging this as a D7 stable release blocker.
#41
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-newandcomment-by-anonymous.#42
#43
Cleaned up the code and comments.
#44
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!
#45
Follow-up fixes for D7 to unbreak broken HEAD. :)
Tested manually.
#46
Great, thanks for the fast turnaround!
Committed and pushed to 7.x. Let's see if testbot likes us better now. :)
#47
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. :-)
#48
Automatically closed -- issue fixed for 2 weeks with no activity.