Support from Acquia helps fund testing for Drupal Acquia logo

Comments

quicksketch’s picture

Yep, I've got a couple "TODOs" in the module where the code would need to be updated. I never personally use private downloads but it shouldn't be too difficult to implement.

marquardt’s picture

Here's a patch that implements the support for private uploads.

I'm not sure if the implementation of hook_file_download() is sufficiently safe; a better way IMHO would be to use the files table to also keep track of the resized files, and then check that any file requested for download has indeed been created by the module (i.e., is listed in the files table). This does add some overhead to handle the database queries to maintain the entries during file generation and clean up, though. And somehow breaks the simplicity of this module which otherwise relies on the file system... I could provide a patch implementing this as well.

marquardt’s picture

Status: Active » Needs review
quicksketch’s picture

Status: Needs review » Needs work

Thanks this looks generally pretty good. Some problems though:

- Code style: Use only two spaces instead of tabs when indenting
- Missing a "break" in the switch statement will cause public downloads to fail. Maybe an IF statement is more appropriate.

+	switch (variable_get('file_downloads', FILE_DOWNLOADS_PUBLIC)) {
+	  case FILE_DOWNLOADS_PUBLIC:
+            $local_path = preg_replace('/(http[s]?:\/\/'. preg_quote($_SERVER['HTTP_HOST'], '/') .')?'. preg_quote(base_path(), '/') .'/', '', $src, 1);
+	  case FILE_DOWNLOADS_PRIVATE:
+            $local_path = file_directory_path() . '/' . preg_replace('/(http[s]?:\/\/'. preg_quote($_SERVER['HTTP_HOST'], '/') .')?'. preg_quote('/system/files/', '/') . '/', '', $src, 1);
+	}

- Although I can see why you used file_create_url(), I don't like using this function because it uses the full URL instead including the http host. We can fix this by using the function, but then using str_replace() to remove the $_SERVER['HTTP_HOST'] (similar to what we do in the above switch statement).

marquardt’s picture

Uhh - you are right. Sorry for the mess with the tabs and break!

Rerolled...

I've kept the preg_replace in order to cater for http[s]. Would it make sense to apply a similar logic when setting image['original'] in line 332? I've found myself copy-pasting full image links from the upload field over and over again.

Thanks for your help!

marquardt’s picture

Status: Needs work » Needs review

Should have updated that straight away...

quicksketch’s picture

Status: Needs review » Needs work

This patch makes the assumption that ALL images will be located at the private file system path if Drupal is set to use private files. There's no reason why Image Resize Filter can't resize images that are not within the private directory. I'm working on rerolling this to overcome this limitation.

quicksketch’s picture

Status: Needs work » Fixed
FileSize
4.09 KB

Attached is a patch that should fully support private files. It does its best to maintain the original file path, other than adding the "resize" and updating the file name. If the original image uses a local, non-private file, Image Resize Filter will keep this format. Same for private files, if private files are enabled, it will keep the same prefix. I also enhanced the original patch in two ways: 1) Added an extra submit handler to the file system settings form so that if the file handling is changed, the filter cache will be cleared so Image Resize Filter can update the URLs and 2) the implementation of hook_file_download() doesn't use the "view uploaded files" permission, which is specific to upload.module. Instead it checks the permissions on the original file and returns based on that information.

I've already committed this patch in preparation for a 1.2 version.

Status: Fixed » Closed (fixed)

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