Problem/Motivation

drupal_alter() allows a maximum of 2 alterable arguments, but file_file_download() tries to send 3 arguments. As a consequence, hook_file_download_access_alter() is unable to see the $entity argument. This makes it difficult for modules to make decisions on altering download access.

Proposed resolution

The easiest fix would be adding an extra argument to drupal_alter().

Remaining tasks

Rewrite drupal_alter() to accept 3 alterable arguments.

API changes

Modules that employ drupal_alter() would now accept 5 arguments, 3 of which are alterable.

Original report by localhost

drupal_alter allows a max of 2 alterable arguments but file_file_download tries to send 3 arguments. As a consequence, hook_file_download_access_alter is unable to see the $entity argument. This makes it difficult for modules to make decisions on altering download access.

<?php
drupal_alter
('file_download_access', $grants, $field, $entity_type, $entity);
?>
Files: 
CommentFileSizeAuthor
#34 drupal_alter_d7_patch-1143460-34.patch2.03 KBDavid_Rothstein
PASSED: [[SimpleTest]]: [MySQL] 39,628 pass(es).
[ View ]
#32 drupal_alter_d7_patch-1143460-31.patch1.18 KBtyphonius
PASSED: [[SimpleTest]]: [MySQL] 39,622 pass(es).
[ View ]
#25 drupal_alter_d7_patch-1143460-25.patch931 bytescolette
PASSED: [[SimpleTest]]: [MySQL] 39,580 pass(es).
[ View ]
#17 fix-hook-file-download-access-alter-parameters_1143460-_17.patch2.56 KBWorldFallz
FAILED: [[SimpleTest]]: [MySQL] Invalid patch format in fix-hook-file-download-access-alter-parameters_1143460-_17.patch.
[ View ]
#15 fix-hook-file-download-access-alter-parameters_1143460_15.patch2.32 KBWorldFallz
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch fix-hook-file-download-access-alter-parameters_1143460_15.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#13 1143460-fix-hook-file-download-access-alter-parameters.patch2.29 KBDave Reid
PASSED: [[SimpleTest]]: [MySQL] 35,629 pass(es).
[ View ]
#12 1143460-fix-hook-file-download-access-alter-parameters.patch2.29 KBDave Reid
PASSED: [[SimpleTest]]: [MySQL] 35,627 pass(es).
[ View ]
#7 hook_file_download_access_alter-missing-entity-argument_1143460_7.patch3.76 KBWorldFallz
FAILED: [[SimpleTest]]: [MySQL] 33,978 pass(es), 7 fail(s), and 9 exception(es).
[ View ]
#2 hook_file_download_access_alter-missing-entity-argument_1143460_2.patch928 bytesWorldFallz
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch hook_file_download_access_alter-missing-entity-argument_1143460_2.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Comments

I just ran into this issue as well. In my case, I wanted to deny access to files on a specific content type based on the presence of a cookie. To work around this issue I also implemented hook_file_download_access(), then in hook_file_download_access_alter(), I check if my module denied access and if so, I return an array with a single FALSE value in it to deny access to the file:

<?php
function my_module_file_download_access($field, $entity_type, $entity) {
  if (
$entity_type === 'node' && $entity->type === 'resource' && !isset($_COOKIE['resources'])) {
    return
FALSE;
  }
  return
TRUE;
}
function
my_module_file_download_access_alter(&$grants, $field, $entity_type) {
  if (isset(
$grants['my_module']) && $grants['my_module'] === FALSE) {
   
$grants = array(FALSE);
  }
}
?>

Version:7.0» 8.x-dev
Assigned:Unassigned» WorldFallz
Status:Active» Needs review
StatusFileSize
new928 bytes
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch hook_file_download_access_alter-missing-entity-argument_1143460_2.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

It's definitely a bug-- I've traced it to the fact that file.module calls drupal_alter with 5 parameters (line 197):

drupal_alter('file_download_access', $grants, $field, $entity_type, $entity);

But module.inc only allows for 4 (line 904):

