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

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 )

*subscribing*

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

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?

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 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.

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

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

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.

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 upload.module is still using a file, do not let other modules delete it.
+      $count = db_result(db_query('SELECT count(*) FROM {upload} WHERE fid = %d', $file->fid));
+      if ($count) {
+        return 'upload';
+      }
+      // Delete all file revision information associated with the file.
+      db_query('DELETE FROM {upload} WHERE fid = %d', $file->fid);
+      break;

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?

+      $values = db_fetch_array(db_query('SELECT vid, description, list FROM {upload} u WHERE u.fid = %d', $file->fid));
+      if (!empty($values)) {
+        foreach ($values as $key => $value) {
+          $file->{$key} = $value;
+        }
+      }

@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.

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.

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?

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.

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:

patching file includes/common.inc
Hunk #1 succeeded at 1787 (offset -3 lines).
Hunk #2 succeeded at 1888 (offset -3 lines).
Hunk #3 succeeded at 2246 (offset -3 lines).
Hunk #4 succeeded at 2256 (offset -3 lines).
patching file includes/file.inc
Hunk #14 FAILED at 474.
Hunk #19 FAILED at 641.
Hunk #20 FAILED at 662.
Hunk #25 FAILED at 1124.
4 out of 25 hunks FAILED -- saving rejects to file includes/file.inc.rej
patching file modules/blogapi/blogapi.module
Hunk #1 succeeded at 378 (offset 9 lines).
patching file modules/system/system.admin.inc
patching file modules/system/system.module
Hunk #1 FAILED at 1223.
1 out of 1 hunk FAILED -- saving rejects to file modules/system/system.module.rej
patching file modules/upload/upload.admin.inc
patching file modules/upload/upload.module
Hunk #2 succeeded at 261 (offset -8 lines).
Hunk #3 succeeded at 434 (offset -8 lines).
Hunk #4 succeeded at 450 (offset -8 lines).
Hunk #5 succeeded at 463 (offset -8 lines).
Hunk #6 succeeded at 493 (offset -8 lines).
Hunk #7 succeeded at 518 (offset -14 lines).
Hunk #8 succeeded at 599 (offset -14 lines).
patching file modules/user/user.module
Hunk #1 succeeded at 389 (offset -29 lines).
Hunk #2 succeeded at 1500 (offset 2 lines).

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)

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:

+ * - Adds thew new file to the files database. If the source file is a 

("thew")

+ * move is performed by copying the file to the new location and the deleting

(the -> then)

+  // file will not be deleted from drupal, but file_delete will

(we use "Drupal")

+  // return a populated array that tests as TRUE.

(Initial caps.)

+  // 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")

+  // Return true for non-existant file, but log that nothing was actually delete, as the intended
+  // result of _file_delete is in fact the current state.

(I think we use "TRUE", and also "delete" -> "deleted".)

+ *   - FILE_EXISTS_REPLACE - Replace the existing file
+ *   - FILE_EXISTS_RENAME - Append _{incrementing number} until the filename is unique
+ *   - FILE_EXISTS_ERROR - Do nothing and return FALSE.

(Inconsistent use of periods.)

+ *   - FILE_STATUS_PERMANENT - A permanant file that will drupal's garbage

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

+ *   A numeric file id or string containg the file path.

("containg" => "containing")

-    '#description' => t('Display attached files when viewing a post.'),
+    '#description' => t('Determines whether files attached to nodes are listed in the node view by default.'),

(For better or worse, we generally avoid mention of a "node" in user-facing text.)

+        // return the name of the module and how many references it has to the file.

(Capitalize "return".)

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

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().

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 life... You can hound people to re review it.

@justinrandell, wow you're really helpful with you subscribe comment! thank you for participating in the development process. We appreciate your extremely useful feedback.

@drewish, @quicksketch, before running off and changing patches that are RTBC... I really wish you guys would wait for a committer to comment on the patch and what needs changing. You basically moved this patch out of their field of vision by restarting development to cover a personal aesthetic issue. I'll point to _ being reserved for private methods which are object constructs, not functions. I'll point to the fact that I'm only keeping the private functions in use in those modules because I want to limit the changes introduced by this patch so it is easier to review. Later I would much rather them using file_* functions instead...

