User module hooks are not always invoked with a full $account object, which can cause user pictures to be deleted on save

I can't reproduce this issue. Can you please describe all steps to reproduce? Thanks.

I have the same problem.

1) I had created about 100 users, they were blocked while the drupal site was in development.
2) Today, I unblocked all the users.
3) They recieved the emails, that their account had been activated
4) They were able to log in with the onetime URL and reset their password
5) They could see their previous avatar (I had already uploaded)
6) They click save and the picture disappears or in my case at first, it used the default avatar.

It doesn't seem to be a problem with the login since, I can also reproduce this behavior doing the following:

As admin.
1) edit a user, upload a picture for the user and save.
2) If I click view (as admin or user) I can see the newly uploaded image
3) If I edit the user (as admin) and click save = The image stays
4) If I edit the user (as the user) and click save = The image disappears or uses the default avatar

This didn't even require the user being blocked/unblocked

This is a bit of a pain in the ass...since I have already uploaded the user pictures one by one, and even as admin I cannot seem to assign the already uploaded images to the user profiles, it only allows me to upload again....ho hum....

Thanks

seems to be related to this problem http://drupal.org/node/935592
this might seem to be the solution... http://drupal.org/node/999004

However I am not sure which is the patch or if the patch was alreday included in 7.8

I have applied the patch in #66 of http://drupal.org/node/999004#comment-4467856, however I see that this has alreday been fixed.

So. I think this is some new problem

This is still a problem...anyone?

Bugs get fixed in D8 first, maybe this will get some more eyes there.

See related: #1232776: Add test to ensure that user picture is not deleted after calls to user_save()

This problem will continue until someone writes a test, or until user pictures are removed from core.

So there won't be a solution until D8 comes out?

@#9 by dwalker51 on October 20, 2011 at 3:50pm:

So there won't be a solution until D8 comes out?

Not necessarily. But in general, there will not be a d7 solution until there is a d8 solution. See the Backport policy.

Secondly, it is very hard to fix a bug that cannot be demonstrated and reliably reproduced. The accepted way to reliably reproduce a bug is to write a test for it. See the Simpletest documentation.

Thirdly, no bug gets fixed by complaining about it. To help the process along, you may:

