Moodle
  1. Moodle
  2. MDL-7425

Divide by zero errors on page listing courses in a category when in edit mode

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 1.7
    • Fix Version/s: 1.8.3, 1.9
    • Component/s: General
    • Labels:
      None

      Description

      In Add/edit courses, click on a category to display the courses in that category. When editing is on, get errors:

      Warning: Division by zero in /Library/WebServer/Documents/moodle/lib/weblib.php on line 4877

      and

      Warning: Division by zero in /Library/WebServer/Documents/moodle/course/category.php on line 300

        Gliffy Diagrams

        1. category.diff
          0.6 kB
          Ken Wilson
        2. weblib.diff
          0.6 kB
          Ken Wilson
        3. weblib-perpage-div-zero.diff
          1.0 kB
          Peter Bulmer

          Activity

          Hide
          Ken Wilson added a comment -

          Possible fix for weblib attached as weblib.diff against HEAD.

          Show
          Ken Wilson added a comment - Possible fix for weblib attached as weblib.diff against HEAD.
          Hide
          Ken Wilson added a comment -

          Possible fix for category attached as category.diff against HEAD.

          Show
          Ken Wilson added a comment - Possible fix for category attached as category.diff against HEAD.
          Hide
          Anthony Borrow added a comment -

          Ken/Martin - Thanks for the diffs - basically you are just checking to make sure that $perpage is greater than 1 before doing the calculation; however, I am wondering if it is a matter of $perpage not being properly initiated. Should there be a $perpage=1 earlier in the files? IMO, the real issue is how is it that it is set to zero. In particular, I'm looking at line 11 in the 1.93.2.10 version of /course/category.php in 18STABLE:

          $perpage = optional_param('perpage', $CFG->coursesperpage, PARAM_INT); // how many per page

          is $CFG->coursesperpage being defined? I upgraded my database from 1.6 and in mdl_config I have a record where the name='coursesperpage' and the value is 20. My guess is that that record is not being created on a fresh 1.8 install. To fix your installation, I would manually add a record to mdl_config using the following:

          INSERT INTO mdl_config (name, value)
          VALUES ('coursesperpage',20);

          Now I will go and track down exactly why this is not being created on a fresh installation and see if we can get that patched.

          Peace - Anthony

          Show
          Anthony Borrow added a comment - Ken/Martin - Thanks for the diffs - basically you are just checking to make sure that $perpage is greater than 1 before doing the calculation; however, I am wondering if it is a matter of $perpage not being properly initiated. Should there be a $perpage=1 earlier in the files? IMO, the real issue is how is it that it is set to zero. In particular, I'm looking at line 11 in the 1.93.2.10 version of /course/category.php in 18STABLE: $perpage = optional_param('perpage', $CFG->coursesperpage, PARAM_INT); // how many per page is $CFG->coursesperpage being defined? I upgraded my database from 1.6 and in mdl_config I have a record where the name='coursesperpage' and the value is 20. My guess is that that record is not being created on a fresh 1.8 install. To fix your installation, I would manually add a record to mdl_config using the following: INSERT INTO mdl_config (name, value) VALUES ('coursesperpage',20); Now I will go and track down exactly why this is not being created on a fresh installation and see if we can get that patched. Peace - Anthony
          Hide
          Anthony Borrow added a comment -

          Martin,

          I'm not sure why or how folks were getting passed defining $coursesperpage on the frontpage settings. It requires an INT. I grep'd the code and did not see anything like a set_config('coursesperpage',20);

          I am assuming that the thinking was that since this was a required param during setup that it was being taken care; however, some folks are discovering that it is not. I'm not quite sure where to look to help resolve this.

          This issue is discussed at:
          http://moodle.org/mod/forum/discuss.php?d=73089
          http://moodle.org/mod/forum/discuss.php?d=69851
          http://moodle.org/mod/forum/discuss.php?d=64261

          In 16STABLE it was defined in the defaults.php table with a value of 20. Could we not do something similar in 1.7 onward?

          Show
          Anthony Borrow added a comment - Martin, I'm not sure why or how folks were getting passed defining $coursesperpage on the frontpage settings. It requires an INT. I grep'd the code and did not see anything like a set_config('coursesperpage',20); I am assuming that the thinking was that since this was a required param during setup that it was being taken care; however, some folks are discovering that it is not. I'm not quite sure where to look to help resolve this. This issue is discussed at: http://moodle.org/mod/forum/discuss.php?d=73089 http://moodle.org/mod/forum/discuss.php?d=69851 http://moodle.org/mod/forum/discuss.php?d=64261 In 16STABLE it was defined in the defaults.php table with a value of 20. Could we not do something similar in 1.7 onward?
          Hide
          Anthony Borrow added a comment -

          I just tried a fresh install of 1.8 and the coursesperpage record is created in mdl_config during the first part of the startup. The default value of 20 automatically appears when the user is asked to enter the frontpage settings. Even if I try to delete this value I am not allowed forward on the page. I'm not sure how to replicate this error - can someone who was having this problem explain how to recreate it. Thanks - Anthony

          Show
          Anthony Borrow added a comment - I just tried a fresh install of 1.8 and the coursesperpage record is created in mdl_config during the first part of the startup. The default value of 20 automatically appears when the user is asked to enter the frontpage settings. Even if I try to delete this value I am not allowed forward on the page. I'm not sure how to replicate this error - can someone who was having this problem explain how to recreate it. Thanks - Anthony
          Hide
          Peter Bulmer added a comment -

          a very similar fix to weblib - only this one doesn't change the functionality.

          Show
          Peter Bulmer added a comment - a very similar fix to weblib - only this one doesn't change the functionality.
          Hide
          Peter Bulmer added a comment -

          The diff I have just attached, is very similar to the one posted months ago by Ken.
          The minor difference is that my one doesn't change the functionality at all (AFAIK).
          I've looked into what happens with this:
          ceil(INT / 0), and it seems that if you get a divide by 0, the result is FALSE, the ceil of that is 0.

          My logic is that people aren't complaining that there is broken functionality, we should just be getting rid of this annoying error.
          It's fairly common for people to use 0 to mean INF in this situation, 'put everything on one page nomatter how many there are', so perhaps a $perpage isn't actually broken?

          My patch leaves the functionality the same - a $perpage of zero still results in $lastpage being set to zero, suppresses the error, and is a lot more transparent to the programmer.

          The issue with weblib certainly affects 1.8 (and development head), but I haven't seen the other problem described above with course/category.php in 1.8. Maybe it's still there, I might not be looking hard enough.

          Pete.

          Show
          Peter Bulmer added a comment - The diff I have just attached, is very similar to the one posted months ago by Ken. The minor difference is that my one doesn't change the functionality at all (AFAIK). I've looked into what happens with this: ceil(INT / 0), and it seems that if you get a divide by 0, the result is FALSE, the ceil of that is 0. My logic is that people aren't complaining that there is broken functionality, we should just be getting rid of this annoying error. It's fairly common for people to use 0 to mean INF in this situation, 'put everything on one page nomatter how many there are', so perhaps a $perpage isn't actually broken? My patch leaves the functionality the same - a $perpage of zero still results in $lastpage being set to zero, suppresses the error, and is a lot more transparent to the programmer. The issue with weblib certainly affects 1.8 (and development head), but I haven't seen the other problem described above with course/category.php in 1.8. Maybe it's still there, I might not be looking hard enough. Pete.
          Hide
          Anthony Borrow added a comment -

          Pete - You patch looks good and it should not change any of the current functionality. My only concern is that I would want to make sure that we check for the case of when $perpage might not be defined at all or 0. From what you are saying, it seems that the only way that this error could be encountered is if the Moodle admin sets the courses per page value to 0. I was concerned that somehow on some sites that the value was not being set during installation; however, I have not been able to replicate this. In any case, double checking to make sure the $perpage has a value set would be helpful. Peace - Anthony

          Show
          Anthony Borrow added a comment - Pete - You patch looks good and it should not change any of the current functionality. My only concern is that I would want to make sure that we check for the case of when $perpage might not be defined at all or 0. From what you are saying, it seems that the only way that this error could be encountered is if the Moodle admin sets the courses per page value to 0. I was concerned that somehow on some sites that the value was not being set during installation; however, I have not been able to replicate this. In any case, double checking to make sure the $perpage has a value set would be helpful. Peace - Anthony
          Hide
          Anthony Borrow added a comment -

          I just ran in to this again on my test server with a backup our production code from last year. It occurs to me that another option would be at the beginning to check the value of perpage and if it is 0 or non-existent then set it to an arbitrarily large number like 999. That way we do not have to keep checking for all of the places where it might be used and putting in an additional if statement each time. Peace - Anthony

          Show
          Anthony Borrow added a comment - I just ran in to this again on my test server with a backup our production code from last year. It occurs to me that another option would be at the beginning to check the value of perpage and if it is 0 or non-existent then set it to an arbitrarily large number like 999. That way we do not have to keep checking for all of the places where it might be used and putting in an additional if statement each time. Peace - Anthony
          Hide
          Anthony Borrow added a comment -

          This also effects /course/category.php which makes me think that it may be better to modify this when we get the $CFG data and if $CFG->perpage = 0 then we should set it to a large number like 999. Then we can forget about how folks use the $perpage variable. Alternatively we could disallow a zero value and instruct admins to use a large number if they want unlimited per page. The more I think about it the more I like this idea of validating the data during input and preventing bad data from being entered. Only positive integers greater than 0 should be allowed.

          Show
          Anthony Borrow added a comment - This also effects /course/category.php which makes me think that it may be better to modify this when we get the $CFG data and if $CFG->perpage = 0 then we should set it to a large number like 999. Then we can forget about how folks use the $perpage variable. Alternatively we could disallow a zero value and instruct admins to use a large number if they want unlimited per page. The more I think about it the more I like this idea of validating the data during input and preventing bad data from being entered. Only positive integers greater than 0 should be allowed.
          Hide
          Martin Dougiamas added a comment -

          Hmm, coursesperpage is normally set to a default 20 in admin/settings/frontpage.php as all the settings are. I don't know why that is not happening.

          But for general robustness, these patches look good. I've checked them in to 1.8 and head (1.9)

          Thanks!

          Show
          Martin Dougiamas added a comment - Hmm, coursesperpage is normally set to a default 20 in admin/settings/frontpage.php as all the settings are. I don't know why that is not happening. But for general robustness, these patches look good. I've checked them in to 1.8 and head (1.9) Thanks!

            People

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

              Dates

              • Created:
                Updated:
                Resolved: