Problem/Motivation

User pictures are being deleted when user objects are re-saved.

user_save() calls drupal_write_record() to save the user account object. drupal_write_record() expects $account->picture to be the fid of the image file. So user_save() converts $account->picture from a file object to an integer before writing it to the database. Thus the file's properties are lost.

Proposed resolution

Save the file object to a variable before the call to drupal_write_record(). Set $account->picture back to the file object after the write.

Remaining tasks

Fix the patch in #84 with the comment change mentioned in #86. (Novice)

Original report by @macgirvin

1. Setup users to allow user pictures on content and profiles and add one. See that it displays on those pages.
2. Create a custom profile field. Let's call it a textfield called Location under the category Personal Information
3. Now edit your profile and set your location
4. Look at your profile. The user photo is gone. Look at a content item - the profile photo is gone there as well.
5. Remove the custom profile field you created and confirm that it will remove any instances of that field.
6. Photos are still gone.
7. Resubmit the user settings page that allows profile photos in content and profiles
8. Photos are still gone
9. Additionally you are left with a "Personal Information" tab on the profile edit page which throws an error if you try to access it.

#9 is a second bug but for now let's focus on the fact that user photos are gone and won't come back

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

macgirvin’s picture

fwiw: I uploaded a fresh picture (the custom profile field no longer exists) and user photos are working normally again.

And some additional background: fresh install of 7.0beta 1, no additional modules

Damien Tournoud’s picture

Version: 7.0-beta1 » 7.x-dev

I can reproduce. The user picture is lost when submitting any of the user edit subpages exposed by the profile module.

Anonymous’s picture

Status: Active » Needs work
FileSize
2.17 KB

attached is a patch for a fairly crude test that reproduces the bug as per the original post. setting to CNW, as it doesn't contain a fix of any kind.

also, we may find this is a more general issue, and we'll want to make the test more general, but this is a start.

Sivaji_Ganesh_Jojodae’s picture

Status: Needs work » Needs review
FileSize
3.41 KB
3.41 KB

Attached patch will fix this. I am not sure whether it is a right way of handling this bug, but it works for me.

pillarsdotnet’s picture

Re-rolled against HEAD, with corrections.

Finally got user pictures working; yay!!!

pillarsdotnet’s picture

Typo...

pillarsdotnet’s picture

And maybe I posted this to the wrong issue...

Status: Needs review » Needs work

The last submitted patch, 935592_user_profile_picture_6.patch, failed testing.

pillarsdotnet’s picture

Status: Needs work » Needs review
FileSize
10.12 KB

Nice testbot...

Status: Needs review » Needs work

The last submitted patch, user.module.patch, failed testing.

spillz’s picture

Priority: Normal » Major
Issue tags: +delete, +profile, +photo, +drupal7, +save

I can confirm that the same thing happens to me.

Actually, any profile that HAS a photo uploaded to it - will have the photo 'removed' if I ever decide to save the profile page while any page other than the main profile page. What I mean by that, is that I have extra 'categories' in the profile -- like one called 'Web', where we list the website and the SkypeID for a user. So, if I'm on this page and enter a website URL and click save, it will delete the photo from this user's profile. Very frustrating indeed. Need to reupload user photos every time a user profile is edited.

pillarsdotnet’s picture

Issue tags: -delete, -photo, -drupal7, -save
Dave Reid’s picture

Issue tags: +user pictures

Subscribing

DocuAnt’s picture

I can confirm this problem (Sorry for the misleading issue here: http://drupal.org/node/1022780).

teh_catt’s picture

subscribing - I have profile2 installed so I can have extra profile types. When I edit and save information in those profile types the user picture dissappears and returns to the default. Anyone have any ideas?

alpritt’s picture

What's happening is the picture is getting marked for deletion any time the $edit array doesn't contain a picture with an fid property. What should happen is this deletion part should be ignored if the $edit array contains no reference to the picture at all.

Currently, if you load the user and then perform a user_save($account) on that user object without passing the existing picture in the $edit array, the picture will be deleted. This is essentially what is happening when you save the account while on a profile page. So #4 isn't going to be robust enough to fix the issue. And the test should be for working directly with user_save() rather than saving a profile page which is just one consequence of the bug.

There's a simple fix (we just need to do an array_key_exists('picture', $edit)) but maybe we can tidy up the code a little so it isn't so confusing to read. At the very least we should add some more commentary.

#9 is quite a dramatic refactoring of the function which I've not looked through properly enough to know if it is a solid fix or if it is going to break anything else. So I'd suggest the simpler fix and then for Drupal 8 there is a @todo which will hopefully make this function less horrible. That said, the logic that patch takes is perhaps better than doing the array_key_exists(). Not sure at the moment; I need to look again when the time is further away from the middle of the night.

andypost’s picture

Status: Needs work » Needs review

subscribe, I got a lot of issues with user profile pictures as maintainer of imageCache Profiles

#9 to hard to understand without description also it breaks tests...

So I think #4 is a way for D7 and this patch also has test coverage...

+++ modules/user/user.module	12 Oct 2010 10:26:12 -0000
@@ -446,13 +446,15 @@ function user_save($account, $edit = arr
+      if ($category == 'account') {
+        // Delete the previous picture if it was deleted or replaced.

This is a actual bug-fix!!!

Powered by Dreditor.

andypost’s picture

#4: 935592_user_profile_picture_4.patch queued for re-testing.

alpritt’s picture

Status: Needs review » Needs work

As I said in #16, #4 is not a robust solution. Try the following:

- Apply patch #4
- Fresh install
- Go to your account and upload a profile picture
- Execute a bit of PHP...

$account = user_load(1);
user_save($account);

- No more profile picture.

I will work on a patch, but feel free to write a new test if anyone has time.

alpritt’s picture

Status: Needs work » Needs review
FileSize
2.55 KB

The patch should be good for a review, but this needs tests.

The patch is with --no-prefix set, but I suppose this will change after the migration to git?

The only part that is absolutely required is the array_key_exists() condition. However, I've rewritten more than that and added more commentary because I think it likely that the bug was caused in the first place by the confusing code.

Tests IMO need to cover:

- uploading a new picture when none exist.
- uploading and replacing an existing picture.
- deleting an existing picture.

Through the UI and using user_save directly.

I've not looked to see what coverage we currently have. Perhaps the test from #4/#3 will cover part of what we need.

alpritt’s picture

Title: User picture gone after creating custom profile field » User picture is deleted after calls to user_save()
Status: Needs review » Needs work

Turns out that the patch in number #9 came (at least in part) from #999004: user_save() relies on $edit instead of $account. So that makes more sense now. That issue is going to break the fix here so we should get that one in first. I'm not sure, but it might actually fix this issue as a consequence anyway. We can still work on test coverage here though.

matrobin’s picture

subscribe

DocuAnt’s picture

Is there a way to push this issue to ensure that this will be fixed in the next Drupal 7 Version?
(losing user content is a very serious and annoying problem causing a lot of frustrations...)

pillarsdotnet’s picture

Version: 7.x-dev » 8.x-dev
Issue tags: +Needs backport to D7

Bumping and tagging according to standard policy.

tcmug’s picture

I feel that this could be related to this: http://drupal.org/node/1126000

D7 - but anyways should apply.

Owen Barton’s picture

Status: Needs work » Needs review
FileSize
2.93 KB

Here is a rerolled patch for 8.x (no changes)

mikeryan’s picture

Ran into this problem in the Migrate module on Drupal 7, applied patch as-is on D7 and fixes things nicely for me.

jdelaune’s picture

Status: Needs review » Reviewed & tested by the community

Worked a treat. Cheers

DocuAnt’s picture

Don't forget comment #21.
There is a second approach out there #999004: user_save() relies on $edit instead of $account.

catch’s picture

#26: 935592-user-pictures-del-26.patch queued for re-testing.

Dries’s picture

I committed #999004: user_save() relies on $edit instead of $account to 8.x (a critical bugfix too). That commit seems to have broken this patch, so a reroll might be in order. I'm asking the testbot for a re-test.

Dries’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +user pictures, +profile, +Needs backport to D7

The last submitted patch, 935592-user-pictures-del-26.patch, failed testing.

alpritt’s picture

Issue tags: +Needs tests

The issue that broke this one may have fixed this bug, but I haven't checked properly. My comment in #19 explains how to recreate this bug through code which gets to the core of the problem and there needs to be a test to make sure that is now working. Dries has just tweeted that he wants to release 7.1 next week so we need to make sure this is definitely fixed before then. If anyone has time to write tests that would be helpful and I will make time to review.

alpritt’s picture

Manual test against 8.x and 7.x branch:

- Fresh install
- Account settings... enable user picture
- My account... upload a profile picture
- View user profile and confirm that picture is there
- Run code:

$account = user_load(1);
user_save($account);

- Check picture is still there.

For kicks I also tested the original steps to reproduce using the profile module. Namely:

- Enable the profile module
- Create a user profile category
- Upload picture
- Go to sub tab for the profile category we just created
- Save
- Check picture is still there

And it is. So all looks good. The #999004: user_save() relies on $edit instead of $account issue fixed this.

Not sure what to do regarding automated tests. Obviously we are getting rid of profile module so it would be a bit silly to test against that.

pillarsdotnet’s picture

Title: User picture is deleted after calls to user_save() » Add test to ensure that user picture is not deleted after calls to user_save()
Category: bug » task
Priority: Major » Normal

Not sure what to do regarding automated tests. Obviously we are getting rid of profile module so it would be a bit silly to test against that.

If you turn off the profile module, what effect does Account settings... enable user picture have?

rfay’s picture

Title: Add test to ensure that user picture is not deleted after calls to user_save() » User picture is deleted after calls to user_save()
Version: 8.x-dev » 7.x-dev
Status: Needs work » Reviewed & tested by the community

This still needs backport to D7.

I'm changing the title back and changing to D7/RTBC. Hate to lose this in the crowd.

I haven't worked on this, just discovered it from a reference elsewhere. You who have can adjust this as you see fit, but it looks to me like just an omission that #26 didn't get committed to D7.

I'm all in favor of getting a test in, of course.

I'm thinking that now that we have issue summaries we should only change the title to improve it to describe the base issue described in the post, not to imply change of direction on the thread. We can show the status of what needs to be done in the issue summary.

rfay’s picture

Version: 7.x-dev » 8.x-dev
Status: Reviewed & tested by the community » Needs work

Looks like I completely misunderstood #31, and this hasn't been committed. Sorry. However, it looks like the original title is appropriate at this point.

rfay’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests, -user pictures, -profile, -Needs backport to D7

#26: 935592-user-pictures-del-26.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Needs tests, +user pictures, +profile, +Needs backport to D7

The last submitted patch, 935592-user-pictures-del-26.patch, failed testing.

pillarsdotnet’s picture

Status: Needs work » Closed (duplicate)

@rfay: According to alpritt in #35,

And it is. So all looks good. The #999004: user_save() relies on $edit instead of $account issue fixed this.

So unless you have evidence to the contrary, it would appear that the originally-reported problem is fixed. However, we still lack tests to ensure that the problem does not reappear in the future. Therefore, I changed the title. Perhaps it would have been better to mark this as a duplicate of #999004 and open a new issue for the missing test.

EDIT: Opened #1232776: Add test to ensure that user picture is not deleted after calls to user_save().

kiwidog’s picture

Hi, does anyone know if this has been fixed in D7? I use 7.14 and user picture disappears whenever I edit a user.
I also looked #999004: user_save() relies on $edit instead of $account and everyone sounds like it's been fixed & issue's closed..

dimsum’s picture

Version: 8.x-dev » 7.14
Priority: Normal » Major
Status: Closed (duplicate) » Needs review

Seconded this. Trying to update userfield thru Rules and User Picture gets deleted in the process.

tim.plunkett’s picture

Version: 7.14 » 8.x-dev
Priority: Major » Normal
Status: Needs review » Closed (duplicate)
chx’s picture

Version: 8.x-dev » 7.x-dev
Category: task » bug
Status: Closed (duplicate) » Active

I am still seeing this.I will check with my client whether I can share the full backtrace, but here is the short of it: $account->picture is a the fid instead of the picture file when this happens:

            [function] => user_save
            [function] => entity_metadata_user_save
            [function] => entity_save
            [function] => save
            [function] => saveNow
            [function] => cleanUp
            [function] => executeByArgs
            [function] => rules_invoke_event
            [function] => rules_entity_update
            [function] => call_user_func_array
            [function] => module_invoke_all
            [function] => user_save
            [function] => user_profile_form_submit
nafmarcus’s picture

Using Drupal 7.15 and am calling user_save() and having the user picture disappear.
Is there a workaround? Is this something I can help with?
--
Figured out a workaround: If I load the $full_user account and copy the picture object into the $new_user array, the picture stays after saving the user.

starlocke’s picture

#46 - @nafmarcus - did you perform $user = user_load($uid); before user_save($user, $edit); ?

As far as I can tell, user_save() requires that its first parameter ($account) be a fully loaded data structure created using user_load(), or else, those pictures are going to get crushed when $account->picture = empty($account->picture->fid) ? 0 : $account->picture->fid; is run inside of user_save()

The documentation about user_save()'s first parameter should be updated to reflect that requirement and its consequence on user_picture trashing.

voughndutch’s picture

Is this for real? Has this issue been fixed? How does something do simple become such a glaring issue. I am having this issue in the latest drupal install this is make or break for my site how do I go about fixing it?

nafmarcus’s picture

Exactly as #47 replied to me. Do a user_load() and call save on that object. It's straightforward.

voughndutch’s picture

straight forward to whom? where does this user_load() call save on whatever object taking place? From my experience in drupal core should never be hacked so I ask again how can I fix this problem? layman's terms please or so child could understand.

voughndutch’s picture

I have core 7.22 I am getting this issue. I have dug into the code and it appears to be inline with comment #47 but the bug still exits. What gives. Is there any thing I can use to debug what maybe causing this issue?

TelFiRE’s picture

I am also experiencing this 7.22, I'm on Panopoly if that makes a difference.

Romlam’s picture

FileSize
1.02 KB

Here a patch that fix the problem, applied on 7.17, but not fully tested

Romlam’s picture

If you call user_save with an account object where the field "picture" is set to the fid, unchanged, you'll lose the picture after the save.

If you call user_save with an account object where the field "picture" is completely loaded (with file_load or user_load), the picture won't be lost.

But after the first save, when you load the user with user_load, the field "picture" contains the fid, so you lose it on the next call to user_save in the same request.

The problem is that after user_save saved the user, it puts it in a cache with the fid instead of the fully loaded field for the picture. So if you load the user with user_load and resave it in the same request (I know it's not logic but it happens more than you believe), you lose the picture field.

So my patch just load the field "picture" in user_save function if necessary and that fix the problem.
A better fix would load or keep the fully loaded field picture in the cache after user_save.

royerd’s picture

Thank you Romlam. Your patch worked for me. I had a rule that was updating the user account and it would remove the picture in the process.

geerlingguy’s picture

Status: Active » Needs work

I'm experiencing this problem both when logging in in tandem with LDAP, and when migrating users (when the Migrate module uses user_save(), the user object being saved sometimes has the fully-loaded file stored in $account->picture, and sometimes just the picture ID).

The patch in #53 does fix this (although it needs better documentation—just pointing at a URL is not enough), but it seems sub-optimal. However, if there's no other way to ensure the file object is always loaded with the user object if there's a picture present, then so be it.

Setting status to CNW since there's a patch, it works, but needs slight improvement before I'd be willing to RTBC.

TelFiRE’s picture

I have three rules on my site. The intention is to adjust their user roles based on their "Clan Status" profile field (a user profile field only my officers have access to). They use "After updating an existing user account, data comparison -- if [account:field-clan-status] is exceptional, good standing, out of date, or inaccurate, add user role GGk Member and remove user role Guest. The other rules are basically the same, but give/take different roles for different values of that field.

The patch solved the issue for me. I feel this is pretty important to go into core, this could really mess up a lot of peoples' databases and shouldn't have been allowed in release IMO.

geerlingguy’s picture

wodenx’s picture

Status: Needs work » Needs review
FileSize
1.22 KB

Here's a slightly different approach -- simply restore the full picture object to $account->picture after saving.

peximo’s picture

Issue summary: View changes
FileSize
737 bytes

The patch in #53 no longer applies, rerolled.

mimes’s picture

Reroll of patch in #60 applies and works. User image remains when custom field is created and set.

andypost’s picture

+++ b/modules/user/user.module
@@ -501,6 +501,10 @@ function user_save($account, $edit = array(), $category = 'account') {
+      // Fixes https://drupal.org/node/935592

this line should be changed to more descriptive

mgifford’s picture

60: 935592-60.patch queued for re-testing.

mgifford’s picture

FileSize
771 bytes
tassaf’s picture

Facing same problem..
which patch is the correct one?

mgifford’s picture

Try the last one and let us know if it works for you. That's usually the best approach (even if I wasn't the one re-rolling it).

If it does work, then you can consider marking it RTBC. The code is pretty simple:

+      // Fixes issue where user picture was deleted after calls to user_save().
+      if (is_object($account) && !is_object($account->picture) && $account->picture > 0) {
+        $account->picture = file_load($account->picture);
+      }
joelpittet’s picture

+++ b/modules/user/user.module
@@ -501,6 +501,10 @@ function user_save($account, $edit = array(), $category = 'account') {
+      // Fixes issue where user picture was deleted after calls to user_save().
+      if (is_object($account) && !is_object($account->picture) && $account->picture > 0) {
+        $account->picture = file_load($account->picture);
+      }
       $account->picture = empty($account->picture->fid) ? 0 : $account->picture->fid;

It's a bit strange that we are loading an object into $account->picture, then checking if the $account->picture->fid is empty and setting the whole object $account->picture back to 0 in the line directly after it.

Shouldn't that be NULL?
$account->picture = empty($account->picture->fid) ? NULL : $account->picture->fid;

That way the other is_numeric() tests later won't have to try to look up a 0 fid?

lokapujya’s picture

Status: Needs review » Reviewed & tested by the community

Been using this patch in #64 since October 2013 (previously #53 before it was rerolled) and it works. Drupal 7 has used 0 for a while, not sure what the consequences of changing to NULL would be.

lokapujya’s picture

Status: Reviewed & tested by the community » Needs review

On second thought, I think it should be reviewed some more.

Barnettech’s picture

Status: Needs review » Reviewed & tested by the community

I would like to see this moved to core. We're using this patch successfully in production and it just keeps getting overwritten if we update modules and core.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 64: 935592-65.patch, failed testing.

rclemings’s picture

Can anyone tell why 935592-65.patch is failing a test that 935592-60.patch passed? They look the same to me other than the comment. The test result is rather cryptic (at least to me):

The test did not complete due to a fatal error. Completion check common.test 659 CascadingStylesheetsTestCase->testRenderFile()

FWIW, 935592-65.patch worked perfectly for me in Drupal 7.26 with Rules 7.x-2.6. Like several other people here, I have a rule firing on "After updating an existing user account." The rule sets values for account:og-user-node (Organic Groups membership) based on the user's roles.

I'll requeue #65 for testing just because I feel the need to do something.

rclemings’s picture

Status: Needs work » Needs review

64: 935592-65.patch queued for re-testing.

rclemings’s picture

Status: Needs review » Reviewed & tested by the community

And so now it passes. Go figure.

voughndutch’s picture

What is the simplest way to apply this patch?

lokapujya’s picture

Issue tags: -Needs backport to D7
joelpittet’s picture

@voughndutch to apply a patch @see https://drupal.org/patch/apply

And I find the easiest is curl https://drupal.org/files/issues/935592-65.patch | patch -p1 from your drupal root. That opens the patch to standard output and sends the results to patch -p1.

Or another way wget https://drupal.org/files/issues/935592-65.patch then patch -p1 < 935592-65.patch

This is assuming you have wget or curl and patch. But the instructions will go into details for windows or other systems you may have.

David_Rothstein’s picture

Status: Reviewed & tested by the community » Needs work

I think we need to find the root cause of this bug - which code is passing in an account object to user_save() that does not have $account->picture set correctly? (It should always be an object or NULL, as returned from user_load().)

One culprit may be user_save() itself; it looks like it incorrectly modifies the passed-in $account object to switch $account->picture from an object to an integer. So if you do this:

user_save($account);
user_save($account);

The second one triggers the bug.

Seems like we should fix that directly and make sure that the version of $account you wind up with after user_save() is called is actually a proper user object.

And also write a test if possible (as per the "Needs test" tag on this issue).

lokapujya’s picture

lokapujya’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
1.23 KB
2.26 KB

Improved the tests. Added the solution from the prior patch, but put it after the database inserts (or else if fails the new tests.)

The last submitted patch, 80: 935592-80-test-only.patch, failed testing.

John Franklin’s picture

FileSize
2.25 KB

Ideally, the picture FID and the picture object should be stored in two separate user object fields, say $user->picture_fid and $user->picture. The way user_save() is coded, the $user->picture field is a dual-typed field, sometimes an integer, sometimes an object. (Such is the joy of dynamically typed languages.) However, adding picture_fid would mean changing the users table schema, and that's just not going to happen.

The current patch reloads the picture object to keep $user->picture always an object outside of user_save(), but at the cost of calling file_load(). Yes, the static cache minimizes the performance hit, but why reload it when the picture object does not need to be dropped in the first place? A slightly more efficient patch would be to save off the $user->picture field and then reassign it back.

Attached patch does exactly that, tests pulled from the previous patch.

lokapujya’s picture

+++ b/modules/user/user.module
@@ -501,6 +501,9 @@ function user_save($account, $edit = array(), $category = 'account') {
+      // Save the picture object, if it is set.  drupal_write_record() ¶

Can you just remove the blank space? and maybe in the comments, see if "expects" fits on the line above without exceeding 80 chars. Then I will RTBC.

John Franklin’s picture

FileSize
2.25 KB

Done. Patch from #82 with white space / comment fixes per #83.

lokapujya’s picture

Status: Needs review » Reviewed & tested by the community
dcam’s picture

Issue summary: View changes
Priority: Normal » Critical
Status: Reviewed & tested by the community » Needs work
+++ b/modules/user/user.test
@@ -1127,6 +1127,17 @@ class UserPictureTestCase extends DrupalWebTestCase {
+      // Verify that the user save does destroy the user picture object.

Shouldn't this be "does not destroy"?

I closed the follow-up issue, #1232776: Add test to ensure that user picture is not deleted after calls to user_save(), as a duplicate. The 8.x test that was in development there became irrelevant when the user image became a field. Now that we're adding a test for 7.x here there's no more need for that issue.

In response to #78 I updated the issue summary with information about the exact cause of this issue.

Also, I'm bumping up the priority to critical. I think it was mistakenly downgraded from major to normal in #44 despite the bug still existing in 7.x. Data loss is included in the definition of critical issues, but if someone else thinks this should be major then feel free to change it.

Something about this fix bothers me, storing the image's file object off to the side then reassigning it later. I think John Franklin is right in #82 though. It would probably take a schema or an API change to do things right. From what I can tell, this is probably the only way we can feasibly fix this old, embarrassing bug.

dcam’s picture

Issue summary: View changes
Issue tags: +Novice

I'm marking this as a Novice issue because all this patch lacks is a change to one of the comments. See #86.

Let's get this old bug fixed in the next release window!

joshi.rohit100’s picture

Status: Needs work » Needs review
FileSize
2.25 KB
855 bytes

Updated patch as per #87.

amitgoyal’s picture

FileSize
2.25 KB
880 bytes

Minor fixes of extra space and full stop.

dcam’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Novice

Good catches, @amitgoyal. Thank you and @joshi.rohit100 for working on this.

The patch looks good to me, so I'm RTBCing it. The tests-only patch is in #80.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 89: 935592-89.patch, failed testing.

dcam queued 89: 935592-89.patch for re-testing.

dcam’s picture

Status: Needs work » Reviewed & tested by the community
royerd’s picture

I guess these things take time, huh? This losing of the picture has been a problem now for 4 years. Odd that it has not been fixed in core.

rich.3po’s picture

Hi - i'm having this problem too, the patch in #89 seem to fix the issue for me thanks

As mentioned in #94, this has been hanging around for some years, can it be rolled into a full release?

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 89: 935592-89.patch, failed testing.

dcam queued 89: 935592-89.patch for re-testing.

dcam’s picture

Status: Needs work » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 89: 935592-89.patch, failed testing.

dcam queued 89: 935592-89.patch for re-testing.

dcam’s picture

Status: Needs work » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 89: 935592-89.patch, failed testing.

mgifford queued 89: 935592-89.patch for re-testing.

dcam’s picture

Status: Needs work » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 89: 935592-89.patch, failed testing.

lokapujya’s picture

Status: Needs work » Reviewed & tested by the community

lokapujya queued 89: 935592-89.patch for re-testing.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 89: 935592-89.patch, failed testing.

dcam queued 89: 935592-89.patch for re-testing.

dcam’s picture

Status: Needs work » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 89: 935592-89.patch, failed testing.

John Franklin’s picture

The tests that are failing have nothing to do with the patch. Can a core maintainer please look at and apply the patch?

lokapujya’s picture

Status: Needs work » Reviewed & tested by the community
wodenx’s picture

@john franklin just for the record, is there a substantive difference between this patch and mine from #59?

John Franklin’s picture

@wodenx, the patch in #59 is missing the automated test case. They are otherwise essentially the same.

garamani’s picture

The last patch #89 works fine.
This is a critical Bug and after upgrading Drupal core I forgot the patch. this patch must get committed.

lokapujya queued 89: 935592-89.patch for re-testing.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 89: 935592-89.patch, failed testing.

mgifford queued 89: 935592-89.patch for re-testing.

mgifford’s picture

Status: Needs work » Reviewed & tested by the community

Ok, back to RTBC then.

garamani’s picture

I don't know if my issue is related:
In my project, the nodes have the author picture and after updating the Author picture It's removed from the node page.
I have to clear the cache to see the image.

lokapujya’s picture

garamani: since your images show up after clearing cache, it is not related.

David_Rothstein’s picture

         return FALSE;
       }
+      // Restore the picture object.
+      $account->picture = $picture;

This should be moved a few lines up, so it still runs if the function aborts early.

+      $this->assert(is_object($account->picture), 'User picture object is valid after user load.');
...
+      $this->assert(is_object($account->picture), 'User picture object is valid after user save.');

These should be $this->assertTrue().

These are relatively small changes, so I'm leaving the issue at RTBC for now despite attaching a new patch.

The last submitted patch, 123: 935592-123-TESTS-ONLY.patch, failed testing.

dcam’s picture

I'll RTBC+1 #123. The test changes are obviously necessary. Sorry I didn't catch them earlier. Is the function change necessary though? $account isn't passed in by reference. I'm just curious and want to know if I'm missing something. Your change still looks ok to me.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 123: 935592-123.patch, failed testing.

dcam queued 123: 935592-123.patch for re-testing.

dcam’s picture

Status: Needs work » Reviewed & tested by the community
lokapujya’s picture

@dcam, regarding #125: It's not that drupal_write_record() changes the $account parameter; we are changing the $account->picture in line 508 to a file ID ( because that is what drupal_write_record() wants). The purpose of the change is to store the $account->picture and set it back after drupal_write_record is done.

David_Rothstein’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: +7.33 release notes

Committed to 7.x - thanks!

Is the function change necessary though? $account isn't passed in by reference.

$account is an object, and in PHP 5 all objects are passed by reference (effectively speaking): http://stackoverflow.com/questions/2715026/are-php5-objects-passed-by-re...

  • David_Rothstein committed 037848a on 7.x
    Issue #935592 by pillarsdotnet, lokapujya, David_Rothstein, John...
dcam’s picture

Of course. I had forgotten that. Thank you for your explanation.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

JamesRobertson’s picture

I'm using 7.34 - user picture still disappearing. Makes me look like an amateur. Hmm... maybe I am. Still could be worse... i could have been one of the 12 million drupal users that had their site hacked into... oh yeah that happened to me too... still at least its all free.. can't complain... mustn't grumble...

djween’s picture

I am also using 7.34 and user pic disappears for some users. Users.picture in DB shows 0 for these users no matter what I do. File_managed table has the file that was uploaded and data looks fine there.

update 3/5/15: Invite module was most likely causing my issue and patch #2 from the issue queue there remediated problem on my site: https://www.drupal.org/node/2292075

Adam Clarey’s picture

The last patch may have passed the style guide tests but it didn't actually fix the problem that has existed for over 4 years.

I've had patch 65 https://www.drupal.org/files/issues/935592-65.patch on production for many months fixing the issue, it was only after a recent core update removed the patch that i noticed the original problem or avatars being removed was back.

A simple test to is to have an avatar saved to an account then run:

$new_pass = array(
  'pass' => 'anything',
);

$account = user_save($user, $new_pass);

After this, the user ($user) will have no avatar in their account.

I've re-applied patch 65 and its working again.

The status of this issue needs to be set back to critical.

lokapujya’s picture

#136, what version of Drupal are you on?
also, did you read the comment right before yours? That poster's problem was that $user was not a fully loaded $user object.

Adam Clarey’s picture

Ive tried it on 7.34 and 7.35.

I'm not user if mine is exactly the same as the poster before. But for mine, $user is set from:

global $user;

lokapujya’s picture

Sounds like you need to load the object. I hope this helps you:

<?php
$account = user_load($user->uid);
?>
Adam Clarey’s picture

Why does that have to be a requirement? Why not just allow a user_save() on the global $user object? Adding the 3 lines of code will fix every situation where a user_save() occurs on the global $user object.

If the full $user object is provided, then the patch will see this and do no additional processing.

It just seems sloppy and prone to errors if you can only perform user_save() on the full user object.

John Franklin’s picture

isn't the right question: why isn't the global user object fully loaded?

Adam Clarey’s picture

I can understand that issue, you could have user profiles with many fields attached so only loading the data from the users table is a lot more efficient as you would not always need all those additional fields etc in general use.

John Franklin’s picture

Then the user record should include an "incomplete" or "user table fields only" flag to indicate this state.

lokapujya’s picture

Why not just allow a user_save() on the global $user object?

It could be risky to make that big of a change. For example: Are there any other fields that aren't loaded?

This issue fixed a bug as shown by the test case with minimal disruption. The fact that user_save requires a loaded object should be another issue, instead of reopening this issue (and could maybe link to this issue.) Infact, user_save() doesn't always return a loaded object is kind of related.: See:https://www.drupal.org/node/1177234.

Adam Clarey’s picture

The issue is "User picture is deleted after calls to user_save()" I've provided an iron-clad example of how to replicate this on the latest version of core.

There is no obvious reason why anyone would expect:

global $user;

user_save($user);

// Avatar is deleted.

This is a clear on obvious bug that has existed for 4 years!

Adding 3 simple lines will fix this in every situation without any detrimental effects. I don't understand why this is still being held back.

joelpittet’s picture

@Adam Clarey could you post those 3 simple lines in a patch with a test case as you put forth in #145?

May have to open a follow-up issue to this one as this one is getting long and is closed.

Adam Clarey’s picture

I used the patch in comment #64

https://www.drupal.org/files/issues/935592-65.patch

It's already passed the tests.

alex.skrypnyk’s picture

This is still an issue for Drupal 7.56.

The committed patch has tests that do not test all possible variations of the issue. More specifically, the test tests that calling the user_save() with an object that was just created works, but it does NOT test user_save() with a 'light' user object usually accessible from global $user. This 'light' user object has picture value loaded as FID rather then object and the code in user_save() is still unable to properly handle this situation.

Adam Clarey points out absolutely correctly that we need to handle the case where FID rather then file object is specified. We also need to add a test that will show that saving 'light' user object actually removes picture.

-----
@maintainers - what with the inability to re-open an issue? If it is still a problem - community has to be able to report it!

alex.skrypnyk’s picture

Ok, so apparently simpletest for D7 has no way of creating a 'light' user for tests (even using drupalLogin() does not refresh neither global user nor passed-in user object). This adds more uncertainty as creating a synthetic 'light' user is not the same is using currently logged in user object. Bummer.

alex.skrypnyk’s picture

FileSize
732 bytes

re-rolling patch from #64 for Drupal 7.56

alex.skrypnyk’s picture

FileSize
736 bytes

and another try with fixed notices

lokapujya’s picture

See comments #142 & #143: Calling user_save() on a non loaded user would be a bug.

Also, See #1177234: user_save doesn't return a "fully-loaded $user object": calling user_save() on an object returned from user_save() would currently be a bug.

numerabilis’s picture

After all, which patch should I apply?