Don't rename user pictures

Please provide a patch. You can learn how to create patches at http://drupal.org/patch/create.

Adding tag

Does the test need updated?

Like this?

We have a constant called REQUEST_TIME which is a bit quicker than calling time() directly.

Please, test the patch before submitting. You can clearly see that the patch fails on some tests because (obviously) they are affected by your change. Make sure they work before attaching the patch file to the issue.

No, reviewed and tested by the community means the patch is OK to be committed to HEAD. This is not the case since the patch doesn't even pass the test bot. The patch needs work and it doesn't really matter if you do it or someone else.

You change the name of the picture file. I don't have the time to look at the test cases right now but I would assume that one of them did not expect this to happen and might thus be checking an incorrect file name. You can easily run these test yourself on your Drupal 7 development copy by enabling the simpletest module.

simpletest is in core - just go to admin/build/modules

Instead of db_result(db_query()) you should now use db_query()->fetchField(); in D7.

Yeah, SimpleTest is a bit confusing at first, but it saves SO much time on the whole by telling us when we've broken things. :D

A few things. I'm going to be extra nit-picky because it looks like this is one of your first core contributions; I do this only so that you learn best practices early and can apply them throughout your wonderful career as a core contributor! :D

1. "reviewed & tested by the community" is a status that means "Ok, this thing looks rock-solid and looks ready to become part of Drupal!" The only person who should ever change a patch status to RTBC is someone who is not the original creator of the patch. It's considered bad form to mark your own patch RTBC because it's tantamount to you saying "Ok, I deem my own code perfect," and a) leaves out the peer review process that makes Drupal core so solid, and b) developers are notoriously bad at finding flaws in their own code. ;)

2. -    $img_info = image_get_info($image->filepath);
   +    $img_info = image_get_info($image->filepath);    
   

   You're introducing some trailing whitespace here (there are invisible spaces at the end of the line). Could you please set your editor to not do that?

3. +    // for some reason the name of the user picture is different in files.filepath (picture-uid-REQUEST_TIME.ext) and files.filename (default simepleTest name, e.g.: image-test.gif)
   +    // therefore the only way to get the saved timestamp is to extract it from the saved full path
   

   Ok, we can't really commit comments like this to Drupal core. ;)

   a) All comments should wrap at 80 characters.
   b) All comments should begin with a capital letter and end with a period.
   c) But most importantly, we should get to the bottom of why that happened and document it appropriately. ;)

4. $pic_pathFromDB
   

   The Coding Standards dictate that variable names should:

   a) Not be abbreviated. (e.g. "pic")
   b) Be all lowercase.
   c) Have words separated by underscore.

On IRC, Damien Tournoud also mentioned that the caches should be invalidated automatically since the browser should issue a GET request with a E-Tag and a If-Not-Modified-Since each time the page is refreshed.

So I do think this needs a bit more looking into before it's ready to commit.

Don't like this as is. Browser caching of these images is useful. I can see how it is a problem in the case where users change images frequently, but thats the special case, not normal case.

Thanks for the clarification. I should have recognized that. This patch is useful as is. Perhaps a follow-up patch could fix this for all images by providing an option to file_create_url so that you could be a URL with the file upload timestamp appended.

Different modules do different things with file API. IMCE lets you browse your library of images and change images as needed. It would be nice if those changed images were immediately downloaded.

This does not seem like a good way to do that. HTTP/1.1 has some very fine-grained cache control strategies. If you are using public file downloads, it's a simple matter of configuring your web server to issue an explicit Expire: header. This will avoid the heuristic expiration defined by HTTP/1.1:

Also, if the response does have a Last-Modified time, the heuristic
expiration value SHOULD be no more than some fraction of the interval
since that time. A typical setting of this fraction might be 10%.

