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!).

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Dave Reid’s picture

Interesting 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.

drewish’s picture

FileSize
16.12 KB

This 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".

drewish’s picture

Status: Needs work » Needs review
FileSize
16.82 KB

fixed 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.

drewish’s picture

Issue tags: +D7FileAPIWishlist
FileSize
18.16 KB

that patch didn't update the test...

drewish’s picture

FileSize
18.15 KB

fixed a comment grammar issue webchick identified.

drewish’s picture

forking some of the cleanup from this patch to: #358021: Cleanup cruft from removal of hook_user($op)

drewish’s picture

FileSize
15.44 KB

re-rolling now that #358021 has been committed.

catch’s picture

Subscribing. 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.

drewish’s picture

FileSize
15.46 KB

catch, 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.

drewish’s picture

FileSize
15.57 KB

ah, 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.

drewish’s picture

[4:13pm] catch:
drewish: + * Implementatin of hook_file_delete().
[4:14pm] catch:
drewish: this bit - can we change to a @TODO
[4:14pm] catch:
+      // It's kind of dirty to do this way but it works. It would be best to
[4:14pm] catch:
+      // get the node and comment code loading the files.

...

[4:27pm] catch:
drewish: I'm having trouble deleting a user picture.
[4:27pm] catch:
    * The file picture_upload could not be saved, because the upload did not complete.
[4:27pm] catch:
    * Failed to upload the picture image; the pictures directory doesn't exist or is not writable.
[4:27pm] catch:
drewish: uploading and replacing is fine though.
[4:28pm] drewish:
catch: oh, that's a separate issue
[4:28pm] drewish:
catch: http://drupal.org/node/30520
[4:28pm] Drupl775:
http://drupal.org/node/30520 => file_save_upload() not notifying user when PHP upload limit is exceeded. => Drupal, file system, normal, patch (code needs review), 11 IRC mentions
[4:28pm] drewish:
apply that and you'll be fine
[4:29pm] drewish:
catch: file_save_upload() should be returning NULL, that's what the patch is expecting

...

[4:36pm] catch:
drewish: one more typo - Foriegn key
drewish’s picture

FileSize
15.91 KB

corrected all the issues catch encountered.

drewish’s picture

FileSize
0 bytes
[4:53pm] catch:
drewish: sorry, just spotted:
[4:53pm] catch:
+  db_update('users')
[4:53pm] catch:
+      ->fields(array('picture' => NULL))
[4:53pm] catch:
+      ->condition('picture', $file->fid)
[4:53pm] catch:
+      ->execute();
[4:54pm] catch:
extra two spaced.
drewish’s picture

FileSize
15.9 KB

whoops the fix didn't attach

drewish’s picture

[5:21pm] catch:
drewish: I think that patch is pretty close to rtbc, wouldn't hurt to have one more look over it though.

...

[5:23pm] catch:
drewish: I reckon yours is a big enough change it could do with a third set of eyes, but it works great.
Dave Reid’s picture

Looks 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:

    $form['picture'] = array(
      '#type' => 'fieldset',
      '#title' => t('Picture'),
      '#weight' => 1,
    );
    $form['picture']['picture'] = array(
      '#type' => 'value',
      '#value' => $edit['picture']
    );
    // If the user does not currently have a picture, hide the preview and the
    // delete checkbox.
    $form['picture']['picture_current'] = array(
      '#markup' => theme('user_picture', (object)$edit),
      '#access' => !empty($edit['picture']->fid),
    );
    $form['picture']['picture_delete'] = array(
      '#type' => 'checkbox',
      '#title' => t('Delete picture'),
      '#description' => t('Check this box to delete your current picture.'),
      '#access' => !empty($edit['picture']->fid),
    );
    $form['picture']['picture_upload'] = array(
      '#type' => 'file',
      '#title' => t('Upload picture'),
      '#size' => 48,
      '#description' => t('Your virtual face or picture. Maximum dimensions are %dimensions and the maximum size is %size kB.', array('%dimensions' => variable_get('user_picture_dimensions', '85x85'), '%size' => variable_get('user_picture_file_size', '30'))) . ' ' . variable_get('user_picture_guidelines', ''),
    );

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.

Dave Reid’s picture

Status: Needs review » Needs work

In order to not serialize the picture_upload/delete values in the $user->data field,

+    unset($edit['picture_upload']);
+    unset($edit['picture_delete']);

Needs to be:

+    $edit['picture_upload'] = NULL;
+    $edit['picture_delete'] = NULL;
drewish’s picture

FileSize
15.48 KB

davereid, 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.

drewish’s picture

Status: Needs work » Needs review
drewish’s picture

Dave Reid’s picture

Issue tags: +user pictures
drewish’s picture

FileSize
15.81 KB
[1:22pm] chx:
drewish: a copypaste error
[1:22pm] chx:
+function user_file_references($file) {
[1:22pm] chx:
+  // If upload.module is still using a file, do not let other modules delete it.
[1:23pm] chx:
drewish: user.module
[1:23pm] drewish:
chx: ah good catch
[1:23pm] chx:
drewish: +    ->fields(array('picture' => NULL))
[1:23pm] chx:
drewish: NULL?
[1:23pm] chx:
drewish: why not 0?
[1:23pm] dru left the chat room. (Read error: 104 (Connection reset by peer))
[1:23pm] chx:
drewish: is that field NULLable at all?
[1:24pm] drewish:
i think not

Fixed those two issues.

catch’s picture

Status: Needs review » Reviewed & tested by the community

Had 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.

Dave Reid’s picture

Status: Reviewed & tested by the community » Needs work

I 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.

Dave Reid’s picture

Status: Needs work » Reviewed & tested by the community

Ok, it's an error in a recent change in HEAD. The update worked fine and confirmed fixes #173493: User picture fixes and cleanup.

drewish’s picture

FileSize
16.16 KB

changed 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.

webchick’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: +Needs documentation

Great. Thanks for all of your work on this, drewish and Dave!

Committed to HEAD. :) Please add documentation.

Status: Fixed » Closed (fixed)

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

Oleksa-1’s picture

I faced with a problem cause of this change in D7 #1613640: Not possible to share user pictures in multi-site d7