Moodle
  1. Moodle
  2. MDL-29835

filemanager UI treats maxfiles=0 as unlimited, inconsistent with back end

    Details

    • Testing Instructions:
      Hide
      • Purge caches (Moodle and browser) to ensure that you're not using any cached JS
      • Change the maxfiles setting of a file manager (such as that in /user/files.php - or /user/filesedit.php in 2.2) to 0
      • Confirm that the file manager does not display the "Add..." button or allow files to be dragged in (dragging in files should result in the maxfilesreached message being displayed)
      • Change the maxfiles setting of the file manager to 1
      • Confirm that the file manager allows one (and only one) file to be chosen / dragged in, and that the chosen file is saved when the form is submitted
      • Change the maxfiles setting of the file manager to 2
      • Confirm that the file manager allows up to two files (but no more) to be chosen / dragged in, and that the chosen files are saved when the form is submitted
      • Change the maxfiles setting of the file manager to -1
      • Confirm that the file manager no longer lists a maximum number of files, that you can choose / drag in several files, and that the chosen file are saved when the form is submitted
      • Set the maxfiles setting of the file manager back to its original value

      NOTE: The above steps which start "Change the maxfiles setting" (and the last step) involve changing the code - no core settings UI (as far as I'm aware) provides all applicable options.

      Show
      Purge caches (Moodle and browser) to ensure that you're not using any cached JS Change the maxfiles setting of a file manager (such as that in /user/files.php - or /user/filesedit.php in 2.2) to 0 Confirm that the file manager does not display the "Add..." button or allow files to be dragged in (dragging in files should result in the maxfilesreached message being displayed) Change the maxfiles setting of the file manager to 1 Confirm that the file manager allows one (and only one) file to be chosen / dragged in, and that the chosen file is saved when the form is submitted Change the maxfiles setting of the file manager to 2 Confirm that the file manager allows up to two files (but no more) to be chosen / dragged in, and that the chosen files are saved when the form is submitted Change the maxfiles setting of the file manager to -1 Confirm that the file manager no longer lists a maximum number of files, that you can choose / drag in several files, and that the chosen file are saved when the form is submitted Set the maxfiles setting of the file manager back to its original value NOTE: The above steps which start "Change the maxfiles setting" (and the last step) involve changing the code - no core settings UI (as far as I'm aware) provides all applicable options.
    • Affected Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE, MOODLE_23_STABLE, MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE
    • Pull from Repository:
    • Pull 2.4 Branch:
    • Pull Master Branch:
      MDL-29835_master
    • Rank:
      19360

      Description

      If you want a filemanager to accept any number of files (i.e. no limit to the number), the maxfiles parameter is treated inconsistently by the UI and the back end. If you set maxfiles to -1, both the UI and back end treat this as "no limit" and let you upload as many files as you like - but if you set it to 0 instead, the (JavaScript) UI allows any number of files to be added but the back end (i.e. file_postupdate_standard_filemanager()) will treat it as literally a limit of 0 files - and therefore won't add any files. It doesn't trigger a warning/error message, though - thereby giving the impression that something is broken.

      Setting maxfiles to 0 can therefore lead to a lot of head-scratching and agony before you realise you should have set it to -1. If the UI properly treated zero as zero, it would prevent uploading of any files (not show the "Add..." button) - which would prevent confusion, as it would immediately become apparent that zero does, in fact, mean zero. It might not be quite so apparent that -1 is the answer, though - perhaps if EDITOR_UNLIMITED_FILES was renamed to something a little more generic, or another constant was added (MANAGER_UNLIMITED_FILES, perhaps?), it would be a little easier for developers to discover.

        Activity

        Hide
        Paul Nicholls added a comment -

        Updated description and added testing instructions. Pull details coming shortly.

        Show
        Paul Nicholls added a comment - Updated description and added testing instructions. Pull details coming shortly.
        Hide
        Paul Nicholls added a comment -

        Pull details added for 2.2 and above

        Show
        Paul Nicholls added a comment - Pull details added for 2.2 and above
        Hide
        Damyon Wiese added a comment -

        Thanks for working on this Paul.

        Note - I don't think this exposes any bug currently in core Moodle because it looks like everywhere that uses a filemanager either does not allow a setting of 0 for maxfiles, or does not show the filemanager when maxfiles is 0 (I did a grep).

        This is still worth fixing though.

        Some feedback on your patch:

        [Warning] Syntax - Your comments all should start with a capital and end with the correct punctuation (.?! etc).
        [-] Output
        [Y] Whitespace
        [-] Language
        [-] Databases
        [Y] Testing - (it could be a little clearer that you mean to change the maxfiles setting in the code)
        [-] Security
        [-] Documentation
        [N] Git - The description in your commit message is really long and I can't read it in a terminal - can you break this into multiple lines?
        [Y] Sanity check

        Thanks again for working on this.

        Show
        Damyon Wiese added a comment - Thanks for working on this Paul. Note - I don't think this exposes any bug currently in core Moodle because it looks like everywhere that uses a filemanager either does not allow a setting of 0 for maxfiles, or does not show the filemanager when maxfiles is 0 (I did a grep). This is still worth fixing though. Some feedback on your patch: [Warning] Syntax - Your comments all should start with a capital and end with the correct punctuation (.?! etc). [-] Output [Y] Whitespace [-] Language [-] Databases [Y] Testing - (it could be a little clearer that you mean to change the maxfiles setting in the code) [-] Security [-] Documentation [N] Git - The description in your commit message is really long and I can't read it in a terminal - can you break this into multiple lines? [Y] Sanity check Thanks again for working on this.
        Hide
        Paul Nicholls added a comment -
        • Note added to testing instructions
        • All branches rebased (and 2.4 branch added)
        • Comments tweaked to start with capitals and end with relevant punctuation
        • Commit message split into multiple lines (no line length was mentioned in the Commit Cheat Sheet (http://docs.moodle.org/dev/Commit_cheat_sheet), so I hope the length I chose is satisfactory)
        Show
        Paul Nicholls added a comment - Note added to testing instructions All branches rebased (and 2.4 branch added) Comments tweaked to start with capitals and end with relevant punctuation Commit message split into multiple lines (no line length was mentioned in the Commit Cheat Sheet ( http://docs.moodle.org/dev/Commit_cheat_sheet ), so I hope the length I chose is satisfactory)
        Hide
        Damyon Wiese added a comment -

        Thanks Paul,

        This patch looks good to me now - sending for integration review.

        Regards, Damyon

        Show
        Damyon Wiese added a comment - Thanks Paul, This patch looks good to me now - sending for integration review. Regards, Damyon
        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 master, 24 and 23. Thanks Paul

        Show
        Dan Poltawski added a comment - Integrated to master, 24 and 23. Thanks Paul
        Hide
        Mark Nelson added a comment -

        Works as expected. Passing. Just a thought, shouldn't 'maximum attachments' be shown if the 'maxfiles' is set to 0. Currently this does not display if maxfiles is set to -1, or 0. I looked at the code and saw that in files/renderer.php $hasmaxfiles is set depending on maxfiles being > 0, which is then used to determine which string is used to display above the filemanager. Personally I think the line:

        $hasmaxfiles = !empty($fm->options->maxfiles) && $fm->options->maxfiles > 0;

        should be changed to

        $hasmaxfiles = isset($fm->options->maxfiles) && $fm->options->maxfiles >= 0;

        which would allow for the string "Maximum size for new files: Unlimited, maximum attachments: 0". Though I have no idea when this situation would ever arise, it is just useful information to have if it does occur.

        Show
        Mark Nelson added a comment - Works as expected. Passing. Just a thought, shouldn't 'maximum attachments' be shown if the 'maxfiles' is set to 0. Currently this does not display if maxfiles is set to -1, or 0. I looked at the code and saw that in files/renderer.php $hasmaxfiles is set depending on maxfiles being > 0, which is then used to determine which string is used to display above the filemanager. Personally I think the line: $hasmaxfiles = !empty($fm->options->maxfiles) && $fm->options->maxfiles > 0; should be changed to $hasmaxfiles = isset($fm->options->maxfiles) && $fm->options->maxfiles >= 0; which would allow for the string "Maximum size for new files: Unlimited, maximum attachments: 0". Though I have no idea when this situation would ever arise, it is just useful information to have if it does occur.
        Hide
        Damyon Wiese added a comment -

        Another option would be to hide the filepicker completely if maxfiles is 0 and remove all the manual checks for this in the code.

        Show
        Damyon Wiese added a comment - Another option would be to hide the filepicker completely if maxfiles is 0 and remove all the manual checks for this in the code.
        Hide
        Mark Nelson added a comment -

        Sure, that would work, though is it worth creating a separate issue for a situation that is unlikely to occur?

        Show
        Mark Nelson added a comment - Sure, that would work, though is it worth creating a separate issue for a situation that is unlikely to occur?
        Hide
        Damyon Wiese added a comment -

        IMO - no it's not worth it.

        Show
        Damyon Wiese added a comment - IMO - no it's not worth it.
        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: