Details

1. Add an instance of every repository possible.
2. Add a folder in a course.
3. Add one file from every repository to the folder.
4. Look in the files table for that contextid, and verify that the "source" field for every file is either a URL to somewhere meaningful (ie you can see the original file) or a description of the location of the file within that repository, prefixed by the name of the repository.

1. Add an instance of every repository possible.
2. Add a folder in a course.
3. Add one file from every repository to the folder.
4. Look in the files table for that contextid, and verify that the "source" field for every file is either a URL to somewhere meaningful (ie you can see the original file) or a description of the location of the file within that repository, prefixed by the name of the repository.

Description

The files->source field is supposed to ALWAYS contain some string that indicates where the file came from. For many repositories (like URL or Wikimedia) it should have the full URL to the original file. For private repositories (like Equella) it can just have the original filename like "somefilename.jpg"

These can be used to produce the human-readable source in the fileinfo dialogue in the file manager, both for referenced files and potentially also for files that have been copied into Moodle.

I've been exploring this in 2.3 and finding that:

1) It's really hard to see how this field should be set. Please explain exactly where and how.
2) When it is set, we sometimes see a bare URL and sometimes see a serialised array.
3) It's not set at all for many repositories (like Equella).

I suggest that we stick to ALWAYS using a URL to the source (either the source file itself or to a page where that source file can be found).

Someone needs to go though and fix up all the repositories to behave consistently.

Marina Glancy
added a comment - 05/Jun/12 9:49 AM there is also a function repository::build_tree that has awkward code
$source = serialize(array($params['contextid'], $params['component'], $params['filearea'], $params['itemid'], $params['filepath'], $params['filename']));
repository/lib.php line 1249
This should never work - when source is parsed, it is expected to be associative array. But at the same time I have not found where this function is called from.

Martin Dougiamas
added a comment - 05/Jun/12 12:49 PM - edited Here is the result of my investigations from adding one file from each repository I could into a Folder and looking at the resulting file records:
Server - seems to copy existing source field, which is correct
Recent - seems to copy existing source field, which is correct
URL - contains the full URL of the file, which is correct
Wikimedia - contains the full URL of the file, which is correct
Upload - NULL. I think this should contain the original file name
Dropbox copy - Unless you can actually get a full usable URL from the API that the original user can actually use, I think it should just contain "Dropbox:/folder/filename.ext"
CURRENTLY: O:8:"stdClass":1:
{s:6:"source";s:95:"https://api-content.dropbox.com/1/files/dropbox/Moodle%20test/A%20test%20document%20old%202.pdf";}
Dropbox reference - NULL. See above.
EQUELLA - NULL. Should be the original filename. "Equella:/somefilename.ext"
Flickr public - Should be a full URL only
CURRENTLY: O:8:"stdClass":1:
{s:6:"source";s:70:"http://www.flickr.com/photos/virtuallearningcenter/7216160924/sizes/n/";}
Flickr private - Should be a full usable URL only
CURRENTLY: O:8:"stdClass":1:
{s:6:"source";s:62:"http://farm8.staticflickr.com/7015/6816164337_80fce174cb_n.jpg";}
Non-JS Filepicker - should be a raw source URL (not serialised)
See MDL-33322 and /repository/filepicker.php around line 308

Linked MDL-33322 to this isuse, where we have made a chance to populate ->source to a serialised object with simply source contained within it. I have no idea why we are doing that, but I am integrating it to be conistent.

Dan Poltawski
added a comment - 07/Jun/12 1:02 PM Linked MDL-33322 to this isuse, where we have made a chance to populate ->source to a serialised object with simply source contained within it. I have no idea why we are doing that, but I am integrating it to be conistent.
Point no 1. of this issue is quite important for that reason!

Martin Dougiamas
added a comment - 07/Jun/12 1:43 PM - edited I updated the list above to add something about the non-JS filepicker to make sure it's not forgotten.
DS, are you able to do this soon? If not I need to get someone else on it soon.

I fixed all copied files's source field, but I am not sure what we do with the file references, and I don't think it's correct to set source field for file references, we should use information in files_reference table to look up file details, we don't always have file name when create file references, that's why we have repository::get_reference_details method, filemanager should use this method to display details for file references.

Dongsheng Cai
added a comment - 09/Jun/12 12:54 PM Martin
I fixed all copied files's source field, but I am not sure what we do with the file references, and I don't think it's correct to set source field for file references, we should use information in files_reference table to look up file details, we don't always have file name when create file references, that's why we have repository::get_reference_details method, filemanager should use this method to display details for file references.
I continue to work on this until I get feedback from you.

Martin Dougiamas
added a comment - 09/Jun/12 9:23 PM Yes but the problem is that in this method even the repository plugin itself has no way to look up the original file name. Eg Equella.
There is no place to save it apart from this unless we add a new table for the Equella repository plugin just for this. I think the source field makes more sense anyway.
In fact this issue started when I was trying to implement that method for the Equella plugin and was exploring the source field.
So if we DO have the original url (or at least name) from when the file was first selected let's store it in the source field when the record is created.

Martin Dougiamas
added a comment - 11/Jun/12 2:41 PM Thanks Dongsheng, that looks clearer. I tested with all repos (except Google) and the source field looks better in all of them except box.net.
I still have links like http://box.net/api/1.0/download/24290tkr3svedf9rx95jzcry5e65631n/301361150
but I think it's more useful to print the original path/filename there, exactly like Dropbox. Can you fix that too? We must have it because it appears as the default name in the filepicker dialog.

Dongsheng Cai
added a comment - 11/Jun/12 3:46 PM Martin, I fixed box.net files' fields, they all have filename now, but I am not able to add file path due the limit of API: http://developers.box.net/w/page/12923934/ApiFunction_get_file_info
This should be fixed by upgrading box.net API to 2.0: MDL-33046

Dan Poltawski
added a comment - 12/Jun/12 6:07 PM Is it right that we are hardcoding the name of the plugin (should we be using component name or something?)
The capitlisation seems incorrect:
boxnet:
Dropbox:
s3:

Martin Dougiamas
added a comment - 12/Jun/12 11:37 PM Dan, they're for humans to read, so I think it's ok to hardcode.
The other alternative would be $this->get_name() which is the name of the current instance of the repository but that is not strictly correct IMO. Those may come and go and the file will remain.
I would fix the names though:
Box:
Dropbox:
Amazon S3:

Dongsheng Cai
added a comment - 13/Jun/12 8:02 AM The problem is get_name is it could be override by the name in db: https://github.com/dongsheng/moodle/blob/master/repository/lib.php#L1512
Then people cannot tell where is this file from. I will fix the names to Box, Dropbox, Amazon S3 later today.

Marina Glancy
added a comment - 13/Jun/12 8:53 AM I noticed those hardcoded names as well. Can we use get_class($this), or some function that returns plugin name?
And also define it in repository class and do not overwrite for each plugin

But Marina the plugin is actually not relevant (and if it is then I think we need a new field for it like sourcerepository). The source refers to the place the file came from in the cases where a direct URL is not possible, specifically so people can find the original. These are for people to read, because I want to show this info in the fileinfo dialog later on, and I think these are OK to hardcode.

Martin Dougiamas
added a comment - 13/Jun/12 11:17 AM - edited But Marina the plugin is actually not relevant (and if it is then I think we need a new field for it like sourcerepository). The source refers to the place the file came from in the cases where a direct URL is not possible, specifically so people can find the original. These are for people to read, because I want to show this info in the fileinfo dialog later on, and I think these are OK to hardcode.

Marina Glancy
added a comment - 13/Jun/12 11:25 AM there is another function for human-readable source: repository::get_reference_details() and it actually shows the repository name as it is configured in Setting->Plugins->Repositories->...