I have a lot of work and time invested in this patch, not just the patch but foundation work in filesystem and fileapi. I could have done all this in contrib by now, had I not decided to work on getting this stuff in core instead. I even got Dries sorta blessing at OSCMS on this path for D6... This patch even received a code freeze exception and couldn't garner enough reviews or feedback from a core committer to get comitted...

I have pretty much lost hope for playing nice with core, and seriously feel like going back to contrib only development. I feel this patch should just be abandoned.

Hey dopry, I wasn't meaning to be discouraging. I was just trying to clean up a little further. The last time Gabor looked at the patch he specifically stated (#39):

- 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

So even if we marked it RTBC, I doubt it would get in without fixing these problems. I gave this code another review today and found that I missed one last _file_copy() instance. Patch attached. Another review would be appreciated.

Okay, eating my own words here. Gabor specifically said *not* to update the documentation (which is exactly what keith.smith did in #91, drewish in #94, and me in #102).

So here's the patch again, only with the documentation fixes not included. (PHPdoc is still correct for new or modified functions though). Sorry for the trouble. I'll re-roll the documentation fixes after this is committed in a different issue.

Still private/_file_* is a matter of scope. Both drewish and I have indicated that we didn't want to convert those modules to use the new file_* functions because it was unclear how to implement it properly and created workflow changes that make the patch harder to review. Neither of us got a return comment from Gabor regarding if it was acceptable to leave them unchanged until after this patch got committed... I think you're addressing the wrong issue by renaming the functions instead of updating modules using private functions that *should* no longer be used outside of file.inc.

I still believe one of the most wonderful features of functional coding and OO design is the ability to break out of OO when it's effective. I still don't agree with the function name change, but then again that is just a matter of preference... Anyways have fun finding more reviewers...

I hate file_*_plain but who cares anymore, eh? So you have you look at the end of the function name to determine if you're working with objects or paths...

Would some who could commit this please take a look and give us a final say on filenaming or remaining reservations, or just commit it?

FYI at this time only Dries can commit to 7.x.

Now, I'm really wondering why it takes so long to get this in (or even get feedback)..
No wonder you gave up on this dopry...

I'll review this patch shortly. Stay tuned, I've been working with/other patches. :)

Btw, note that I'm not the only one who can review patches. If we all think it is truly ready to be committed, than it should be easy to get this in as soon as I've looked at it.

The patch from #111 still applied with some offset, just re-rolling to apply cleanly to HEAD.

Tested the patch as well, and I can confirm the RTBC status.

@Dries, I don't for one second thing you're the only person who can review patches, but there are bandwidth issues for community review. There are several questions floating that are waiting on a commiter's feedback... and there hasn't been a reply from a committer in almost a year... Really this issue has needed is someone to step in and make an architecture and name space decision the principals working on this patch haven't been able to agree upon. It would have take a 5 minute note...

like follow up #100 to present should sum it up.

Like 95% of active patches against HEAD at the moment, this needs re-rolling for the concat operator spacing changes.

Rerolled with concat operator spaces. cvs diff is darn slow for me at the moment, so this is svn diff output instead. Pushing back to its previous state.

Subscribe.

Please don't use hook_file($op). instead use hook_file_$op to take advantage of hook registry.

Agreed, so this needs to be updated for the registry.

Updated patch to use hook_file_$op instead of hook_file($op).

We also want to be able to test this patch, but at the moment the upload.test has multiple failures and exceptions. I have a patch to clean it up (see #253506: contact.test broken (was upload.test)), but simulating file uploads with simpletest seems to be broken at the moment. If you are familiar with file uploads using curl, please review that issue.

Tested against a fresh copy of HEAD. Although it works, and works well, several things stand out as possibly needing attention.

1. File uploads while creating a new node (as uid1) work as expected, but the form admonished me that the maximum file upload size was 1MB, and then proceeded to save the node with a 9MB music file attached to it.

2.

Weird. My point number 2 above got cut off, and now I can't remember what it was. Ah well.

I went to test the patch again, to see if it would remind me, but the patch no longer applies. Re-rolled, retested, and still RTBC.

Re-rolled with minor fixes. Still needs review and feedback by Dries (since more than a month now; I can understand dopry).

IMO, file_*_plain() sounds a bit awkward -- maybe file_*_direct(), file_*_unsync(), file_*_now()... or to call it appropriately by layer file_*_filesystem(), resp. file_*_fs()?

I gave this patch another test and it really _is_ RTBC.

Now, Dries why are you not commenting/reviewing this patch? What is wrong, and why aren't we getting any pointers what could be wrong or does not meet your vision?

You wanted better file/media handling, and this is certainly a big part of it!

dopry helped me with some file-handling questions, and I promised him I'd take a look at this patch in return. That probably can't happen before the weekend, but subscribing. :)

