Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-36708

file_is_draft_area_limit_reached should not consider references

    Details

    • Type: Bug
    • Status: Closed
    • Priority: 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 Master Branch:
      MDL-36708-master

      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.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            fred 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
            fred 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
            rajeshtaneja 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
            rajeshtaneja 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
            fred 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
            fred 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
            stronk7 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
            stronk7 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
            poltawski Dan Poltawski added a comment -

            Integrated to 24 and master, thanks Fred.

            Show
            poltawski Dan Poltawski added a comment - Integrated to 24 and master, thanks Fred.
            Hide
            poltawski 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
            poltawski 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
            fred Frédéric Massart added a comment -

            Just pushed a commit to fix master. Thanks!

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

            Looks good, integrated thanks.

            Show
            poltawski Dan Poltawski added a comment - Looks good, integrated thanks.
            Hide
            rwijaya 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
            rwijaya 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
            fred 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
            fred 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 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 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
            fred Frédéric Massart added a comment -

            Cool, thanks. Working on a fix right now!

            Show
            fred Frédéric Massart added a comment - Cool, thanks. Working on a fix right now!
            Hide
            fred 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
            fred 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
            poltawski Dan Poltawski added a comment -

            Should this be sent back to testing?

            Show
            poltawski Dan Poltawski added a comment - Should this be sent back to testing?
            Hide
            fred 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
            fred 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
            rwijaya 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
            rwijaya 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
            poltawski 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
            poltawski 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:
                  Fix Release Date:
                  11/Mar/13