(ie. "if your image has not been modified for 10 days, expiration will be set 1 day in the future)

I don't believe that this patch is needed at all.

Because Drupal ships with a good example .htaccess, it is just a matter of changing the interval in:

# Requires mod_expires to be enabled.
<IfModule mod_expires.c>
  # Enable expirations.
  ExpiresActive On

  # Cache all files for 2 weeks after access (A).
  ExpiresDefault A1209600

  # Do not cache dynamically generated pages.
  ExpiresByType text/html A1
</IfModule>

You can change it to 1 hour (3600s) or less for picture users by adding some code inside the IfModule block.

Yes, separate issue is fine ... Modules that just change the filesytem directly are bypassing the API and will suffer for it just like the rest of drupal (e.g. not using node_save() and changing a term directly - no action gets fired.)

I was bitten by this today (again). The issue is that server directs the browser to cache images via mod_expires and not ask the server again for the same file during that time-frame.

I've researched whether a Files, Location, or Directory directive could work out to special-case the files/pictures directory, but that turned out to be a mess, because only Files is allowed in .htaccess.

Of course, user.module could put another .htaccess file into files/pictures to set a different expiration time or completely disable mod_expires -- but that would not fix the downstream issue with ImageCache.

The only viable solution was (actually) the same change as in this patch (without knowing this issue beforehand) - appending filemtime($picture->filepath).

I never had this issue with other files and images. Only regarding user pictures, innocent end-users do not understand (reasonably), why their user picture does not change after uploading a new one.

So... +1 for this patch.

Why do we rename user pictures in the first place again?

I believe this was done to explicitly ask for the new file before any cache alternative had been proposed.

Sorry, I mean why do we rename pictures to picture-UID.jpg instead of just leaving the original filename?

After an IRC discussion I believe a better solution than appending the timestamp might be available.

Since the implementation of #357403: Move user picture to managed files there is no longer a need to rename the uploaded picture files to the format picture-uid.ext. I assume this has been done in previous Drupal releases since the only reference to the picture file was the filename and the storage of the filename in the users table.

If we keep the filename provided by the user who uploads the picture, and let the Drupal file API handle file-exists cases we don't need to modify the headers or append a timestamp to the file since it will already be unique. We can simply mark the previous user picture as temporary and have the files garbage collector remove it.

I will attempt to provide a patch later today when I have some time to fiddle around with it.

I attached a simple patch which does what I suggested earlier in comment #51.

Interesting to point out that the previous user picture is removed AFTER the current picture has been uploaded, thus it will not overwrite the file but rename it. I also used pathinfo() to extract the original filename without extension and append the extension retrieved from the image information to make sure that's still correct.

We should benchmark this a bit. The nice thing about picture-UID.png is that it doesn't require a hit to the database. (Or at least I think so...)

Can we see before/after a node with 100 comments with user pictures enabled?

@webchick: Wrong assumption. ;)

      // Multiple comment view.
      $query_count = 'SELECT COUNT(*) FROM {comment} c WHERE c.nid = %d';
      $query = 'SELECT c.cid as cid, c.pid, c.nid, c.subject, c.comment, c.format, c.timestamp, c.name, c.mail, c.homepage, u.uid, u.name AS registered_name, u.signature, u.picture, u.data, c.thread, c.status FROM {comment} c INNER JOIN {users} u ON c.uid = u.uid WHERE c.nid = %d';

Ah, great. :) Thanks, sun!

@derjochenmeyer: Those characters are - technically - no problem. However, you are free to use http://drupal.org/project/transliteration to intercept all uploaded files and (automatically) convert their filenames to ASCII.

I guess I should have replied earlier: Removing this needless renaming is definitely the proper fix.

Patch looks good. However, bot seems to hang in re-testing (?).

Setting to needs review to get a bot test.

Bot's green. RTBC.

While webchick's assumption in #55 was wrong, it is still an interesting idea ... why wouldn't we do it like that and save some database queries?

I also agree with the comment in #57 -- it feels more clean to rename the picture, plus it avoids people playing games with URLs, etc.

Last but not least, it seems like I might be able to overwrite another user's picture now?

Still needs review.

It would indeed be possible to overwrite another user's picture. Didn't think of that.

We could still add the UID to the picture somehow, add it as a suffix to the original filename:

$destination = file_create_path(variable_get('user_picture_path', 'pictures') . '/' . $account->uid . '-' . $path_info['filename'] . '.' . $image_info['extension']);

Renaming the picture is indeed a good idea and it would, in some cases, allow you to save some queries, but I don't know whether or how we could really solve the problem otherwise.

What's wrong with the _0 thing which upload module does?

I was about to work on this more only to discover that it was fixed in a duplicate issue
#668058: Resubmitting a user picture does show the first uploaded picture