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
Comment | File | Size | Author |
---|---|---|---|
#151 | 935592-151.patch | 736 bytes | alex.skrypnyk |
#150 | 935592-150.patch | 732 bytes | alex.skrypnyk |
#123 | interdiff.txt | 2.09 KB | David_Rothstein |
#123 | 935592-123.patch | 2.3 KB | David_Rothstein |
#123 | 935592-123-TESTS-ONLY.patch | 1.24 KB | David_Rothstein |
Comments
Comment #1
macgirvin CreditAttribution: macgirvin commentedfwiw: 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
Comment #2
Damien Tournoud CreditAttribution: Damien Tournoud commentedI can reproduce. The user picture is lost when submitting any of the user edit subpages exposed by the profile module.
Comment #3
Anonymous (not verified) CreditAttribution: Anonymous commentedattached 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.
Comment #4
Sivaji_Ganesh_Jojodae CreditAttribution: Sivaji_Ganesh_Jojodae commentedAttached patch will fix this. I am not sure whether it is a right way of handling this bug, but it works for me.
Comment #5
pillarsdotnet CreditAttribution: pillarsdotnet commentedRe-rolled against HEAD, with corrections.
Finally got user pictures working; yay!!!
Comment #6
pillarsdotnet CreditAttribution: pillarsdotnet commentedTypo...
Comment #7
pillarsdotnet CreditAttribution: pillarsdotnet commentedAnd maybe I posted this to the wrong issue...
Comment #9
pillarsdotnet CreditAttribution: pillarsdotnet commentedNice testbot...
Comment #11
spillz CreditAttribution: spillz commentedI 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.
Comment #12
pillarsdotnet CreditAttribution: pillarsdotnet commentedComment #13
Dave ReidSubscribing
Comment #14
DocuAnt CreditAttribution: DocuAnt commentedI can confirm this problem (Sorry for the misleading issue here: http://drupal.org/node/1022780).
Comment #15
teh_catt CreditAttribution: teh_catt commentedsubscribing - 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?
Comment #16
alpritt CreditAttribution: alpritt commentedWhat'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.
Comment #17
andypostsubscribe, 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...
This is a actual bug-fix!!!
Powered by Dreditor.
Comment #18
andypost#4: 935592_user_profile_picture_4.patch queued for re-testing.
Comment #19
alpritt CreditAttribution: alpritt commentedAs 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.
Comment #20
alpritt CreditAttribution: alpritt commentedThe 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.
Comment #21
alpritt CreditAttribution: alpritt commentedTurns 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.
Comment #22
matrobin CreditAttribution: matrobin commentedsubscribe
Comment #23
DocuAnt CreditAttribution: DocuAnt commentedIs 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...)
Comment #24
pillarsdotnet CreditAttribution: pillarsdotnet commentedBumping and tagging according to standard policy.
Comment #25
tcmug CreditAttribution: tcmug commentedI feel that this could be related to this: http://drupal.org/node/1126000
D7 - but anyways should apply.
Comment #26
Owen Barton CreditAttribution: Owen Barton commentedHere is a rerolled patch for 8.x (no changes)
Comment #27
mikeryanRan into this problem in the Migrate module on Drupal 7, applied patch as-is on D7 and fixes things nicely for me.
Comment #28
jdelaune CreditAttribution: jdelaune commentedWorked a treat. Cheers
Comment #29
DocuAnt CreditAttribution: DocuAnt commentedDon't forget comment #21.
There is a second approach out there #999004: user_save() relies on $edit instead of $account.
Comment #30
catch#26: 935592-user-pictures-del-26.patch queued for re-testing.
Comment #31
Dries CreditAttribution: Dries commentedI 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.
Comment #32
Dries CreditAttribution: Dries commented#26: 935592-user-pictures-del-26.patch queued for re-testing.
Comment #34
alpritt CreditAttribution: alpritt commentedThe 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.
Comment #35
alpritt CreditAttribution: alpritt commentedManual 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.
Comment #36
pillarsdotnet CreditAttribution: pillarsdotnet commentedIf you turn off the profile module, what effect does Account settings... enable user picture have?
Comment #37
rfayThis 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.
Comment #38
rfayLooks like I completely misunderstood #31, and this hasn't been committed. Sorry. However, it looks like the original title is appropriate at this point.
Comment #39
rfay#26: 935592-user-pictures-del-26.patch queued for re-testing.
Comment #41
pillarsdotnet CreditAttribution: pillarsdotnet commented@rfay: According to alpritt in #35,
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().
Comment #42
kiwidog CreditAttribution: kiwidog commentedHi, 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..
Comment #43
dimsum CreditAttribution: dimsum commentedSeconded this. Trying to update userfield thru Rules and User Picture gets deleted in the process.
Comment #44
tim.plunkettResetting status. Please follow-up in #1232776: Add test to ensure that user picture is not deleted after calls to user_save()
Comment #45
chx CreditAttribution: chx commentedI 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:Comment #46
nafmarcus CreditAttribution: nafmarcus commentedUsing 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.
Comment #47
starlocke CreditAttribution: starlocke commented#46 - @nafmarcus - did you perform
$user = user_load($uid);
beforeuser_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.
Comment #48
voughndutch CreditAttribution: voughndutch commentedIs 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?
Comment #49
nafmarcus CreditAttribution: nafmarcus commentedExactly as #47 replied to me. Do a user_load() and call save on that object. It's straightforward.
Comment #50
voughndutch CreditAttribution: voughndutch commentedstraight 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.
Comment #51
voughndutch CreditAttribution: voughndutch commentedI 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?
Comment #52
TelFiRE CreditAttribution: TelFiRE commentedI am also experiencing this 7.22, I'm on Panopoly if that makes a difference.
Comment #53
Romlam CreditAttribution: Romlam commentedHere a patch that fix the problem, applied on 7.17, but not fully tested
Comment #54
Romlam CreditAttribution: Romlam commentedIf 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.
Comment #55
royerd CreditAttribution: royerd commentedThank 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.
Comment #56
geerlingguy CreditAttribution: geerlingguy commentedI'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.
Comment #57
TelFiRE CreditAttribution: TelFiRE commentedI 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.
Comment #58
geerlingguy CreditAttribution: geerlingguy commentedSee also/related: #1177234: user_save doesn't return a "fully-loaded $user object".
Comment #59
wodenx CreditAttribution: wodenx commentedHere's a slightly different approach -- simply restore the full picture object to $account->picture after saving.
Comment #60
peximo CreditAttribution: peximo commentedThe patch in #53 no longer applies, rerolled.
Comment #61
mimes CreditAttribution: mimes commentedReroll of patch in #60 applies and works. User image remains when custom field is created and set.
Comment #62
andypostthis line should be changed to more descriptive
Comment #63
mgifford60: 935592-60.patch queued for re-testing.
Comment #64
mgiffordComment #65
tassaf CreditAttribution: tassaf commentedFacing same problem..
which patch is the correct one?
Comment #66
mgiffordTry 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:
Comment #67
joelpittetIt'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?
Comment #68
lokapujyaBeen 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.
Comment #69
lokapujyaOn second thought, I think it should be reviewed some more.
Comment #70
Barnettech CreditAttribution: Barnettech commentedI 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.
Comment #72
rclemings CreditAttribution: rclemings commentedCan 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.
Comment #73
rclemings CreditAttribution: rclemings commented64: 935592-65.patch queued for re-testing.
Comment #74
rclemings CreditAttribution: rclemings commentedAnd so now it passes. Go figure.
Comment #75
voughndutch CreditAttribution: voughndutch commentedWhat is the simplest way to apply this patch?
Comment #76
lokapujyaComment #77
joelpittet@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 topatch -p1
.Or another way
wget https://drupal.org/files/issues/935592-65.patch
thenpatch -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.
Comment #78
David_Rothstein CreditAttribution: David_Rothstein commentedI 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:
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).
Comment #79
lokapujyaA patch to add an initial test.
Comment #80
lokapujyaImproved the tests. Added the solution from the prior patch, but put it after the database inserts (or else if fails the new tests.)
Comment #82
John Franklin CreditAttribution: John Franklin commentedIdeally, 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.
Comment #83
lokapujyaCan 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.
Comment #84
John Franklin CreditAttribution: John Franklin commentedDone. Patch from #82 with white space / comment fixes per #83.
Comment #85
lokapujyaComment #86
dcam CreditAttribution: dcam commentedShouldn'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.
Comment #87
dcam CreditAttribution: dcam commentedI'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!
Comment #88
joshi.rohit100Updated patch as per #87.
Comment #89
amitgoyal CreditAttribution: amitgoyal commentedMinor fixes of extra space and full stop.
Comment #90
dcam CreditAttribution: dcam commentedGood 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.
Comment #93
dcam CreditAttribution: dcam commentedComment #94
royerd CreditAttribution: royerd commentedI 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.
Comment #95
rich.3po CreditAttribution: rich.3po commentedHi - 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?
Comment #98
dcam CreditAttribution: dcam commentedComment #101
dcam CreditAttribution: dcam commentedComment #104
dcam CreditAttribution: dcam commentedComment #106
lokapujyaComment #110
dcam CreditAttribution: dcam commentedComment #112
John Franklin CreditAttribution: John Franklin commentedThe tests that are failing have nothing to do with the patch. Can a core maintainer please look at and apply the patch?
Comment #113
lokapujyaComment #114
wodenx CreditAttribution: wodenx commented@john franklin just for the record, is there a substantive difference between this patch and mine from #59?
Comment #115
John Franklin CreditAttribution: John Franklin commented@wodenx, the patch in #59 is missing the automated test case. They are otherwise essentially the same.
Comment #116
garamani CreditAttribution: garamani commentedThe last patch #89 works fine.
This is a critical Bug and after upgrading Drupal core I forgot the patch. this patch must get committed.
Comment #120
mgiffordOk, back to RTBC then.
Comment #121
garamani CreditAttribution: garamani commentedI 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.
Comment #122
lokapujyagaramani: since your images show up after clearing cache, it is not related.
Comment #123
David_Rothstein CreditAttribution: David_Rothstein commentedThis should be moved a few lines up, so it still runs if the function aborts early.
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.
Comment #125
dcam CreditAttribution: dcam commentedI'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.
Comment #128
dcam CreditAttribution: dcam commentedComment #129
lokapujya@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.
Comment #130
David_Rothstein CreditAttribution: David_Rothstein commentedCommitted to 7.x - thanks!
$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...
Comment #132
dcam CreditAttribution: dcam commentedOf course. I had forgotten that. Thank you for your explanation.
Comment #134
JamesRobertson CreditAttribution: JamesRobertson commentedI'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...
Comment #135
djween CreditAttribution: djween commentedI 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
Comment #136
Adam Clarey CreditAttribution: Adam Clarey commentedThe 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:
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.
Comment #137
lokapujya#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.
Comment #138
Adam Clarey CreditAttribution: Adam Clarey commentedIve 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;
Comment #139
lokapujyaSounds like you need to load the object. I hope this helps you:
Comment #140
Adam Clarey CreditAttribution: Adam Clarey commentedWhy 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.
Comment #141
John Franklin CreditAttribution: John Franklin commentedisn't the right question: why isn't the global user object fully loaded?
Comment #142
Adam Clarey CreditAttribution: Adam Clarey commentedI 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.
Comment #143
John Franklin CreditAttribution: John Franklin commentedThen the user record should include an "incomplete" or "user table fields only" flag to indicate this state.
Comment #144
lokapujyaIt 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.
Comment #145
Adam Clarey CreditAttribution: Adam Clarey commentedThe 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.
Comment #146
joelpittet@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.
Comment #147
Adam Clarey CreditAttribution: Adam Clarey commentedI used the patch in comment #64
https://www.drupal.org/files/issues/935592-65.patch
It's already passed the tests.
Comment #148
alex.skrypnykThis 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!
Comment #149
alex.skrypnykOk, 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.
Comment #150
alex.skrypnykre-rolling patch from #64 for Drupal 7.56
Comment #151
alex.skrypnykand another try with fixed notices
Comment #152
lokapujyaSee 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.
Comment #153
numerabilis CreditAttribution: numerabilis commentedAfter all, which patch should I apply?