I have attached a patch that will enable users with private file systems to be able to see the images for cropping.

The code is based on that from the imagefield module directly.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

yhager’s picture

Status: Needs review » Needs work

Something is strange with that diff - it deletes the whole files and builds it from scratch.
Can you fix your diff program and reroll, it's very hard to see what you've changed.

doq’s picture

doq’s picture

Assigned: Unassigned » doq
yhager’s picture

First, receiveing patches is every maintainer's dream - so thank you very much for the patch.

As for the content, here are my comments:

1) I assume you would want to invoke only filefield_hook_download, and not ones made by other modules
2) by not sending the .crop_display.jpg file, we are losing the 'save original image' feature, so after you crop, you will get the cropped image on edit, and not the original. I think we should find a way to make all access checks in the same way filefield does, but send the .crop_display.jpg file at the end. Maybe a patch to filefield that will separate access checks with file's data will be appropriate here.

magoo’s picture

This limitation must be specified in the module presentation page.

I had to migrate to private file system in order to control access to attachments and sadly this module stopped working.

It would be nice to see this COOL module work on private file systems in a near future.

yhager’s picture

@magoo: thanks, I update the module's page with this regard. Can you try the patch in this thread? it needs work, but it might be a quick solution for you, until we deploy it properly.

magoo’s picture

I installed the patch adding the hook_file_download implementation.

At least the image is now reachable from the browser.

