Add hook_file and make files into a 1st class Drupal object

it might make sense to roll hook_file_download into this as a $op = 'download'

subscribe .. a reasonable enhancement

I'd rather keep the op names as terminology related to whats actually going on with the files, instead of trying to copy form or node events, I think the op names should correlate to the functions they're being called from as well.

so maybe upload, validate, load, copy, move, update status, delete. I'll try to roll a patch over the weekend...

validate would allow modules to validate new uploads before saving to db.

upload would allow modules to act on new uploads while they're still in temp status. generating admin ui thumbnails.

load would allow modules to extend file objects. This doesn't exists because file_load doesn't exist yet.

copy would allow modules to react to copy events, creating new derivatives, access control entries, or whatever they may need to do. this would happen after a file has been successfully copied on the filesystem and inserted into the database.

move is the same as copy, but you would probably move existing derivs or update existing information.

update status will allow modules to react to status updates.. changing from temporary to permanent, or whatever we dream up in the future.

delete would be called after successful file_deletion.

Subscribe

I marked http://drupal.org/node/18934 as a duplicate of this (since it was super old).

However there was a patch (basically the same idea as here) - so go check it out anyway :)

(subscribe)

Good idea! Finally this could solve my long-awaited problem of conversion of filenames containing i18n characters to alphanumerics. ( http://drupal.org/node/146752 )

Now that #115267 has been committed we can get started. I talked with dopry on IRC a bit yesterday and got an idea of his plan, this patch reflects my (probably incorrect) understanding of it. This patch is not anywhere close to done but I've got to run out and wanted to get it posted.

It turns file_copy, file_delete and file_move into wrappers which handle the database updating end of things while _file_copy and _file_delete do the file end of things. It also adds file_load() and file_save() to read and write to the database.

*subscribing*

I definatly want this in Drupal 6. Not sure yet what I can do to help..

here's a bit better patch. it moves the code in a more logical manner so that the changes are clearer.

stefan, if you wanted to add some code to delete the record when the file is deleted that'd be good. i've tried to add a couple of TODOs to make it easy to see what needs to happen.

whoops, that was missing some stuff. this has better TODOs. feel free to jump in and work on some.

I'll try to come up with something after 10 hours from now...
I don't promiss anything, but i'll give it a shot..

I did some quick reviewing on the patch and while I am not sure it's in scope of this patch, I think using arrays instead of objects is much easier to grasp.

Perhaps this could be a personal taste, but drupal is moving to arrays more and more.. (Think FAPI as an example)

Furthermore this goes pretty much above my knowledge of the file api. While I would *love* to see the functionality hit the trunk, my PHP skills are not up to the level where it meets the criterium to move this further.

I'll keep an eye on the issue, and review/test wherever I can to help, but unfortunatly cooperation on developing it wouldn't be a good idea.

@drewish: Any progress on this?

here's something closer. i've done a little testing on it but i almost need to write a module that uses files to shake all the bugs out.

the current hook operations look like:

• load - fired by a call to file_load(), ignores returned values.
• insert - fired by a call to file_save() for a new file, ignores returned values.
• update - fired by a call to file_save() for an existing file, ignores returned values.
• status - fired by a call to file_set_status(), ignores returned values.
• copy- fired by a call to file_copy(), ignores returned values.
• move - fired by a call to file_move(), ignores returned values.
• delete - fired by a call to file_delete(), ignores returned values.
• validate - fired during file_save_upload(), expects an array of error messages.

If we would use file arrays instead of file objects we could spare some drupal_clones. Second, if we are changing the name of the operating functions, like _file_copy, it's a good time to make that a variable, like $function = variable_get('file_backend', '_file') .'_copy' -- can you spell s3_backend.module :) ?

I liked chx's suggestion of adding a layer of indirection on the underlying file operations. I started looking at converting $file to an array and it'll be a major undertaking. I think it's the right thing to do but it'd touch a lot of files and result in a brittle patch that's out of the scope of this issue.

Digging into this I realized that there are really two classes of file operations, database tracked ones and non-database ones. Things like the aggregated JS and CSS should only be written locally and, at least in this patch, aren't tracked by the DB.

At this point there are only three functions a file driver would need to implement: _copy, _delete, _save_data.

I like the patch and the approach it takes. Makes sense.

What can I do to help?

Is this the place to add folder handling logic?

A module implementing this hook could, in theory, do that but it can be a chicken and egg problem, depending on module logic and invocation time of the hook. We could separate the path from filename, or just store the path separately, the memory overhead will be minimal considering the possible use cases - uploads and file manipulations are rare, usially one off ops on a few of files per run.

I'm think about the implications for modules creating or using some form of logical folders

I would not convert $file objects to arrays, especially not in this patch, if you want this to get reviewed ;)

