Posted by localhost on April 30, 2011 at 10:44pm
14 followers
| Project: | Drupal core |
| Version: | 8.x-dev |
| Component: | file system |
| Category: | bug report |
| Priority: | normal |
| Assigned: | WorldFallz |
| Status: | closed (duplicate) |
Issue Summary
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);
?>
Comments
#1
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);
}
}
?>
#2
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:
<?phpfunction 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.
#3
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.
#4
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?
#5
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.
#6
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.
#7
Here's a patch based on #6 for evaluation as well.
#8
The last submitted patch, hook_file_download_access_alter-missing-entity-argument_1143460_7.patch, failed testing.
#9
#2: hook_file_download_access_alter-missing-entity-argument_1143460_2.patch queued for re-testing.
#10
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.
#11
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.
#12
#13
Fixed 'TThe' in the docs.
#14
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.
#15
here's a patch for d7.
#16
The last submitted patch, fix-hook-file-download-access-alter-parameters_1143460_15.patch, failed testing.
#17
tested in d8-- patch in 13 applies cleanly and works fine.
and here's a d8 patch for folding everything but $grants into $context.
#18
The last submitted patch, fix-hook-file-download-access-alter-parameters_1143460-_17.patch, failed testing.
#19
let's concentrate in one place: #1245220: file_file_download() passed bogus $field to field_access()