Posted by salvis on November 25, 2007 at 3:22pm
19 followers
| Project: | Drupal core |
| Version: | 5.x-dev |
| Component: | comment.module |
| Category: | bug report |
| Priority: | normal |
| Assigned: | tanper |
| Status: | closed (fixed) |
Issue Summary
If the authenticated user role does not have the 'post comments' permission, then the "Login or register to post comments" links are inappropriate, because the user may not be able to post comments even after logging in.
Users in that position might see this as a broken promise. This problem may be the reason for inventing the "you can't post comments" text (see http://drupal.org/node/137100), but that text doesn't solve the problem at all, on the contrary: it's a constant reminder of the broken promise and the fact that the user apparently belongs to a lesser user category.
I propose to remove "Login or register to post comments", if the authenticated user role does not have 'post comments' permission.
See also http://drupal.org/node/169938.
Comments
#1
+1 on this, not too hard to do but probably best dealt with in a general cleanup of comment module.
#2
Here's a patch that addresses this problem, but I narrowed the application a bit from what salvis described. Namely, rather than removing "Login or register to post comments" if the authenticated user role doesn't have a 'post comments' permission, I removed it if no user role has such a permission. The idea being that if another created role has the permission, we generally wouldn't want this link to be automatically hidden, even if only some authenticated users can post comments. And if you wanted it to be hidden in that case, you can always override the theme function. But if absolutely no users can post comments, there's no reason to display "Login or register to post comments."
#3
+1 on this.
I can't believe I never noticed this little issue. Slightly changed the patch to make it simpler and use the user_access() instead of a doing a select.
Cheers.
Index: comment.module
===================================================================
RCS file: /cvs/drupal/drupal/modules/comment/comment.module,v
retrieving revision 1.604
diff -u -r1.604 comment.module
--- comment.module 6 Dec 2007 09:58:30 -0000 1.604
+++ comment.module 7 Dec 2007 04:03:04 -0000
@@ -1639,7 +1639,7 @@
$destination = "destination=". drupal_urlencode("node/$node->nid#comment-form");
}
- if (variable_get('user_register', 1)) {
+ if (variable_get('user_register', 1) && (user_access('post comments'))) {
return t('<a href="@login">Login</a> or <a href="@register">register</a> to post comments', array('@login' => url('user/login', array('query' => $destination)), '@register' => url('user/register', array('query' => $destination))));
}
else {
#4
Sorry, amanuel, your patch isn't going to work, for two reasons:
1) user_access just checks the currently logged in user's permissions, unless you pass in an $account. But either way, it's not going to check if no user has 'post comments' permissions.
2) You actually should be checking both 'post comments' and 'post comments without approval' -- my patch checks both by virtue of a LIKE query.
I'm pretty sure the select on the permission table is necessary to check to see if the permission exists for any role. If you think the functionality of my patch in comment #2 should be changed, or the approach but keeping the same functionality, I'm happy to discuss that.
#5
- Introducing an SQL query in a theme function is a big no-no. Even if there are examples to this somewhere, we are not going down this route.
- Because there might be several calls to this theme function, the result of the query should be cached.
#6
@mcarbone: I see your reasoning, but I don't understand it. If only some roles have posting privileges, then those users can be assumed to be more knowledgeable, more trustworthy, in closer touch with your site, etc.
The higher ranking users should know that they get more functionality when they log in. There's no use in telling them to "Login or register to post comments" because they already know that, and for the lowly ones who don't know — well, for those it doesn't apply...
Or do you give posting permissions for first-time registrants and remove those permissions as your users become more mature?
#7
@salvis: Hmm, I guess you have a point. It only makes sense to display this message if core knows for a fact that logging in/registration gives comment posting access, which it can only do if the authenticated user has that permission. If it doesn't, but another role does, in that case one could override the theme to display an alternative message if they want. (e.g., "Login or register to learn how to become eligible to post comments," or whatever).
I will work on a patch with this, and Gábor's comments, in mind.
#8
Here's the patch. It displays the message only when the authenticated user has comment posting permissions. It puts the query in a helper function that uses caching. Naming convention suggestions welcome.
#9
@mcarbone: You are right about my patch not working correctly. However isn't issue to solve the situation where an unregistered user is given the promise of posting upon registration only to find out that is not allowed.
Having thought about it some more, we should check the 'destination role' (usually authenticated role) to see if a newly registered user WILL have permission.
As for already registered I agree with @salvis, they should already know they need to login.
perhaps using user_roles(TRUE,'comments') instead of your select statement may make your patch more acceptable.
thoughts?
#10
I'm not sure if I understood the first part of your comment, but my latest patch was already checking the authenticated role. However, you're right that user_roles is a much better way to go about it, and it obviates the need to add a helper function. So here's a much more economical patch that uses that function.
#11
Ha, I found a bug in user_roles, so I tweaked my patch so as to not rely on the bug.
The bug is this: it uses a LIKE query to check which roles satisfy a permission. But that means that if one permission is a substring of another one, it can return incorrect data. So for example, imagine a role that has the 'post comments without approval' permission, but not the 'post comments' permission. If you called user_roles(TRUE, 'post comments'), it will return that role, even if it doesn't have that specific permission! Now, that might not matter with the 'post comments' permission, but it could with other cases where being a substring of another permission doesn't necessarily imply it being a subpermission.
edit: someone has already reported this bug: http://drupal.org/node/198362
#12
mcarbone: Can you please submit an issue with that bug and put a link up here. That sounds pretty important to fix. Thanks!
#13
As I said, it's already been reported. Someone just bumped it up to critical, so it'll probably get more attention now:
http://drupal.org/node/198362
#14
I think you can remove the "you can't post comments" text. It must have been put there to back out of the false promise, but since you're removing the false promise, there's nothing to back out of. The Drupal way is to hide what's not available.
#15
This was being discussed here as well:
http://drupal.org/node/137100
I recommend clearing the initial if in the theme function, because we do not need to let the person know that they are not allowed to post on every possible comment page. Just keep the area blank, as to not bring attention to their lack of permissions:
- if ($user->uid) {- return t("you can't post comments");
- }
Then, just change the else to represent the if 'Not' uid:
- else {+ if (!$user->uid) {
#16
I'm not sure I've been convinced that getting rid of "you can't post comments" is the right thing to do, but in case everyone else is, here's the patch with that removed. We can just use the patch in comment #11 if we decide to leave it in for now.
#17
@mcarbone: What benefit do you see in displaying that "you can't post comments" text?
#18
None, really, but I didn't want to presume as to why it was there in the first place. But if we take it out, why should that theme function every be called if the user is logged in?
#19
To answer my own question, maybe it's to allow it to be themed when we have logged in users who can't comment.
#20
There's a large numbers of permissions that any given role or user may NOT have. The Drupal way of handling the situation is to not display whatever is not available to the current user. I'm not aware of any other functionality in Drupal, where Drupal says "you cannot do this."
It's not Drupal-like, and it alienates users.
#21
mcarbone's patch in #16 certainly removes the login or register to post comments links for anonymous users, but it did so even when my authenticated user role does have permission to post comments!
#22
Odd, it's testing completely fine for me. Can anyone else confirm? Or, @Benjamin Melançon, can you think of why this line wouldn't return true in your case?
$authenticated_post_comments = array_key_exists(DRUPAL_AUTHENTICATED_RID, array_merge(user_roles(TRUE, 'post comments'), user_roles(TRUE, 'post comments without approval')));I assume that you're testing with an anonymous user, and that the built-in 'authenticated' role has either 'post_comments' or 'post comments without approval' -- is that correct?
#23
array_merge is resetting your keys:
This is my output (on http://d6.openzuka.net/node/1)
From all this testing:
<?phpif (!isset($authenticated_post_comments)) {
drupal_set_message("must set it, based on ". DRUPAL_AUTHENTICATED_RID . " pc " . print_r(user_roles(TRUE, 'post comments'), TRUE));
$authenticated_post_comments = array_key_exists(DRUPAL_AUTHENTICATED_RID, array_merge(user_roles(TRUE, 'post comments'), user_roles(TRUE, 'post comments without approval')));
drupal_set_message("gah" . print_r( array_merge(user_roles(TRUE, 'post comments'), user_roles(TRUE, 'post comments without approval')), TRUE));
}
drupal_set_message("at the gates of authenticated_post_comments: " . $authenticated_post_comments);
if ($authenticated_post_comments) {
drupal_set_message("got inside the if authenticated_post_comments");
if (variable_get('user_register', 1)) {
drupal_set_message("inside user register now!");
return t('<a href="@login">Login</a> or <a href="@register">register</a> to post comments', array('@login' => url('user/login', array('query' => $destination)), '@register' => url('user/register', array('query' => $destination))));
}
else {
return t('<a href="@login">Login</a> to post comments', array('@login' => url('user/login', array('query' => $destination))));
}
}
?>
#24
From PHP.net: http://us.php.net/array_merge
Worth a shot?
benjamin, Agaric Design Collective
#25
@Benjamin Melançon -- Nice find. Yet another reason why testing on a fresh install isn't good enough. Here's a patch using + instead of array_merge. Give it a shot.
#26
#27
Works great! Thanks for updating the patch.
One question I had was about was whether the login link should show up on teasers (for logged in users an "Add new comment" link does show up on teasers). But that's a separate issue from this patch.
This is ready to be committed based on my tests.
benjamin, Agaric Design Collective
#28
Why can't we use:
<?php- if (variable_get('user_register', 1)) {
+ if (variable_get('user_register', 1) && (user_access('post comments') || user_access('post comments without approval')))) {
return t('<a href="@login">Login</a> or <a href="@register">register</a> to post comments', array('@login' => url('user/login', array('query' => $destination)), '@register' => url('user/register', array('query' => $destination))));
?>
#29
Dries: user_access() uses the current user. In the issue at hand, the links are printed with the assumption that *after the user is loggedn in*, she will have permission to post comments. With your code example above, the body of the if() will never run, as this function is only invoked, if the user was checked to not have permission to post comments already.
The patch checks whether the assumption that after the login, the user will have permission to post comments is right. If not, it is misleading to print this note suggesting the user to log in or register.
#30
Based on the situation this patch fixes and my test and review, setting back to ready to be committed.
#31
I have tried to patch this into 5.5, but with no success. I see you'll were talking about D6 though.
Has somebody worked out a patch for 5.5 and can post that? Or better send me the patched comment.module, since I am working under Windows and have problems getting patch to work.
Thanks
#32
Eh, do not take over issues please.
#33
I don't know what you mean. I didn't try to take over anything.
#34
spade, you changed the version, status and category (and changed the subject to patching on windows) - which made this ready-to-be-fixed bugfix against 6.x an inactive (won't fix) support request against drupal 5. That's called 'taking over'. When this patch is committed to 6.x, it might be worth opening a request for a backport. http://drupal.org/patch has information for getting started on patching.
#35
Thanks catch,
I am still new here and trying to find my way around ...
Thanks for the advise ...
#36
Gabor: OK, that makes sense. I suggest we document your explanation in the code, and that we commit this patch. Let's make sure this is easy to understand for other developers.
#37
I went ahead, cleaned up the code a bit (ie. moved the $destination handling to the part where we already know we actually need that value), and added some comments to this function. Committed this one.
#38
Small typo in a code comment; follow up patch attached.
#39
#40
Thanks, committed.
#41
Thank you!
Is it possible to get this back-ported to D5? Can I retag this issue, or do I need to open a new issue, as catch suggested in #34?
#42
As was to be expected, I support the request to get this back-ported to D5. ;-)
#43
spade: you changed this to ready to be committed, but did not change the version? Did you check that this is ready to be committed in Drupal 5 as-is, without modification?
#44
No, it doesn't apply as-is, and will have to be ported.
#45
Sorry, I didn't knowingly change anything and can't answer your question. I just ment to support the previous posters request (and left everything in the head of the form unchanged [I thought I had learned that much from my previous mistake]). I appologize should I have caused any confusion ...
#46
Ported one-to-one to 5.x-dev and tested.
#47
#48
subscribing
#49
Moving all usability issues to Drupal - component usability.
#51
Committed to 5.x.
#52
But in D7 this issue will still exist?
#53
#40 committed it to D6. I'm not sure whether that was before or after D6 was branched.
@Bojhan: Please check D7 and report back — thanks!
#54
Looks to me that it's in there:
http://cvs.drupal.org/viewvc.py/drupal/drupal/modules/comment/comment.mo...
#55
Automatically closed -- issue fixed for two weeks with no activity.
#56
Found another solution..
In comment.module look for function
function comment_link($type, $node = NULL, $teaser = FALSE) {
...
....
else {
// this is for teaser - comment this line
// $links['comment_forbidden']['title'] = theme('comment_post_forbidden', $node->nid);
}
this should hide the "login or register to post comment" link in the teaser..
Gopisathya
#57
Fixing some changes made by spammers.
#58
This is still getting spam, so I'm going to turn off new comments on this issue.