Moodle
  1. Moodle
  2. MDL-26890

Forum file size limit is not used if a file is added from privat area.

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: 2.0.4, 2.1.1, 2.2
    • Fix Version/s: 2.0.5, 2.1.2
    • Component/s: Files API, Forum
    • Labels:
    • Testing Instructions:
      Hide

      1) Login as admin>goto my profile> private files
      2) Upload atleast two files, one less than 100 kb in size and another more than 100kb in size.
      3) Goto any forum click on edit settings under forum administration
      4) Change max attachment size to 100 kb and save.
      5) try to create a new topic and/or reply to existing topic
      6) try to attach files to the post from your private area
      7) You should be able to attach the file that has a size <100kb easily and should get an error for the other one.
      8) In the later case make sure the file is not present in the post after you close the file picker and make the post.

      Show
      1) Login as admin>goto my profile> private files 2) Upload atleast two files, one less than 100 kb in size and another more than 100kb in size. 3) Goto any forum click on edit settings under forum administration 4) Change max attachment size to 100 kb and save. 5) try to create a new topic and/or reply to existing topic 6) try to attach files to the post from your private area 7) You should be able to attach the file that has a size <100kb easily and should get an error for the other one. 8) In the later case make sure the file is not present in the post after you close the file picker and make the post.
    • Affected Branches:
      MOODLE_20_STABLE, MOODLE_21_STABLE, MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_20_STABLE, MOODLE_21_STABLE
    • Pull Master Branch:
      MDL-26890-master

      Description

      moodle .org Using Moodle course forum file upload is limited to 100 KB.So I uploaded a file (140 KB) to my private folder, and added this file from private folder as forum posting attachment. File picker showed to me that the file is to large for the forum and didn't show the file as attachment in edit mode of forum posting. After sending the post to the forum the file was shown as attachment in the forum.

      We've two options to handle this:

      • accept that bigger files can be used by uploading from private area, In this case the error message should be deleted..
      • but normally a teacher will define the max size for file . Then the attachment shouldn't be added to the posting.

        Gliffy Diagrams

        1. 0001-patch-for-MDL-26890.patch
          3 kB
          Ankit Agarwal
        2. patch for MDL--26890.patch
          3 kB
          Ankit Agarwal

          Issue Links

            Activity

            Ralf Hilgenstock created issue -
            Petr Skoda made changes -
            Field Original Value New Value
            Assignee Petr Škoda (skodak) [ skodak ] Dongsheng Cai [ dongsheng ]
            Hide
            Ankit Agarwal added a comment -

            suggested patch for the issue

            Show
            Ankit Agarwal added a comment - suggested patch for the issue
            Ankit Agarwal made changes -
            Attachment 0001-patch-for-MDL-26890.patch [ 23366 ]
            Hide
            Dongsheng Cai added a comment -

            Hi, Ankit

            Thanks for the patch, problems in your patch:
            1. code indent problem, we use four spaces (not tab) in moodle code, and use unix file format all the time (see more: http://docs.moodle.org/en/Development:Coding_style)

            2. The patch is not a appropriate fix, it is basically copying the code from local/lib.php, the problem is actaully copy_to_area is called before filesize limit check.

            Show
            Dongsheng Cai added a comment - Hi, Ankit Thanks for the patch, problems in your patch: 1. code indent problem, we use four spaces (not tab) in moodle code, and use unix file format all the time (see more: http://docs.moodle.org/en/Development:Coding_style ) 2. The patch is not a appropriate fix, it is basically copying the code from local/lib.php, the problem is actaully copy_to_area is called before filesize limit check.
            Hide
            Ankit Agarwal added a comment -

            Hi Dongsheng,

            Thanks for the comments.

            I figured out that we need to check the filesize before we make a call to copy_to_area function.But I didnt get any pre-defined function of directly checking the filesize of the local files.Did i missed something in the docs?

            On the other note..i am trying to get familiar with the moodle coding style..will keep your suggestion in mind.

            Thanks

            Show
            Ankit Agarwal added a comment - Hi Dongsheng, Thanks for the comments. I figured out that we need to check the filesize before we make a call to copy_to_area function.But I didnt get any pre-defined function of directly checking the filesize of the local files.Did i missed something in the docs? On the other note..i am trying to get familiar with the moodle coding style..will keep your suggestion in mind. Thanks
            Hide
            Ankit Agarwal added a comment -

            As suggested by Dongsheng ,i have used a method instead of hard coding the process.
            attached patch for the same.
            Thanks

            Show
            Ankit Agarwal added a comment - As suggested by Dongsheng ,i have used a method instead of hard coding the process. attached patch for the same. Thanks
            Ankit Agarwal made changes -
            Attachment patch for MDL--26890.patch [ 23422 ]
            Hide
            Dongsheng Cai added a comment -

            Thanks for the patch, it looks good to me, raised the priority of this issue, added to stable backlog.

            Show
            Dongsheng Cai added a comment - Thanks for the patch, it looks good to me, raised the priority of this issue, added to stable backlog.
            Dongsheng Cai made changes -
            Labels triaged
            Fix Version/s STABLE backlog [ 10463 ]
            Priority Major [ 3 ] Critical [ 2 ]
            Helen Foster made changes -
            Labels triaged patch triaged
            Martin Dougiamas made changes -
            Workflow MDL Workflow [ 68740 ] MDL Full Workflow [ 76165 ]
            moodle.com made changes -
            Fix Version/s STABLE Sprint 14 [ 11050 ]
            Fix Version/s STABLE backlog [ 10463 ]
            moodle.com made changes -
            Assignee Dongsheng Cai [ dongsheng ] Ankit Agarwal [ ankit_frenz ]
            Ankit Agarwal made changes -
            Status Open [ 1 ] Development in progress [ 3 ]
            Ankit Agarwal made changes -
            Affects Version/s 2.1.1 [ 10750 ]
            Affects Version/s 2.0.4 [ 10652 ]
            Affects Version/s 2.0.2 [ 10421 ]
            Ankit Agarwal made changes -
            Pull Master Diff URL https://github.com/ankitagarwal/moodle/compare/master...MDL-26890-master
            Pull Master Branch MDL-26890-master
            Testing Instructions 1) Login as admin>goto my profile> private files
            2) Upload atleast two files, one less than 100 kb in size and another more than 100kb in size.
            3) Goto any forum click on edit settings under forum administration
            4) Change max attachment size to 100 kb and save.
            5) try to create a new topic and/or reply to existing topic
            6) try to attach files to the post from your private area
            7) You should be able to attach the file that has a size <100kb easily and should get an error for the other one.
            8) In the later case make sure the file is not present in the post after you close the file picker and make the post.
            Pull from Repository git://github.com/ankitagarwal/moodle.git
            Ankit Agarwal made changes -
            Status Development in progress [ 3 ] Waiting for peer review [ 10012 ]
            Sam Hemelryk made changes -
            Original Estimate 0 minutes [ 0 ]
            Remaining Estimate 0 minutes [ 0 ]
            Status Waiting for peer review [ 10012 ] Peer review in progress [ 10013 ]
            Peer reviewer samhemelryk
            Hide
            Sam Hemelryk added a comment -

            Hi Ankit looks good to me.
            Only thing to do before submitting for integration is clean up the indenting of `return $filesize` lib.php line 1381.
            Other than that spot on.

            Cheers
            Sam

            Show
            Sam Hemelryk added a comment - Hi Ankit looks good to me. Only thing to do before submitting for integration is clean up the indenting of `return $filesize` lib.php line 1381. Other than that spot on. Cheers Sam
            Sam Hemelryk made changes -
            Status Peer review in progress [ 10013 ] Development in progress [ 3 ]
            Hide
            Ankit Agarwal added a comment -

            Thanks Sam for the review

            Show
            Ankit Agarwal added a comment - Thanks Sam for the review
            Ankit Agarwal made changes -
            Status Development in progress [ 3 ] Waiting for integration review [ 10010 ]
            Hide
            Petr Skoda added a comment -

            oh!
            $filearea = clean_param($params['filearea'], PARAM_ALPHAEXT);
            $component = clean_param($params['component'], PARAM_ALPHAEXT);

            is very wrong, it is already in repository/lib.php:
            1/ unfortuantely we do not have PARAM_FRANKENSTYLE yet for component
            2/ PARAM_SAFEDIR might be best for the filearea

            I have create a new issue for that MDL-29401

            Show
            Petr Skoda added a comment - oh! $filearea = clean_param($params ['filearea'] , PARAM_ALPHAEXT); $component = clean_param($params ['component'] , PARAM_ALPHAEXT); is very wrong, it is already in repository/lib.php: 1/ unfortuantely we do not have PARAM_FRANKENSTYLE yet for component 2/ PARAM_SAFEDIR might be best for the filearea I have create a new issue for that MDL-29401
            Sam Hemelryk made changes -
            Currently in integration Yes
            Aparup Banerjee made changes -
            Status Waiting for integration review [ 10010 ] Integration review in progress [ 10004 ]
            Integrator nebgor
            Hide
            Aparup Banerjee added a comment -

            just linking Petr's issue.

            Show
            Aparup Banerjee added a comment - just linking Petr's issue.
            Aparup Banerjee made changes -
            Link This issue has a non-specific relationship to MDL-29401 [ MDL-29401 ]
            Hide
            Aparup Banerjee added a comment - - edited

            Hi Ankit,
            I've just had a look at your patch and I have some notes:

            • there seems to be a consistent space after calls to the empty function, perhaps this could be your editor setting ?
            • as Petr said, PARAM_SAFEDIR does seem appropriate for filearea. (checkout clean_param() function)

            ps: you may want to watch MDL-29401 in case he's already got a patch there to validate component names.
            cheers,
            Aparup

            Show
            Aparup Banerjee added a comment - - edited Hi Ankit, I've just had a look at your patch and I have some notes: there seems to be a consistent space after calls to the empty function, perhaps this could be your editor setting ? as Petr said, PARAM_SAFEDIR does seem appropriate for filearea. (checkout clean_param() function) ps: you may want to watch MDL-29401 in case he's already got a patch there to validate component names. cheers, Aparup
            Hide
            Aparup Banerjee added a comment -

            Thanks for the fixes , they've now been integrated.

            Show
            Aparup Banerjee added a comment - Thanks for the fixes , they've now been integrated.
            Aparup Banerjee made changes -
            Status Integration review in progress [ 10004 ] Waiting for testing [ 10005 ]
            Affects Version/s 2.2 [ 10656 ]
            Michael de Raadt made changes -
            Tester salvetore
            Michael de Raadt made changes -
            Status Waiting for testing [ 10005 ] Testing in progress [ 10011 ]
            Hide
            Michael de Raadt added a comment -

            Test result: passed. Well done!

            Show
            Michael de Raadt added a comment - Test result: passed. Well done!
            Michael de Raadt made changes -
            Status Testing in progress [ 10011 ] Tested [ 10006 ]
            Eloy Lafuente (stronk7) made changes -
            Fix Version/s 2.0.5 [ 10950 ]
            Fix Version/s 2.1.2 [ 10851 ]
            Hide
            Eloy Lafuente (stronk7) added a comment -

            And this code has been spread to all Moodle git and cvs repositories. Many thanks! Closing.

            Ciao

            Show
            Eloy Lafuente (stronk7) added a comment - And this code has been spread to all Moodle git and cvs repositories. Many thanks! Closing. Ciao
            Eloy Lafuente (stronk7) made changes -
            Status Tested [ 10006 ] Closed [ 6 ]
            Resolution Fixed [ 1 ]
            Currently in integration Yes
            Integration date 21/Sep/10
            Eloy Lafuente (stronk7) made changes -
            Fix Version/s STABLE Sprint 14 [ 11050 ]

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved: