user_is_anonymous() can return TRUE when the user is not anonymous. That's just wrong.

function user_is_anonymous() {
  // Menu administrators can see items for anonymous when administering.
  return !$GLOBALS['user']->uid || !empty($GLOBALS['menu_admin']);
}

Comments

dave reid’s picture

Because of the comment, this appears to be very much by design. I'll leave it up to more comments if this should be actually marked 'by design.'

David_Rothstein’s picture

I'm sure it does serve a purpose, but a function named "user_is_anonymous" should do what it says... otherwise, the API is broken and unusable. So I think this is definitely a bug ;)

I haven't looked at all the places in the code that call this function, but any of them that require this extra check could instead just call a wrapper function that checks user_is_anonymous() || !empty($GLOBALS['menu_admin']); and that would solve the problem.

Gurpartap Singh’s picture

user_is_anonymous() is used in _menu_site_is_offline() as the api functions reference says. I am not exactly sure why menu administrators need to pass through the check.

!user_is_logged_in() can actually dictate the result correctly for user_is_anonymous(). Hence there is a chance to drop one of these functions, and merge the check into _menu_site_is_offline() itself, if at all required.

David_Rothstein’s picture

Well, the API website doesn't always show you all the references. I tried running a grep on the D7 codebase, and in addition to the reference you mention above, this function is also called in:
http://api.drupal.org/api/function/user_register_access/7

It also appears to be the access callback for three menu items in core:
user/login
user/password
openid/authenticate
(see http://api.drupal.org/api/function/user_menu/7 and http://api.drupal.org/api/function/openid_menu/7)

It's not clear which, if any, of the above might be broken as a result of this (the intention appears to have been to grant access to the menu items only).

It looks like #191914: You cannot add user/register to a menu was the issue which introduced this code.

alexanderpas’s picture

Priority: Normal » Critical

should be renamed to menu_anonymous_access_check() or something like that...

_menu_site_is_offline() seems to be using the old school function, before the change, and thus should be changed to !user_is_logged_in()

damien tournoud’s picture

Title: user_is_anonymous() is a lie » user_is_anonymous() is ugly
Category: bug » task
Priority: Critical » Normal

This is nothing more than an alternative implementation, required by the menu system when managing permissions for the anonymous users. It could read:

function user_is_anonymous() {
  // Menu administrators can see items for anonymous when administering.
  if (!empty($GLOBALS['menu_admin'])) {
    return TRUE;
  }
  else {
    return !$GLOBALS['user']->uid;
  }
}

Module writers *must* use it in access callbacks, even if it doesn't "technically" always returns the truth.

Because $GLOBALS['menu_admin'] is only set during a limited period of time, it shouldn't hurt to use that function outside the menu context.

This is not a bug. Lowering the priority, and reassigning as a task to study how we could do this is in a less hackish way for Drupal 7.

alexanderpas’s picture

Category: task » bug
Priority: Normal » Critical

not looking towards the menu issue... what's the difference between:
!user_is_logged_in() and user_is_anonymous()
actually nothing, and it should stay that way! (or even better, one as alias of the other.)

as user_is_anonymous() might be used in access-checks or similar, i still consider this a critical bug, as it might restrict or allow when it explicitly shouldn't.

I still favor at least the menu_anonymous_access_check() or something similar to that (what's in a name) which has a less global scope.

also, this needs a testcase.

damien tournoud’s picture

Priority: Critical » Normal

@alexanderpass: actually you *should* and *have to* use user_is_anonymous() in your menu access check. The behavior changes only in a very small period of time (when menu list is generated in the menu admin page), so it's *completely* safe to use this function outside of menu access checks.

So please, this is clearly not a critical bug.

The only valid point I see is that user_is_logged_in() should mimic the behavior to guarantee that user_is_logged_in() == !user_is_anonymoud.

alexanderpas’s picture

menu access check, yes... but what about other kind of access checks??? (like the one for the page offline? or another.)

and what about undefined variables...

damien tournoud’s picture

menu access check, yes... but what about other kind of access checks??? (like the one for the page offline? or another.)

?

I already told you it is totally safe to use user_is_anonymous() *anywhere*.

alexanderpas’s picture

so $GLOBALS['menu_admin'] is actually kind of a private global for the menu access check... doesn't sound very clean...

lotyrin’s picture

Version: 7.x-dev » 8.x-dev
Issue tags: +API change

Changing the behavior and/or name of this is too late for D7.

tim.plunkett’s picture

chi’s picture

Version: 8.0.x-dev » 7.x-dev
Issue summary: View changes

Cannot find user_is_anonymous() in Drupal 8. Has it been removed?

star-szr’s picture

Status: Active » Closed (won't fix)

@Chi indeed. In procedural code, \Drupal::currentUser()->isAnonymous(), in OO you would inject the current_user service (\Drupal\Core\Session\AccountInterface).

I would also add that the it's no longer a lie :) there's no special case for menu admin there any more.

Closing as won't fix because I agree that changing this for D7 is too late.