imagecache private and public downloads

Ok, I see how your method is supposed to work, there's only one thing that you either forgot or just don't know:
Public downloads are not supposed to be handled by Drupal code. The point of public downloads is that Drupal doesn't have to be loaded at all, which makes for a tremendous speedup on high-traffic sites.

What the original module does is store the cached image to a place where it can be accessed without ever invoking Drupal (in the files/ directory). The only additional action is that it provides a menu path for the exact same location as long as the file isn't created there, but when it is then this menu path will be gone and you can download the file directly from http://example.com/files/imagecache/preset/image.png. Again, without Drupal being invoked at that point.

And that is also the reason why Public downloads can never work without Clean URLs, because without Clean URLs, Drupal can't register a menu path that exactly matches the real file path (there's always the "?q=" in between, which makes Drupal being called, and the "alias name" can't work this way).

What your code does is make all the imagecache requests go to a single location, and transfer it with file_transfer(), no matter if public or private downloads are selected. So yes, your patch makes it work with all download methods, but at the expense of scrapping all of the Public download method's advantages, and essentially making it a Private download as well. This is bad.

If you want Private downloads, select Private downloads as download method. But do not sacrifice the one advantage of Public downloads by patching it all to the least common denominator.

Thanks for your explanation. In that sense there is no way in the current method of imagecache to have direct access to public files. On an image request, imagecache first needs to check if the file exists in the 'imagecache' directory, and if not, create the file, so every file request needs to go through drupal php code.
A possible way to prevent this, is to create the imagcache file immediately on uploading a file, recreate all imagecache files on a flush, and delete the imagecache file on the original file's deletion. In that way image requests can be relocated to the existing files in the 'imagecache' directory, and no check needs to be made if the file exists there (because it always will). So, the imagecache_cache function will only be called at file uploads to create the corresponding imagecache file, and access to imagecache files is handled by drupal in case of private files, and direct in case of public files. The MENU_CALLBACK for requests in the 'imagecache' directory won't be necessary anymore.