• Write code.
• Recruit someone else to write code.
• Review the code that someone else wrote.
• Recruit someone else to review the code that has been written.
• Supply detailed and specific information (as in #3 above) to help the code-writers and code-reviewers along.

Drupal is a do-ocracy. Changes are made first in those areas where someone cares enough to do something about them.

Thanks...I didn't know about the backport policy...and yes I try to help along with code and reviews as much as posible and to give detailed reports of how I have come across a bug so as to help the developers...comment #3 was mine.

This sounds pretty stupid. We should make sure we capture this use case in tests so it never breaks again.

Still having this problem...not sure how to proceed.

Just decided to create a new image field in the user fields called avatar and have the user upload their image. Works fine with views, exposed filters and helped me finish my staff list on time. Thanks for the input, should have thought of this a while ago.

Can someone try using the information in #3 and confirm if they can reproduce this from a clean install, and write out the steps to reproduce? (I.e. "Install Drupal 8.x with the standard profile" as the first step.)

On both D7 and D8: With approach #2 from comment #3 I could not reproduce it. Anyone who can still reproduce this on a fresh install?

Can't reproduce this on fresh d7 or fresh d8 following approach number 2 of comment 3.

From reading the related issues, I think this might be related to the deprecated Profile module, not the User module. I also can't reproduce the problem on a clean install of D7 or D8 (with no profile module enabled, of course)

I just came across a bug like this in a custom module. In particular, if you have any code like this:

  global $user;
  user_save($user, array(...));

it will cause the user's picture to disappear.

The correct replacement for that code is:

  global $user;
  $account = user_load($user->uid);
  user_save($account, array(...));

I don't think we have any code with that problem in Drupal core, but there could very well be a contrib module out there that is doing something along those lines, and thereby leading to the bug reports here.

Aha! So since the $user global doesn't carry the user picture around everywhere, they're inadvertently deleting it.

So, anyone who encounters this error should look at the contributed modules installed on the particular site, and try to determine through some manual testing or educated guessing which is causing the problem. Once we have a culprit, we can move this issue to that module's queue to be fixed.

This is an issue in the User module, related to:

http://drupal.org/node/935592

The eventual fix to that issue (which involved fixing user_save() such that items *not* included in $edit would be preserved) has a bug in the user image section. Any routine that calls user_save *with* $edit and *not* including a new user icon in $edit will wipe out the existing user image because of this code:

$account->picture = empty($account->picture->fid) ? 0 : $account->picture->fid;

This is wrong because $account->picture->fid will only be populated if a picture object was included in $edit (that inclusion from $edit happens at line 464). In all other cases, $account->picture is still just the value of the 'picture' column from the database, so picture->fid is non-existent.

I can reproduce this with a fresh Drupal 7 or 8 install by enabling the LDAP suit of modules, configuring to point to an actual LDAP server for SSO, and doing an initial login with a user for whom I have already uploaded an image as site admin. It's triggered by the user_save call in ldap/ldap_authorization/ldap_authorization.inc -- but, as I said, any call to user_save that includes the $edit argument without a user picture object will do the same thing.

I fixed it by changing the above conditional to:

// We only want to proactively set $account->picture to 0 if $edit['picture']
// was nulled by user_presave. Otherwise, leave it alone.
$account->picture = ( isset($edit['picture']) && $edit['picture'] == NULL ) ? 0 :
isset($account->picture->fid) ? $account->picture->fid :
$account->original->picture->fid;

A shorter way might be to actually load the user picture object into $account->picture before this point, but this solution seems more correct and less reliant on "side-effects" to me--explicitly checking against $account->original.

Feel free to modify to conform to drupal code standards and test....

Thanks for tracking down LDAP as one of the possible causes of this!

However, this looks like a case of the issue I raised in #20? As far as I know, if you have a fully loaded user object then $account->picture will always be an object representing the picture file (see UserController::attachLoad()), so if it isn't that seems like an LDAP bug.

Maybe it actually is possible to change the logic in user_save() to deal with this case, although in general it makes sense to me that user_save() only works correctly if you pass in an object that came from user_load()... At a minimum we should change the user_save() documentation to better explain this.

Fair enough. At the very least, yes, the documentation of user_save should be changed. The documentation says:

$account: (optional) The user object to modify or add. If you want to modify an existing user account, you will need to ensure that (a) $account is an object, and (b) you have set $account->uid to the numeric user ID of the user account you wish to modify. If you want to create a new user account, you can set $account->is_new to TRUE or omit the $account->uid field.

Which doesn't just fail to mention that it assumes $account is a fully-loaded user object--it explicitly states all you need in $account in order to modify the user is uid. That's way, way misleading.

Back on topic, the sequence in this case is that the user authenticates, and ldap_authorization_user_login (implementing hook_user_login) checks for authorizations. If it's a first-time login, ldap/ldap_authorization/ldap_authorization.inc -> _ldap_authorizations_user_authorizations loads any authz from ldap and calls user_save to attach them to the user record in the db, which is where the deletion of the user picture is happening.

Is there a reason why it's up to an implementor of hook_user_login to load the user object fully, rather than the user module handling it up front? That would make more sense imo, but I'm pretty new to Drupal so maybe there's a good reason it's not set up that way. Either way, the user module and the ldap auth module apparently need to decide to agree on the division of responsibility between themselves. :)

Ouch, you are right, it looks like hook_user_login() isn't invoked with an actual, complete user account object (just with global $user), and I would tend to agree that's a bug in the user module... same thing goes for hook_user_logout(), and maybe others.

So if that's where LDAP is ultimately getting the user object from, this is somewhere between a core bug and a bug in that module :)

Not sure how much of this can be backported to Drupal 7 (except documentation) but it does sound like a problem that the user module is not invoking its hooks with consistent data.

Related or possibly duplicate: #1232776: Add test to ensure that user picture is not deleted after calls to user_save()

Looks like this has been fixed in: #999004: user_save() relies on $edit instead of $account

There is an issue open to add tests for the fix:
#1232776: Add test to ensure that user picture is not deleted after calls to user_save()

The Solution in #22 was OK for me in D7.
Thaaaaanks!!!!