Fix improper user loading

We're seeing this here too on PHP 5.1 and MySQL 5. Investigating further...

Mmmmh, tried to replicate the bug but couldnt.

1. Created a new user 'test1', logged in as test1.

2. Installed legal.module

3. Created T&C

4. T&C now shows up in test1 account, 'accepted' not checked

5. Logged out.

6. Logged in as test1, presented with T&C

7. Accept T&C, log in completely successful.

MySQL 4.1, PHP 4.4.1

I wonder if it could be a problem with MySQL 5 or maybe it's another module.

I get a similar message when I log out and devel.module is enabled.

Warning: array_keys(): The first argument should be an array in /Users/robertcastelo/Sites/Drupal/drupal-4-7/modules/user.module on line 349
.....

Try switching devel.module off if you have it enabled.

whatistocome are you running Drupal 4.7.2?

I think user.module line 352 is probably line 349 on current version?

Hm. I also was unable to duplicate this on a clean install. Sigh. :P Time for process of elimination I guess. :P

whatistocome what version of MySQL and PHP are you running?

One thing that's I don't understand....

Warning: array_keys(): The first argument should be an array in /home/domain/modules/user.module on line 352

Looking at line 352 in user.module

while ($row = db_fetch_object($result)) {

array_keys is used on line 349, is this the line where the problem is actually occuring?

In which case $account->roles not being an array would trigger the error.

Now to work out why that might be happening.....

Regarding user roles, bear in mind that a somewhat last-minute change to 4.7 before it was released was to remove role 1 and 2 from the user roles array; now, only "extra" roles are stored there; if a user's not logged in, it's implied that they are in role 1, and if they are, it's implied that they're role 2.

I can't replicate this, so difficult for me to fix.

Does anyone have an install where this is a problem?

If so I can try and create a solution which you can test out.

This set of errors appears for me ONLY when a user has registered prior to terms being set.
(user is old, legal module added after they registered)

At next login they must accept terms and this is when the mysql errors pop up.
New registrations are fine.

hit submit too soon - i'll investigate further but can test any fixes on my beta site.

I don't follow all of this, but I wonder if its something to do with the simulated logout/login sequence that legal uses? E.g. in function legal_login_submit, line 571 of legal.module 4.7.x (v 1.12.2.2 2006/07/16 11:59:40) we have

    $user = db_fetch_object( db_query('SELECT * FROM {users} WHERE uid = %d LIMIT 1', $form_values['uid']) );
    // [etc]
    user_module_invoke('login', $edit, $user);

while in user.module something similar-ish happens in user_load (lines 64-81):

    $result = db_query('SELECT * FROM {users} u WHERE ' . implode(' AND ', $query), $params);
    // [stuff removed here]
    $user->roles = array();
    if ($user->uid) {
      $user->roles[DRUPAL_AUTHENTICATED_RID] = 'authenticated user';
    // [etc]
    user_module_invoke('load', $array, $user);

So it looks like legal.module doesn't load any roles in this sequence. It may be that if any modules have a hook_user() that responds to type "login" by trying to check access on anything, that will call user_access() with the current global $user with $user->roles not set, so user_access() will barf.

This is pure guess mind, since I don't really follow what legal.module does.

As an aside...

user_access() currently assumes that $user->roles is an array (except for uid 1). Maybe it should protect itself against bugs in other modules by checking it really is first? It could be as simple as changing lines 349+ in user.module from

  if (!isset($perm[$account->uid])) {
	    $result = db_query("SELECT DISTINCT(p.perm) FROM {role} r INNER JOIN {permission} p ON p.rid = r.rid WHERE r.rid IN (%s)", implode(',', array_keys($account->roles)));

to

  if (!isset($perm[$account->uid]) && is_array($account->roles)) {
	    $result = db_query("SELECT DISTINCT(p.perm) FROM {role} r INNER JOIN {permission} p ON p.rid = r.rid WHERE r.rid IN (%s)", implode(',', array_keys($account->roles)));

or even

  if (!isset($perm[$account->uid]) && isset($account->roles) && is_array($account->roles)) {

which has the advantage of not generating Notices either if roles is not set. Anyway, at least this might prevent the nasty SQL error (and probably just give a user with no permissions until the next page load) while looking for what's not setting $user->roles properly.

This patch corrects the user loading which is causing these issues. Users should always be loaded via user_load() to avoid the problems reported above and others.

Thanks Neil, patch commited to 4.7 branch in CVS.

Can anyone who previously experienced this problem try out the new version and confirm the bug has been fixed.

I'm marking this as 'fixed', but will wait for confirmation before closing the issue.

I'm not sure if this is related, but I'm getting all the same errors on a test site running the latest version of this module. I tried both the 4.7 and CVS versions (which seem to be identical anyway)

A couple of things: We're using logintoboggan, I don't think there are any collisions but there might be something with the logins?

Also, my uid=1 superadmin account is NOT affected by this bug. All other accounts are (including admin accounts that have all perms)

Anyway, I'm following the code all the way through; I'll check back if I find anything.

Re-opening issue.

Found it!

Line 383:

$account = db_fetch_object( db_query('SELECT * FROM {users} WHERE uid = %d LIMIT 1', $uid) );

$account in hook_user is passed by reference. This means this call tainted the $account variable further down the line when it is used later in the code, generating our error.

The solution is to rename this variable- I have done so and the module works great after that, but I'm not sure what naming should actually be used so I'll leave that up to the pros.

This took me 3 hours to figure out but it was worth it.

Fool2: thanks for spending 3 hours of your life so I don't have to. Your fix regarding the $account variable was spot on and resolved my issue here too!

Before the fix the site would simply crash on anybody (except uid 1) logging in - showing the same MySQL error as mentioned in an earlier post.

Yeah, thanks Fool2, much appreciated.

I'll test it out at the weekend and commit it.

I'm not experiencing this bug, so I can't check if it fixes the problem (there seems to be consensus that it does), so if it doesn't cause any harm I'll happily commit it.

Attached patch to resolve the problem. I changed the $account variable to $u. Thought I'd do it whilst I was rollin a patch for the other bug in legal.module

If only some people are experiencing the bug it must be a conflict with another module, either optional or contributed. I'm not sure what, though, since the code is supposed to exit with the goto() and that variable should be destroyed

Confirmed, this patch fixes my setup as well. I did some digging for a while, couldn't figure out what was conflicting and why this might work in some cases.

Regardless, renaming the variable makes sense. I'm not even sure how this *could* work with it the same name. :-p

I was having exactly the same issue with old users who had not signed an updated agreement. The patch fixed this for me and seems to have no other issues. Any chance of committing this?

Thanks for the feedback.

Yes, I'll commit it as soon as I have time.

Patched the cvs version.

I'll patch the 4.7 version tomorrow if there's no complaints.

Patch commited to 4.7