Moodle
  1. Moodle
  2. MDL-33832

Check files.source field is set even in non-filepicker situations.

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.3
    • Fix Version/s: 2.3
    • Component/s: Files API, Repositories
    • Labels:
    • Testing Instructions:
      Hide

      With Javascript enabled:
      Create a folder resource and insert the following files:

      1.

      • upload a file
      • upload a file with a name conflict

      2.

      • pick file from Server files and insert it as copy
      • pick file from Server files and insert it as reference
      • pick file from Server files and insert it as copy with a name conflict
      • pick file from Server files and insert it as reference with a name conflict

      3. pick file from Private files

      4. pick file from Course legacy files

      5. pick file from Recent files

      6. repeat #2 for file from Dropbox

      7. pick file from Box.net

      8.

      • pick file from Wikimedia
      • pick file from Wikimedia with a name conflict

      you may try other repositories if you have time

      Save folder.

      Select from DB directly all files in this filearea, make sure the field source is filled human-readable in all cases and it is one of:

      • just filename in case of upload
      • "Admin user Private files: filepath" (in case of Private files)
      • "Server files: category/coursename/...." (in case of Server files and course legacy files)
      • something readable in case of Recent files (not that important actually)
      • full URL (for repositories that have public access to files)
      • repository name + path inside repository (for other external repositories)

      With Javascript disabled:
      Create a folder resource and

      • upload a file
      • upload a file with name conflict
      • pick a file from local repository
      • pick a file from local repository with name conflict
      • pick a file from external repository
      • pick a file from external repository with name conflict

      Save the folder
      Check the files.source field as described above

      Show
      With Javascript enabled: Create a folder resource and insert the following files: 1. upload a file upload a file with a name conflict 2. pick file from Server files and insert it as copy pick file from Server files and insert it as reference pick file from Server files and insert it as copy with a name conflict pick file from Server files and insert it as reference with a name conflict 3. pick file from Private files 4. pick file from Course legacy files 5. pick file from Recent files 6. repeat #2 for file from Dropbox 7. pick file from Box.net 8. pick file from Wikimedia pick file from Wikimedia with a name conflict you may try other repositories if you have time Save folder. Select from DB directly all files in this filearea, make sure the field source is filled human-readable in all cases and it is one of: just filename in case of upload "Admin user Private files: filepath" (in case of Private files) "Server files: category/coursename/...." (in case of Server files and course legacy files) something readable in case of Recent files (not that important actually) full URL (for repositories that have public access to files) repository name + path inside repository (for other external repositories) With Javascript disabled: Create a folder resource and upload a file upload a file with name conflict pick a file from local repository pick a file from local repository with name conflict pick a file from external repository pick a file from external repository with name conflict Save the folder Check the files.source field as described above
    • Affected Branches:
      MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE
    • Pull Master Branch:
      wip-MDL-33832-master
    • Rank:
      41932

      Description

      From Marina in MDL-33513:

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

        Issue Links

          Activity

          Hide
          Marina Glancy added a comment -

          now I tested it. This is how the source field is filled in cases of dnd upload, filepicker upload, picking from local repository and dropbox:

          filename source
          dnd.jpg dnd.jpg
          dnd_2.jpg dnd.jpg
          upload.jpg upload.jpg
          upload_2.jpg upload.jpg
          from_local.pdf NULL
          from_local_ref.pdf YTo2OntzOjk6ImNvbnRleHRpZCI7...
          from_local_ref_2.pdf NULL
          from_local_2.pdf NULL
          dropboxjpg.jpg Dropbox:/dropboxjpg.jpg
          dropboxjpg_2.jpg Dropbox:/dropboxjpg.jpg
          dropboxjpg_ref.jpg Dropbox:/dropboxjpg.jpg
          dropboxjpg_ref_2.jpg NULL

          postfix _ref means that I picked file by reference
          postfix _2 means that I had a name conflict and choose to rename a file

          Show
          Marina Glancy added a comment - now I tested it. This is how the source field is filled in cases of dnd upload, filepicker upload, picking from local repository and dropbox: filename source dnd.jpg dnd.jpg dnd_2.jpg dnd.jpg upload.jpg upload.jpg upload_2.jpg upload.jpg from_local.pdf NULL from_local_ref.pdf YTo2OntzOjk6ImNvbnRleHRpZCI7... from_local_ref_2.pdf NULL from_local_2.pdf NULL dropboxjpg.jpg Dropbox:/dropboxjpg.jpg dropboxjpg_2.jpg Dropbox:/dropboxjpg.jpg dropboxjpg_ref.jpg Dropbox:/dropboxjpg.jpg dropboxjpg_ref_2.jpg NULL postfix _ref means that I picked file by reference postfix _2 means that I had a name conflict and choose to rename a file
          Hide
          Marina Glancy added a comment -

          Also in case of picking a file from moodle repository, field source should contain readable name
          see repository::get_reference_details()
          and not packed reference. The value of the source field will be used to show a missed-source-warning during restore. For example, when file references to the contextid that does not exist (any more or on new site), we can show that it used to be "Admin User Private files: bla-bla-bla"

          Show
          Marina Glancy added a comment - Also in case of picking a file from moodle repository, field source should contain readable name see repository::get_reference_details() and not packed reference. The value of the source field will be used to show a missed-source-warning during restore. For example, when file references to the contextid that does not exist (any more or on new site), we can show that it used to be "Admin User Private files: bla-bla-bla"
          Hide
          Dan Poltawski added a comment -

          I've integrated this now.

          Not sure I really like think silencing those unserialize calls is a good solution. But time is of the essence.

          Show
          Dan Poltawski added a comment - I've integrated this now. Not sure I really like think silencing those unserialize calls is a good solution. But time is of the essence.
          Hide
          David Mudrak added a comment -

          I am afraid there is other way how to hide E_NOTICE produced by PHP. See the unserialize() man page:

          In case the passed string is not unserializeable, FALSE is returned and E_NOTICE is issued.

          AFAIK there is no other way how to check if a string is unserializeable. So using the @ operator seems to be appropriate here. Of course, the returned false value must be handled correctly.

          Show
          David Mudrak added a comment - I am afraid there is other way how to hide E_NOTICE produced by PHP. See the unserialize() man page: In case the passed string is not unserializeable, FALSE is returned and E_NOTICE is issued. AFAIK there is no other way how to check if a string is unserializeable. So using the @ operator seems to be appropriate here. Of course, the returned false value must be handled correctly.
          Hide
          Frédéric Massart added a comment -

          All good!

          Show
          Frédéric Massart added a comment - All good!
          Hide
          Eloy Lafuente (stronk7) added a comment -

          And this has been incorporated to all the weekly builds and also, to Moodle 2.3 Release Candidate 1, yay!

          Many, many thanks for your hard work!

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - And this has been incorporated to all the weekly builds and also, to Moodle 2.3 Release Candidate 1, yay! Many, many thanks for your hard work! Ciao

            People

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

              Dates

              • Created:
                Updated:
                Resolved: