Hey there,

I was looking into making my file_force module work with image link options, as posted in #358800: Image module support over at file_force, and #22700: Option to "download" original image within your module, but it seems that there is a slight problem.

The problem is that the image_fetch module calls the image_file_download function. However, that function is actually a hook and should not be usually called in a direct fashion, rather I would say that the file_download function should be called, which in turns make sure to invoke all the hook_file_download implementations, which will include yours, and also all the others, i.e. the one I have implemented in file_force. See http://api.drupal.org/api/function/file_download/6 for the documentation.

I hope you can look into this in the near future.

Cheers,
Martin

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

joachim’s picture

Status: Active » Needs review
FileSize
716 bytes

Agreed; we should not be calling a hook directly, but invoking it.

Here's a patch, but I'm afraid at the moment I don't have the time to test this at all.

arski’s picture

Hey, this works fine for me in a simple standard setup and also with the file_force hook!

To be honest I see no reason why this shouldn't work for some other module - all this changes is that now you can get more headers returned, which should be perfectly safe, and nothing else.

Anyway, thanks for replying.

Status: Needs review » Needs work

The last submitted patch, 883338.image_.invoke-hook_file_download.patch, failed testing.

arski’s picture

Version: 6.x-1.0-beta5 » 6.x-1.x-dev

hmm, any idea why it failed the simpletest? I can't understand anything in the log..

joachim’s picture

Looks like the testing rig is borked.

arski’s picture

Status: Needs work » Needs review
joachim’s picture

The bit I'm concerned about is

> If a module returns -1 drupal_access_denied() will be returned.

arski’s picture

Hmm, it looks right. If you look at http://api.drupal.org/api/function/hook_file_download/6 then the standard definition is exactly the same: if a user has access, add some headers, otherwise return -1; which is exactly what is done in image_file_download. Or am I missing something?

Good to see the test has passed :)

joachim’s picture

Yup, but what I mean is -- what if some wacky module we don't know about returns a -1 and thus foils image downloads completely?

This is why I'd like to see this getting tested by a wider pool of users.

arski’s picture

Hmm, this is true, but then again, if some whacky module has some silly implementation of the hook it's that module's fault and not yours really.. as far as I can tell you're doing everything right and if some module is doing some weird stuff.. well.. even if you can find one such module - you can't really fix what they're doing from inside this module, can you?

arski’s picture

sorry to bug, but I'm really eager to see this patch accepted as it's the last thing I need to be able to release a stable version of the file_force module :)

I have to reiterate that your implementation of the hook is fully correct and if some module does something wrong or incompatible, we can post an issue in that module and get it sorted :)

My apologies again.

Cheers,
Martin

joachim’s picture

Status: Needs review » Fixed

Committed.

Thanks for reporting this.

#883338 by joachim: Fixed direct calling of hook_file_download() implementation.

Status: Fixed » Closed (fixed)

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