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

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

    Details

    • Type: Bug
    • Status: Closed
    • Priority: 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

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

            suggested patch for the issue

            Show
            ankit_frenz Ankit Agarwal added a comment - suggested patch for the issue
            ankit_frenz Ankit Agarwal made changes -
            Attachment 0001-patch-for-MDL-26890.patch [ 23366 ]
            Hide
            dongsheng 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 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_frenz 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_frenz 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_frenz 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_frenz 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_frenz Ankit Agarwal made changes -
            Attachment patch for MDL--26890.patch [ 23422 ]
            Hide
            dongsheng 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 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 Dongsheng Cai made changes -
            Labels triaged
            Fix Version/s STABLE backlog [ 10463 ]
            Priority Major [ 3 ] Critical [ 2 ]
            tsala Helen Foster made changes -
            Labels triaged patch triaged
            dougiamas Martin Dougiamas made changes -
            Workflow MDL Workflow [ 68740 ] MDL Full Workflow [ 76165 ]
            moodle.com moodle.com made changes -
            Fix Version/s STABLE Sprint 14 [ 11050 ]
            Fix Version/s STABLE backlog [ 10463 ]
            moodle.com moodle.com made changes -
            Assignee Dongsheng Cai [ dongsheng ] Ankit Agarwal [ ankit_frenz ]
            ankit_frenz Ankit Agarwal made changes -
            Status Open [ 1 ] Development in progress [ 3 ]
            ankit_frenz 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_frenz 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_frenz Ankit Agarwal made changes -
            Status Development in progress [ 3 ] Waiting for peer review [ 10012 ]
            samhemelryk 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
            samhemelryk 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
            samhemelryk 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
            samhemelryk Sam Hemelryk made changes -
            Status Peer review in progress [ 10013 ] Development in progress [ 3 ]
            Hide
            ankit_frenz Ankit Agarwal added a comment -

            Thanks Sam for the review

            Show
            ankit_frenz Ankit Agarwal added a comment - Thanks Sam for the review
            ankit_frenz Ankit Agarwal made changes -
            Status Development in progress [ 3 ] Waiting for integration review [ 10010 ]
            Hide
            skodak 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
            skodak 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
            samhemelryk Sam Hemelryk made changes -
            Currently in integration Yes
            nebgor Aparup Banerjee made changes -
            Status Waiting for integration review [ 10010 ] Integration review in progress [ 10004 ]
            Integrator nebgor
            Hide
            nebgor Aparup Banerjee added a comment -

            just linking Petr's issue.

            Show
            nebgor Aparup Banerjee added a comment - just linking Petr's issue.
            nebgor Aparup Banerjee made changes -
            Link This issue has a non-specific relationship to MDL-29401 [ MDL-29401 ]
            Hide
            nebgor 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
            nebgor 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
            nebgor Aparup Banerjee added a comment -

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

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

            Test result: passed. Well done!

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

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

            Ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - And this code has been spread to all Moodle git and cvs repositories. Many thanks! Closing. Ciao
            stronk7 Eloy Lafuente (stronk7) made changes -
            Status Tested [ 10006 ] Closed [ 6 ]
            Resolution Fixed [ 1 ]
            Currently in integration Yes
            Integration date 21/Sep/10
            stronk7 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:
                  Fix Release Date:
                  10/Oct/11