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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

drewish’s picture

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.

Bevan’s picture

> 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.

drewish’s picture

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.

Bevan’s picture

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?

drewish’s picture

Status: Needs review » Needs work

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

Bevan’s picture

Status: Needs work » Needs review
FileSize
800 bytes

Patch attached

Bevan’s picture

Title: Files with ampersands can not be cached » Recommend transliteration module
Version: 6.x-2.0-beta2 » 6.x-2.0-beta3
Component: Code » Documentation
drewish’s picture

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.

Bevan’s picture

> 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.

drewish’s picture

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.

drewish’s picture

Title: Recommend transliteration module » Require 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.

drewish’s picture

Version: 6.x-2.x-dev » 5.x-2.x-dev
Status: Patch (to be ported) » Fixed
FileSize
1.28 KB

Status: Fixed » Closed (fixed)

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

drewish’s picture

Component: Documentation » Code
Status: Closed (fixed) » Reviewed & tested by the community
FileSize
594 bytes
529 bytes

backing this out temporarily.

drewish’s picture

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.

joachim’s picture

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

I'm not seeing the dependency in beta10.

earthday47’s picture

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!

yettyn’s picture

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.

drewish’s picture

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.

nicholasThompson’s picture

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...

Bevan’s picture

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

drewish’s picture

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