Moodle
  1. Moodle
  2. MDL-33686

core_files_renderer::fm_print_restrictions(): $options instead of $fm->options?

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.3
    • Fix Version/s: 2.3.1
    • Component/s: Libraries
    • Labels:
    • Testing Instructions:
      Hide

      Edit a forum's setting and set "Maximum number of attachments" to a number greater than 0.

      As a student reply to a post in the forum. Above the attachments file picker business should be a message similar to "Maximum size for new files: 500KB, maximum attachments: 2 - drag and drop available"

      Check that the number of attachments matches the forum setting.

      Go to the students private files. The message should now be something like "Maximum size for new files: 2MB - drag and drop available". It shouldn't mention attachments.

      Show
      Edit a forum's setting and set "Maximum number of attachments" to a number greater than 0. As a student reply to a post in the forum. Above the attachments file picker business should be a message similar to "Maximum size for new files: 500KB, maximum attachments: 2 - drag and drop available" Check that the number of attachments matches the forum setting. Go to the students private files. The message should now be something like "Maximum size for new files: 2MB - drag and drop available". It shouldn't mention attachments.
    • Affected Branches:
      MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE
    • Pull Master Branch:
      MDL-33686_options
    • Rank:
      41684

      Description

      On searching for the string "dndupload" (details in http://tracker.moodle.org/browse/MDL-32937?focusedCommentId=163010&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-163010) I've also opened files/renderer.php in PhpStorm which claims for an undefined variable in fm_print_restrictions():

          /**
           * Displays restrictions for the file manager
           *
           * @param form_filemanager $fm
           * @return string
           */
          private function fm_print_restrictions($fm) {
              $maxbytes = display_size($fm->options->maxbytes);
              if (empty($options->maxfiles) || $options->maxfiles == -1) {
                  $maxsize = get_string('maxfilesize', 'moodle', $maxbytes);
                  //$string['maxfilesize'] = 'Maximum size for new files: {$a}';
              } else {
                  $strparam = (object)array('size' => $maxbytes, 'attachments' => $options->maxfiles);
                  $maxsize = get_string('maxsizeandattachments', 'moodle', $strparam);
                  //$string['maxsizeandattachments'] = 'Maximum size for new files: {$a->size}, maximum attachments: {$a->attachments}';
              }
              // TODO MDL-32020 also should say about 'File types accepted'
              return '<span>'. $maxsize. '</span>';
          }
      

      Not sure what this missing could cause: it seems it could really be a minor issue, maybe important just for MDL-27156.

        Activity

        Hide
        Michael de Raadt added a comment -

        Hi, Matteo.

        Could you provide any further information about the error reported? What variable did it suggest was undefined?

        Show
        Michael de Raadt added a comment - Hi, Matteo. Could you provide any further information about the error reported? What variable did it suggest was undefined?
        Hide
        Andrew Davis added a comment -

        I see the problem

        $maxbytes = display_size($fm->options->maxbytes);
        if (empty($options->maxfiles) || $options->maxfiles == -1) {
            $maxsize = get_string('maxfilesize', 'moodle', $maxbytes);
            //$string['maxfilesize'] = 'Maximum size for new files: {$a}';
        } else {
        

        $options isnt defined so empty($options->maxfiles) returns true to the first branch of the if statement is always executed. I'm not sure of the actual efffects this has been having however its clearly incorrect.

        Show
        Andrew Davis added a comment - I see the problem $maxbytes = display_size($fm->options->maxbytes); if (empty($options->maxfiles) || $options->maxfiles == -1) { $maxsize = get_string('maxfilesize', 'moodle', $maxbytes); //$string['maxfilesize'] = 'Maximum size for new files: {$a}'; } else { $options isnt defined so empty($options->maxfiles) returns true to the first branch of the if statement is always executed. I'm not sure of the actual efffects this has been having however its clearly incorrect.
        Hide
        Andrew Davis added a comment -

        Adding a branch with a fix.

        Show
        Andrew Davis added a comment - Adding a branch with a fix.
        Hide
        Andrew Davis added a comment -

        Added testing instructions.

        Show
        Andrew Davis added a comment - Added testing instructions.
        Hide
        Michael de Raadt added a comment -

        Well done, guys.

        Show
        Michael de Raadt added a comment - Well done, guys.
        Hide
        Dan Poltawski added a comment -

        Marina, perhaps you could peer review this one?

        Show
        Dan Poltawski added a comment - Marina, perhaps you could peer review this one?
        Hide
        Marina Glancy added a comment -

        I did not know about this issue and have just made exactly the same modifications:
        http://tracker.moodle.org/browse/MDL-33948

        Show
        Marina Glancy added a comment - I did not know about this issue and have just made exactly the same modifications: http://tracker.moodle.org/browse/MDL-33948
        Hide
        Aparup Banerjee added a comment -

        This looks A-OK to me, sending up for integration.

        ps : for master and 23.

        Show
        Aparup Banerjee added a comment - This looks A-OK to me, sending up for integration. ps : for master and 23.
        Hide
        Sam Hemelryk added a comment -

        Thanks Andrew, this has been integrated now

        Show
        Sam Hemelryk added a comment - Thanks Andrew, this has been integrated now
        Hide
        Ankit Agarwal added a comment -

        This is working as expected.
        Just to note:- The attachment count constraint is implemented for admin as well where as the filesize constraint is not present for the admin.
        Thanks

        Show
        Ankit Agarwal added a comment - This is working as expected. Just to note:- The attachment count constraint is implemented for admin as well where as the filesize constraint is not present for the admin. Thanks
        Hide
        Matteo Scaramuccia added a comment -

        Made a Link Issue based on Marina's comment: I'm not allowed to access to MDL-33948, maybe the link type could be different from my generic one.

        Show
        Matteo Scaramuccia added a comment - Made a Link Issue based on Marina's comment: I'm not allowed to access to MDL-33948, maybe the link type could be different from my generic one.
        Hide
        Sam Hemelryk added a comment -

        Congratulations your code is upstream - gold star for you!

        This issue + 79 others made it in in time for the minor releases.
        Thank you everyone involved for your exuberant efforts.

        Show
        Sam Hemelryk added a comment - Congratulations your code is upstream - gold star for you! This issue + 79 others made it in in time for the minor releases. Thank you everyone involved for your exuberant efforts.

          People

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

            Dates

            • Created:
              Updated:
              Resolved: