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

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

    Details

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

      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.

        Gliffy Diagrams

          Activity

          Hide
          salvetore 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
          salvetore 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
          andyjdavis 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
          andyjdavis 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
          andyjdavis Andrew Davis added a comment -

          Adding a branch with a fix.

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

          Added testing instructions.

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

          Well done, guys.

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

          Marina, perhaps you could peer review this one?

          Show
          poltawski Dan Poltawski added a comment - Marina, perhaps you could peer review this one?
          Hide
          marina 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 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
          nebgor Aparup Banerjee added a comment -

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

          ps : for master and 23.

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

          Thanks Andrew, this has been integrated now

          Show
          samhemelryk Sam Hemelryk added a comment - Thanks Andrew, this has been integrated now
          Hide
          ankit_frenz 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_frenz 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 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 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
          samhemelryk 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
          samhemelryk 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:
                Fix Release Date:
                9/Jul/12