Yes, but that is for the source of a REFERENCE/alias/shortcut/symlink. (A) In that case it makes sense because it matches the thing that a human needs to look at (in the filepicker, say) to go and find that file.

This source field is for the origin of ALL files. (B) Even ones copied in. And this is outside Moodle.

In the case of plugins Equella, we actually need (B) to construct (A), because the actual references that we use to serve files are full of code and tokens and are not useful to users. And there is no way to retrieve B from A, so we need to store B at the time the file is created.

Martin Dougiamas
added a comment - 13/Jun/12 11:31 AM Yes, but that is for the source of a REFERENCE/alias/shortcut/symlink. (A) In that case it makes sense because it matches the thing that a human needs to look at (in the filepicker, say) to go and find that file.
This source field is for the origin of ALL files. (B) Even ones copied in. And this is outside Moodle.
In the case of plugins Equella, we actually need (B) to construct (A), because the actual references that we use to serve files are full of code and tokens and are not useful to users. And there is no way to retrieve B from A, so we need to store B at the time the file is created.

well my comment was just from code reviewing without much understanding what this field is for. And in code this function returns the name of the current class. So if you think it's all right to hardcode - no objections from me

Marina Glancy
added a comment - 13/Jun/12 11:39 AM well my comment was just from code reviewing without much understanding what this field is for. And in code this function returns the name of the current class. So if you think it's all right to hardcode - no objections from me

I made a commit fixing misleading phpdoc added. Please - don't bother with phpdoc if you are going to add incorrect phpdoc its worse than none at all.

I'm still unclear on why we are not generating this source information at the point of generating the files in repositories.

We populate a 'source' property of files in repositories when returning files, this is another 'source' property which seems rather misleading. Maybe it could be human readable source or something?

As far as I can tell the way this is working now means we are doing additional requests to external services after the file has been selected and is in the draft files area. I haven't checked what happens if we are editing a file area - does this happen constantly to files which have been copied?

I'm also still unclear why we need to store a serialised structure with one field in the source field in normal circumstances.

So in fact, after writing this post-integration message i'm even less convinced by it.

Dan Poltawski
added a comment - 14/Jun/12 11:29 AM I've integrated this.
I made a commit fixing misleading phpdoc added. Please - don't bother with phpdoc if you are going to add incorrect phpdoc its worse than none at all.
I'm still unclear on why we are not generating this source information at the point of generating the files in repositories.
We populate a 'source' property of files in repositories when returning files, this is another 'source' property which seems rather misleading. Maybe it could be human readable source or something?
As far as I can tell the way this is working now means we are doing additional requests to external services after the file has been selected and is in the draft files area. I haven't checked what happens if we are editing a file area - does this happen constantly to files which have been copied?
I'm also still unclear why we need to store a serialised structure with one field in the source field in normal circumstances.
So in fact, after writing this post-integration message i'm even less convinced by it.

> I'm still unclear on why we are not generating this source information at the point of generating the files in repositories.

But yes, that's exactly when it gets set - whenever a file is created in Moodle.

> We populate a 'source' property of files in repositories when returning files, this is another 'source' property which seems rather misleading. Maybe it could be human readable source or something?

Two different things:

A) File references/aliases/shortcuts can have a source file.
B) All files stored in Moodle come from somewhere and have a source location that explains that a bit.

> As far as I can tell the way this is working now means we are doing additional requests to external services after the file has been selected and is in the draft files area. I haven't checked what happens if we are editing a file area - does this happen constantly to files which have been copied?

What? Really?

> I'm also still unclear why we need to store a serialised structure with one field in the source field in normal circumstances.

