Anonymous user should not have contact form

This patch totally works. Before patching, there's a contact form for the anonymous user at user/0/contact, which, if you actually submit it, delivers you to the "Access denied" page at user/0 with a message proudly (and falsely) stating that "Your message has been sent". After patching, user/0/contact gives you an "Access denied" page, no matter what your permissions are.

BUT. I have an issue with "Access denied" messages appearing where, really, we're not denied access to anything; there's just nothing there. I think it would be better if we could display a "Page not found" error at user/0 and user/0/contact instead. Thoughts?

I think it's fine to issue a 403, but can we add a test to visit the path and assertResponse() that it's actually denied? Should be a two line change to contact.test

Given that this is pretty common requirement, I suggest we add a %user_authenticated placeholder, and the corresponding loading function:

function user_authenticated_load($uid) {
  $user = user_load($uid);
  if (!empty($user) && $user->uid > 0) {
    return $user;
  }
  else {
    return FALSE;
  }
}

I like Damien's suggestion, could clean up a lot of things elsewhere.

I like %user_registered

I like %user_not_anonymous, too.

Please just check $account->uid instead of $account->uid > 0. Looking good.

+++ modules/contact/contact.test	Wed Aug 05 10:01:52 2009
@@ -380,5 +380,19 @@ class ContactPersonalTestCase extends Dr
+    // Personal contact form tests for a user administrator.
+    $this->drupalLogout();
+    $admin_user2 = $this->drupalCreateUser(array('administer users'));
+
+    $this->drupalLogin($admin_user2);

Please re-use the $admin_user variable from earlier in the test because it already has the 'administer users' permission.

This review is powered by Dreditor.

Ah crap. That was an overlap from my own patch, so you were right. :)

But I'm a big fan of re-using what we have instead of creating new things. Let's change
$admin_user = $this->drupalCreateUser(array('administer site-wide contact form'));
to
$admin_user = $this->drupalCreateUser(array('administer site-wide contact form', 'administer users'));

Then we can just re-use that user.

Revsied patch with more documentation and way cleaner tests (meaning to do that for a while). gpk does this look good?

Executive summary:
1. Adds a user_not_anonymous_load($uid) menu-load callback in user.module that will return FALSE if the anonymous user is loaded.
2. Cleans up some documentation in user.module.
3. Makes _contact_personal_tab_access() look a lot easier to read and understand, with inline comments to help!
4. Splits the existing personal contact tests into an access test, and a flood test. Adds a submitPersonalContact function and some private user objects in the test to help re-use code.
5. Adds test conditions for:
a. User trying to access their own contact form.
b. Admin user can still access contact forms that have been disabled by users
c. Users cannot contact the anonymous user (user/0/contact)
d. Make sure admins don't get locked out of personal contact forms when flood protection is triggered.

Re-rolled for recent changes.

Adding tag