The layer of indirection is out of scope. Fear the scope creep! There are more issue involved than just the indirection. Settings for particular backends, backend selection, and handling http timeouts on your webserver with large transfers to and from services like S3 or over FTP none of which none are addressed in this patch. I would prefer the scope be limited to just the hooks, and the indirection layer become another feature request.

pulled out the indirection layer. it was a bit off topic.

cleaned up a lot of the phpdocs. started working the upload module. it really should be implementing hook_file to load its data into the file object. i made some changes to it and everything seems to be working correctly. i added some caching to file_load based on what's in node_load.

feedback on the best way to treat upload.module would be much appreciated.

re-rolled. there's probably still some issues but at this point i need reviews.

the patch still applies.

little bit better title.

Very nice architecture improvement. I tested just about all the code changed by this patch. I just have a laundry list of minor improvements.

• typos: search for scalled and memebers and thew
• FILE UPLOAD SETTINGS: 'List files by default' should be a checkbox
• FILE UPLOAD SETTINGS: "If an image toolkit is installed, files exceeding this value will be scalled down to fit.". this message could be more helpful if it looked for a toolkit and tailored the text accordingly. furthermore, the toolkit page says "No image toolkits found. Drupal will use PHP's built-in GD library". So i don't know if my images will be scaled or not.
• when uploading an image to a node: notice: Trying to get property of non-object in /Users/mw/htd/multisite/modules/upload/upload.module on line 632.
• perhaps document the status values in the comments for file_set_status() or tell the developer where to look.
• tabs in upload_file(). use spaces instead.
• i got these errors when uploading a user picture:
  # Object of class stdClass could not be converted to string in /Users/mw/htd/multisite/includes/file.inc on line 252.
# notice: Object of class stdClass to string conversion in /Users/mw/htd/multisite/includes/file.inc on line 252.
# The selected file /Users/mw/htd/multisite/Object could not be copied, because no file by that name exists. Please check that you supplied the correct filename.
# Failed to upload the picture image; the pictures directory doesn't exist or is not writable.
• upon uploading a logo in theme settings: Fatal error: Cannot use object of type stdClass as array in /Users/mw/htd/multisite/modules/system/system.module on line 2224

Thanks for the review Moshe. I think I've corrected all the issues you raised in your review other than:

perhaps document the status values in the comments for file_set_status() or tell the developer where to look.

The thing is at this point there are only values temporary and permanent. And dopry wanted the status as a bitmapped field.

I ran into a little, related problem with this testing the upload quotas on non-uid-1 users. It looks like their temp files are counted against their quota, which makes sense but it's a pain for them because they can't force the cron run that will delete their old files. That might be in the scope of a post feature freeze fix.

As a status update, I'm going to be offline next week. I'll try to keep this patch up until I leave and pick it up when I get back if it hasn't been committed.

