Petr Škoda - 01/Jul/08 07:39 PM sending draft file storage implementation - it is still missing methods and interface definitions...

Petr Škoda - 21/Jul/08 06:30 AM sending latest patch - the only part that fully works if blog attachments ;-)

Petr Škoda - 22/Jul/08 06:38 AM user image and group image uploading fixed

Eloy Lafuente (stronk7) - 24/Jul/08 02:52 AM 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-)

Petr Škoda - 24/Jul/08 08:07 PM sendig patch nothing new, just merged after latest messaging commit

Petr Škoda - 24/Jul/08 08:12 PM 1/ hmmm, that is a bug 2/ if anything goes wrong the file is not deleted, but the upgrade continues - I agree there should be some feedback 3/ if we move we can not call that function repeatedly - that is why I copied it 4/ I like the "local" because it means "not repository" - this could be easily changed any time, my -1 6/ user and group icons can be in old location or we could move them to pool - not sure, fortunately this is a separate problem 7/ good idea, I thought about this too, but was lazy ;-) 8/ I still like "local" ;-) 9/ sure 10/ thanks!

Martin Dougiamas - 25/Jul/08 02:24 PM - edited OK, some random thoughts as I"m going through. M1) Cool, it works! :-) M2) Some naming/text things ...   a) Can "filepool" just be "files"? Or to keep it unique and easy to discuss "filedata" (we should avoid new jargon for non-english admins).   b) In file->fileareas there is "coursefiles" ... should that not probably just be "course" to be consistent?   c) The file->fileareas are a single word ... won't there be collisions when two plugins have the same name? eg a mod and block ... how about we use the path of the module like mod/fred and blocks/fred as a convention for everybody to follow? and if mod/fred wants multiple fileareas then mod/fred/blah and mod/fred/foo   d) "Please do not edit, delete, move or change in any way files or subdirectories here!!" --> "This directory contains the content of uploaded files and is controlled by Moodle code. Do not manually move, change or rename any of the files and subdirectories here." M3) userid is always null in my table even for new files I add ... shouldn't we record who uploaded the file there? (I know it's "owned" by the module) more coming...

Martin Dougiamas - 25/Jul/08 02:53 PM M4) Tiny thing in blog/lib.php: this line is not used ... $fb = get_file_browser(); M5) I agree with Eloy above about removing the "local" names ... by definition everything in the File API is "local" (not and external repository). M6) The name get_file_browser() threw me off for a while ... I guessed from the name that it was something that allowed the user to browse files because that word is usually a *user* activity. So I guessed wrongly that it was something to do with the user browsing files. It's not clear at all at first what the difference is to get_local_file_storage(). Is this like the difference between dmllib and ddlib? Are there clearer names we can come up with? M7) I think we should define $CFG->filedir in lib/setup.php rather than use $CFG->dataroot/filepool ... so that admins with big complex servers can put it on another drive etc. it's just more flexible and consistent with libdir etc

Petr Škoda - 25/Jul/08 03:41 PM M2) a) I personally do not care much about the naming scheme, anything consistent is ok for me; I did not spent much time on this b) I would vote for "content" instead of "course" or "coursefiles" c) file->fileareas is not a problem, we have contextid there -> context level -> we know exact module or block f) sure M3) during upgrade we do not know the original author, later we know him only sometimes, I did not concentrate on this, going to fix it M4) sure M5) oki M6) * file_storage is a low level implementation of file storage, each module may access only own files through this API, it must not access anything outside it's own scope * file_browser is an abstraction of all files that are available - they may or may not be stored in file_storage; the primary use will be in file manager UI and image selector/manager in html editor M7/ sure

Martin Dougiamas - 25/Jul/08 04:51 PM OK cool. Just one little bone left for this dog. the filearea names are for self-documentation for humans in table and URLs (I realise the contextid is there and is sufficient for code to work it out). This is just about a coding guide to suggest standard format names for developers of modules to choose. I agree with the argument from the chat we just had that full pathnames are too much, so I think we have three things to choose from: Choice 1) http://mymoodlesite.edu/pluginfile.php/787/content/1/sample.jpg Choice 2) http://mymoodlesite.edu/pluginfile.php/787/assignment_content/1/sample.jpg Choice 3) http://mymoodlesite.edu/pluginfile.php/787/assignmentcontent/1/sample.jpg My +1 for Choice 3.

Petr Škoda - 25/Jul/08 05:06 PM my +1 for Choice 1

Eloy Lafuente (stronk7) - 25/Jul/08 05:07 PM My +1 for Choice 2 (better readability) hehe :-D