+++ modules/contact/contact.module	9 Oct 2009 03:27:05 -0000
@@ -91,7 +91,7 @@ function contact_menu() {
-  $items['user/%user/contact'] = array(
+  $items['user/%user_not_anonymous/contact'] = array(
@@ -104,20 +104,42 @@ function contact_menu() {
 function _contact_personal_tab_access(stdClass $account) {
+++ modules/user/user.module	9 Oct 2009 03:27:06 -0000
@@ -1413,12 +1413,47 @@ function user_menu() {
+function user_not_anonymous_load($uid) {

Given this menu access callback, I do not really understand why we need the new menu argument loader. The access callback seems to handle anonymous users properly already...?

+++ modules/contact/contact.module	9 Oct 2009 03:27:05 -0000
@@ -104,20 +104,42 @@ function contact_menu() {
-/**
- * Determine permission to a user's personal contact form.
- */
+ /**
+  * Menu access callback for a user's personal contact form.

Wrong indentation of entire PHPDoc block here.

+++ modules/contact/contact.test	9 Oct 2009 03:27:05 -0000
@@ -284,78 +288,99 @@ class ContactPersonalTestCase extends Dr
+  protected function submitPersonalContact($account, array $message = array()) {
+    $message += array(
+      'subject' => $this->randomName(16),
+      'message' => $this->randomName(64),
+   );

Wrong indentation of closing parenthesis here.

+++ modules/user/user.module	9 Oct 2009 03:27:06 -0000
@@ -1413,12 +1413,47 @@ function user_menu() {
+ * @param $uid
+ *   An optional user ID of the user to load.
+ * @return
+ *   A user object if $uid was valid or not provided, otherwise FALSE.
+ */
+function user_uid_optional_load($uid = NULL) {
+  if (isset($uid)) {
+    return user_load($uid);
+  }
+  else {
+    return $GLOBALS['user'];
+  }

@return docs do not map to the actual returned value.

+++ modules/user/user.module	9 Oct 2009 03:27:06 -0000
@@ -1413,12 +1413,47 @@ function user_menu() {
+ *   A user object if $uid was valid and not the anonymous user, otherwise

s/was/is/

I'm on crack. Are you, too?

Good point. I guess the only advantage to having a non-anonymous-user loader would be that it gives a 404 on pages where it would be uid 0 instead of a 403, but not really a big deal either way. I can live without the loader. It probably needs to have more discussion about things like included blocked users, etc.

Rerolled patch without the new menu loader and fixed coding standards.

+++ modules/contact/contact.module	9 Oct 2009 17:54:05 -0000
@@ -105,19 +105,41 @@ function contact_menu() {
+  // If the contacted user has not set their contact form preference, do not
+  // allow users to content them yet.
+  if (empty($account->contact)) {
+    return FALSE;
+  }
+
+  // If above conditions have not failed, the user can access the contact form.
+  return TRUE;

"to content them" ? :)

This actually also catches the case where the to be contacted user disabled his contact form, while the comment only speaks about a missing preference.

I agree that adding a check for blocked users would be good. But aside from that, I don't see any further options to delay this patch.

+++ modules/user/user.module	9 Oct 2009 17:54:06 -0000
@@ -1413,12 +1413,31 @@ function user_menu() {
+ * @return
+ *   A fully-loaded $user object upon successful user load, FALSE if user
+ *   cannot be loaded, or the current user object if $uid was not provided.

oh wow! I actually didn't know that one can just do user_load() to load the current user. Nice docs! :)

This review is powered by Dreditor.

We have an existing issue for blocked users in #586664: Users should not be able to contact blocked users but that's another bug report that can wait since I want this access function cleaned up for some other issues in contact.module.

Looks good.

Good catches in the documentation. I guess it's pretty obvious that in the end of the function is reached, access is granted, so I just chunked out that comment and moved it to the function's @return doc.

I see how I messed up user_uid_optional_load(). /me face palms.

The quality of my patches has been decreasing as I've been trying to pump them out with upcoming slush freeze. I need to slow down a little bit. :(

Adding backport tag.

You can prove me wrong, but I really think that there are no issues left now ;)

Looks great. Committed to CVS HEAD. Thanks.

Love to re-roll this for D6 since it doesn't change APIs. Thanks very much Dries!

This is a bug (user/0/contact) and should be fixed in d6 too.

Backport D6 patch attached including the documentation improvements to user.module as well. Does not change any APIs so should be good.

Applies cleanly, so RTBC as straight backport

Thanks, committed this to Drupal 6. I noticed that the code inside user_uid_optional_load() clearly treated the uid optional (just like the function was named), but $arg was not technically optional. Good catch.

Also, fixed "a either a" to "either a" in the code docs for this function before committing. That minimal fix might be useful to bring in D7 as well.

As pointed in #48 there'sa small typo, so let's back to 7 and return to 5

Committed #50. Now back to 5.x.

7.0 is out tomorrow, 5.x is obsolete, won't fix, sorry.