1 hunk now fails on upload.module :(. please reroll and i will retest.

This version of the patch updated to to work with HEAD.

It makes behavioral changes.

• file_space_used to only return total filesize used by permanent files when passed a uid.
• file_delete will not delete a file if another module blocks the delete and return an array of modules blocking the deletion, that will still test as true.

I also made a few minor documentation improvements, and document the implementation of hook_file in upload.module.

Applies with offset. Could you re-roll?

/me heard that fuzz was fine
review starts in a few minutes

I really don't like that hook_file(op=delete) can block. It doesn't jive with the rest of Drupal's deletion. hook_delete and hook_nodeapi(op=delete) don't allow other modules to prevent a node's deletion, they allow modules to react to the deletion. I'd also like to see the deletion queries put into the delete API's package but there's probably not a clean way to do that.

The documentation for hook_file shouldn't be put into upload_file()'s comment, but rather added to contributions/docs/developer/hook/core.php

File deletion is not node deletionm if it were the same we would use the same api. There should be another hook in file_delete to react to the deletion.

The idea is based on the behavior of hard links in posix file systems. Where you can have multiple links to the same file data. In Drupal the file data would be the row in the files table and the file itself. If you call file_delete from a module and the deletion is blocked by another module, file_delete still returns true, since there wasn't an error with the actual file deletion, so only the reference to the file gets removed. Once no modules are blocking the physical file_deletion the file will be removed. It is the responsibility of the blocking modules to provide any notices about why they blocked the file delete.

The deletion API has been brought up. There is one problem with the deletion API. It seems to enforce a process of Database query, then callbacks. We really cannot remove files from the database until we confirm that the file has been physically deleted, otherwise the database becomes inconsistent.

One major issue has arisen in this patch. I'll try to re-roll with corrections tonight.

My last implementation of file_delete only allows the delete hook to be a reference count. Modules implementing hook_file cannot react to file deletion consistently this way. Drewish has reservations to the approach.

okay after lots of chatting on IRC with hunmonk and dopry i think we've got a solution that extends the deletion api.

i wanted to make one, unrelated, note though: _file_delete should return TRUE if the file doesn't exist... it's only an error if the file exists and can't be deleted.

i opened up a deletion api issue: http://drupal.org/node/154604

i'd had high hopes for getting #154604 committed and having a nice clean way of implementing the file deletion but dries is talking about rolling back the deletion api patch.

at this point i don't really care how the pre/post-delete hooks are setup. #30 is fine by me but i think dopry wants to make some changes. i'd settle for just about anything so long as the rest gets committed.

I am just reviewing the patch in #30. IMO, it is a *huge* improvement over what we currently have in Drupal. This opens up many many more possiblities for file handling.

I tested upload module functionality and the new or changed API functions. Everything worked as advertised. The concept of hook_file is great. Deleting a file that is still used on a node by upload.module successfully fails. Documentation is very good.

The attached patch only fixes a minor code style issue, removes some debug code that was still in the last patch and fixes one single E_ALL notices in upload.module hook_file op load.

This is RTBC. Great work.

Code-reviewed the patch:

- you introduce wrappers around internal _file_* functions, but still use some internal looking functions in blogapi and system module with this patch
- the phpdoc is not up to current conventions... I understand that the file.inc comments are not properly formatted, and to keep this reviewable, it should not be reformatted to comply to current conventions... but new functions added elsewhere should not follow that bad convention
- I don't understand the rationale behind this:

If there are rows, then the deletion is blocked. If there are no rows, we delete these rows??

Why is this useful? Maybe add some code comments so we got to understand this a bit better?

I still question the rationale behind the delete blocking stuff and since I'm re-rolling I'm going to drop it. I think it complicates the patch and I just want this done with.

Gábor Hojtsy,
- I intentionally didn't modify the blogapi to use file_save_data because the workflow of that module doesn't make it clear to me that it should be tracked in the database. Where is that file deleted? The system.module is a little different case, if that's what people want to do then I'm willing to modify it.
- The attached patch fixes all the PHPDoc in file.inc to bring it to current standards. I'm of the opinion that while we're in there we might as well fix it all.

Dries,
- I added a comment to explain what was happening there.

I also pulled out this block of text from upload.module. It needs to go in the hooks.php file once this patch is committed:

@drewish - sorry i get a hunk fail in upload.module. i will review if you can fix this. it is more than 2 lines so i bailed.

here's a re-roll that fixes conflicts in system.module and upload.module.

tested uploads and user pictures and all works fine. code looks good as well. RTBC.

Here is a reintroduction of file_delete with reference counting. I'll leave it to the powers that be to decide which to commit. It also fixes some direct deletions from the files table in upload_delete and upload_delete_revision.

re-rolled to sync with head. Set back to code needs review, since I added the reference counting on delete.

I guess it helps if I attach the patch.

There are also some changes in _file_delete I've added two additional tests when $path is not a file.

1. in the case a path doesn't exists it returns true, but watchdogs an error since the desired final state of _file_delete($path) is already true, This makes the soft delete of file_delete behave nicely in case you attempt to delete a file from the db where the file has already been removed from the file system.
2. in the case $path is a directory it returns false and watchdogs an error.

Test
- Applied fine, uploaded two files attached to a page node okay.

Bugs
- When editing page node I checked "Delete" next to an existing upload File B and then attached a third file upload File C. When hitting Submit, File B gets deleted but File C doesn't stick around leaving only File A.
- Are files supposed to be deleted from file system when not used anymore? If so, they weren't as all files stuck around after checking "Delete" and hitting save, and still stayed after deleting the node to which they were uploaded.
- By default the file upload box desc says "The maximum size of file uploads is 1 MB. Images larger than 0 will be resized." Images larger than 0? I know that's the default, but is poor usability and confusion. So we need a check in there if (!empty(variable_get('...', 0)) { $desc .= ' '. t('Images larger than...'); }
- Also, I think we should set the default for the above var to '' instead of 0. We tell users to leave setting blank for no restriction elsewhere IIRC, why not here? 0 seems unnecessary and inconsistent. If we keep the !empty() code we'll be okay with 0 or ''.
- Why do all files get an underscore before the .ext? Maybe this is stated above, if so, sorry.
- We should remove "node" from user facing content on the File uploads config screen. I know it was there before, but since we're changing it, let's change line 185 of upload.module to something like 'Determines whether files attached to posts are listed or not in the full page view by default.' That might suck, but we should try and remove node. :P

1. - When editing page node I checked "Delete" next to an existing upload File B and then attached a third file upload File C. When hitting Submit, File B gets deleted but File C doesn't stick around leaving only File A.

   I could not reproduce.

2. - Are files supposed to be deleted from file system when not used anymore? If so, they weren't as all files stuck around after checking "Delete" and hitting save, and still stayed after deleting the node to which they were uploaded.

   There was indeed a missing file_delete.

3. - By default the file upload box desc says "The maximum size of file uploads is 1 MB. Images larger than 0 will be resized." Images larger than 0? I know that's the default, but is poor usability and confusion. So we need a check in there if (!empty(variable_get('...', 0)) { $desc .= ' '. t('Images larger than...'); }

   This is fixed using the !empty test instead of isset.

4. - Also, I think we should set the default for the above var to '' instead of 0. We tell users to leave setting blank for no restriction elsewhere IIRC, why not here? 0 seems unnecessary and inconsistent. If we keep the !empty() code we'll be okay with 0 or ''.

   This is not from this patch. It should be filed as a separate UI issue.

5. - Why do all files get an underscore before the .ext? Maybe this is stated above, if so, sorry.

   I am unable to reproduce. although some files get that so apache won't try to execute them via it's multi type handling.

6. - We should remove "node" from user facing content on the File uploads config screen. I know it was there before, but since we're changing it, let's change line 185 of upload.module to something like 'Determines whether files attached to posts are listed or not in the full page view by default.' That might suck, but we should try and remove node. :P

   This is not from this patch and should be filed as a separate UI issue. There are too many API changes happening in this patch for me to want to entertain scope creep.

Uploading files does not work with this recent patch. Did a clean install and attaching files appears to work, but hitting Submit results in no files attached.

Upload failures are not related to this patch, but to changes in FAPI, see http://drupal.org/node/157747, for details. Setting back to CNR, so I can solicit architectural and code style reviews until 157747 is resolved.

patch still applies

I'm looking forward to these improvements!

For those interested in this bug, I wanted to point out this module I built for drupal 5.x. In particular it has a form element for file uploads. If files are really a first class entity in Drupal, I feel that should be part of it.

My form field is somewhat complex because the various actions that must take place when a file is uploaded (and previewed) do not map well to the form hooks. I understand improvements to FAPI will make this sort of thing easier in the future (yeah!). But I'd encourage folks to check out what I did and consider it's features when adding this stuff to core. That's all - thanks for your time.

And just to keep things somewhat close to head.. here is an updated patch.

Just tested #56 on head, it works fine.

Only strange behavior :

- add a node
- browse for a file
- click attach
- click delete checkbox
- save node

-> File is still attached

This is a very good addition to Drupal too bad it's too late for 6. This is my first patch review, I came too late to the game

subscribing.

subscribing. should have done that long ago.

subscribing

A couple conflicts... Updating to current HEAD.

@phillipjadine, I can no longer reproduce the bug in comment #57. Can you confirm?

i don't think 'reference' is a good name for the operation name. all the others ('copy', 'validate', 'load', 'move') are verbs directing the module to take some action. if 'reference' were to be consistent the module would have to add a reference to that file. what you're really doing is directing the module to report the number of references. something like 'count_references', 'get_usage', 'are_using' would also be better.

how about just 'references'? that is a noun and it almost feels like a interrogative to me.

list_references

Cannot reproduce bug in #57 so consider it fixed. Thank you

Overall, this is a good patch and great addition to filesystem api

I like 'list_references'.

but I so don't like the underscore and I like the single word for brevity... So I made it references.

@peanut gallery:
How about reviewing the patch? Please post some actual feed back. Things like 'work's for me', 'doesn't work for me', 'part Y is confusing can you document it better' are useful.

@drewish, phillipe:
You most definitely aren't in the peanut gallery. :) Thanks for your continued attention to this patch. I really appreciate your feedback.

@drewish:
I really appreciate you doing the lions share of the work to port this to core.

OK stepping out of the peanut gallery.

I'm not sure I understand why the private _file_copy function has exactly the same phpdoc as file_copy() Maybe I missed something and that's standard for private functions, but it looked weird to me.

Otherwise I applied it, enabled upload module, and everything worked as normal.

+1 for this. I hate to having to hack the core to fix broken filenames :(

@catch: the doc isn't the same.. It's just similar. file_copy work on file objects _file_copy works on file paths.

@all: 6 is out, head is open? *wink* *wink* can we get some reviews or gatekeeper reservations?

@dopry: I'd appreciate your comments here: http://drupal.org/node/214934. I'm not sure yet if there's overlap… will give it a look this weekend.

Should hook_file also interact with Forms API? If so, we could move the Form API stuff to this related issue.

Wim Leers, I know that one of dopry's goals was to allow that type of functionality.

Rob Loach, they're definitely related but I don't think there's much form stuff going on in this patch...

drewish, ok cool! :)

I'll await dopry's feedback to see if they can continue to be separate patches or not. I'd prefer separate patches because I have to ship my patch in D5 and D6 versions as well, for my CDN integration module.

@Rob Loach: No the form stuff is out of scope for this patch. This patch only includes api changes to file.inc and hook_file. I am working on a new file form element that leverages the new file api, since it allows file operations to be done asynchronously of the node creation process.

@Wim Leers: Again I think it is out of scope. file_create_url will need modification to support public and private files simultaneously.

@all: can we get reviews of this patch or a RTBC?

oops didn't mean to change status...

I'd be happy to give it a review, but I'm not too sure how to test it. Could you provide a module or something that makes use of hook_file()?

This will allow node-independent files for uploading, browsing and downloading, and move the n:1 file:node relationship to upload.module? Subscribing.

The patch from #68 doesn't apply cleanly anymore:

Otherwise, I'm just subscribing :)

This patch has been updated for head as of today, including the removal of drupal_clone.

@Rob Loach: hook_file upload module implements hook_file op==load and op==references. I don't think we will be able to fully test hook_file in this patch. I suspect as long as core file handling works as expected there should be no issues with getting it committed. We can clear up bugs and issues with additional hook_file ops in the future. Once hook_file is in core I will begin port filemeta and derivative to core, both of which utilize hook_filefield thoroughly.

@Arancaytar: how about reviewing the patch instead of just subscribing and adding useless noise to the issue queue. The file/node relationships were moved to upload.module in D6.

@breyten: this new patch should apply cleanly In the future you can just let me know the patch doesn't apply cleanly. I don't need to know which hunks failed :).

