User photos in user.module should be managed files. This would let the D7 version of imagecache know that a user's profile picture has changed and flush the old thumbnail. I've done a little work on this and it's been a good exercise because it illustrates some problems with the reference counting blocking deletions and the question of overwriting managed files in #334303: Handling overwriting of managed files (with unittests!).
Comment | File | Size | Author |
---|---|---|---|
#26 | user_357403.patch | 16.16 KB | drewish |
#22 | user_357403.patch | 15.81 KB | drewish |
#20 | user_357403.patch | 15.5 KB | drewish |
#18 | user_357403.patch | 15.48 KB | drewish |
#14 | user_357403.patch | 15.9 KB | drewish |
Comments
Comment #1
Dave ReidInteresting patch drewish. Looks like there's some other small patches integrated into this (no file upload showing an error message), but this seems like a very appropriate direction to head towards.
Comment #2
drewish CreditAttribution: drewish commentedThis gets it closer. I'm leaving the field name as {users}.picture because I didn't realize that node and comment are loading the field as part of their queries and that theme_user_picture() can get a node, comment or user as an "account".
Comment #3
drewish CreditAttribution: drewish commentedfixed some really stupid bugs. seems like this is working pretty well. the current tests pass but i need to expand the tests some to double check that if you upload the same type of file it recycles the fid but if the extension changes that a new fid is generated and the old one deleted.
Comment #4
drewish CreditAttribution: drewish commentedthat patch didn't update the test...
Comment #5
drewish CreditAttribution: drewish commentedfixed a comment grammar issue webchick identified.
Comment #6
drewish CreditAttribution: drewish commentedforking some of the cleanup from this patch to: #358021: Cleanup cruft from removal of hook_user($op)
Comment #7
drewish CreditAttribution: drewish commentedre-rolling now that #358021 has been committed.
Comment #8
catchSubscribing. Only had a quick scan through, but why the old placeholder style for db_query_range()? Is that not converted yet? Otherwise this is very much needed to make user pictures less annoying.
Comment #9
drewish CreditAttribution: drewish commentedcatch, good eye (i'd have said good catch but that might have sounded odd), just old code. the first version of this patch was done back in november before most of DBTNG was committed. Fixed that in the attached patch.
Comment #10
drewish CreditAttribution: drewish commentedah, so rerunning that update on an empty pgsql database helped locate a problem with the update. when there were no records to update it got stuck in an infinite loop. fixed that.
Comment #11
drewish CreditAttribution: drewish commentedComment #12
drewish CreditAttribution: drewish commentedcorrected all the issues catch encountered.
Comment #13
drewish CreditAttribution: drewish commentedComment #14
drewish CreditAttribution: drewish commentedwhoops the fix didn't attach
Comment #15
drewish CreditAttribution: drewish commentedComment #16
Dave ReidLooks like this patch will fix most of the user picture issues I've been working on. :)
#173493: User picture fixes and cleanup - This is not actually fixed in this issue, although it looked like it was going to.
#343785: picture_upload and picture_delete should not be saved into $user->data + form cleanup - Incorrectly fixed (see #17 below).
What do you think of making the user_edit_form code this:
It seems cleaner to use the FAPI #access instead of having to redeclare the picture_delete element. Also note the renamed 'picture_current' from 'current_picture' to match the naming of the other elements.
Anyway, the upgrade path worked just fine for my test install, albeit with not a large number of users, but it worked.
Comment #17
Dave ReidIn order to not serialize the picture_upload/delete values in the $user->data field,
Needs to be:
Comment #18
drewish CreditAttribution: drewish commenteddavereid, excellent ideas. it seemed to make more sense to just always call theme('user_picture') so we show their default image if one is available.
Comment #19
drewish CreditAttribution: drewish commentedComment #20
drewish CreditAttribution: drewish commentedthis fixes #173493: User picture fixes and cleanup
Comment #21
Dave ReidComment #22
drewish CreditAttribution: drewish commentedFixed those two issues.
Comment #23
catchHad another look through and everything looks great. Test bot is also happy.
webchick raised possible performance concerns in irc - when sites with millions of users end up with millions of rows in the {files} table - that's possible, but IMO we should use our APIs in core as much as possible and optimise them where necessary. The more special cases we can remove the better.
Comment #24
Dave ReidI put in a couple test users with pictures and then tried the update, and it just processed and processed and processed without end. When I just hit my browser's stop button and went back to update.php, I had a metric crap-load of "Warning: Division by zero in _batch_process() (line 301 of /home/davereid/Projects/drupal-head/includes/batch.inc" errors, and the user picture update was still listed as available.
Comment #25
Dave ReidOk, it's an error in a recent change in HEAD. The update worked fine and confirmed fixes #173493: User picture fixes and cleanup.
Comment #26
drewish CreditAttribution: drewish commentedchanged the update to process 500 rows at a time.
fixed the comment's formatting template_preprocess_user_picture() and expanded the @TODO comment to clarify what needs to change in the future.
Comment #27
webchickGreat. Thanks for all of your work on this, drewish and Dave!
Committed to HEAD. :) Please add documentation.
Comment #29
Oleksa-1 CreditAttribution: Oleksa-1 commentedI faced with a problem cause of this change in D7 #1613640: Not possible to share user pictures in multi-site d7