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.

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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

m4olivei’s picture

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:

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);
  }
}
WorldFallz’s picture

Version: 7.0 » 8.x-dev
Assigned: Unassigned » WorldFallz
Status: Active » Needs review
FileSize
928 bytes

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:

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.

WorldFallz’s picture

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.

Weaver’s picture

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?

WorldFallz’s picture

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.

WorldFallz’s picture

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.

WorldFallz’s picture

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

Status: Needs review » Needs work
kasunpeiris’s picture

Status: Needs work » Needs review
WorldFallz’s picture

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.

Dave Reid’s picture

Yeah it would be best to do:

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

Dave Reid’s picture

Dave Reid’s picture

Fixed 'TThe' in the docs.

WorldFallz’s picture

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.

WorldFallz’s picture

Status: Needs review » Needs work

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

WorldFallz’s picture

Status: Needs work » Needs review
FileSize
2.56 KB

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.

WorldFallz’s picture

Status: Needs work » Closed (duplicate)
David_Rothstein’s picture

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.

David_Rothstein’s picture

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.

WorldFallz’s picture

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?

BrockBoland’s picture

Needs issue summary

colette’s picture

Issue summary: View changes

added issue summary

colette’s picture

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

colette’s picture

I rewrote the patch at #6 for Drupal 7.

colette’s picture

Status: Needs work » Needs review
pjcdawkins’s picture

adammalone’s picture

adammalone’s picture

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

warmth’s picture

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

adammalone’s picture

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

adammalone’s picture

That awkward moment where you forget to attach the patch

adammalone’s picture

Status: Needs review » Reviewed & tested by the community
David_Rothstein’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
2.03 KB

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.

WorldFallz’s picture

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.

adammalone’s picture

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.

haydeniv’s picture

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.

Iritscen’s picture

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?

David_Rothstein’s picture

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.

David_Rothstein’s picture

haydeniv’s picture

Bot is still happy. This is good to go.

Iritscen’s picture

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.

David_Rothstein’s picture

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

WorldFallz’s picture

David_Rothstein FTW! Thanks Dave.

David_Rothstein’s picture

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.

David_Rothstein’s picture

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.

David_Rothstein’s picture

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 :)

David_Rothstein’s picture

.

David_Rothstein’s picture

Issue summary: View changes

fixed editing

ThirstySix’s picture

I used Drupal core version 7.41. but, I got same error.

PDOException: SQLSTATE[23000]: Integrity constraint violation: 1048 Column 'id' cannot be null: INSERT INTO {download_count} (fid, uid, type, id, ip_address, referrer, timestamp) VALUES (:db_insert_placeholder_0, :db_insert_placeholder_1, :db_insert_placeholder_2, :db_insert_placeholder_3, :db_insert_placeholder_4, :db_insert_placeholder_5, :db_insert_placeholder_6); Array ( [:db_insert_placeholder_0] => 543 [:db_insert_placeholder_1] => 0 [:db_insert_placeholder_2] => node [:db_insert_placeholder_3] => [:db_insert_placeholder_4] => 192.168.1.17 [:db_insert_placeholder_5] => http://192.168.1.12/test/content/lorem-ipsumfdsfs [:db_insert_placeholder_6] => 1459427707 ) in download_count_file_download_access_alter() (line 196 of /..../modules/contrib/download_count/download_count.module).

Thanks in advance.

ThirstySix’s picture

Issue summary: View changes

Patch #25 already there in version 7.41. but, I still got an error. any idea?