This patch implements:

• hook_file which allows module developers to interact with the file lifecycle
• stratifies the file.inc api into two layers, one that works on drupal file objects and one that works on filepaths
• provides a reference counting hook so file_delete is only a soft delete, and will only remove records from the files table and delete the actual file if no modules claim to be using it.

How to test:

1. Core file handling:
   1. upload logo in the admin >> themes section of your test site. (only tests path based layer of file.inc api)
   2. upload a favicon in the admin >> themes section of your site.(only tests path based layer of file.inc api)
2. User.module
   1. upload a user picture. (only tests path based layer of file.inc api)
3. Upload.module
   1. create a story and attach/remove files. (This tests the drupal file object layer of the file.inc api and hook_file op==load)
   2. create another story without any uploads. manually insert a record in the upload table point to a file
      that is associated with another node. Delete the file from one of the nodes through the UI. (This tests the soft delete feature)

looks like your last patch had some unrelated dblog changes that have since been committed... re-rolled to fix those failed hunks.

there's a problem with the way the user.module's picture are being tracked. the comment says that it should be making an untracked copy but it's calling file_copy() (rather than _file_copy()) but isn't calling files_set_status() to mark it as permanent. i kind of think it makes sense to track the user's profile picture in the database. it'd make it a lot easier to avoid bugs like #92224. if we do track them then we also need to delete the files in user_delete().

