Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: 2.2
    • Fix Version/s: 2.3
    • Component/s: Files API, Repositories
    • Labels:
    • Testing Instructions:
      Hide

      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.

      Show
      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.
    • Affected Branches:
      MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE
    • Pull Master Branch:
      dev_MDL-33513_files_source
    • Rank:
      41417

      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.

        Issue Links

          Activity

          Hide
          Marina Glancy added a comment -

          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.

          Show
          Marina Glancy added a comment - 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.
          Hide
          Martin Dougiamas added a comment - - 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
          Show
          Martin Dougiamas added a comment - - 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
          Hide
          Dan Poltawski added a comment -

          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!

          Show
          Dan Poltawski added a comment - 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!
          Hide
          Martin Dougiamas added a comment - - 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.

          Show
          Martin Dougiamas added a comment - - 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.
          Hide
          Dongsheng Cai added a comment -

          Hi, Martin, I started to work on this today.

          Show
          Dongsheng Cai added a comment - Hi, Martin, I started to work on this today.
          Hide
          Dongsheng Cai added a comment -

          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.

          Show
          Dongsheng Cai added a comment - 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.
          Hide
          Martin Dougiamas added a comment -

          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.

          Show
          Martin Dougiamas added a comment - 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.
          Hide
          Martin Dougiamas added a comment -

          Thanks for working on it btw. Is there a github link yet?

          Show
          Martin Dougiamas added a comment - Thanks for working on it btw. Is there a github link yet?
          Hide
          Dongsheng Cai added a comment -

          Martin, I will have to add a new callback repository::get_file_source_info(), I will post the github link when I finish.

          Show
          Dongsheng Cai added a comment - Martin, I will have to add a new callback repository::get_file_source_info(), I will post the github link when I finish.
          Hide
          Dongsheng Cai added a comment -

          Martin, I updated the github link, my branch is on top of integration branch

          Show
          Dongsheng Cai added a comment - Martin, I updated the github link, my branch is on top of integration branch
          Hide
          Martin Dougiamas added a comment -

          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.

          Show
          Martin Dougiamas added a comment - 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.
          Hide
          Dongsheng Cai added a comment -

          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

          Show
          Dongsheng Cai added a comment - 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
          Show
          Dongsheng Cai added a comment - v2.0 doc: http://developers.box.com/docs/#files-file-object-2
          Hide
          Martin Dougiamas added a comment -

          Looks OK to me, thanks DS!

          Show
          Martin Dougiamas added a comment - Looks OK to me, thanks DS!
          Hide
          Dan Poltawski added a comment -

          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:

          Show
          Dan Poltawski added a comment - 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:
          Hide
          Martin Dougiamas added a comment -

          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:

          Show
          Martin Dougiamas added a comment - 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:
          Hide
          Dongsheng Cai added a comment -

          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.

          Show
          Dongsheng Cai added a comment - 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.
          Hide
          Marina Glancy added a comment -

          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

          Show
          Marina Glancy added a comment - 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
          Hide
          Martin Dougiamas added a comment - - 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.

          Show
          Martin Dougiamas added a comment - - 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.
          Hide
          Marina Glancy added a comment -

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

          Show
          Marina Glancy added a comment - 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->...
          Hide
          Martin Dougiamas added a comment -

          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.

          Show
          Martin Dougiamas added a comment - 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.
          Hide
          Marina Glancy added a comment -

          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

          Show
          Marina Glancy added a comment - 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
          Hide
          Dongsheng Cai added a comment -

          Source field prefix fixed.

          Show
          Dongsheng Cai added a comment - Source field prefix fixed.
          Hide
          Dan Poltawski added a comment -

          What are the testing instructions for this?

          Show
          Dan Poltawski added a comment - What are the testing instructions for this?
          Hide
          Dan Poltawski added a comment -

          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.

          Show
          Dan Poltawski added a comment - 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.
          Hide
          Dan Poltawski added a comment -

          Working on something else and I think this is causing breakage.

          [Thu Jun 14 13:25:55 2012] [error] [client 127.0.0.1] PHP Notice: unserialize(): Error at offset 0 of 10 bytes in /Users/danp/git/integration/lib/filelib.php on line 601, referer: http://dan.moodle.local/integration/mod/forum/post.php?forum=11
          [Thu Jun 14 13:25:55 2012] [error] [client 127.0.0.1] PHP Stack trace:, referer: http://dan.moodle.local/integration/mod/forum/post.php?forum=11
          [Thu Jun 14 13:25:55 2012] [error] [client 127.0.0.1] PHP 1.

          {main}

          () /Users/danp/git/integration/repository/draftfiles_ajax.php:0, referer: http://dan.moodle.local/integration/mod/forum/post.php?forum=11
          [Thu Jun 14 13:25:55 2012] [error] [client 127.0.0.1] PHP 2. file_get_drafarea_files() /Users/danp/git/integration/repository/draftfiles_ajax.php:62, referer: http://dan.moodle.local/integration/mod/forum/post.php?forum=11
          [Thu Jun 14 13:25:55 2012] [error] [client 127.0.0.1] PHP 3. unserialize() /Users/danp/git/integration/lib/filelib.php:601, referer: http://dan.moodle.local/integration/mod/forum/post.php?forum=11

          Show
          Dan Poltawski added a comment - Working on something else and I think this is causing breakage. [Thu Jun 14 13:25:55 2012] [error] [client 127.0.0.1] PHP Notice: unserialize(): Error at offset 0 of 10 bytes in /Users/danp/git/integration/lib/filelib.php on line 601, referer: http://dan.moodle.local/integration/mod/forum/post.php?forum=11 [Thu Jun 14 13:25:55 2012] [error] [client 127.0.0.1] PHP Stack trace:, referer: http://dan.moodle.local/integration/mod/forum/post.php?forum=11 [Thu Jun 14 13:25:55 2012] [error] [client 127.0.0.1] PHP 1. {main} () /Users/danp/git/integration/repository/draftfiles_ajax.php:0, referer: http://dan.moodle.local/integration/mod/forum/post.php?forum=11 [Thu Jun 14 13:25:55 2012] [error] [client 127.0.0.1] PHP 2. file_get_drafarea_files() /Users/danp/git/integration/repository/draftfiles_ajax.php:62, referer: http://dan.moodle.local/integration/mod/forum/post.php?forum=11 [Thu Jun 14 13:25:55 2012] [error] [client 127.0.0.1] PHP 3. unserialize() /Users/danp/git/integration/lib/filelib.php:601, referer: http://dan.moodle.local/integration/mod/forum/post.php?forum=11
          Hide
          Dan Poltawski added a comment -

          Reopening this - it dosen't look right for master now and I don't think we can have it unstable like this in the meantime.

          Show
          Dan Poltawski added a comment - Reopening this - it dosen't look right for master now and I don't think we can have it unstable like this in the meantime.
          Hide
          Dan Poltawski added a comment -

          (i've reverted the changes)

          Show
          Dan Poltawski added a comment - (i've reverted the changes)
          Hide
          Martin Dougiamas added a comment -

          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?

          Show
          Martin Dougiamas added a comment - 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?
          Hide
          Dongsheng Cai added a comment -

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

          Show
          Dongsheng Cai added a comment - > 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).
          Hide
          CiBoT added a comment -

          Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

          Show
          CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
          Hide
          Dongsheng Cai added a comment -

          Dan, could you please tell me how to reproduce that error, what repository you were using, you added a file in editor or attachment?

          Show
          Dongsheng Cai added a comment - Dan, could you please tell me how to reproduce that error, what repository you were using, you added a file in editor or attachment?
          Hide
          Martin Dougiamas added a comment -

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

          Show
          Martin Dougiamas added a comment - 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().
          Hide
          Dongsheng Cai added a comment -

          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.

          Show
          Dongsheng Cai added a comment - 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.
          Hide
          Martin Dougiamas added a comment -

          Ah it's for the brand-new files! OK, seems obvious now, thanks!

          +10 to integrate!

          Show
          Martin Dougiamas added a comment - Ah it's for the brand-new files! OK, seems obvious now, thanks! +10 to integrate!
          Hide
          Dan Poltawski added a comment -

          This is conflicting with Marinas changes now

          Show
          Dan Poltawski added a comment - This is conflicting with Marinas changes now
          Hide
          Dan Poltawski added a comment -

          I've integraated it now - those errors have gone awya so they must've been related to something else.

          Show
          Dan Poltawski added a comment - I've integraated it now - those errors have gone awya so they must've been related to something else.
          Hide
          Martin Dougiamas added a comment -

          See MDL-33777 for a follow-on bug about missing code for Equella.

          Show
          Martin Dougiamas added a comment - See MDL-33777 for a follow-on bug about missing code for Equella.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Big thanks for the effort. This is now part of Moodle upstream. Let's wait for regressions, yay! LOL

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Big thanks for the effort. This is now part of Moodle upstream. Let's wait for regressions, yay! LOL Ciao
          Hide
          Marina Glancy added a comment -

          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.

          Show
          Marina Glancy added a comment - 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.
          Hide
          Marina Glancy added a comment -

          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)?

          Show
          Marina Glancy added a comment - 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)?

            People

            • Votes:
              0 Vote for this issue
              Watchers:
              5 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: