This report is similar to the issue for node/$nid where $nid does not exist: http://drupal.org/node/82764

There are two related bugs, here.
In user_menu():

    if (arg(0) == 'user' && is_numeric(arg(1)) && arg(1) > 0) {
      $account = user_load(array('uid' => arg(1)));

      if ($user !== FALSE) {
         //...
      }

$user !== FALSE obviously should be $account !== FALSE.

And when it is false, it means that there are no user with this $uid, and we should return "page not found" instead of "access denied": i.e. do not imply that the user exists when it does not.

The patch creates a callback to a function that will call drupal_not_found();

CommentFileSizeAuthor
#9 user_not_found.3.diff992 bytesbeginner
#5 user_not_found.2.diff1.06 KBbeginner
user_not_found.diff1.39 KBbeginner

Comments

beginner’s picture

beginner commented

The patch here and the one there http://drupal.org/node/82764 both propose a menu callback that are very similar, that simply call drupal_not_found().
Maybe a simple function could be created in some required .inc file, to be used in every similar cases:

function menu_callback_drupal_not_found() {
  drupal_not_found();
}
ChrisKennedy’s picture

ChrisKennedy commented

Confirmed that this error exists for anonymous users and that the patch fixes the problem.

Since we aren't checking if $user === FALSE now, don't a few of the $user->uid checks within that IF block need to ensure that $user isn't FALSE?

beginner’s picture

beginner commented

The $user object within that block are all related to the user currently requesting the page, and it is used in this context only to check whether the user is requesting her own user page or not. As such, $user is always a properly declared object and has nothing to do with the $account object in the if statement at the beginning of the block.

The one check that could be removed is at the top of user_view(): $account === FALSE || could be removed from the if statement, but I am not sure, so I didn't include it in my patch.

beginner’s picture

beginner commented

Actually, it seems wasteful to load the same $account twice in the same page view: once in user_menu() and once at the top of user_view().

Instead of passing the $uid, shouldn't we be passing the whole $account object??

beginner’s picture

beginner commented
StatusFileSize
new user_not_found.2.diff1.06 KB

RobRoy most rightly pointed out the obvious to me: drupal_not_found() can itself be used as a callback!!
What was I thinking?

ChrisKennedy’s picture

ChrisKennedy commented
Status: Needs review » Reviewed & tested by the community

Righteo.

pwolanin’s picture

pwolanin commented
Status: Reviewed & tested by the community » Needs work

This is a very minor issue, but I don't think you need to set the title in this callback. drupal_not_found() set its own page title.

RobRoy’s picture

RobRoy commented

Also, drupal_not_found() doesn't take any arguments so you should remove the 'callback arguments' key.

beginner’s picture

beginner commented
Status: Needs work » Needs review
StatusFileSize
new user_not_found.3.diff992 bytes

Of course. Done.

pwolanin’s picture

pwolanin commented

One mire minor thing- you probably don't need the 'access' element.

beginner’s picture

beginner commented

Yes, we do, precisely for the reason you mentioned in the other issue: we need to be consistent.
If a user, anonymous or not, does not have the 'view profiles' perm, then accessing user/1 to user/9999999999 will all give "access denied" regardless of anything else.
If they have that perm, they will be able to infer the nb of registered members by checking from which user they get 404.

I am not sure I am explaining this clearly, but if you think it through and test it, you'll see what I mean.

pwolanin’s picture

pwolanin commented

Ah, I think I understand, but i'll have to look at the code again.

joshk’s picture

joshk commented
Status: Needs review » Reviewed & tested by the community

Applies cleanly, works as advertised and seems "right" to me.

drumm’s picture

drumm
he/him
Location NY, US
commented
Version: 5.x-dev » 6.x-dev
pwolanin’s picture

pwolanin commented

bump

dries’s picture

dries commented
Status: Reviewed & tested by the community » Needs work

This patch does not apply to CVS HEAD and needs to be re-rolled. It might apply against DRUPAL-5 though.

pwolanin’s picture

pwolanin commented
Version: 6.x-dev » 5.x-dev
Status: Needs work » Needs review

Since the menu system has changed, the last patch cannot work for 6.x and, in fact, the bug is already fixed in 6.x.

Probably it's still RTBC for 5.x. as a backport of the behavior in 6.x

drumm’s picture

drumm
he/him
Location NY, US
commented
Status: Needs review » Closed (duplicate)

Already fixed in 5.x.

toemaz’s picture

toemaz commented

Applied patch attached at #9 to D5.16 and I confirm that it works as advertised.