i wonder if it might be better to rename the _file_* functions that deal with untracked files to something like file_raw_*() since the leading underscore implies a private function and there are good reasons that people might not want to manipulate non-db tracked files. thoughts?

oh i should mention that i banged on the upload module portion of the code and that all seemed to be working correctly. i didn't get around to messing with the theme portions.

This patch fixes to user.module to use the fileapi as documented, also removes my db_log snafu which is another patch that snuck in there.

I think renaming the functions and altering the way user.module behaves is scope creep. We're doing a lot of API changes as is in this patch. I don't want to change more than absolutely necessary. I think _ is a perfectly acceptable prefix for the low level file functions. If you really want raw file access you have php's file functions. I also hope to implement the mountpoint system in the _file_* layer which will make them more private and drupal specific and not raw file access at all. If you want raw file access user php's file functions.

I read through the patch and found only 2 small issues:

• case 'delete':
+      // Delete all information associated with the file.
+      db_query('DELETE FROM {upload} WHERE fid = %d', $file->fid);
+      break;

  Can we save a query here by checking if $file "belongs" to upload module?

• file_save() does separate insert and update statements with each field named. It looks like the old code does a cleaner drupal_write_record(). search for drupal_write_record in the patch.

To check if the file is owned by upload.module would require a select against the upload table. I opted for one query instead of two.

