Comments about patch 28:
1) Once set, it's applied to all the items?
+ if ($item->isLink()) {
+ // do not delete symbolic links
+ $delete = false;
+ }
2) If anything goes wrong in migration of files... cannot copy something or so... what happens? Exception (so savepoint is no set?) Should we also provide some feedback of the process? We need the upgrade heartbeat here, I think.
+ if ($result && $oldversion < 2008071803) {
+ /// move all course, backup and other files to new filepool based storage
+ upgrade_migrate_files();
+ /// Main savepoint reached
+ upgrade_main_savepoint($result, 2008071803);
+ }
3) Do we need to copy files from temp to moodledata this way? Cannot the file be moved or copied directly?
+ while (!feof($temp)) {
+ $data = fread($temp, 65536);
+ fwrite($file, $data);
+ }
4) Lines like this (in core) assume we are using local file storage. Could we call that, simply, get_file_storage() and then, effectively, the local one (filepool). But that would allow to get others if someday we see new ones. i.e. in general I always would use file_storage, send_file... and so on (without the "local" part) because that could be overwritten by other storages. We should, simply be using "storages".
+ $fs = get_local_file_storage();
Same with functions like: file_browser-> build_local_file_children(), that "local" naming.... -1 for it.
5) I haven't looked too much about formslib internals. Really I need to learn a bit before understanding what you've changed there. 
6) One question I think we haven't commented... user & group icons... are they going to be in new filepool or will continue in /user dirs?
7) I'd define the algorithm to calculate path-hashes within one method somewhere, instead of having it here and there:
+ $pathnamehash = sha1($contextid.$filearea.$itemid.$filepath.'.');
....
+ $newrecord->pathnamehash = sha1($newrecord->contextid.$newrecord->filearea.$newrecord->itemid.$newrecord->filepath.$newrecord->filename);
....
8) Repeating 4), this name, abusing of the "local" token... we need to make one glossary about how to name everything.
+ public function create_file_from_localfile($file_record, $fid) {
9) Not critical now, but it has been difficult to me to guess what are all those file_info_XXXX classes. Need documenting.
10) Cool stuff! Congrats!
Going to test it now! B-)
sending draft file storage implementation - it is still missing methods and interface definitions...