Moodle
  1. Moodle
  2. MDL-36708

file_is_draft_area_limit_reached should not consider references

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.4
    • Fix Version/s: 2.4.2
    • Component/s: Files API
    • Labels:
      None
    • Testing Instructions:
      Hide
      1. Login as a student.
      2. Go to your private files.
      3. Add some file references (alias/shortcut) which altogether weigh about half of the overall limit of the area.
      4. Add some files to reach the overall limit.
      5. Make sure you can upload files even if the files and references sizes combined exceed the limit.
      6. Make sure you can't upload more files once the files alone exceed the overall limit.
      Show
      Login as a student. Go to your private files. Add some file references (alias/shortcut) which altogether weigh about half of the overall limit of the area. Add some files to reach the overall limit. Make sure you can upload files even if the files and references sizes combined exceed the limit. Make sure you can't upload more files once the files alone exceed the overall limit.
    • Affected Branches:
      MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE
    • Pull from Repository:
    • Pull 2.4 Branch:
    • Pull Master Branch:
      MDL-36708-master
    • Rank:
      46211

      Description

      lib/filelib.php:file_is_draft_area_limit_reached() should not consider the file references while checking the size of an area. This means that we have to update file file_get_draft_area_info() which itself could be improved a lot.

      Marina's comment on MDL-32639:

      There could be a performance improvement, actually not related to the issue itself. While doing review I noticed that function file_get_draft_area_info() is terribly inefficient. Basically all we want is to check if there are any files/subfolders in the area (or path). But instead of doing one simple count() query we retrieve all files and folders and create an instance of stored_file for each of them. There is a function file_storage::is_area_empty() but it does not really do what we need.
      Also file_get_draft_area_info() gets the total filesize regardless of whether file is reference or not. I would make another attribute in return value to show filesize of not-reference files.

      This issue can potentially create a regression introduced by MDL-33766, which would mean that the Private Files userquota could be reached where as they used references.

        Issue Links

          Activity

          Hide
          Frédéric Massart added a comment -

          I have backported the patch as file references should not have been considered in the first place, also file_is_draft_area_limit_reached() has been introduced really recently and is not likely to be used by any 3rd party (or core) functions.

          Show
          Frédéric Massart added a comment - I have backported the patch as file references should not have been considered in the first place, also file_is_draft_area_limit_reached() has been introduced really recently and is not likely to be used by any 3rd party (or core) functions.
          Hide
          Rajesh Taneja added a comment -

          Patch looks Good Fred, few thing you might want to consider:

          1. filesize can be probably be renamed as it is not one file size but size of all the files in area.
          2. I think you should consider if newfilesize is a size of reference or file https://github.com/FMCorz/moodle/compare/moodle:master...MDL-36708-master#L0R515. (Looking at repository/filepicker.php - line 327, it doesnot differentiate between reference/local file.)

          [y] Syntax
          [-] Output
          [y] Whitespace
          [-] Language
          [-] Databases
          [y] Testing
          [y] Security
          [y] Documentation
          [y] Git
          [y] Sanity check

          Show
          Rajesh Taneja added a comment - Patch looks Good Fred, few thing you might want to consider: filesize can be probably be renamed as it is not one file size but size of all the files in area. I think you should consider if newfilesize is a size of reference or file https://github.com/FMCorz/moodle/compare/moodle:master...MDL-36708-master#L0R515 . (Looking at repository/filepicker.php - line 327, it doesnot differentiate between reference/local file.) [y] Syntax [-] Output [y] Whitespace [-] Language [-] Databases [y] Testing [y] Security [y] Documentation [y] Git [y] Sanity check
          Hide
          Frédéric Massart added a comment -

          Thanks for your review Raj,

          • I could not update the name of 'filesize' as it was already present and used. I wish I could though
          • You made a good point about making sure that the filesize passed to file_is_draft_area_limit_reached() is not a reference, but I personally think that this should be handled by the developer on case by case basis. Also I checked the filepicker.php implementation and it is not using references at all, so it is safe to leave it as is.

          Cheers, I am pushing this issue for integration.

          Show
          Frédéric Massart added a comment - Thanks for your review Raj, I could not update the name of 'filesize' as it was already present and used. I wish I could though You made a good point about making sure that the filesize passed to file_is_draft_area_limit_reached() is not a reference, but I personally think that this should be handled by the developer on case by case basis. Also I checked the filepicker.php implementation and it is not using references at all, so it is safe to leave it as is. Cheers, I am pushing this issue for integration.
          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 -

          Integrated to 24 and master, thanks Fred.

          Show
          Dan Poltawski added a comment - Integrated to 24 and master, thanks Fred.
          Hide
          Dan Poltawski added a comment -

          Uncaught Error: Invalid JSON string:
          <br />
          <b>Notice</b>: Undefined index: filesize_without_references in <b>/Users/danp/git/integration/lib/filelib.php</b> on line <b>502</b><br />
          {"path":[

          {"name":"Files","path":"\/","icon":"https:\/\/dan.moodle.local\/integration\/theme\/image.php\/standard\/core\/1358306386\/f\/folder-24"}

          ],"itemid":259094472,"list":[

          {"filename":"Screen Shot 2012-12-26 at 23.16.18.png","filepath":"\/","fullname":"Screen Shot 2012-12-26 at 23.16.18.png","size":452762,"filesize":"442.2KB","sortorder":"0","author":"Admin User","license":"allrightsreserved","datemodified":1358306691,"datecreated":1358306691,"isref":false,"mimetype":"Image (PNG)","type":"file","url":"https:\/\/dan.moodle.local\/integration\/draftfile.php\/5\/user\/draft\/259094472\/Screen%20Shot%202012-12-26%20at%2023.16.18.png","icon":"https:\/\/dan.moodle.local\/integration\/theme\/image.php\/standard\/core\/1358306386\/f\/png-24","thumbnail":"https:\/\/dan.moodle.local\/integration\/theme\/image.php\/standard\/core\/1358306386\/f\/png-80","realthumbnail":"https:\/\/dan.moodle.local\/integration\/draftfile.php\/5\/user\/draft\/259094472\/Screen%20Shot%202012-12-26%20at%2023.16.18.png?preview=thumb&oid=1358306691","realicon":"https:\/\/dan.moodle.local\/integration\/draftfile.php\/5\/user\/draft\/259094472\/Screen%20Shot%202012-12-26%20at%2023.16.18.png?preview=tinyicon&oid=1358306691","image_width":1377,"image_height":876,"size_f":"442.2KB","license_f":"All rights reserved","dimensions":"1377 x 876 px","datemodified_f":"16 January 2013, 11:24 am","datemodified_f_s":"16\/01\/13, 11:24","datecreated_f":"16 January 2013, 11:24 am","datecreated_f_s":"16\/01\/13, 11:24"}

          ],"filecount":1,"filesize":452762,"tree":{"children":[]}}

          Show
          Dan Poltawski added a comment - Uncaught Error: Invalid JSON string: <br /> <b>Notice</b>: Undefined index: filesize_without_references in <b>/Users/danp/git/integration/lib/filelib.php</b> on line <b>502</b><br /> {"path":[ {"name":"Files","path":"\/","icon":"https:\/\/dan.moodle.local\/integration\/theme\/image.php\/standard\/core\/1358306386\/f\/folder-24"} ],"itemid":259094472,"list":[ {"filename":"Screen Shot 2012-12-26 at 23.16.18.png","filepath":"\/","fullname":"Screen Shot 2012-12-26 at 23.16.18.png","size":452762,"filesize":"442.2KB","sortorder":"0","author":"Admin User","license":"allrightsreserved","datemodified":1358306691,"datecreated":1358306691,"isref":false,"mimetype":"Image (PNG)","type":"file","url":"https:\/\/dan.moodle.local\/integration\/draftfile.php\/5\/user\/draft\/259094472\/Screen%20Shot%202012-12-26%20at%2023.16.18.png","icon":"https:\/\/dan.moodle.local\/integration\/theme\/image.php\/standard\/core\/1358306386\/f\/png-24","thumbnail":"https:\/\/dan.moodle.local\/integration\/theme\/image.php\/standard\/core\/1358306386\/f\/png-80","realthumbnail":"https:\/\/dan.moodle.local\/integration\/draftfile.php\/5\/user\/draft\/259094472\/Screen%20Shot%202012-12-26%20at%2023.16.18.png?preview=thumb&oid=1358306691","realicon":"https:\/\/dan.moodle.local\/integration\/draftfile.php\/5\/user\/draft\/259094472\/Screen%20Shot%202012-12-26%20at%2023.16.18.png?preview=tinyicon&oid=1358306691","image_width":1377,"image_height":876,"size_f":"442.2KB","license_f":"All rights reserved","dimensions":"1377 x 876 px","datemodified_f":"16 January 2013, 11:24 am","datemodified_f_s":"16\/01\/13, 11:24","datecreated_f":"16 January 2013, 11:24 am","datecreated_f_s":"16\/01\/13, 11:24"} ],"filecount":1,"filesize":452762,"tree":{"children":[]}}
          Hide
          Frédéric Massart added a comment -

          Just pushed a commit to fix master. Thanks!

          Show
          Frédéric Massart added a comment - Just pushed a commit to fix master. Thanks!
          Hide
          Dan Poltawski added a comment -

          Looks good, integrated thanks.

          Show
          Dan Poltawski added a comment - Looks good, integrated thanks.
          Hide
          Rossiani Wijaya added a comment -

          As I mentioned to Fred verbally, reference file can't be save if it exceed the user quota.

          Failing this test because of the above issue.

          Show
          Rossiani Wijaya added a comment - As I mentioned to Fred verbally, reference file can't be save if it exceed the user quota. Failing this test because of the above issue.
          Hide
          Frédéric Massart added a comment -

          Adding Marina a watcher.
          Marina can you confirm that the size of an alias/shortcut should never be considered while checking maxbytes and maxareabytes?
          I was about to fix this when I thought that perhaps a teacher who would limit the upload to 2MB is not expecting a student to add a link to a file that is more than 2MB.

          Show
          Frédéric Massart added a comment - Adding Marina a watcher. Marina can you confirm that the size of an alias/shortcut should never be considered while checking maxbytes and maxareabytes? I was about to fix this when I thought that perhaps a teacher who would limit the upload to 2MB is not expecting a student to add a link to a file that is more than 2MB.
          Hide
          Marina Glancy added a comment -

          I confirm that references can be bigger than area limits. See MDL-33445
          Second, students are not allowed to insert references anyway - at least in forums, workshops and assign submissions

          Show
          Marina Glancy added a comment - I confirm that references can be bigger than area limits. See MDL-33445 Second, students are not allowed to insert references anyway - at least in forums, workshops and assign submissions
          Hide
          Frédéric Massart added a comment -

          Cool, thanks. Working on a fix right now!

          Show
          Frédéric Massart added a comment - Cool, thanks. Working on a fix right now!
          Hide
          Frédéric Massart added a comment -

          Ok, well there are two issues here, and none of them are related to this patch.

          • The Private Files do not take in account the limitation imposed by $CFG->maxbytes, it seems like it does as the ajax call returns an error when a hard copy has a size exceeding the maxbytes, but if for some reason we skip this validation and add the file to the draft area, it would then be validated.
          • Saving draft files to an area file_save_draft_area_files() does not ignore the size of references.

          I have raised MDL-37560 to fix those.

          Show
          Frédéric Massart added a comment - Ok, well there are two issues here, and none of them are related to this patch. The Private Files do not take in account the limitation imposed by $CFG->maxbytes, it seems like it does as the ajax call returns an error when a hard copy has a size exceeding the maxbytes, but if for some reason we skip this validation and add the file to the draft area, it would then be validated. Saving draft files to an area file_save_draft_area_files() does not ignore the size of references. I have raised MDL-37560 to fix those.
          Hide
          Dan Poltawski added a comment -

          Should this be sent back to testing?

          Show
          Dan Poltawski added a comment - Should this be sent back to testing?
          Hide
          Frédéric Massart added a comment -

          If both master and 2.4 have been tested, then we can pass it. If not, then re-test and ignore the above issue.

          Show
          Frédéric Massart added a comment - If both master and 2.4 have been tested, then we can pass it. If not, then re-test and ignore the above issue.
          Hide
          Rossiani Wijaya added a comment -

          Apart from the bug that reported for MDL-37560, this is working as expected in 2.4 and master.

          Integrators please passed this test. Thanks

          Test passed.

          Show
          Rossiani Wijaya added a comment - Apart from the bug that reported for MDL-37560, this is working as expected in 2.4 and master. Integrators please passed this test. Thanks Test passed.
          Hide
          Dan Poltawski added a comment -

          Hurray! We did it! Thanks to all the reporters, testers, user and watchers for a bumper week of Moodling!

          Show
          Dan Poltawski added a comment - Hurray! We did it! Thanks to all the reporters, testers, user and watchers for a bumper week of Moodling!

            People

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

              Dates

              • Created:
                Updated:
                Resolved: