Drupal 6's menu handler stops doesn't parse characters after an ampersand in arguments. (Nor should it)

Therefore imagecache is unable to find the targeted file to cache when an incomplete path is passed to it as an argument to the menu callback. (If the file has a an ampersand in it's name).

The tranliteration module resolves this issue handsomely by removing characters like ampersands from file names before writing them to disk. This patch introduces a dependency on transliteration module. This fixes the bug in an elegant way.

Comments

Status:Active» Needs review

I'm reluctant to force people to use transliterate... perhaps we should just add a link to it from the imagecache project page.

> I'm reluctant to force people to use transliterate.

How come? This is a major bug in imagecache. It's not reasonable to expect end users (let alone developers) to know that they need to remove ampersands and non-latin characters from filenames, and filenames with these characters are common enough in all languages that it's likely the bug will be encountered on most sites.

> perhaps we should just add a link to it from the imagecache project page.

IMHO this is not a good solution, because it admits that imagecache is buggy yet forces the IC user to search for the fix themselves (even though the fix may apparently be obvious), when the bug could easily be avoided in the first place.

i'm not convinced because in many cases it's platform specific. the real bug is in core not imagecache so feel free to work on getting transliterate committed to core.

Hmmm. I think we can agree to disagree. By adding transliteration as a dependency to IC, transliteration is going to get a lot more attention that will help it get into core someday.

Would you consider or accept a patch adding documentation to imagecache.info's description that transliteration module is highly recommended?

Status:Needs review» Needs work

yeah, documenting it in that manner would be very helpful.

Status:Needs work» Needs review
StatusFileSize
new800 bytes

Patch attached

Title:Files with ampersands can not be cachedRecommend transliteration module
Version:6.x-2.0-beta2» 6.x-2.0-beta3
Component:Code» Documentation

Version:6.x-2.0-beta3» 6.x-2.x-dev
Status:Needs review» Needs work

I added a note to the project page. I think I'd rather avoid adding it to the .info file. A note in the README.txt or INSTALL.txt would probably be more appropriate.

> A note in the README.txt or INSTALL.txt would probably be more appropriate.

People generally don't read these files. I think it's a shame you aren't willing to give this issue the attention it deserves.

Status:Needs work» Fixed

you'd be surprised. but it's fine with as is since it's on the freaking project page.

Status:Fixed» Closed (fixed)

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

Title:Recommend transliteration moduleRequire transliteration module
Status:Closed (fixed)» Patch (to be ported)

Bevan, I've changed my mind. I'm going to require transliteration. Committed the original patch to HEAD, need to back port to DRUPAL-5--2.

Version:6.x-2.x-dev» 5.x-2.x-dev
Status:Patch (to be ported)» Fixed
StatusFileSize
new1.28 KB

Status:Fixed» Closed (fixed)

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

Component:Documentation» Code
Status:Closed (fixed)» Reviewed & tested by the community
StatusFileSize
new594 bytes
new529 bytes

backing this out temporarily.

Status:Reviewed & tested by the community» Fixed

recommitted now that the security fixes have gone out.

Status:Fixed» Closed (fixed)

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

Version:5.x-2.x-dev» 6.x-2.0-beta10
Status:Closed (fixed)» Active

I'm not seeing the dependency in beta10.

Confirmed - Imagecache can be installed without the Transliteration module.

Thought this was a bug until I read this. And yes, even though it's written on the project page, I did not read it. There's a lot of links on that page!

Version:6.x-2.0-beta10» 6.x-2.x-dev
Component:Code» Documentation

Just passing by as I need this, but...

It's obvious to anyone who actually read the available information that the Transliteration patch was applied to the dev version, and temporary roled back before the Beta (security) release, than put back in again.

So this bug should really have stayed closed as it has nothing to do with 6.x-2.0-beta10, but... if we now should be nitty gritty pedantic, it should say on the project page for dependencies:
Install and enable Transliteration module (dev version only)

possibly with a note it's recommended for others. I don't know what went round in the head of drewish but I can guess he didn't want to add it as it would have mock up things for those who have a beta installed but not Transliteration, taking into account the beta is the currently recommended version and as such sort of expected to be "stable", while using dev you sort of expect odd things as a sudden dependency happen. Now anyone using the beta can take action and install Translitteration and be prepared for the next step w/o sudden surprises.

So setting this back to dev and Documentation for the dev note above to be added on project page.

Status:Active» Closed (won't fix)

I'm going to back this out after all. I changed my mind on making it required, I don't think it makes sense to have on every site and therefore should be optional.

It said on the 6.x-2.0-beta12 page that this was committed...
http://drupal.org/node/1159260

I assume that's an error? beta12 doesn't seem to have it as a dependancy...

I think it was added, removed, added again, then finally removed again. I don't know it's current status.

Yeah it was added, then removed twice. It will not be added again in the future.