So I noted that file_move is probably incorrect.. I should probably only return a boolean, and not clone the $source, only update the file path if the move is successful.

First of all, I know how this feels. You have such a great patch and all you get is nitpicking. Sorry, that's my duty. Please understand that I would not have spent so much time with reviewing this patch if I would not think Drupal needs it. I have patched up file.inc and began to read where the patch began so it's not impossible I am addressing old wrongs in file.inc -- feel free to say "this was always so, we will fix in a new issue" but then please open said issue and link to it.

1. Line 249, A file object of the original file. <= I am sorry but what's that? What about "A stdClass object with properties A, B, C" listed on place and adding @see everywhere else? file_save_upload sounds good because that's where the file object is born.
2. Drupal's 'files' directory will be used <= system_file_system_settings says t('File system path'), which might be more udnerstandable.
3. the FILE_EXISTS_RENAME line is 88 chars long.
4. the @see file_copy_plain() would be better at the end of the copy block.
5. "If the source file is a temporary file, the resulting file will also be a temporary file" -- as file_save_upload explains what's a temporary file, we need a "@see file_save_upload about temporary files".
6. $dest really shoudl be $destination. Everywhere in file.inc
7. clone($source); there is no need for () around $source, that was a trick for PHP4 w/ drupal_clone.
8. When should I use and when I should use file_copy_plain? And why "plain" ? I understand it's not easy to name these functions (see drupal_get_form vs drupal_retrieve_form ) but as the doxygens are almost the same, there are no directions for the aspiring developers. From the issue it's clear I more want to use file_copy but then why I would use file_copy_plain...? Help!
9. By changing the incoming parameters to file_copy_plain , it remains a pain to debug -- experience shows that by the time the messages fire, one or another variable becomes empty.
10. file_destination does not handle FILE_EXISTS_REPLACE at all.
11. file_move_plain does not use rename or move_uploaded_file. I do not readily understand why.
12. file_delete "Force File Deletion ignoring reference counting" Too Many Caps Words to begin with. But then again what's a reference in this content? there is documentation inside the function, why not in the doxygen? Also, consider a better word because reference and reference counting are already pwnd by Zend Engine itself.
13. if (!$force && $references = module_invoke_all('file_references', $file)) please add () around the assignment just to avoid guessing on operator precedence.
14. You can save a little code by if ($return = file_delete_plain($file->filepath)) { } return $return;
15. file_delete_plain @return description is too long, 81 chars.
16. Again, at the end of file_delete_plain you can save a return statement.
17. file_space_used o_O is the bitwise AND ANSI SQL? Can't we use = IN or something? Also, I would be so much happier if you would concat the SQL together based on $uid than doubling (almost) the same code.
18. file_save_upload doxygen, "The file will be added to the files table as a temporary file. Temporary files" ends at 82th column.
19. you put the $validators assigment before the cache check...? Why?
20. I find a bit strange the $validators array AND the file_validate hook. Why do we need both?
21. $message .= '<ul><li>' . implode('</li><li>', $errors) . '</li></ul>'; HTML tags? In Drupal 7? Heathens! theme('item_list')
22. if ($file = file_save($file)) erm, references were invented quite long ago and I would prefer that. This is weird.
23. file_save_data_plain really should use file_put_contents and the doxygen should mention that like file_copy and file_move doxygen mentions the relevant php functions.
24. A numeric file id or string containing the file path just say no. Move this function into _file_load and give me wrappers for a numeric file id and another for the string.
25. return clone($files[$file_id]); another clone with superflous().

About #22, I am getting tired. $file is an object, no need for references, then... just file_save($file) will do.

#11 that's the original drupal behavior... <= not a really good argument as you are just throwing out the original drupal behaviour :D

than have to deal with people bitching about the security of dynamically building the where clause. <= What "people"? There are very few people who would bitch over the security of something I suggested :) But you have not answered the more important very strange bitwise SQL operator.

