Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
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.
Comment | File | Size | Author |
---|---|---|---|
#38 | privatefilesystem-423652-38.patch | 2.61 KB | Lendude |
#32 | privatefilesystem.423652.32.patch | 2.25 KB | Lendude |
#30 | privatefilesystem.423652.30.patch | 2.25 KB | Lendude |
#23 | 423652-23-access-to-cropped-images.patch | 1.48 KB | DeFr |
#2 | Add missing Imagefield crop's hook_file_download().patch | 809 bytes | doq |
Comments
Comment #1
yhager CreditAttribution: yhager commentedSomething 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.
Comment #2
doq CreditAttribution: doq commentedComment #3
doq CreditAttribution: doq commentedComment #4
yhager CreditAttribution: yhager commentedFirst, 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.
Comment #5
magoo CreditAttribution: magoo commentedThis 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.
Comment #6
yhager CreditAttribution: yhager commented@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.
Comment #7
magoo CreditAttribution: magoo commentedI 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.
Comment #8
yhager CreditAttribution: yhager commented@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...
Comment #9
doq CreditAttribution: doq commentedComment #10
arhak CreditAttribution: arhak commentedmaybe 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
Comment #11
jessepinho CreditAttribution: jessepinho commentedSubscribing
Comment #12
agerson CreditAttribution: agerson commentedSubscribing
Comment #13
imp7 CreditAttribution: imp7 commentedI 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.
Comment #14
geerlingguy CreditAttribution: geerlingguy commentedPatch 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!
Comment #15
silid CreditAttribution: silid commentedI don't know why my patch in #1 appeared so strangely.
I added the following function to the end of imagefield_crop_file.inc
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
Comment #16
yhager CreditAttribution: yhager commented@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'.
Comment #17
TyraelTLK CreditAttribution: TyraelTLK commentedI 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.
Comment #18
mobcdi CreditAttribution: mobcdi commentedIs the function from #15 built into the version 6.x-1.0-rc1 or is there still a need to hand code it?
Comment #19
robby.smith CreditAttribution: robby.smith commentedsubscribing
Comment #20
yhager CreditAttribution: yhager commented@mobcdi, it is not. That patch needs some work.
Comment #21
diaxpro CreditAttribution: diaxpro commented#15 work for me thanks
Comment #22
ajaysolutions CreditAttribution: ajaysolutions commented#15 works for me too, as long as I'm using .jpg extension files I suppose.
Thanks!!!
Comment #23
DeFr CreditAttribution: DeFr commentedI'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.
Comment #24
DeFr CreditAttribution: DeFr commentedComment #25
DeFr CreditAttribution: DeFr commentedSorry, forgot the status change to CNR.
Comment #26
BladeduI used this patch and the module works fine!
Comment #27
DeFr CreditAttribution: DeFr commented@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".
Comment #28
Bladedu@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.
Comment #29
LendudeUsing 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)
Comment #30
LendudeRan 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
Comment #31
DeFr CreditAttribution: DeFr commentedInstead 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 casefield_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 ?Comment #32
LendudeI 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
Comment #33
fraweg CreditAttribution: fraweg commentedPlease submit a patch for Drupal 7 too!
Best regards
Frank
Comment #34
phenaproximaI 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:
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.
Comment #35
joetsuihk CreditAttribution: joetsuihk commentedCan 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
Comment #36
fraweg CreditAttribution: fraweg commentedHello,
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
Comment #37
phenaproxima@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.
Comment #38
Lendude3.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.