Tests needed: file.inc
| Project: | Drupal |
| Version: | 7.x-dev |
| Component: | tests |
| Category: | bug report |
| Priority: | critical |
| Assigned: | jhedstrom |
| Status: | closed |
| Issue tags: | File API |
This information is from the code coverage report (see http://coverage.cwgordon.com/coverage).
We need to test:
1) Private file downloads.
2) file_create_path() for uncreatable file paths and/or no destination.
3) file_check_directory() for unwritable directories.
4) file_copy() with an unreachable destination.
5) file_copy() with a nonexistent source.
6) file_copy() with FILE_EXISTS_ERROR.
7) file_move() with bad parameters.
8) Attempts to upload insecurely named files.
9) file_unmunge_filename().
10) file_delete() with bad parameters.
11) file_space_used() with no uid.
12) file_save_upload() with various possible errors.
13) Uploading of very long filenames.
14) Uploading of files that push us over the quota.
15) Uploading images beyond the maximum dimensions.
16) file_save_data() with bad parameters.
17) file_set_status() with bad parameters.
18) file_transfer().
19) file_download().
#1
Part of #142995: Add hook_file and make files into a 1st class Drupal object is to create tests for file.inc
#2
Better to get tests along with shiny new code than testing dusty old code, so let's mark this postponed.
#3
Reading over http://acquia.com/files/test-results/includes_file.inc.html it looks like this is much improved.
Functions that really need coverage:
- file_transfer()
- file_download()
- file_munge_filename()
- file_unmunge_filename()
#4
I should be able to take these on today or tomorrow
#5
The attached patch adds the following:
* Tests that hit file_transfer() and file_download() via secure downloads. In order to accomplish this, the file_test module needed to be able to set a custom header, so I added that functionality too.
* Tests for file_munge_filename and file_unmunge_filename
#6
i'd really like to get #329226: Store file_test.module's values in variables rather than globals in first so we can pass the return values into the hooks in a uniform way.
#7
here's a re-roll/cleanup now that #329226: Store file_test.module's values in variables rather than globals has been committed.
#8
The last submitted patch failed testing.
#9
See: #335122: Test clean HEAD after every commit and http://pastebin.ca/1258476
#10
Looks good, but we no longer have phpdoc for setUp(), getInfo() or tearDown()
http://drupal.org/node/325974
#11
catch, i'm going to leave the comments in place since there's still a ton of other places (common.test, session.test, etc) where those functions are still commented. if they're really no longer the standard (as opposed to someone just editing the handbook page which has happened before) then we need a massive patch to remove them from all the tests.
here's a re-roll to get rid of some fuzz and maybe trick the test bot in to retesting the patch.
#12
While the tests for hook_file_download() are all well and good as unit tests for that function, I'd really love to also see a functional test that handles a "real world" scenario for the private file system, such as:
1. Private files and upload module are turned on.
2. An unpublished node is created with a file attached to it via upload module.
3. The author of the node can correctly download the file.
4. Someone who is NOT the author gets a 403.
I guess this would more accurately go into upload.test which makes this slightly kitten-eating, but they are pretty closely related and would go well together.
Also, could you clarify this line:
+ $this->name = $this->randomName() . '.' . $this->bad_extension . '.txt';I don't understand why we start with an already-munged filename when the behaviour we're trying to test (I think) is that if you try and upload a dangerous file like 'rootkit.php' it gets correctly munged. I tried removing
. '.txt'and the tests indeed fail with that but I still don't quite see why. Either this is a bug, or it needs some comments to explain what's going on.catch is right that I've been committing patches lately without PHPdoc on the inherited functions. I won't hold this up for that though, but as long as you're making other adjustments you might as well take them out. I do agree that we need a separate patch that removes them everywhere though, so this doesn't continue to be ambiguous.
#13
webchick, yeah the private uploads would be kitten eatting... and i'm hoping that if we don't support it private files will just disappear... kind of like image toolkits.
all that the name munging does is to work around a bug(?) in apache that lets it execute files with names like "foo.php.txt" so the name munging looks for things like this and changes the .php. to a .php_. it doesn't deal with dangerous exensions that should be handled by file_save_upload() take a look at: http://api.drupal.org/api/function/file_munge_filename/6
#14
since i'm not changing the other bits at this point i'm going to leave the comments alone and bump the status back on this.
#15
okay fine... created #338403: Remove implementation of... comments from setUp(), getInfo() or tearDown() so i'll pull these comments out. also added a comment to the munge test's setUp() to make it a little clearer what's going on.
#16
The last submitted patch failed testing.
#17
i'd duplicated this as #353032: Unit tests for file_munge_filename(), file_unmunge_filename() and private downloads
#18
#19
fixing some white space issues, a missing PHPDoc comment, and Text -> Test that webchick pointed out on irc.
#20
missed some spacing on a phpdoc block.
#21
okay actually missed another one.
#22
fixing the comment on testMungeIgnoreInsecure().
#23
the simplest patches are the hardest because people devote the same amount of time to it as they would a 50kb one... even better comment for testMungeIgnoreInsecure().
#24
No, I'm just a pain in the ass when it comes to patch reviews, period. ;)
Committed to HEAD. Thanks, guys!!
#25
Automatically closed -- issue fixed for two weeks with no activity.