function drupal_alter($type, &$data, &$context1 = NULL, &$context2 = NULL) {

The question then becomes which is incorrect-- the function definition or the function call? imo, having $entity available in that context is critical, so I would opt to change the function as follows:

<?php
function drupal_alter($type, &$data, &$context1 = NULL, &$context2 = NULL, &$context3 = NULL) {
.
.
.
  foreach (
$functions[$cid] as $function) {
   
$function($data, $context1, $context2, $context3);
  }
}
?>

I verified d8 still has the bug-- patch for d8 attached.

FYI, I also grepped core for other drupal_alter calls, and I didn't spot any other drupal_alter calls with more than 4 parameters (thought it was just a quick look atm). I'll check more thoroughly when I get a chance.

I noticed in the latest Drupal Core that this issue was resolved by not passing $entity to download_access_altar. I would have to agree with WorldFallz that having $entity available in that context is critical, would it not be possible to solve the issue in the same manner as above?

Actually, it could be argued that since $entity_type is available at $entity->type, the file.module call should be changed to:

drupal_alter('file_download_access', $grants, $field, $entity);

But if there are modules that already rely on $entity_type, it could be a more disruptive change. There shouldn't be modules that rely on $entity being available there since the bug doesn't appear to have been reported before this.

Thing is, there's several places in core that rely on the '$entity_type $entity' pattern for hook_file_download_access :

comment.module - Line 2711: function comment_file_download_access($field, $entity_type, $entity) {

file.api.php - function hook_file_download_access($field, $entity_type, $entity) {

file.module - Line 193: $grants = array_merge($grants, array($module => module_invoke($module, 'file_download_access', $field, $entity_type, $entity)));
file.module - Line 196:drupal_alter('file_download_access', $grants, $field, $entity_type, $entity);

node.module - Line 3937: function node_file_download_access($field, $entity_type, $entity) {

user.module - Line 3894: function user_file_download_access($field, $entity_type, $entity) {

So it's definitely a bigger change.

StatusFileSize
new3.76 KB
FAILED: [[SimpleTest]]: [MySQL] 33,978 pass(es), 7 fail(s), and 9 exception(es).
[ View ]

Here's a patch based on #6 for evaluation as well.

Status:Needs review» Needs work

Status:Needs work» Needs review

actually, based on http://drupal.org/node/1245220#comment-5646934, it looks like creating a single arrayed variable is preferable. There's also #1462538: Change drupal_alter() to use only one context variable which makes sense as well.

Yeah it would be best to do:

<?php
$context
= array(
 
'entity_type' => $entity_type,
 
'entity' => $entity,
);
drupal_alter('file_download_access', $grants, $field, $context);
?>

Or better yet also put $field into $context. Yes this is an API change but its necessary to backport this to Drupal 7.

I'm not sure why we're discussing changes to hook_file_download_access() since we don't need to change them.

StatusFileSize
new2.29 KB
PASSED: [[SimpleTest]]: [MySQL] 35,627 pass(es).
[ View ]

StatusFileSize
new2.29 KB
PASSED: [[SimpleTest]]: [MySQL] 35,629 pass(es).
[ View ]

Fixed 'TThe' in the docs.

I don't have anywhere handy to test this on d8 atm, but it works fine on d7. Though I would tend to think adding $field to $context makes sense for d8.

I'll test on d8 and roll a d7 patch as well as soon as I get a chance.

StatusFileSize
new2.32 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch fix-hook-file-download-access-alter-parameters_1143460_15.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

here's a patch for d7.

Status:Needs review» Needs work

The last submitted patch, fix-hook-file-download-access-alter-parameters_1143460_15.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new2.56 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid patch format in fix-hook-file-download-access-alter-parameters_1143460-_17.patch.
[ View ]

tested in d8-- patch in 13 applies cleanly and works fine.

and here's a d8 patch for folding everything but $grants into $context.

Status:Needs review» Needs work

The last submitted patch, fix-hook-file-download-access-alter-parameters_1143460-_17.patch, failed testing.

Status:Needs work» Closed (duplicate)

Version:8.x-dev» 7.x-dev
Status:Closed (duplicate)» Needs work

This was fixed in Drupal 8 (as part of #1245220: file_file_download() passed bogus $field to field_access()), but let's reopen this for Drupal 7. It's a targeted bug whereas that other issue dealt with a number of other things also.

This patch as written seems like a bit of a scary API change. In the other issue, I wondered if the best way to fix this wasn't just to add an extra parameter to drupal_alter()? This might be a scary change also, but the main effect would be to fix any other modules that have a similar issue (invoking a hook with a parameter that they expect to be passed but isn't), so in all cases it's making code behave the way it was originally intended to, at least.

A final alternative would be to not use drupal_alter() at all here, but just whip up something custom (or create a new helper function). This would be the safest, but also ugliest, way to fix this in Drupal 7.

Oh, hah, I just realized that my first suggestion (add a parameter to drupal_alter() itself) is what the first patch in this issue already did.

I wondered if the best way to fix this wasn't just to add an extra parameter to drupal_alter()?

As I said in the other issue-- imo that's the 'best' of the alternatives. It doesn't break anything and simply provides a means of fixing the bug for any modules (like download_count) that encounter it, leaving everything else as is.

I'm happy to push this forward again-- do we just need a new patch?

Needs issue summary

Issue summary:View changes

added issue summary

I've added an issue summary, with the proposed solution of David_Rothstein and WorldFallz.

StatusFileSize
new931 bytes
PASSED: [[SimpleTest]]: [MySQL] 39,580 pass(es).
[ View ]

I rewrote the patch at #6 for Drupal 7.

Status:Needs work» Needs review

If patch in 25 still passes all tests (after the upgrade to 7.17) I feel this patch is RTBC.

:( so bad this patch didn't make it to 7.17

Added in a little doco for the additional parameter and rerolling to try and get a pass result for tests.

StatusFileSize
new1.18 KB
PASSED: [[SimpleTest]]: [MySQL] 39,622 pass(es).
[ View ]

That awkward moment where you forget to attach the patch

Status:Needs review» Reviewed & tested by the community

Status:Reviewed & tested by the community» Needs review
StatusFileSize
new2.03 KB
PASSED: [[SimpleTest]]: [MySQL] 39,628 pass(es).
[ View ]

Hm, I don't think we should document it like that, though. This parameter is "deprecated by design" and we don't want to encourage people to use it, or otherwise Drupal 8 will face the same situation Drupal 7 is facing now :)

The attached patch adjusts the documentation for that and fixes another instance of out-of-date documentation also.

More to the point, though, does anyone have any additional comments on whether this is a safe way to fix the bug? It's pretty far-reaching, but I've yet to think of any real problems with it myself.

I can't think of anything off hand, and I've been using it extensively for a while now -- and so have the 300 or so users of download_count 7.x.

I too have been using it on a couple of production sites. Haven't experienced any issues in the duration since I've patched it.

Assigned:WorldFallz» Unassigned
Status:Needs review» Reviewed & tested by the community

I've been using this patch for some time as well on a prod site and adding an extra parameter with good documentation seems pretty safe for the API change so RTBC.

Status:Reviewed & tested by the community» Needs work

Not sure, but it looks like we need a new version of the patch as of 7.18?

Status:Needs work» Reviewed & tested by the community

The patch still applies and I can't think of any reason we would, but will trigger a quick retest of the testbot to see what happens.

Bot is still happy. This is good to go.

Apologies, I was looking at older Core code yesterday when I thought I was looking at 7.18, so I thought the patch no longer applied to it. Thanks for the update.

Status:Reviewed & tested by the community» Fixed

OK, let's do it. Committed to 7.x - http://drupalcode.org/project/drupal.git/commit/2ca4228

I went ahead and added a change notification (http://drupal.org/node/1882722) since with this commit we are not only changing the API a bit in Drupal 7, but also introducing a "change" (rather, inconsistency) with Drupal 8. But hopefully I managed to document that all successfully.

Also interesting that there's no test for this hook, but it got fixed in Drupal 8 without one being added either, so I guess it's not up to this issue to add...

David_Rothstein FTW! Thanks Dave.

Drupal 7.19 was a security release only, so this issue is now scheduled for Drupal 7.20 instead.

Fixing tags accordingly.

Status:Fixed» Closed (fixed)

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

Drupal 7.20 was a security release only, so this issue is now scheduled for Drupal 7.21 instead. For real this time... I think :)

Fixing tags accordingly.

Issue tags:+7.22 release notes

Fixing tags since Drupal 7.21 only contained a fix to deal with fallout from the Drupal 7.20 security release. Hopefully this is really the last time I do this and this will be released in Drupal 7.22 for real :)

.

Issue summary:View changes

fixed editing