I still do not understand why a module can't simply define hook validators. Maybe we want to instead name/tag/whatever the file objects so the hook_validator knows when to fire?

node_load and user_load takes complex data structures or a scalar. NOT two different kind of scalars. You are opening up for really weirdo bizarre bugs when the file is named "2".

@chx, I chose the bit wise operator for the file.status column, and that is not being added in this patch. It was added in a previous patch. Therefore it's of no consequence in the review of the patch.

The time to lodge your complaints against the bitwise approach would have been when the file table restructuring and initial file handling changes were made. This is fact in D6 as well. My original intention was for this field to also include public, private, viewable, processing currently unavailable, read only etc as core status... However to date only FILE_STATUS_TEMPORARY and FILE_STATUS_PERMANENT are valid statuses.
@see: http://drupal.org/node/115267 where this column and behavior was introduced.

to answer your question regarding ANSI SQL and the bitwise AND operator.... No the bitwise AND is not a part of the ANSI SQL92 specification but is implemented in Oracle, MSSQL, MySQL, PostGres, SQLlite, Sybase, and a few others.

Several hunks had offsets and some hunks failed. Re-rolled against head. Also learning how to cvs diff properly.

Parsing error; you're missing a curly brace on the end of an if-block on line 119 of file.inc.

Here's a fix.

In file_create_path, shouldn't a more sensible default be used, like the empty string, instead of a 0? It would further emphasize that $destination is a string.

Also, can the test plan in #81 be turned into a real SimpleTest? I'm not familiar enough with the framework to know if uploads are possible, but it seems like this patch is ready to roll based on others comments. I'll try to test it out later today.

Uploads are definitely possible with SimpleTest. :)

update for anyone subscribed: we're in the middle of the Media Code Sprint right now, and this issue is top of the list. You're welcome to jump in for some heavy-duty testing and whatnot on this patch over the next couple of days! find aaronwinborn, drewish, or dopry at #drupal in irc if you're interested.

file_create_filename should probably be called file_create_filepath, based on what it does. not sure if we want to get into that in this patch, but i wanted to record that thought while it was freshly thunk.

or maybe it should even be file_create_new_filepath, since my first recommendation could be confusing if a developer thought it would simply create the path to an existing file, which the function name could be interpreted as doing without reading the documentation.

In the interests of naming things in plain, understandable, non-RTFM style, I vote a hearty +1 for file_create_new_filepath. Nice call.

This patch rolls the latest additions to file.test into #150. It includes more complete documentation, and FileSaveTest. It also includes better documentation for file_check_location, which previously confused the heck out of me while writing tests... Note that two tests included currently fail. And there are more tests to be written.

This patch adds to #154, creating a FileDirectoryTest class with many directory handling tests, and moves one test (testFileDirectoryPath) from FileSaveTest to FileDirectoryTest. The patch also fixes an error in file_check_directory() where it would return true if a directory wasn't writeable, but the mode was 0. I also edited the comments referring to file_create_directory(), which is a non-existent function, to correctly refer to file_check_directory().

I took a look at the two failing tests. The first one points out a bug with file_check_location(), so I'm creating a separate issue for it.

The second failing test is a simple $this->assertEqual() instead of $this->assertNotEqual(). Here's a patch with that minor change.

The testing environment
Tested against a clean copy of HEAD, but not a clean db.

9 PHP notices against core form handling in system.admin.inc
With patch #157 applied, I get a

notice: Undefined index: #value in /www/drupalhead/modules/system/system.admin.inc on line 776

on the admin/build/modules page, which triggers about 8 other related notices as well. Line 776 looks like this:

<?php
772.  else {
773.    $form['enable'] = array(
774.      '#markup' =>  theme('image', 'misc/watchdog-error.png', t('incompatible'), $status_short),
775.    );
776.    $form['description']['#value'] .= theme('system_modules_incompatible', $status_long);
777.  }
?>

Simpletests
When running the File tests, Directory Tests passed, File Save failed, and File Validation passed. The single point of failure in File Save is the

Non-existent file fails when checked against realpath().

which happens in the testFileCheckLocation() function. Since this is a separate issue now, I'll paste a link in here once flobruit makes that issue know to me. (Florian!!! Send me the linky!)

