Jump to:
| Project: | ImageCache Actions |
| Version: | 6.x-1.6 |
| Component: | Color Actions Module |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | active |
Issue Summary
Some versions of Firefox and Safari do not view images whose format has been converted with the "Change File Format" action properly. This appears to be because they do not like, for example, a jpeg file which has a .png extension. This problem seems to occur even more if the converted file has been a PDF document - see #366373: PDF support ( i.e. convert PDF to JPG support).
To fix this, I have written a module (currently called imagecache_rename), which it might be worth considering incorporating into imagecache_coloractions. The principle is to make a converted file (say example.jpg, which has been converted to png) available from:imagecache_rename/presetname/files/example.jpg.png
and for formatters to use this URL to get at the image. Thus a png file has the right ending.
This is acheived by
1. Creating a new menu item
$items[file_directory_path() .'/imagecache_rename'] = array(
'page callback' => 'imagecache_rename_cache',
'access callback' => TRUE,
'type' => MENU_CALLBACK,
);2. Creating a callback function strips off the last extension, and then
function imagecache_rename_cache() {
$args = func_get_args();
// Strip the last extension off the $filename, and then
// retrieve from imagecache as usual
$filename = array_pop($args);
$filename_parts = pathinfo($filename);
array_push($args, $filename_parts['filename']);
call_user_func_array('imagecache_cache', $args);
}3. Overriding CCK formatters so that they link images to URLs like imagecache_rename/presetname/files/example.jpg.png rather than imagecache/presetname/files/example.jpg . This is demonstrated in the attached imagecache_rename.module file.
The third step could simply be achieved by overriding theme_imagecache() in a theme file. This might be a better approach, as it avoids creating two sets of theme functions, as the approach at (3) does.
If you think this is worth trying to incorporate into imagecache_coloractions, I'd be happy to try rolling a patch. This would involve cleaning up the very rough code (much borrowed from imagecache).
| Attachment | Size |
|---|---|
| imagecache_rename.module.txt | 14.04 KB |
| imagecache_rename.info.txt | 229 bytes |
Comments
#1
P.S. Note that I haven't written the code for private files.
#2
Fair enough, that hack was improper, and bordering on illegal, I know :-)
It just seemed to work anyway in the browsers I tested at the time, but it made me feel dirty.
Hm.
This approach is certainly a move in the right direction but it doesn't feel holistic enough. There are any number of other things out there that may make assumptions about imagecache naming schemes that would have to be tweaked individually. Overriding formatter callback functions (though logical/inevitable) makes using this a lot tricker than before, IMO.
Thinking about it freshly (a year since I put the hack in) I'd possibly approach it with a subtle little .htaccess rewrite in the file dir itself. Basically content-negotiation at the file-level.
Leaving everything else as it is...
if file.jpg does not exist in imagecache/profilename/files/ folder,
see if file.jpg.png does, and sned a 301 to that real file.
This will (should) give browsers enough to go on, while being transparent to all callers.
So, three lines of .htaccess (although I can't invent that OTTOMH right now)
In leu of htaccess, we can get the same effect using hook_menu - that's how imagecache works now anyway. Less efficient though.
OR - a full though possibly chunkier solution - dig at imagecache_url() (or whatever that function is called) and insert a hook callback so I can change the imagecache pathnames on the fly. Most well-behaved other modules that leverage imagecache should be going through that. IIRC
#3
Thanks for your reply. I thought that your hack was pretty good actually!
I agree that the idea of setting up a new menu item is by no means elegant, though I've some queries about some of your points:
The proposed module/patch doesn't remove any imagecache files - you would still be able to see imagecache/profilename/files/fred.jpg, as well as being able to see imagecache_rename/profilename/files/fred.jpg.png
The formatter callback functions don't need to be overridden - it could be left to the user to modify theme_imagecache(). This would probably be safer than overriding formatters, and would make people more aware of how the code worked - which would probably be a good thing if things started to go wrong.
I can't quite see how this would work, and I'm not convinced of its advantages. Supposing file.jpg is actually a png file, I think we still need to either override formatters or theme functions to get the browser to ask for imagecache/profilename/files/file.jpg.png (as discussed above). As you say, it is less efficient, requiring two requests per image. And it wouldn't work for sites where rewrite rules aren't enabled. That said, we don't introduce the nasty imagecache_rename path.
Also shouldn't the rewrite rule be for file.jpg.png to file.jpg ?
Before doing that, I think it would be worth checking out the D7 image code. As we're in code freeze, it's probably too late to get any such API change in. It would be good for any solution we come up with now to more-or-less work in D7 - though having said that I haven't checked out my current hack in D7!
#4
Yeah, but I'd rather see a configurable solution that works out-of-the-box.
? Then someone has been evil like have, and is introducing confusion like I have. :=}
Two requests to the browser are no problem because that's not two data transfers, just header negotiation. Two Apache requests are much more lightweight than a Drupal bootstrap.
I'm pretty sure imagecache just won't work on a system that doesn't already have rewrites in place. Not going to let IIS users bother me.
I'd have to try this out on a platfom or two to see if it really gels together. I'm uncomfortable about multiplying factors here - by introducing a whole new storage area and behaviour here. I'd hope for a more transparent fix - a fallback that just happens - but am not sure it exists.
I'll have to have a play and a think
#5
I agree, an out of the box solution would be best.
I'm still not convinced by the approach, but I'd be interested to see how you get on - though if I don't come back for a while it might be because subscriptions don't seem to be working for me any more.
#6
egfrith, I found a small bug in your code. You are retrieving the file format two times, once in
<?phpfunction imagecache_rename_get_extension($preset) {
$format = '';
$extension = '';
foreach ($preset['actions'] as $action) {
if (($action['action'] == 'imagecache_convert') && isset($action['data']['format'])) {
$format = $action['data']['format'];
}
}
if (isset($format)) {
$formats = imagecache_file_formats();
$extension = $formats[$format];
}
return $extension;
}
?>
and then in line 142, which I changed to the following:
<?php//$formats = imagecache_file_formats();
//$filepath .= '.' . $formats[$format];
$filepath .= '.' . $format;
$path = _imagecache_strip_file_directory($filepath);
?>
With the change your module works great! Thanks for the effort, the ability to have the correct extension is a must have for IE6 and the pngfix scripts!
--Alex Sartogo
Hot Sauce Studios
#7
True, I hadn't considered the effect this thing has on pngfix.
Re the patch/fix/module.
I appreciate and understand the effort, and I'm sorry it takes so much work to work around.
My gut feeling is that something so small should not take so much code to change, although approaching it as a drop-in pluggable rewrite module was ... appropriate.
I'm thinking there is a simpler, tidier fix out there, but I'm not sure what it is.
If it was a 3-line option we could patch into the appropriate place in imagecache itself, then maybe these three pages could get trimmed.
#8
The current behaviour (not updating the file extension) also causes issues with PDFs in the Print module. Subscribing.
#9
subscribing
#10
subscribing
#11
renaming
#12
Another potential approach that doesn't require creating a new module and could a file with the change extension such as:
Original file:
sites/default/files/foo.png
File with type changed:
sites/default/files/imagecache/[preset-name]/foo.jpg
1) Use hook_menu_alter() to alter the imagecache menu callback, pointing it to a new function such as imagecache_coloractions_cache() rather than imagecache_cache().
2) At this callback check if the requested path contains an ImageCache preset defined by imagecache_coloractions that changes a file extension
3) If the path does not refer to such a preset, return imagecache_cache() with the default parameters. If it does contain such a preset, rename the file extension requested in the path per the preset (eg, png.jpg)
4) return imagecache_cache() with the altered file path, so we know which source file we want to serve
This could take care of making the altered file available at that path, but not *rendering* images to use this path. There are lots of places that imagecache paths to images are generated. As egfrith points out, we'd have to chance that on the display side of things, such as in CCK formatters and in the Insert module. Luckily, Insert makes this possible for us with hook_alter_styles, which lets imagecache_coloractions() define its own insert style that could use the altered path with new file extension. However, we'd probably want to also alter imagecache_create_url, which doesn't seem to have a drupal_alter() implementation in it or around it in the codeflow within ImageCache, so this part would probably require a patch to imagecache.
#13
Actually, you don't need to rename the files.
What you need is to serve the right mime type.
Do that by creating the appropriate .htaccess file in the preset folder when it's created.
Elegant enough? :-)
#14
The point in #13 is correct -- the best solution is to serve the right MIME type. The following code will create a
.htaccesswithin each pre-set directory that forces the MIME type iff theimagecache_convertaction is used.<?php// Build the .htaccess folder
if (!file_exists($dir.'/.htaccess')) {
if (!touch($dir.'/.htaccess')) {
watchdog('imagecache', 'Failed to create imagecache settings file: %dir/.htaccess', array('%dir' => $dir), WATCHDOG_ERROR);
return FALSE;
} else {
// Find the format
$forcetype = "None";
foreach ($actions as $action) {
if ($action['action'] == 'imagecache_convert') {
$forcetype = $action['data']['format'];
}
}
file_save_data("ForceType $forcetype\n", $dir.'/.htaccess', FILE_EXISTS_REPLACE);
}
}
?>
I've put it right after the block that uses
mkdirinimagecache.module.#15
Is this code/solution usable for drupal 7.x?
#16
AFAIK, the image system does serve the right mime type. It certainly does so in D7 and I thought it currently does as well in D6. But I would have to sort that out to be sure.
Are there still (relatively modern versions of) browsers around that do have problems with this?