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);
Comments
Comment #1
m4oliveiI 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:
Comment #2
WorldFallz CreditAttribution: WorldFallz commentedIt's definitely a bug-- I've traced it to the fact that file.module calls drupal_alter with 5 parameters (line 197):
But module.inc only allows for 4 (line 904):
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:
I verified d8 still has the bug-- patch for d8 attached.
Comment #3
WorldFallz CreditAttribution: WorldFallz commentedFYI, 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.
Comment #4
Weaver CreditAttribution: Weaver commentedI 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?
Comment #5
WorldFallz CreditAttribution: WorldFallz commentedActually, it could be argued that since $entity_type is available at $entity->type, the file.module call should be changed to:
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.
Comment #6
WorldFallz CreditAttribution: WorldFallz commentedThing 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.
Comment #7
WorldFallz CreditAttribution: WorldFallz commentedHere's a patch based on #6 for evaluation as well.
Comment #9
kasunpeiris CreditAttribution: kasunpeiris commented#2: hook_file_download_access_alter-missing-entity-argument_1143460_2.patch queued for re-testing.
Comment #10
WorldFallz CreditAttribution: WorldFallz commentedactually, 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.
Comment #11
Dave ReidYeah it would be best to do:
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.
Comment #12
Dave ReidComment #13
Dave ReidFixed 'TThe' in the docs.
Comment #14
WorldFallz CreditAttribution: WorldFallz commentedI 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.
Comment #15
WorldFallz CreditAttribution: WorldFallz commentedhere's a patch for d7.
Comment #17
WorldFallz CreditAttribution: WorldFallz commentedtested in d8-- patch in 13 applies cleanly and works fine.
and here's a d8 patch for folding everything but $grants into $context.
Comment #19
WorldFallz CreditAttribution: WorldFallz commentedlet's concentrate in one place: #1245220: file_file_download() passed bogus $field to field_access()
Comment #20
David_Rothstein CreditAttribution: David_Rothstein commentedThis 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.
Comment #21
David_Rothstein CreditAttribution: David_Rothstein commentedOh, 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.
Comment #22
WorldFallz CreditAttribution: WorldFallz commentedAs 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?
Comment #23
BrockBoland CreditAttribution: BrockBoland commentedNeeds issue summary
Comment #23.0
colette CreditAttribution: colette commentedadded issue summary
Comment #24
colette CreditAttribution: colette commentedI've added an issue summary, with the proposed solution of David_Rothstein and WorldFallz.
Comment #25
colette CreditAttribution: colette commentedI rewrote the patch at #6 for Drupal 7.
Comment #26
colette CreditAttribution: colette commentedComment #27
pjcdawkins CreditAttribution: pjcdawkins commented#25: drupal_alter_d7_patch-1143460-25.patch queued for re-testing.
Comment #28
adammalone#25: drupal_alter_d7_patch-1143460-25.patch queued for re-testing.
Comment #29
adammaloneIf patch in 25 still passes all tests (after the upgrade to 7.17) I feel this patch is RTBC.
Comment #30
warmth CreditAttribution: warmth commented:( so bad this patch didn't make it to 7.17
Comment #31
adammaloneAdded in a little doco for the additional parameter and rerolling to try and get a pass result for tests.
Comment #32
adammaloneThat awkward moment where you forget to attach the patch
Comment #33
adammaloneComment #34
David_Rothstein CreditAttribution: David_Rothstein commentedHm, 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.
Comment #35
WorldFallz CreditAttribution: WorldFallz commentedI 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.
Comment #36
adammaloneI too have been using it on a couple of production sites. Haven't experienced any issues in the duration since I've patched it.
Comment #37
haydeniv CreditAttribution: haydeniv commentedI'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.
Comment #38
Iritscen CreditAttribution: Iritscen commentedNot sure, but it looks like we need a new version of the patch as of 7.18?
Comment #39
David_Rothstein CreditAttribution: David_Rothstein commentedThe 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.
Comment #40
David_Rothstein CreditAttribution: David_Rothstein commented#34: drupal_alter_d7_patch-1143460-34.patch queued for re-testing.
Comment #41
haydeniv CreditAttribution: haydeniv commentedBot is still happy. This is good to go.
Comment #42
Iritscen CreditAttribution: Iritscen commentedApologies, 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.
Comment #43
David_Rothstein CreditAttribution: David_Rothstein commentedOK, 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...
Comment #44
WorldFallz CreditAttribution: WorldFallz commentedDavid_Rothstein FTW! Thanks Dave.
Comment #45
David_Rothstein CreditAttribution: David_Rothstein commentedDrupal 7.19 was a security release only, so this issue is now scheduled for Drupal 7.20 instead.
Fixing tags accordingly.
Comment #47
David_Rothstein CreditAttribution: David_Rothstein commentedDrupal 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.
Comment #48
David_Rothstein CreditAttribution: David_Rothstein commentedFixing 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 :)
Comment #49
David_Rothstein CreditAttribution: David_Rothstein commented.
Comment #49.0
David_Rothstein CreditAttribution: David_Rothstein commentedfixed editing
Comment #50
ThirstySix CreditAttribution: ThirstySix commentedI 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.
Comment #51
ThirstySix CreditAttribution: ThirstySix commentedPatch #25 already there in version 7.41. but, I still got an error. any idea?