File uploads and saves
While testing the capability of Drupal to upload, attach, save, and delete files, well, I didn't come across any problems. Nor did I find anything that could even remotely be considered a bug.

EDIT: Woops, I found something right after I wrote that. warning: strtr() [function.strtr.html]: The second argument is not an array in /www/drupalhead/modules/syslog/syslog.module on line 106. occurs when attempting to delete a picture that's been attached to an article by a user greater than uid1.

I'm calling this RTBC once the other issue gets resolved.

I'll clean house until I get a virgin sandbox, and try again.

EDIT ~ Yup drewish, A fresh copy of core plus a fresh db yields these same "line 776" notices. I tried it with and without the #157 patch. It's not me, it's you. I mean, it's not this patch, it's core. ;-)

As soon as that one little problem with testFileCheckLocation() gets fixed, this baby is ready to take to the skies. Over a year later too. W00t!

Apparently, since PHP 5.2, most distributions' PHP package has a patch which alters the behavior of realpath(): http://ch2.php.net/manual/en/function.realpath.php#82770

Since realpath() can have two different return values for a non-existing file, we can't test for it, attached patch removes the bad assertion. The file tests now run without failures on Ubuntu Hardy (which uses the BSD version of the realpath() behavior).

After a quick look through the instances of realpath(), the only place in core that seem to be dependent on the alternate behavior of realpath() is in file_check_location().

Tested again, and still working.

Nice job. I reviewed my list above and I agree with this being RTBC.

Subscribing and adding words of encouragement. When Dries returns from vacation I'm sure he'll feel it's like Christmas with such a nice patch waiting for him!

subscribing

What needs to happen to get this RTBC?

I was going to raise a feature request, having picked through the Form Interface stuff, which was going to be something like this:

PROBLEM:
file_save_data() does not add your file to the 'files' table. However, file_save_upload() does.

RESOLUTION:
Make file_save_upload more flexible, so it doesn't have to get the file from a form field - it could be passed as binary data in variable after being received over a web service, or something like that.

