I've gone through the patch on #142995: Add hook_file and make files into a 1st class Drupal object and tried put pull out all the doc clean ups and minor fixes that clean up file.inc. It also adds some basic unit tests for a good chunk of the file. Hopefully this can be committed easily and make the process of reviewing hook_file more approachable.

Changes include:
- improved documentation.
- replacing 0's with FALSE values.
- file_move() and file_copy() now only take strings as a source path and return the new path.
- adds unit tests for validators, move, copy, delete, and directory/path functions.
- adds a file_validate() function to handle calling an array of validators.
- removes the uid = 1 check from file_validate_extensions().
- passes the parameters to db_query() as an array now.
- dumps the directory separator crap from file_directory_temp()--windows uses / just fine.

CommentFileSizeAuthor
#17 file_308434_cleanup.patch2.68 KBdrewish
#7 file_cleanup.patch73.07 KBdrewish
#4 file_cleanup_c4.patch73.63 KBjpetso
#5 file_cleanup.patch73.75 KBdrewish
#3 file_cleanup.patch73.64 KBdrewish
file_cleanup.patch71.82 KBdrewish
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Damien Tournoud’s picture

Damien Tournoud CreditAttribution: Damien Tournoud commented

I start reviewing this, but stopped in the middle (that's a huge patch!).

-function file_create_path($dest = 0) {
+function file_create_path($destination = NULL) {
$file_path = file_directory_path();
- if (!$dest) {
+ if (!$destination) {
return $file_path;
}

What about making those is_null() to also manage an hypothetical directory named '0' (could be common in hashed directory scenarios: 000/ 001/ 002/ 003/ etc.)?

I did a code review up to file_copy() and I could not find anything to say to the changes made. I'll finish that review and run a full test suite tomorrow, if webchick is not faster than me on this one :)

Oh, one nitpick note: there are lots of "else if" that should be "elseif" per coding standards. But we have another issue for this (#282405: Enforce coding convention on "elseif").

jpetso’s picture

jpetso CreditAttribution: jpetso commented

subsub

drewish’s picture

drewish CreditAttribution: drewish commented
FileSize
file_cleanup.patch73.64 KB

re-rolled with some of webchick's comments. moving the one failing test to #200586: file_check_location and non-existing directories.

jpetso’s picture

jpetso CreditAttribution: jpetso commented
FileSize
file_cleanup_c4.patch73.63 KB

And with "elseif" instead of "else if", as per Damien Tournoud. (file.inc is now "else if" free, the other files should probably be handled by the mentioned styleguide issue itself.)

drewish’s picture

drewish CreditAttribution: drewish commented
FileSize
file_cleanup.patch73.75 KB

forgot to fix file_create_filename().

drewish’s picture

drewish CreditAttribution: drewish commented

missed jpetso's patch but thing that whole thing is a stupid change.

drewish’s picture

drewish CreditAttribution: drewish commented
FileSize
file_cleanup.patch73.07 KB

wacked the testsetup and tear downs that weren't doing anything

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented

To clarify, "some of webchick's comments" was about an hour and a half in #drupal of me going painstakingly line-by-line through the patch and finding little niggling problems and asking really annoying questions and drewish fixing things as we went along. ;)

One thing to be mindful of: a new function, file_validate() was added. This centralizes file validation operations, and has the side-effect of removing the special-casing of uid 1 in the file extension uploads, so you can no longer upload .exe files as user 1 without adding that to your list of allowed extensions. If we really want that (afaik upload module is the only one that uses it), it can be handled at the module level in a separate patch.

Damien also looked this over thoroughly. We're awaiting a couple small fixes to the .test, and then I think this is ready to go.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Needs review » Fixed

Oops. Cross-posted.

While I was waiting for the re-roll, I also manually tested Color module, Upload module, private file downloads, CSS/JS compressor, etc. and didn't find any errors except:

notice: ob_end_clean() [ref.outcontrol]: failed to delete buffer. No buffer to delete. in /Applications/MAMP/htdocs/core/includes/file.inc on line 899.

randomly when editing a node with files attached sometimes. But this particular line wasn't touched by this patch. I'll go see if it's been reported tomorrow.

I should also point out that this patch doesn't do DBTNG stuff, so that'd need to be handled in a separate patch. But seeing as this patch has been sitting there from before we had SimpleTests even, I think that's ok. :)

Tests pass with the latest patch. I'm pressing the big green button. YAY!!!!!!!

drewish’s picture

drewish CreditAttribution: drewish commented

there's already an issue for the ob_clean_end() warning: #205227: If output_buffering = Off then ob_end_clean fails to delete buffer

drewish’s picture

drewish CreditAttribution: drewish commented
Status: Fixed » Postponed (maintainer needs more info)

i think i might have broken the aggregator tests... i think we need a patch with a few blocks like these:

-    file_save_data($opml, $path);
-    return $path;
+    return file_save_data($opml, $path);
drewish’s picture

drewish CreditAttribution: drewish commented

maybe color.module too?

+++ modules/color/color.module  15 Sep 2008 11:13:24 -0000
@@ -308,9 +308,9 @@ function color_scheme_form_submit($form,
   foreach ($info['copy'] as $file) {
     $base = basename($file);
     $source = $paths['source'] . $file;
-    file_copy($source, $paths['target'] . $base);
+    $filepath = file_copy($source, $paths['target'] . $base);
     $paths['map'][$file] = $base;
-    $paths['files'][] = $paths['target'] . $base;
+    $paths['files'][] = $filepath;
   }
Damien Tournoud’s picture

Damien Tournoud CreditAttribution: Damien Tournoud commented

#253702: Clarification on session handling functions renamed session_save_session to drupal_save_session, and thus probably broke that test. I'll roll a patch in a few hours if nobody beats me at it.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented

I can confirm that the tests were all passing for me at the point that this patch was committed.

drewish: Does this need a follow-up patch with the return statements?

drewish’s picture

drewish CreditAttribution: drewish commented
Assigned: Unassigned » drewish
Status: Postponed (maintainer needs more info) » Active

yeah we should probably just fix it here. it doesn't seem to be affecting the tests, i noticed it try to track down an the problem with the hook_file patch throwing a bunch of exceptions. i'll get on it this afternoon.

c960657’s picture

c960657 CreditAttribution: c960657 commented

The patch that was checked in contains a line that looks like it was temporarily commented out.

+    // Clear out any resizing messages.
+#    drupal_get_messages();
+  }
drewish’s picture

drewish CreditAttribution: drewish commented
Status: Active » Needs review
FileSize
file_308434_cleanup.patch2.68 KB

here's a patch that fixes these but i'm still running into the problem i mentioned over on the hook_file issue. basically every test is throwing exceptions during cleanup.

drewish’s picture

drewish CreditAttribution: drewish commented

okay, it was a permissions problem with my files directory that was screwing up the tests. this patch should be fit for review.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Needs review » Fixed

Looks good. Tests continue to pass. Committed. Thanks!

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented
Status: Fixed » Closed (fixed)

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