I didn't keep the drupal_write_record on purpose. I'd still have to do the insert/update logic in file_save and call drupal_write_record ( which includes a bunch of query building insanity ) in both conditional blocks. So it doesn't improve readability or enhance performance I didn't see any advantage in using it in this case.

One of the benefits of drupal_write_record() is that you can add a column in your own install and add a properly named value to the $file object (for example) and drupal will automatically write to the custom column. I'm not going to +1 a patch that avoids using schema API functions as you are proposing.

@moshe, here is one with drupal_write_record...

The test plan in #81 works fine except for the node upload which does not work properly at all. I could not attach a file when clicking the Preview button and I saw a number of NOTICE errors. After thats fixed, I'll test soft delete and we should be good to go.

There is a regression also for the small bug fixed in http://drupal.org/node/193331.

This patch fixes the $replace regression, and uses the correct table name in drupal_write_record which is what broke node uploads.

I read through this, and I think it all looks good. There are (a very) few typos in code comments:

Just on reading those lines...

+  // Make sure the file is deleted before removing it's row from the
+  // database, so UI's can still find the file in the database.

("it's" -> "its")

And UI's -> UIs. It seems that the apostrophe is only added when the acronym has periods, as in U.I.'s.

+ *   - FILE_STATUS_PERMANENT - A permanant file that will drupal's garbage
+ *                             collection will not remove.

("permanant" => "permanent", capitalize "drupal's".)

And drop the extra "will".

subscribing

made corrections based on Arancaytar's fixes.

subscribe.

I went through the test plan in #81 and all tests pass. Nice work.

subscribe

subscribe

This is so exciting! (And, *subscribes*)

Subscribing.

I went over this patch today and found a few bugs which have been fixed in this version.

Bugs:
- There wasn't a way to test file_move() in core, so I added file_move() to index.php to try to move a file with a record in the database. But I received that the function "c()" was undefined. Looks like it's supposed to be _file_delete() from a previous patch.
- _file_move() was undefined, so I was getting undefined function when enabling CSS/JS caching. I wrote a function for this purpose and utilized it in file_move() rather than doing a _file_copy() and _file_delete inside of file_move().

Docs:
- I updated all the documentation so that the first line of PHPDoc is a brief description, with a longer description separated into another paragraph. I also elaborated a bit on file_move vs. _file_move (and other similarly named functions).
- Added @see lines where appropriate.

I find that using _file_move/_file_copy/_file_save all seems very dirty. The preceding underscore is supposed to mean "private", and not to be used outside of the module it's defined in. Encouraging module developers to use "non-API" functions throughout their modules as system, upload, and user have done in this patch would introduce new inconsistencies in what it means to prefix a function with an underscore.

So, to correct this I've renamed all the prefixed functions with a new name: the suffix '_plain'. So we now have file_move_plain, file_copy_plain, and file_save_plain. I considered trying to rename 'file_move' to 'file_record_move' (or similar), but because file_move invokes hook_file_move(), I think it's the right decision to rename the prefixed versions of the functions.

Fixes remaining _file_delete() calls to file_delete_plain().

quicksketch, I'm glad you changed those function names, back on comment #82 I'd proposed _raw but I think _plain is a better choice. I'll try to give this a review shortly.

You guys have fun. I'm totally off this issue. There has been no feedback from a committer in a while, and I'm not going to review superfluous change at this juncture in this patches