Martin Dougiamas
added a comment - 14/Jun/12 2:15 PM I think there is some confusion!
> I'm still unclear on why we are not generating this source information at the point of generating the files in repositories.
But yes, that's exactly when it gets set - whenever a file is created in Moodle.
> We populate a 'source' property of files in repositories when returning files, this is another 'source' property which seems rather misleading. Maybe it could be human readable source or something?
Two different things:
A) File references/aliases/shortcuts can have a source file.
B) All files stored in Moodle come from somewhere and have a source location that explains that a bit.
> As far as I can tell the way this is working now means we are doing additional requests to external services after the file has been selected and is in the draft files area. I haven't checked what happens if we are editing a file area - does this happen constantly to files which have been copied?
What? Really?
> I'm also still unclear why we need to store a serialised structure with one field in the source field in normal circumstances.
That's what we just fixed, no?

> As far as I can tell the way this is working now means we are doing additional requests to external services after the file has been selected and is in the draft files area. I haven't checked what happens if we are editing a file area - does this happen constantly to files which have been copied?

It only happens when we pick a file from repository (creating file reference or copying).

Dongsheng Cai
added a comment - 14/Jun/12 2:21 PM > As far as I can tell the way this is working now means we are doing additional requests to external services after the file has been selected and is in the draft files area. I haven't checked what happens if we are editing a file area - does this happen constantly to files which have been copied?
It only happens when we pick a file from repository (creating file reference or copying).

Martin Dougiamas
added a comment - 15/Jun/12 12:12 AM I'm not sure about the code, but after testing again just now the result in the files->source field is looking correct for everything I tried (all repos except Google).
Dongsheng, the code that Dan was worried about was here where it serialises the data (why is that done and when is that used?)
https://github.com/dongsheng/moodle/compare/integration...dev_MDL-33513_files_source#L7R2220
Note it's different to the code you added in file_prepare_draft_area().

I serializes the source field because we have to pack the original file info in source field, we already serialize all files when preparing the draft area, it makes thing easier if we serialize source field for all files added to draft area, then later, file_restore_source_field_from_draft_file could fix the source field for draft files, this fixed the issue that you saw php serialized data after saving the draft files.

Dongsheng Cai
added a comment - 15/Jun/12 7:55 AM Martin, you are referring this https://github.com/dongsheng/moodle/blob/dev_MDL-33513_files_source/lib/filelib.php#L394 right?
I serializes the source field because we have to pack the original file info in source field, we already serialize all files when preparing the draft area, it makes thing easier if we serialize source field for all files added to draft area, then later, file_restore_source_field_from_draft_file could fix the source field for draft files, this fixed the issue that you saw php serialized data after saving the draft files.

field source is set:
If file is picked by reference (from external or moodle repository) and there is no name conflict
If file is picked as copy from external repository

field source is not set:
If file is picked by reference and there is a name conflict (user is asked whether to rename or overwrite an existing file);
If file is picked as a copy from internal moodle repository.

Why don't we just set $record->source before this big "if" statement and it will be always consistent?

And besides I don't see any usage of get_file_source_info() in non-js filepicker

Marina Glancy
added a comment - 19/Jun/12 10:49 AM I know this is closed. I saw a lot of notifications but did not actually review the code. Now I started documenting it and found that for some reason setting of files.source at
https://github.com/moodle/moodle/blob/master/repository/repository_ajax.php#L269
is included inside the different parts of "if" statement but not all.
field source is set:
If file is picked by reference (from external or moodle repository) and there is no name conflict
If file is picked as copy from external repository
field source is not set :
If file is picked by reference and there is a name conflict (user is asked whether to rename or overwrite an existing file);
If file is picked as a copy from internal moodle repository.
Why don't we just set $record->source before this big "if" statement and it will be always consistent?
And besides I don't see any usage of get_file_source_info() in non-js filepicker
Again, I did not test it, this is just from code review.

Marina Glancy
added a comment - 19/Jun/12 10:51 AM by the way, is the files.source set the same when we upload a file in JS filepicker, non-js filepicker and drag-n-drop? (With overwriting dialogue and without)?