But I remember you saying that imagecache was not meant to pre-generate images, so when the choice is between slow access or not working at all (by displaying the error about public downloads and clean URL's) I opt for slow access.

The good thing about my patch is that it doesn't show the full file path (server path in case of private downloads) as a GET variable. It just strips off the file_create_path() part of the filename, and adds it back in the imagecache_cache function. This means that the entire structure of the drupal file system directory is not repeated in the 'drupal_file_system/imagecache' directory. Furthermore, I rearranged the code of the file_create_path(), which makes it a bit faster (and more logical).

I decided to merge the ideas of jpetso and my own in another patch. In this new patch, theme_imagecache doesn't relocate to the same location (as in my previous patch), but to file_create_url(file_directory_path(). /imagecache/presetname/relpath), where presetname is the name of the preset, and relpath the name of the original file, relative to the drupal file system directory. The result of this is always a clean url (e.g. http://yoursite/files/imagecache/presetname/file1.jpg). It seems that drupal still correctly links clean urls to the menu system when the drupal clean url function is turned off (however, I tested this only on my own site). If I'm wrong here, then there is indeed no way to have 'unclean' urls work with imagecache, which means that my previous statements concerning 'unclean' urls are wrong. So, I should notice here that I'm not sure if drupal always intercepts clean urls to the MENU_CALLBACKs, even when clean urls is turned off. If not, than a clean url requirement warning should be in place.

At least, I've taken into account the remarks of jpetso, and my patch now works correctly with public downloads; they can directly be referred to as clean urls.

Furthermore, relative private paths in the form of '../../files' are supported (which was also a major point in the previous version of this patch, which I forgot to say).

The modified examples:

An example:

• A file request is made to theme_imagecache. The presetname is 'preset1', and the original file is in '/files/image1.jpg'. (download method is public).
• theme_imagecache strips the part of the path that is also in file_create_path(), so the remaining part is 'image1.jpg'.
• the stripped path is added to 'imagecache/preset1/' and a url is created to this path: file_create_url('imagecache/preset1/image1.jpg');
• if the file exists there, it can be directly referred to. If not the following happens:
• Since the first part of this path is file_directory_path(). 'imagecache', it is intercepted by the MENU_CALLBACK, and the callback function imagecache_cache is executed
• the imagecache_cache function creates the modified file in file_create_path(). 'imagecache/preset1/image1.jpg', and returns its content

Another example:

• A file request is made to theme_imagecache. The presetname is 'preset2', and the original file is in '../../files/test2/image2.jpg'. (download method is private).
• theme_imagecache strips the part of the path that is also in file_create_path(), so the remaining part is 'test2/image2.jpg'.
• the stripped path is added to 'imagecache/preset2/' and a url is created to this path: file_create_url('imagecache/preset2/test2/image2.jpg');
• since the first part of this path is 'system/files/imagecache', it is intercepted by the MENU_CALLBACK, and the callback function imagecache_cache is executed
• the imagecache_cache function creates the modified file in file_create_path(). 'imagecache/preset2/test2/image2.jpg';, if it not already existed there, and returns its content

Watching...

Something just came into my mind: I think I known why in my case clean urls are still intercepted by drupal if clean urls are turned off and the corresponding file does not exist: it's because I have used both clean and unclean urls for my website in the past, so the .htaccess files are probably in the right place, doing the right thing, even though the clean url setting is turned off. This also brings me to a solution for 'unclean' urls with public downloads that has only a minor speed disadvantage:

In the function theme_imagecache we first check if the combination of 'unclean' urls and public downloads occurs

• true:
  • We subsequently check if the corresponding imagecache file exists in the imagecache location
  • true: produce a link to the file as normal, with file_create_url(file_directory_path(). '/imagecache/presetname/relpath'),
    where presetname is the name of the preset, and relpath the name of the original file, relative to the drupal file system directory.
  • false: We should refer to the file using a query string instead of a clean url. I don't know which function can do this, or maybe it should be hard-coded. The drupal system will intercept this query call and execute the imagecache_cache function. This happens only when the imagecache file does not exists, i.e. only the first time.
• true: produce a link to the file as normal, with file_create_url(file_directory_path(). '/imagecache/presetname/relpath'),
  where presetname is the name of the preset, and relpath the name of the original file, relative to the drupal file system directory.

In this fashion, it is possible to have 'unclean' urls and public downloads, at the expense of checking if the imagecache file exists, which, I think, is faster than returning the file through file_transfer. Only when the imagecache file needs to be created for the first time, the file is returned through the file_transfer function.

Sorry, I'm working at home now, so no patch file this time.

Second

• true

should be

• false

sorry for that.

And here is the patch implementing the idea described above.

Subscribing, will test on lighttpd.

I updated my patch from issue #137826 to do the same thing. Slightly less intrusive (188 lines in the diff file instead of 354), and achieving the same thing. I incorporated most of leo_pape's good ideas and improved on other ones.

• On public download / clean URL systems, imagecache works as before. On public download / unclean URL systems or with private downloads enabled, imagecache uses system/files/imagecache/[path].
• imagecache_cache() is slightly restructured in order to avoid generation of multiple files with the newly supported download methods, to delay code execution as late (and conditional) as possible, to get rid of the obsolete $generate variable, and to scrap the "Content-Length:" header for file_transfer().

The "Content-Length:" header was removed because
a) it also works without this one, also the Image module doesn't provide it
b) we had a problem downloading certain files with "Content-Length:" included, even though the length was set to the right size, whereas without this one it worked fine.

Would someone care to test and review?

To test these patches I am using private files, clean urls and a preset called 'gallery' called using the theme() function. This 2-step preset performs a scale outside of 165x165 and then crops to 165x165 from center.

Patch #7 works for me! Imagecache sucessfully shows the scaled, cropped image.

With patch #9, imagecache presents the original image and does not perform any preset actions on it :-(.

I realize that these patches refer to different versions of the module and have used the proper patch for each and flushed the imagecache preset images before testing each.

Please let me know if I can contribute to the progress of this issue. Thank you!

would love to help test these patches... i'm looking for exactly the functionality they implement.

