Moodle
  1. Moodle
  2. MDL-35376

Shortcut/alias not working for Private file repo

    Details

    • Testing Instructions:
      Hide
      1. Login as teacher
      2. Go to "Manage private files"
      3. Upload text file called test.txt and click save changes
      4. Go to a course and add a file resource using the uploaded text file as a shortcut/alias
      5. Login as student (in another browser for convenience) and try to download/view file from course
      6. Make sure this is a correct file
      7. Work again as teacher
      8. Edit text file and then go to "Manage private files" again
      9. Upload edited text file and choose "Overwrite" when the prompt appears after file upload
      10. Then click save changes
      11. Verify error is not thrown
      12. As a student try to download/view file from course
      13. Make sure this is a correct (new) file
      Show
      Login as teacher Go to "Manage private files" Upload text file called test.txt and click save changes Go to a course and add a file resource using the uploaded text file as a shortcut/alias Login as student (in another browser for convenience) and try to download/view file from course Make sure this is a correct file Work again as teacher Edit text file and then go to "Manage private files" again Upload edited text file and choose "Overwrite" when the prompt appears after file upload Then click save changes Verify error is not thrown As a student try to download/view file from course Make sure this is a correct (new) file
    • Affected Branches:
      MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE
    • Pull from Repository:
    • Pull Master Branch:
    • Rank:
      44052

      Description

      Steps to reproduce problem:

      1. Go to QA site: http://qa.moodle.net/
      2. Login as teacher
      3. Go to "Manage private files"
      4. Upload text file called test.txt and click save changes
      5. Go to a course and add a file resource using the uploaded text file as a shortcut/alias
      6. Edit text file and then go to "Manage private files" again
      7. Upload edited text file and choose "Overwrite" when the prompt appears after file upload
      8. Then click save changes
      9. Following error message appears:
      Coding error detected, it must be fixed by a programmer: PHP catchable fatal error
      
      Debug info: Argument 1 passed to file_storage::update_references_to_storedfile() must be an instance of stored_file, null given, called in /html/lib/filelib.php on line 835 and defined
      Error code: codingerror
      Stack trace:
      line 397 of /lib/setuplib.php: coding_exception thrown
      line 1823 of /lib/filestorage/file_storage.php: call to default_error_handler()
      line 835 of /lib/filelib.php: call to file_storage->update_references_to_storedfile()
      line 291 of /lib/filelib.php: call to file_save_draft_area_files()
      line 64 of /user/files.php: call to file_postupdate_standard_filemanager()
      Output buffer: <br /> <b>Notice</b>: Undefined variable: this in <b>/html/lib/filelib.php</b> on line <b>835</b><br />
      
      1. Also, another error message that appears is that in the function update_references_to_storedfile in lib/filestorage/file_storage is missing the reference to a global $DB object.

        Issue Links

          Activity

          Hide
          Rex Lorenzo added a comment -

          Using the latest Moodle 2.3.2 version I am trying to get the shortcut/alias feature to work for private files and it isn't working for me.

          For the bug messages that I posted I tried to fix them by changing:

          $fs->update_references_to_storedfile($this);
          

          To:

          $fs->update_references_to_storedfile($newfile);
          

          Then that resulted in another error message about update_references_to_storedfile not having a reference to a global $DB object. I added that to the function and now I get no more errors.

          However, a weird thing is that when I shortcut/alias the text file as an embedded resource for a course, my file changes weren't showing up when I displayed the resource. However, when I switched the file resource to "force download", I got the latest copy of the file. Then I switched it back to embedded and now I see the latest version. Maybe it was a caching issue?

          Show
          Rex Lorenzo added a comment - Using the latest Moodle 2.3.2 version I am trying to get the shortcut/alias feature to work for private files and it isn't working for me. For the bug messages that I posted I tried to fix them by changing: $fs->update_references_to_storedfile($ this ); To: $fs->update_references_to_storedfile($newfile); Then that resulted in another error message about update_references_to_storedfile not having a reference to a global $DB object. I added that to the function and now I get no more errors. However, a weird thing is that when I shortcut/alias the text file as an embedded resource for a course, my file changes weren't showing up when I displayed the resource. However, when I switched the file resource to "force download", I got the latest copy of the file. Then I switched it back to embedded and now I see the latest version. Maybe it was a caching issue?
          Hide
          Rex Lorenzo added a comment -

          I changed

          $fs->update_references_to_storedfile($this);
          

          to

          $fs->update_references_to_storedfile($oldfile);
          

          Since it seems to be more correct.

          Show
          Rex Lorenzo added a comment - I changed $fs->update_references_to_storedfile($ this ); to $fs->update_references_to_storedfile($oldfile); Since it seems to be more correct.
          Hide
          Marina Glancy added a comment -

          looks good for me, thanks for fixing this

          Show
          Marina Glancy added a comment - looks good for me, thanks for fixing this
          Hide
          kiswap added a comment -

          $newfile or $oldfile is better? what's the difference in results?

          Show
          kiswap added a comment - $newfile or $oldfile is better? what's the difference in results?
          Hide
          Rex Lorenzo added a comment -

          I thought $oldfile was better because in the code $oldfile is being updated with the updated values in $newfile. So it seems like $oldfile is the storedfile that is already on the system that needs to be updated.

          Also, in our further testing for shortcuts/aliasing Dropbox doesn't seem to be updating its file references even with this patch. Going to open another bug ticket.

          Show
          Rex Lorenzo added a comment - I thought $oldfile was better because in the code $oldfile is being updated with the updated values in $newfile. So it seems like $oldfile is the storedfile that is already on the system that needs to be updated. Also, in our further testing for shortcuts/aliasing Dropbox doesn't seem to be updating its file references even with this patch. Going to open another bug ticket.
          Hide
          Rex Lorenzo added a comment -

          Okay, so Dropbox does work, but the lifetime before it syncs is set to be 1 day! Is there a config option to change how often files are sync'd? Why so long?

          Show
          Rex Lorenzo added a comment - Okay, so Dropbox does work, but the lifetime before it syncs is set to be 1 day! Is there a config option to change how often files are sync'd? Why so long?
          Hide
          Rex Lorenzo added a comment -

          More findings. For the Dropbox repository there is a new config setting called "Cache limit" and it is set to 1 MiB by default. It seems that if a file is less than this cache limit, then the file is just served from the local cache, rather than getting an updated copy from Dropbox. Is that correct? The online documentation doesn't mention anything about this behavior, but my testing does seem to reflect that.

          For example, if I have a text file over 1 MiB, then the changes are displayed instantly. But for a small one line text file the changes don't appear unless I invalidate the lastsync timestamp.

          I would be happy to update the Moodle documentation, but I have no clue what is suppose to happen.

          Show
          Rex Lorenzo added a comment - More findings. For the Dropbox repository there is a new config setting called "Cache limit" and it is set to 1 MiB by default. It seems that if a file is less than this cache limit, then the file is just served from the local cache, rather than getting an updated copy from Dropbox. Is that correct? The online documentation doesn't mention anything about this behavior, but my testing does seem to reflect that. For example, if I have a text file over 1 MiB, then the changes are displayed instantly. But for a small one line text file the changes don't appear unless I invalidate the lastsync timestamp. I would be happy to update the Moodle documentation, but I have no clue what is suppose to happen.
          Hide
          Marina Glancy added a comment -

          Hi,
          $oldfile or $newfile makes no difference. I'd prefer $oldfile for the very odd case where the function update_references_to_storedfile gets changed over the time and analyzes the file id.

          Rex,
          you are completely right about Dropbox. I will try to find time to update docs. It was code of another person, I just had to re-write many things but I did not change the constant. Feel free to create an issue to convert it to the setting.
          At the moment you can change the setting (Site Administration->Plugins->Repositories->Dropbox->Cache limit) to 1 byte and files will not be cached at all.

          Show
          Marina Glancy added a comment - Hi, $oldfile or $newfile makes no difference. I'd prefer $oldfile for the very odd case where the function update_references_to_storedfile gets changed over the time and analyzes the file id. Rex, you are completely right about Dropbox. I will try to find time to update docs. It was code of another person, I just had to re-write many things but I did not change the constant. Feel free to create an issue to convert it to the setting. At the moment you can change the setting (Site Administration->Plugins->Repositories->Dropbox->Cache limit) to 1 byte and files will not be cached at all.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

          TIA and ciao

          Show
          Eloy Lafuente (stronk7) added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
          Hide
          Dan Poltawski added a comment -

          I've copied the replication instructions to testing instructions. Please modify them if that doesn't make sense.

          Show
          Dan Poltawski added a comment - I've copied the replication instructions to testing instructions. Please modify them if that doesn't make sense.
          Hide
          Dan Poltawski added a comment -

          Integrated to master and 2.3, thanks.

          Show
          Dan Poltawski added a comment - Integrated to master and 2.3, thanks.
          Hide
          Rossiani Wijaya added a comment -

          This is working great.

          Thank for fixing.

          Test passed.

          Show
          Rossiani Wijaya added a comment - This is working great. Thank for fixing. Test passed.
          Hide
          Dan Poltawski added a comment -

          Congratulations, you've done it!

          Thanks, this change is now in the latest weekly release!

          Join the crowds of people tomorrow from 8am and download this Moodle release from your local apple store!

          Show
          Dan Poltawski added a comment - Congratulations, you've done it! Thanks, this change is now in the latest weekly release! Join the crowds of people tomorrow from 8am and download this Moodle release from your local apple store!
          Hide
          Ashley Holman added a comment -

          Shouldn't it pass $newfile instead of $oldfile? The update_references_to_storedfile is supposed to "Updates all files that are referencing this file with the new contenthash and filesize". It calls:

          $this->update_references($record->id, $now, $lifetime, $storedfile->get_contenthash(), $storedfile->get_filesize(), 0);

          So shouldn't it use the new size and content hash, otherwise the update isn't changing anything?

          Show
          Ashley Holman added a comment - Shouldn't it pass $newfile instead of $oldfile? The update_references_to_storedfile is supposed to "Updates all files that are referencing this file with the new contenthash and filesize". It calls: $this->update_references($record->id, $now, $lifetime, $storedfile->get_contenthash(), $storedfile->get_filesize(), 0); So shouldn't it use the new size and content hash, otherwise the update isn't changing anything?
          Hide
          Marina Glancy added a comment -

          Ashley, all fields in $oldfile are already updated by that time. See my comment above

          Show
          Marina Glancy added a comment - Ashley, all fields in $oldfile are already updated by that time. See my comment above
          Hide
          Mary Cooch added a comment -

          I've removed the docs_required label as the ability to make a shortcut was already documented -but if I have got the aspect of documentation wrong please add it back and I'll take another look.

          Show
          Mary Cooch added a comment - I've removed the docs_required label as the ability to make a shortcut was already documented -but if I have got the aspect of documentation wrong please add it back and I'll take another look.

            People

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

              Dates

              • Created:
                Updated:
                Resolved: