This is a backport of: https://www.drupal.org/project/drupal/issues/3016814

-------------------------
Original text:
-------------------------
I have a implementation of hook_file_download because I check if this is a private/specific file and do a redirection for specific link:

function my_file_download($uri) {

 //some code here
 // if(substr($uri,0,26) =='private://specificcontent/'){
    header("location:http://mysite.com/myURL");

}

But, when I remove a file using user interface, this function too is called, generating a error, because the code try redirect for "http://mysite.com/myURL". I believe that when the file is deleted, not is correct receive a call in hook_file_download. But, case if it is not evitable, what I need do for verify that is a delete command and ignore redirection command?

There is a useful solution for version 8, but I haven't found the same solution for version 7 of drupal:

https://www.drupal.org/project/drupal/issues/3016814

Issue fork drupal-3212823

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

vctlzac created an issue. See original summary.

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Title: Don't trigger hook_file_download when no file is requested in drupal 7 » Don't trigger hook_file_download when no file is requested
Assigned: vctlzac » Unassigned
poker10’s picture

poker10 commented
Component: ajax system » file system
Priority: Major » Normal
Status: Needs review » Active

Thanks for reporting this. I do not see any patch, so it seems like Active is the correct status (there is nothing to review).

poker10 opened merge request !7522

poker10’s picture

poker10 commented
Title: Don't trigger hook_file_download when no file is requested » [D7] Don't trigger hook_file_download when no file is requested
Category: Feature request » Bug report
Status: Active » Needs review
Parent issue: » #3016814: Don't trigger hook_file_download when no file is requested

I have backported the change from #3016814: Don't trigger hook_file_download when no file is requested together with test changes.

Regular pipeline is green: https://git.drupalcode.org/project/drupal/-/pipelines/148353
Test only job is failing here: https://git.drupalcode.org/project/drupal/-/jobs/1343442

---- FileDownloadTest ----
Status    Group      Filename          Line Function                            
--------------------------------------------------------------------------------
Fail      Browser    file.test         2647 FileDownloadTest->testPrivateFileTr
    Correctly returned 404 response for a private file url without a file
    specified.
Fail      Other      file.test         2649 FileDownloadTest->testPrivateFileTr
    Value array (
    ) is equal to value array (
      0 => 
      array (
        0 => 'private://',
      ),
    ).

Moving to Needs review.

poker10’s picture

poker10 commented
Issue summary: View changes
poker10’s picture

poker10 commented
Issue tags: +Pending Drupal 7 commit

Adding the tag for the final review.

mcdruid’s picture

mcdruid commented

This looks pretty good but I'm not sure about the assertions that reference an element on the return value of variable_get() - e.g.:

    $this->assertEqual($file->uri, variable_get('file_test_results', array())['download'][0][0]);

Perhaps it's safe to assume that the variable will always be set and that element will be present, but if we do something similar in isolation:

$ drush php
Psy Shell v0.9.12 (PHP 7.4.33 — cli) by Justin Hileman
>>> print_r(variable_get('file_test_results', array())['download'][0][0]);
PHP Notice:  Undefined index: download in phar:///usr/local/bin/drush8/vendor/composer/..eval()'d code on line 1

It may make the code more long-winded but can we avoid this?

poker10’s picture

poker10 commented

Yes, I was on the edge when backporting these rows. Seems like we can use file_test_get_all_calls() to retrieve these results too, so I have updated the MR with this change. Hopefully now it is cleaner and safer. Thanks!

Tests are still green: https://git.drupalcode.org/project/drupal/-/pipelines/187591
And test-only job is still failing: https://git.drupalcode.org/project/drupal/-/jobs/1743067

  • mcdruid committed 7a6e1783 on 7.x 
    Issue #3212823 by poker10, vctlzac, apaderno, mcdruid: [D7] Do not...
mcdruid’s picture

mcdruid commented
Status: Needs review » Fixed
Issue tags: -Pending Drupal 7 commit

Thanks - it's a bit circuitous but I think better to avoid potentially(/theoretically) referencing non-existent elements.

Status: Fixed » Closed (fixed)

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