Martin Dougiamas - 25/Jul/08 05:21 PM Penny agrees with me so #3 wins! Next bikeshed please!

Petr Škoda - 27/Jul/08 10:34 PM hmm, "_" is allowed character in blocks, just name of module may not be enough because names may collide with block ok, I give up it is easy to change this anyway if the collective mind changes ;-) luckily any module author is free to use whatever he/she wants

Petr Škoda - 29/Jul/08 06:36 AM latest version, applies to current HEAD

Mathieu Petit-Clair - 29/Jul/08 11:47 AM On a brand new checkout, with files used a bit everywhere (some files used in content, some as attachments, some multiple times, etc.), in blog, assignements, forums, summaries... Upgrade fails and outputs following messages: (mysqli): SELECT * FROM mdl_context WHERE contextlevel = 50 AND instanceid = '1' LIMIT 2 Warning: sha1() expects at most 2 parameters, 5 given in /home/mathieu/workspace/head_fresh/lib/file/file_storage.php on line 43 ... (mysqli): SELECT 'x' FROM mdl_files WHERE pathnamehash IS NULL LIMIT 1 Warning: Missing argument 2 for file_storage::get_pathname_hash(), called in /home/mathieu/workspace/head_fresh/lib/file/file_storage.php on line 390 and defined in /home/mathieu/workspace/head_fresh/lib/file/file_storage.php on line 42 Warning: Missing argument 3 for file_storage::get_pathname_hash(), called in /home/mathieu/workspace/head_fresh/lib/file/file_storage.php on line 390 and defined in /home/mathieu/workspace/head_fresh/lib/file/file_storage.php on line 42  Warning: Missing argument 4 for file_storage::get_pathname_hash(), called in /home/mathieu/workspace/head_fresh/lib/file/file_storage.php on line 390 and defined in /home/mathieu/workspace/head_fresh/lib/file/file_storage.php on line 42 Warning: Missing argument 5 for file_storage::get_pathname_hash(), called in /home/mathieu/workspace/head_fresh/lib/file/file_storage.php on line 390 and defined in /home/mathieu/workspace/head_fresh/lib/file/file_storage.php on line 42 Notice: Undefined variable: filearea in /home/mathieu/workspace/head_fresh/lib/file/file_storage.php on line 43 Notice: Undefined variable: itemid in /home/mathieu/workspace/head_fresh/lib/file/file_storage.php on line 43 Notice: Undefined variable: filepath in /home/mathieu/workspace/head_fresh/lib/file/file_storage.php on line 43 Notice: Undefined variable: filename in /home/mathieu/workspace/head_fresh/lib/file/file_storage.php on line 43 Warning: sha1() expects at most 2 parameters, 5 given in /home/mathieu/workspace/head_fresh/lib/file/file_storage.php on line 43 ... Query: INSERT INTO mdl_files (contextid,filearea,itemid,filepath,filename,timecreated,timemodified,mimetype,userid,filesize,contenthash,pathnamehash) VALUES('2','course_content',0,'/','prof_vieux.jpeg',1217302754,1217302754,'image/jpeg',NULL,2479,'f6b5dbc5a7a6ba41ce809a9f234c849befac9af0',NULL) failed. Column 'pathnamehash' cannot be null .... (mysqli): INSERT INTO mdl_files_cleanup (contenthash) VALUES('f6b5dbc5a7a6ba41ce809a9f234c849befac9af0') Can not create file "2/course_content/0///prof_vieux.jpeg" # line 402 of lib/file/file_storage.php: stored_file_creation_exception thrown # line 161 of lib/db/upgradelib.php: call to file_storage->create_file_from_pathname() # line 70 of lib/db/upgradelib.php: call to upgrade_migrate_files_course() # line 487 of lib/db/upgrade.php: call to upgrade_migrate_files() # line 326 of admin/index.php: call to xmldb_main_upgrade() The file 'prof_vieux.jpeg' is in the site's files, and used multiple times in different summaries.

Petr Škoda - 29/Jul/08 02:41 PM fixed wrong refactoring, should work again

Mathieu Petit-Clair - 29/Jul/08 04:00 PM Hi Petr, on the same installation (reverted to pre-previous update database), most things (forum post, blogs post - both in text and attachements - summary text for course and forum) work nicely now after the upgrade. The only problem I see now (though it might just be because you haven't attacked this yet) is with assignments. In both "advanced uploading of files" and "upload a single file" types, uploaded files are not available anymore. I get a "filenotfound" error. More testing on the way...