i don't have much experience applying patches, and i'm confused as to which version of imagecache.module i should be applying these to. i tried using the current dev version from the imagecache project page, but that gave numerous errors.

could someone give a little guidance on this? thanks all for your work on this!

It would be great if we could get more feedback about these patches. If you view the patch file in a text editor you can see the Revision # to which the patch refers and then download that revision from cvs. If you like using the web view of Drupal CVS, you can find imagecache.module at http://cvs.drupal.org/viewcvs/drupal/contributions/modules/imagecache/im... with all revisions listed.

Thanks!

can someone attach a fully patched version of the imagecache module please please please. I've been trying to install a patching program with no luck and I don't want to have to go through this thing line by line.

(Oh yeah I'm running vista and Cygwin won't work properly because it says that it does not have permission to write in that directory.)

So yeah...will someone please attached a completely patched module here. (I was thinking of using patch #7)

Thanks in advance.

Patch 7 is relative to version 1.19.2.27 2007/04/19 22:58:44

After trying the various patches from this thread, I'm still having trouble getting cached images to show using the views module in Drupal 5.1 with downloads set to "private". Do I have to use the theme() function to call them or am I missing something else?

patch #7 works perfectly for me as well! thanks a lot for the fix, leo_pape

just realized a problem with this patch, it completely ignores any permission checks that are performed for the original unmodified files and that should also apply to the cached image. Just like the code in upload_file_download() it should not be allowed to download an image if the user is not allowed to access the node the image is associated with. I think it should be quite easy to fix the patch to account for this problem by adding some checks similar to those in the upload_file_download() method somewhere in the beginning of the imagecache_cache method.

Just updating status.

An update to my patch from comment #9, adding a node access check and the best possible Last-Modified header (containing the time of the last file modification = mtime = stat($file)[9]) for file downloads. upload.module's "view uploaded files" check is module specific and therefore not included, and whatever caused the preset actions not to work is likely to still not work at your place if it didn't do so before.

In any case, I push this back to "code needs review" status.

Watching

Subscribing.

*subscribing*

I'm still concerned that this patch completely breaks protecting images from the public eye... Once a user with perms to view a file views it... well everyone can see it...

Or is it not saved with this patch, but generated on the fly each time... (this is scary)....

Or
if private files then check access
if !file_exists then generate derivative
serve file

I can't really tell from the patch and it doesn't apply anymore... I can probably blame myself for that :) anyone up to re-rolling it?

> I'm still concerned that this patch completely breaks protecting images from the public eye...
> Once a user with perms to view a file views it... well everyone can see it...

Well, the image is in the file directory, that's true. But then, if you're worried about privacy (like we are) you'll deny direct access to the file directory anyways, right? Could you explain why everyone can see the file after it has been generated?

> Or is it not saved with this patch, but generated on the fly each time... (this is scary)....

You mean the patch causes the image to always be regenerated, even if it's there? The
if (!is_file($destination) && !is_file($tmpdestination))
condition should actually prevent the image always being generated.

> Or
> if private files then check access
> if !file_exists then generate derivative
> serve file

I don't quite get the point of that one, but it doesn't really matter anyways because:

> I can't really tell from the patch and it doesn't apply anymore... I can probably blame myself for that :) anyone up to re-rolling it?

Yeah, sure, I'm in for rerolling it. (It's my patch after all.) Not immediately, however... I've got some more pressing issues that I need to tackle before coming back to this one.

>> Or
>> if private files then check access
>> if !file_exists then generate derivative
>> serve file

> I don't quite get the point of that one, but it doesn't really matter anyways because:

The point of that one is to provide access control over the derivatives... a more verbose version would be
I would probably add a perm per preset...

function imagecache_cache_private() {
if !file_exists($derivative) {
create derivative
}
if user_access('view private imagecache files') {
file_transfer($derivative)
exit
}
drupal_access_denied;
}

committed to HEAD with modifications, see rev 1.26.

Whoo! Three cheers for dopry!

Thank you!!!

that made my day! Thanks dopry!

thanks peeps. was just needing this code.