Enable 'access comments' permission for anonymous users by default.
catch - January 26, 2009 - 01:42
| Project: | Drupal |
| Version: | 7.x-dev |
| Component: | comment.module |
| Category: | bug report |
| Priority: | normal |
| Assigned: | rszrama |
| Status: | needs work |
| Issue tags: | anonymous users, Novice |
Description
I have spent a few hours trying to work out why nodes were so much slower than comments in HEAD. I discovered today, that anonymous users don't have access to view comments by default - so I'd been benchmarking a single node against 90, instead of a node with 90 comments against 90. I can't think of a good reason to keep the permission disabled - we allow authenticated users to post comments without approval by default already, which is arguably more prone to spamming.
#1
#2
#3
Sounds good to me. This is usually step 4 on any Drupal site I ever build.
#4
Hmm... just noticed the Novice tag and think I read about leaving these jobs for beginners. I already patched and tested it, so I suppose it's still better to post up the code. Hopefully a novice could still review it and appreciate it. : P
fwiw, I never liked the fact that there wasn't an API function to add a permission to a role. Would that be a considerable separate patch?
This patch just adds the appropriate permission in system.install. I tested on the latest unstable release, both available profiles, and it obviously sets the permission fine. ; )
#5
The last submitted patch failed testing.
#6
Hmm... curious... any way to see what test failed? My local test site keeps failing w/ a 500 error on "/drupal7/batch?id=3&op=do".
#7
The test is failing because of the new comment block test that I wrote (CommentBlockFunctionalTest in comment.test), which expects that an anonymous user by default does not have access to view comments.
<?php
// Test that a user without the 'access comments' permission can not see the block.
$this->drupalLogout();
$this->drupalGet('');
$this->assertNoText($block['title'], t('Block was not found.'));
$this->drupalLogin($this->web_user);
$this->drupalGet('');
$this->assertText($block['title'], t('Block was found.'));
?>
What will work best is if you create a new user in the test that does not have the access comments permission and instead of using the anonymous user, login with the new user:
<?php$no_comments_user = $this->drupalCreateUser(array());
$this->drupalLogin($no_comments_user);
?>
BTW, the role/permission APIs are in #300993: User roles and permissions API which also moves this default permissions to default.profile so I'll need to keep an eye on this patch in case it lands before 300993.
#8
The above patch is dead now. The code has been refactored.
The issue is still valid: http://d7p2.dev/admin/config/people/permissions/1
Rerolling patch...
#9
- Edited both default install profiles to include "access comments" for anon user.
- Verified "comment block" test above still fails.
- Then fixed it with Dave's suggestion :-)
TO TEST PATCH:
1) download d7 from CVS.
2) FIRST apply patch (patch is to install code!)
3) THEN run install.php
4) Check permissions here: http://d7p2.dev/admin/config/people/permissions/1 Both anon and auth should have access comments. Admin doesn't need (is auth)
Hurray for new install profiles!
#10
The last submitted patch failed testing.
#11
Let's try again. Fixed the test.
What happened? createDrupalUser(array()) does not return a user with empty permissions. It returns a user with a user with authorized user default permissions (which include "access comments").
There isn't a practical way to test the absence of the block. General block access control is outside the appropriate coverage for comments.test. So I removed the failing bit from the test. Left in the test for presence of the block.
See testing notes above. Moving on!
#12
#13
Looks good and access comments is enabled by default for anon. Tested with the default install profile. not with the expert profile. But the changes are identical, so should work there too.
#14
This change makes a lot of sense. But I'd really rather not lose that test for access denied in the process.
Can we not test to make sure that they can access the comment, then call http://api.drupal.org/api/function/user_role_revoke_permissions/7 to revoke the permission and ensure that they're successfully blocked?