It seems this new hook will resolve this issue though, right? It looks like it, at first glance. For Drupal 6.x I guess I will have to manually insert in to the 'files' table. =(

Re-rolling to patch cleanly. Also removed references to dsm() from file.inc. The file.test from #166 is still the latest set of tests, although many of them are currently failing. If I get a chance tomorrow I'll try to get those fixed and finish the tests mentioned in #167.

patching file includes/common.inc
Hunk #1 succeeded at 1819 (offset 19 lines).
Hunk #2 succeeded at 1920 (offset 19 lines).
Hunk #3 succeeded at 2283 (offset 24 lines).
Hunk #4 succeeded at 2294 (offset 24 lines).
patching file includes/file.inc
Hunk #18 FAILED at 705.
Hunk #33 succeeded at 1598 (offset 372 lines).
1 out of 33 hunks FAILED -- saving rejects to file includes/file.inc.rej
patching file modules/blogapi/blogapi.module
patching file modules/simpletest/simpletest.install
patching file modules/simpletest/simpletest.module
Hunk #1 succeeded at 556 (offset 6 lines).
patching file modules/system/system.admin.inc
patching file modules/system/system.module
patching file modules/upload/upload.admin.inc
patching file modules/upload/upload.module
patching file modules/user/user.module
Hunk #4 succeeded at 1535 (offset 3 lines).

greg.harvey, how about actually trying out the patch? if you did you'd notice file_save_data() puts records into the database while file_save_data_plain() does not. i know it's almost as 200 comment issue but at least look at the patch before chiming in.

Woah, that's a bit harsh!

I can't try the patch because I'm not running Drupal 7.x - perhaps that wasn't clear enough, but I did elude to Drupal 6.x in my original post. I was actually trying to do the decent thing and make sure this was being addressed in 7.x *before* raising a feature request in Drupal 6.x. Since I don't have the luxury of *any* free time on my current project, I think it's grossly unfair of you to have a pop at me for not spending several hours installing a totally new instance of D7 just to see how a patch works. Sure, I could've read the patch file - but I read the jist of the post instead and decided it was easier to just seek clarification.

A simple "Yes" would have sufficed. I didn't need a lecture.

"When should I use and when I should use file_copy_plain? And why "plain" ? I understand it's not easy to name these functions (see drupal_get_form vs drupal_retrieve_form ) but as the doxygens are almost the same, there are no directions for the aspiring developers. From the issue it's clear I more want to use file_copy but then why I would use file_copy_plain...? Help!"

Edit: in general the issue is stalled because of this: " I care, though not enough to dive through the issue and read up on it and test it (I wouldn't even know where to start), etc. It's just so overwhelming" quoted straight from #drupal. We either need smaller pieces or we need testing instructions or something. There is absolutely no writeup about what does the patch do, what were the design considerations... nothing. It's very hard to comprehend such a big change without any help whatsoever.

Testing instructions and issue summary can go at Patch spotlight. That's what it's there for; so people don't have to read all 400 replies to help get something in.

The attached patch adds the ability to test for file_hook_*() being invoked via a file_test helper module which implements the various hook_file hooks (although not all of them yet). I've moved file.test back into the patch, since the patch also adds file_test.module and file_test.info. Many tests still fail (from actual errors in file.inc from what I can tell).

Edit: changed file.inc to file.test, which is now back in the patch.

I was able to get the upload test working, although hook_file_save is currently not getting called. I think this is a problem with the file_test module's implementation, rather than a bug in file.inc.

I didn't get a chance to separate out the upload tests into their own class.

I would like to give this a thorough look over this weekend. So if there's a way to get it RTBC by then, that would be awesome. :)

Awesome!! I've allocated 2 hours tomorrow to dedicate to this patch. Thanks so much, drewish.

So it turns out 2 hours is not nearly enough, but here's what I managed to get done. This is my first major API patch review, so please be kind. :D

So what's this all about then?
Step 1 was figuring out what exactly this patch does, which took the bulk of the time. Unfortunately, at ~200 replies, it's really difficult to sift through all the comments and determine which are still valid and which are not. Please add this information to http://drupal.org/patch/spotlight to make it easier for testers. I've put the bulk of the contents of this post over there as a starter.

According to what I've studied, this table summarizes the changes made in this patch. Please feel free to correct me if I'm wrong (or better yet, correct patch spotlight! :D).

There are obviously some other things, such as tests for file API!!!! :D, renaming $dest to $destination in various places, and cleaning up bucket-loads of code comments which cant help but jump out at you when you're staring at something like file.inc for 300 hours. ;) But this is the major jist: treat file operations similar to node operations, and let other modules step in and do things when files are being manipulated in the database. We also retain a group of low-level file manipulation functions which just do their basic deeds (delete a file) without invoking hooks or manipulating the database. This is to allow for things like the CSS aggregator or favicon.ico upload, which don't need that level of overhead.

Patch review plz!
Ok, so now on to the patch review. Unfortunately, this is only a partial review because that sucker is BIG and most of my time was spent figuring out what exactly was going on. Anyway!

This is absolutely beautiful, gorgeous work. This addresses a number of glaring inconsistencies in file.inc and starts to make working with files in Drupal really powerful. Two of the top 13 things in Dries's hit-list at Szeged are images handling in core and better document management in core. I feel like this patch is going to get us there. :)

I have two classes of feedback: actual "looks like a bug" stuff, and nit-picky webchick stuff.

Buggy stuff
I know that this has been brought up by others, and I know you're sick of hearing it, but _plain() just... it just doesn't work. :( My first thought upon reading the patch was that the _plain() extension meant that this function was for dealing with plaintext files. Obviously, this is completely and utterly wrong. So far the only alternative I like is file_save_fs() but then it becomes "why not file_save_db()?" and if we do that, it ruins the simple elegance of the API as you have it currently. Maybe we can brainstorm on IRC about it this week?

Some code-level stuff:

Looks like you forgot to remove the line above this that's referencing the old file_delete() function?

during...? :) Also, this line should not be indented, for consistency with filename.

Is this (and other) TODOs in this patch pending the bugs you found with simpletest.module, or..?

There are a couple of places like this in user.module where you've removed file_exists() wrappers. Just curious, why is that? Are we instantiating this earlier in the process so we know that it's guaranteed to be there, or..?

Nit-picky stuff

Needs a - before filename like the line above it.

The description for file_copy_plain() is "Copy a file to a new location without **saving a record** in the database." Let's make all of these functions' descriptions consistent, so it's clear they all belong together. Same with the PHPDoc descriptions of the ones that do save to the db.

I really liked seeing inline comments like this to explain what other modules might do with the hook. Could you add a little sentence like this to each one?

Ok, what next?
In order for this to be committed, a couple of things need to happen:

1. I need a pow-wow with Dries about it; since this is a pretty major thing that touches a lot of stuff, I wouldn't feel right about committing it without running it past him first. At least not until I get a couple hundred more commits under my belt. ;) j/k
2. I would love for everything (or at least most things) that touches a file to be run through by a human. There are enough weird things going on with our testing framework that it'd be nice to get old-fashioned human confirmation on some of this stuff. As far as I can tell, that means the following:
   - Adding, re-adding, and deleting a user picture, and posting a comment and node with user pictures enabled on the theme to ensure that they show up properly.
   - Adding, re-adding, and deleting a favicon and site logo in the themes settings.
   - Adding, re-adding, and deleting multiple files through upload.module.
   - Post a file to Drupal with an desktop editor that uses BlogAPI.
   - Testing the CSS and JS aggregators, clearing cache.
   - Testing picking various color schemes in Color module.
   I estimate this won't take someone longer than about 15-20 minutes to do.
3. I need to find another uninterrupted chunk of time to sit down and go over this again -- for example, I only glanced through the .test file and didn't get a chance to run it yet. Drewish, are you coming to the Lullabot Portland meetup next Tuesday? If so, maybe you, Nate, and I could hammer out any final issues if they remain, and I'll work on "sitting down" with Dries between now and then and get his shortlist as well?

drewish, webchick: If I understand correctly, file_delete_plain() checks for non-existing files, so I think it's unneccessary to check again. Additionally, having an entry in $user->picture without the file being existent should be an illegal state and thus be logged (which file_delete_plain() does), so I think it's even _better_ without the file_exists() check.

For the _plain() issue, maybe it could help to swap the verb and the suffix? I would think that file_plain_copy() is more intuitive to read than file_copy_plain(). It might also open up more suffixes prefixes to consider, e.g. file_pure_copy(), file_unmanaged_copy(), or whatever you think of.

file_copy_raw (or file_raw_copy) sounds better to me than _plain. Maybe _unmanaged would work too.

As for the ordering, I'm actually totally indifferent on that, it was just an idea to maybe get better ideas.

I only thought of _unmanaged when I had already written the whole post, and the more I think about it, the more I like it. Might sound a little C#-ish, but it seems like a nice mixture of "caution, don't do that unless you know what you're doing" and actually describing what it does (or rather, doesn't) accomplish. Putting it in front of the verb would also have the effect that autocompleting IDEs don't show it together with the "managed" function but apart from those in a separate section.

And as drewish seems to like it, let's play my 1337 regex skills and have the exact same patch from above with file_unmanaged_* instead of file_*_plain.

drewish: As I said, I think a watchdog message is really adequate here, because $user->picture shouldn't point to a file that doesn't exist. Or am I wrong with that assumption? If you leave the file_exists() in the if() this illegal state is just skipped without reporting it.

When your mind feels narrowed and you're lazy as hell, boring janitorial stuff might be the appropriate response. Consequently, yay for fixing a few spelling errors and ending sentences with dots. I guess I should feel embarrassed now.

Questions:
- Is there any logic behind marking asserts with the 'File' group and not doing so?
- In file.test, testFileSaveData(), why is there a drupal_get_messages() that's commented out? (Shouldn't it be either in use or not exist at all?)

+    $this->assertTrue(file_delete($this->file), t("Delete worked."));

If file_delete() returns either TRUE, FALSE or an array with references, then this would also catch references, right? Does it make sense to have it as

+    $this->assertTrue(file_delete($this->file) === TRUE, t("Delete worked."));

or am I missing something?

Other than this pointless nitpicking, I don't find coding issues. Not tested functionality-wise, but style and concepts are fine. (I had the pleasure of working with the latter in filefield, and they also work in practice :P )
Good work!

If someone rerolls the patch instead of modifying the .patch file (yeah, *that* lazy), you might also replace the remaining "existant" with "existent" in file.test. Not that someone would care about spelling errors or missing sentence-trailing dots in comments.

Merging my changes into drewish's new patch. Sorry for the testbot and overall being an annoyance today.