This will make it temporarily. Sadly I see that I have another issue which is the crop on multiple images: all images are cropped with the same setup :-( - I think I've seen that in the issue list.

yhager’s picture

@magoo: it's unfortunate that you have hit two limitations of this module. There is a patch for the second issue (see on the main module page), but it is still in the works.

This is the nature of open source development...

doq’s picture

Assigned: doq » Unassigned
arhak’s picture

maybe trying join forces might help #181003: private download method and dynamically generated CSS and JS

[edit] saving you some time go directly to comment #24

jessepinho’s picture

Subscribing

agerson’s picture

Subscribing

imp7’s picture

I am interested in any progress, this patch #2 doesn't seem to allow new images after the user clicks remove and uploads a new one.

geerlingguy’s picture

Patch in #2 works for me - I was unable to see the preview using a private file system, and now the preview appears correctly.

Luckily, I only need Imagefield Crop on one image per node, so this is a godsend! Thanks!

silid’s picture

Version: 6.x-1.0-beta3 » 6.x-1.0-rc1
Status: Needs work » Needs review

I don't know why my patch in #1 appeared so strangely.

I added the following function to the end of imagefield_crop_file.inc

function imagefield_crop_file_download($filepath) {
  // Return headers for crop display if private files are enabled.
  if (strpos($filepath, 'crop_display') !== FALSE) {
    $original_path = str_replace('crop_display.jpg', '', $filepath);
    $original_full_path = file_create_path($original_path);
    $display_full_path = file_create_path($filepath);

    // Allow access to temporary images, since they're not yet associated
    // with a node. If not temporary, check access on the original file.
    $status = db_result(db_query("SELECT status FROM {files} WHERE filepath = '%s'", $original_full_path));
    $access = ($status == 0 || module_invoke_all('file_download', $original_path));
    if ($access && $info = getimagesize($display_full_path)) {
      return array(
        'Content-Type: ' . $info['mime'],
        'Content-Length: ' . filesize($display_full_path)
      );
    }
  }
}

The main difference between this and the patch at #2 is it checks permissions on the image. I am using a private file system to have permissions applied to images so I wanted to make sure they were checked here. The code is based on that found in imagefield.module

yhager’s picture

Status: Needs review » Needs work

@silid: thanks again.
I don't like polluting the 'crop_display' string all over the place - as it might change in the future. Can you try and refactor this to use a central place, like imagefield_crop_file_admin_crop_display_path()?
There are other issues in the queue about this arbitrary file name, and especially the hard code of '.jpg'.

TyraelTLK’s picture

I added the function at #15 to imagefield_crop_file.inc and now it works for me.
Please add this limitation in the module presentation page.

mobcdi’s picture

Is the function from #15 built into the version 6.x-1.0-rc1 or is there still a need to hand code it?

robby.smith’s picture

subscribing

yhager’s picture

@mobcdi, it is not. That patch needs some work.

diaxpro’s picture

#15 work for me thanks

ajaysolutions’s picture

#15 works for me too, as long as I'm using .jpg extension files I suppose.

Thanks!!!

DeFr’s picture

I'm attaching a patch that's pretty much #15, loosely using imagefield_crop_file_admin_crop_display_path to get the suffix. Hopefully that will fulfill the requirement layed out in #16 not to hardcode the .crop_display.jpg part of the path.

DeFr’s picture

Status: Needs work » Needs review
DeFr’s picture

Sorry, forgot the status change to CNR.

Bladedu’s picture

Status: Needs review » Fixed

I used this patch and the module works fine!

DeFr’s picture

Status: Fixed » Needs review

@Bladedu, #26: The "fixed" status would mean that the patch got commited to the project, thus an issue should only be set to "fixed " by a project maintainer when a patch is involved. The proper status to say "I've tested this patch, it fixes the problem for me and I've read the code, it makes sense" is "reviewed & tested by the community".

Bladedu’s picture

Status: Needs review » Reviewed & tested by the community

@DeFr: I'm sorry about that. I just didn't know... I'm setting the status to reviewd & tested by the community since I'm still using the patch and is working perfectly.

Lendude’s picture

Using patch #23, fixed problem for me.

small change:
$access = ($status == 0 || module_invoke_all('file_download', $original_path));
to
$access = ($status === '0' || module_invoke_all('file_download', $original_path));

why:
$status = db_result(db_query("SELECT status FROM {files} WHERE filepath = '%s'", $original_full_path));
would return FALSE if $original_full_path was not in the files table, but $status == 0 would evaluate to TRUE and grant access to a file not in the files table (instead of just to a temporary file in the files table as $status === '0' would do).
(I don't really see how $status == 0 could be exploited, with all the other checks that go before and after it but it does grant access to theoretical files it has no right granting access to, so better to just check for it)

Lendude’s picture

Ran into another issue with the private filesystem and imagefield_crop. I'll stick to this thread to try and keep it in one place.

When Drupal servers the images through the private filesystem it generates its headers based on the file entry in the 'files' table. The entry in the files table is not modified by imagefield_crop, so Drupal would send out a header with a size of the original uncropped file. So the browser would wait for 15 seconds for data it would never recieve expecting a bigger file then it would actually get.

I made a patch that fixes this issue and incorparates #23 (and my mod in #29).

In short, it changes the file entry in the files table after cropping and clearing the filefield cache for the image (the cache bit took a while to find....)

Len

DeFr’s picture

Status: Reviewed & tested by the community » Needs work

Instead of
_field_file_cache(NULL, (object) field_file_load($data['fid'])); //clear the old file from the cache, you should stick to FileField public API to clear the cache, in this case field_field_load($data['fid'], TRUE);

That being said, there's something else that sound strange. The whole point of the 'Content-Length: ' . filesize($display_full_path) in the hook_file_download implementation is to provide the correct content length header in the response. Does that code return invalid result ? Or is it somehow overridden later for you ?

Lendude’s picture

I presume you mean use field_file_load to clear the cache (not field_field_load, that function doesn't seem to exist, but I might be wrong since I am by no means an expert on CCK)

I opted for the internal function because that gives the option to reset the cache for a specific file where field_file_load() just resets the whole cache (even if you hand it a file argument), but I agree that using the wrapper function is probably the 'Drupal way' to go.

The 'Content-Length: ' . filesize($display_full_path) in the the hook_file_download implementation is after the "if (substr($filepath, -1 * strlen($suffix)) == $suffix)" check so only works for the '.crop_display.jpg' version of the file, not the original file.

What I may not have made clear in #30 was that this problem occured when viewing the 'view' page, not the 'edit' page. So when the server was trying to serve up 'original_image.jpg', not 'original_image.jpg.crop_display.jpg'. So access calls and header generation are handled by other modules (that check the database file entry, not the actual file).

Thanks for looking at the code! I added a new patch that uses field_file_load() directly.

Len

fraweg’s picture

Please submit a patch for Drupal 7 too!

Best regards
Frank

phenaproxima’s picture

I made a very rough, quick patch for D7. Add this function to imagefield_crop.module in order for private files to work with the cropping field:

/**
 * Implements hook_file_download().
 */
function imagefield_crop_file_download($uri) {
  $files = file_load_multiple(array(), array('uri' => $uri));
  foreach ($files as $file) {
    $usage = file_usage_list($file);

    if (isset($usage['imagefield_crop'])) {
      return array(
        'Content-Type' => $file->filemime,
        'Content-Length' => $file->filesize,
      );
    }
  }
}

All this does is check that the file is being used (i.e., has imagefield_crop record(s) in the file_usage table). imagefield_crop already adds a file_usage record during the cropping process, so this doesn't add any additional data or anything.

joetsuihk’s picture

Version: 6.x-1.0-rc1 » 6.x-1.x-dev
Status: Needs work » Needs review

Can someone tell me if #32 is working on 6.x-1.x
and
#34 is on 7.x-1.x or 7.x-2.x?
#34 switch to private download seems do not have problems

fraweg’s picture

Hello,

I dont know how to use #34.. Is this a patch a module? I am not a real developer so I do not understand..So I did not use this ..

Best regards
Frank

phenaproxima’s picture

@fraweg - I will try to roll #34 as a real patch, although it was only intended as a quick-fix. But if you're in a hurry and need to use it as-is, all you need to do is copy the code I posted in #34 and paste it right at the end of the file imagefield_crop.module.

Lendude’s picture

Issue summary: View changes
FileSize
2.61 KB

3.5 years later I find myself working on this again!

This is a cleaned up version of the patch in #32. Got rid of all the whitespace errors and actually implemented the fix suggested in #31.