Moodle
  1. Moodle
  2. MDL-29173

round function throws error 'wrong number of arguments (2 given, 1 expected)'

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: 2.1.1
    • Fix Version/s: 2.1.7, 2.2.4
    • Component/s: Gradebook
    • Labels:
    • Testing Instructions:
      Hide

      While testing you may see php strict standards warnings unless MDL-33084 has already been integrated.

      Go to the settings for a gradeable activity and set an ID number. For example I gave a database activity the ID "dat" without quotes but use whatever you like. This is ID used to refer to the activity in calculations.

      Go into a course and go to the gradebook. Go to the categories and items full view screen.

      Click add grade item. Give your new grade item a name and click save.

      Click the calculator icon in the same row as the new grade item. You're going to create a series of calculations in the form "=round([[dat]])" (without quotes) Replace "dat" with the ID number for your activity. Notice that activity IDs are contained with double square brackets.

      Enter the formula to "=round([[dat]],1,2)". That should also be invalid as now you have too many parameters.

      Enter "=round([[dat]])" (without quotes) as the calculation. Click save changes. It should save successfully.

      Change it to "=round([[dat]],1)". That should save successfully.

      Go the grader report and check that the calc grade item is displayed correctly. Some things to be aware of that affect "correct".

      1) A student will need a grade for the activity before a grade for the calc item will appear. I recommend entering one like 1.2345 so that its obvious how many decimal places are being displayed.

      2) The grader report may re-add decimal places removed by round(). For example, for me an activity grade entered as 1.2345 gets displayed in the activity column as 1.23 and in the calc column as 1.20 as round([[dat]],1) is rounding to one decimal place but then the grader report is adding a 2nd decimal place. This oddity is beyond the scope of this bug.

      Also run the unit tests in each version. You can limit it to lib/simpletest/testmathslib.php in 2.2 and 2.1 and to lib/tests/mathslib_test.php in master. There are instructions on how to do this on the unit test page under site settings.

      Show
      While testing you may see php strict standards warnings unless MDL-33084 has already been integrated. Go to the settings for a gradeable activity and set an ID number. For example I gave a database activity the ID "dat" without quotes but use whatever you like. This is ID used to refer to the activity in calculations. Go into a course and go to the gradebook. Go to the categories and items full view screen. Click add grade item. Give your new grade item a name and click save. Click the calculator icon in the same row as the new grade item. You're going to create a series of calculations in the form "=round([ [dat] ])" (without quotes) Replace "dat" with the ID number for your activity. Notice that activity IDs are contained with double square brackets. Enter the formula to "=round([ [dat] ],1,2)". That should also be invalid as now you have too many parameters. Enter "=round([ [dat] ])" (without quotes) as the calculation. Click save changes. It should save successfully. Change it to "=round([ [dat] ],1)". That should save successfully. Go the grader report and check that the calc grade item is displayed correctly. Some things to be aware of that affect "correct". 1) A student will need a grade for the activity before a grade for the calc item will appear. I recommend entering one like 1.2345 so that its obvious how many decimal places are being displayed. 2) The grader report may re-add decimal places removed by round(). For example, for me an activity grade entered as 1.2345 gets displayed in the activity column as 1.23 and in the calc column as 1.20 as round([ [dat] ],1) is rounding to one decimal place but then the grader report is adding a 2nd decimal place. This oddity is beyond the scope of this bug. Also run the unit tests in each version. You can limit it to lib/simpletest/testmathslib.php in 2.2 and 2.1 and to lib/tests/mathslib_test.php in master. There are instructions on how to do this on the unit test page under site settings.
    • Workaround:
      Hide

      I think removing 'round' from the '$fb' array will resolve the issue, but not sure if that will have other impacts.

      Show
      I think removing 'round' from the '$fb' array will resolve the issue, but not sure if that will have other impacts.
    • Affected Branches:
      MOODLE_21_STABLE
    • Fixed Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE
    • Pull Master Branch:
      MDL-29173_calc
    • Rank:
      18721

      Description

      When attempting to use the round function in the gradebook, If you set up a custom calculation along the lines of '=round([[ec]],2)' it will return the previous error.

      When you track it this is being thrown in 'moodle/lib/evalmath/evalmath.class.php' line 256.

      It looks like 'round' was added to the '$fb' array in moodle2+, even though it was no in moodle1.9

      This makes it match on the first if statement, which only allows for one argument or throws an exception. In previous versions it was not in the '$fb' array only the '$fc' array (which it still is in moodle2+). My guess is removing it from the '$fb' array will fix the problem, but I am not sure if that will break anything else.

      line 247
      $fnn = $matches[1]; // get the function name
      $arg_count = $stack->pop(); // see how many arguments there were (cleverly stored on the stack, thank you)
      $fn = $stack->pop();
      $output[] = array('fn'=>$fn, 'fnn'=>$fnn, 'argcount'=>$arg_count); // send function to output
      if (in_array($fnn, $this->fb)) { // check the argument count
          if($arg_count > 1) {
              $a= new stdClass();
              $a->expected = 1;
              $a->given = $arg_count;
              return $this->trigger(get_string('wrongnumberofarguments', 'mathslib', $a));
          }
      } elseif (array_key_exists($fnn, $this->fc)) {
          $counts = $this->fc[$fnn];
          if (in_array(-1, $counts) and $arg_count > 0) {}
          elseif (!in_array($arg_count, $counts)) {
              $a= new stdClass();
              $a->expected = implode('/',$this->fc[$fnn]);
              $a->given = $arg_count;
              return $this->trigger(get_string('wrongnumberofarguments', 'mathslib', $a));
          }
      } 
      

      Replication instructions:

      1. Add a new grade item.
      2. Add a custom calculation.
      3. Use the 'round' function in that calculation.
      4. Will receive 'invalid formula' error (in non-development mode)

        Issue Links

          Activity

          Hide
          Vangel Ajanovski added a comment -

          I removed "round" from "fb" in our setup and that fixed the problem, but I don't know if this change has some other impact.

          Show
          Vangel Ajanovski added a comment - I removed "round" from "fb" in our setup and that fixed the problem, but I don't know if this change has some other impact.
          Hide
          Michael de Raadt added a comment -

          This issue has been duplicated, but I think the priority currently appropriate.

          Show
          Michael de Raadt added a comment - This issue has been duplicated, but I think the priority currently appropriate.
          Hide
          Vangel Ajanovski added a comment -

          Due to this students are not able to see the points and grades in all courses where part of the grade was calculated using the round function.
          This is not just a problem when you create a new course and new grade item based on formula with round. It is also the case for all old courses and we noticed it right after migration from Moodle 1.9.x

          For us it was critical and we had to put the patch, althuough we are not sure if the patch is safe and does not have side-effects.

          Show
          Vangel Ajanovski added a comment - Due to this students are not able to see the points and grades in all courses where part of the grade was calculated using the round function. This is not just a problem when you create a new course and new grade item based on formula with round. It is also the case for all old courses and we noticed it right after migration from Moodle 1.9.x For us it was critical and we had to put the patch, althuough we are not sure if the patch is safe and does not have side-effects.
          Hide
          Tõnis Tartes added a comment -

          Just stumbled upon this in Moodle 2.2.2...
          Thank you very much for the workaround!

          Our university is planning to upgrade to Moodle 2.x from Moodle 1.9 this summer and we have used alot grade calculations in our courses.
          I think, its a very critical issue and should be fixed ASAP.

          Show
          Tõnis Tartes added a comment - Just stumbled upon this in Moodle 2.2.2... Thank you very much for the workaround! Our university is planning to upgrade to Moodle 2.x from Moodle 1.9 this summer and we have used alot grade calculations in our courses. I think, its a very critical issue and should be fixed ASAP.
          Hide
          Andrew Davis added a comment -

          Adding a branch containing a possible fix and some testing instructions.

          Show
          Andrew Davis added a comment - Adding a branch containing a possible fix and some testing instructions.
          Hide
          Andrew Davis added a comment -

          I've put this up for peer review. I've also raised MDL-33112. I noticed some other functions that I suspect can simply be removed but I'll need to do more research.

          Show
          Andrew Davis added a comment - I've put this up for peer review. I've also raised MDL-33112 . I noticed some other functions that I suspect can simply be removed but I'll need to do more research.
          Hide
          Rajesh Taneja added a comment -

          Thanks Andrew,

          Patch looks good to me. IMO you should backported this on 22 and 21 branches.

          Show
          Rajesh Taneja added a comment - Thanks Andrew, Patch looks good to me. IMO you should backported this on 22 and 21 branches.
          Hide
          Andrew Davis added a comment -

          Adding branches for 2.2 and 2.1. Submitting for integration.

          Show
          Andrew Davis added a comment - Adding branches for 2.2 and 2.1. Submitting for integration.
          Hide
          Dan Poltawski added a comment -

          Hi Andrew,

          Do you know if what you are changing is changes in the third party library or changes to our changes to that library?

          I think that we might need to make a comment on this in the readme_moodle.txt (ideally we try to avoid any changes to third party libraries because it makes it difficult to track and also for upstreams (e.g. debian/ubuntu) to strip out our bundled libraries).

          Show
          Dan Poltawski added a comment - Hi Andrew, Do you know if what you are changing is changes in the third party library or changes to our changes to that library? I think that we might need to make a comment on this in the readme_moodle.txt (ideally we try to avoid any changes to third party libraries because it makes it difficult to track and also for upstreams (e.g. debian/ubuntu) to strip out our bundled libraries).
          Hide
          Dan Poltawski added a comment -

          Adding Petr here.

          Petr - could you give input on this?

          Show
          Dan Poltawski added a comment - Adding Petr here. Petr - could you give input on this?
          Hide
          Andrew Davis added a comment -

          Hi. These are changes to our changes. The addition of the round() function is already in the readme_moodle.txt file.

          Show
          Andrew Davis added a comment - Hi. These are changes to our changes. The addition of the round() function is already in the readme_moodle.txt file.
          Hide
          Petr Škoda added a comment -

          The more functions the better, if unsure how it should work look into Excel and do the same.

          Show
          Petr Škoda added a comment - The more functions the better, if unsure how it should work look into Excel and do the same.
          Hide
          Dan Poltawski added a comment -

          Thanks Andrew for the clarification.

          I've integrated this now.

          Show
          Dan Poltawski added a comment - Thanks Andrew for the clarification. I've integrated this now.
          Hide
          Dan Poltawski added a comment -

          I'm failing this as it has broken unit tests

          Show
          Dan Poltawski added a comment - I'm failing this as it has broken unit tests
          Hide
          Dan Poltawski added a comment -

          Looks like the function was designed to always round to 0 decimal places by the unit tests:

          Unexpected PHP error [wrong number of arguments (1 given, 2 expected)] severity [512] in [/lib/evalmath/evalmath.class.php line 440]
          1) mathsslib_testcase::test_rounding_function
          Failed asserting that 3 matches expected false.

          /Users/danp/git/integration/lib/tests/mathslib_test.php:146
          /Users/danp/git/integration/lib/phpunit/classes/basic_testcase.php:64

          Show
          Dan Poltawski added a comment - Looks like the function was designed to always round to 0 decimal places by the unit tests: Unexpected PHP error [wrong number of arguments (1 given, 2 expected)] severity [512] in [/lib/evalmath/evalmath.class.php line 440] 1) mathsslib_testcase::test_rounding_function Failed asserting that 3 matches expected false. /Users/danp/git/integration/lib/tests/mathslib_test.php:146 /Users/danp/git/integration/lib/phpunit/classes/basic_testcase.php:64
          Hide
          Dan Poltawski added a comment -

          Reverted this change, since it looks like it breaks backwards compatibility and fails unit tests on all branches.

          Show
          Dan Poltawski added a comment - Reverted this change, since it looks like it breaks backwards compatibility and fails unit tests on all branches.
          Hide
          Andrew Davis added a comment -

          For anyone who is interested, here is the Excel doco for round() http://office.microsoft.com/en-us/excel-help/round-HP005209239.aspx?CTT=5&origin=HP003056144

          I've altered this code so that it will work with two arguments as Excel does. It will also work with only 1 argument in which case 0 decimal places is assumed.

          That makes the unit tests work. I've added some new ones that test the 2 argument behaviour. The tests are the same in 2.1, 2.2 and in master although they are in different files due to changes in the Moodle unit test framework.

          Show
          Andrew Davis added a comment - For anyone who is interested, here is the Excel doco for round() http://office.microsoft.com/en-us/excel-help/round-HP005209239.aspx?CTT=5&origin=HP003056144 I've altered this code so that it will work with two arguments as Excel does. It will also work with only 1 argument in which case 0 decimal places is assumed. That makes the unit tests work. I've added some new ones that test the 2 argument behaviour. The tests are the same in 2.1, 2.2 and in master although they are in different files due to changes in the Moodle unit test framework.
          Hide
          Dan Poltawski added a comment -

          Hi Andrew,

          (Not stricly doing a peer review)
          I have just noticed a few lines above, round is already defined in array $fc, so actually I think we need to remove round completely there?

          Show
          Dan Poltawski added a comment - Hi Andrew, (Not stricly doing a peer review) I have just noticed a few lines above, round is already defined in array $fc, so actually I think we need to remove round completely there?
          Hide
          Andrew Davis added a comment -

          Good spotting. Fixed.

          Show
          Andrew Davis added a comment - Good spotting. Fixed.
          Hide
          Rajesh Taneja added a comment -

          Thanks Andrew,

          Looks Good, it might be nice to request tester, to run unit test.

          Show
          Rajesh Taneja added a comment - Thanks Andrew, Looks Good, it might be nice to request tester, to run unit test.
          Hide
          Andrew Davis added a comment -

          Updated testing instructions. Submitting for integration.

          Show
          Andrew Davis added a comment - Updated testing instructions. Submitting for integration.
          Hide
          Dan Poltawski 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
          Dan Poltawski 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 -

          Thanks Andrew,

          i've integrated that now

          Show
          Dan Poltawski added a comment - Thanks Andrew, i've integrated that now
          Hide
          Ankit Agarwal added a comment -

          I was able to save =round([[x]]) as the formula (x being the id)
          Grade of 1.2345 is converted to 1.00 in that case
          it does generate error if I use something like =round([[x]],1,2)
          Failing this, sorry

          Show
          Ankit Agarwal added a comment - I was able to save =round([ [x] ]) as the formula (x being the id) Grade of 1.2345 is converted to 1.00 in that case it does generate error if I use something like =round([ [x] ],1,2) Failing this, sorry
          Hide
          Andrew Davis added a comment -

          My fault. I forgot to update the testing instructions (which I have done now). That behaviour is correct. Both =round([[x]]) and =round([[x]], 1) are valid. If you only supply one parameter it assumes you want it rounded to 0 decimal places.

          I dont seem to be able to reset this issue.

          Show
          Andrew Davis added a comment - My fault. I forgot to update the testing instructions (which I have done now). That behaviour is correct. Both =round([ [x] ]) and =round([ [x] ], 1) are valid. If you only supply one parameter it assumes you want it rounded to 0 decimal places. I dont seem to be able to reset this issue.
          Hide
          Ankit Agarwal added a comment -

          I am getting following warnings on 22, when entering an invalid formula
          Warning: wrong number of arguments (3 given, 1/2 expected) in /var/www/int/22/moodle/lib/evalmath/evalmath.class.php on line 440
          if you enter something like =round([[y]],1,1)

          and
          if you enter something like =round([[x]],1,1) (x is an invalid id) I get the following
          Warning: illegal character '[' in /var/www/int/22/moodle/lib/evalmath/evalmath.class.php on line 440

          Please advise
          Thanks

          Show
          Ankit Agarwal added a comment - I am getting following warnings on 22, when entering an invalid formula Warning: wrong number of arguments (3 given, 1/2 expected) in /var/www/int/22/moodle/lib/evalmath/evalmath.class.php on line 440 if you enter something like =round([ [y] ],1,1) and if you enter something like =round([ [x] ],1,1) (x is an invalid id) I get the following Warning: illegal character '[' in /var/www/int/22/moodle/lib/evalmath/evalmath.class.php on line 440 Please advise Thanks
          Hide
          Andrew Davis added a comment -

          That looks correct.

          Show
          Andrew Davis added a comment - That looks correct.
          Hide
          Ankit Agarwal added a comment -

          Hi Andrew,
          These warnings do not appear on master. And if I was not clear earlier these are PHP(not moodle form) warnings displayed on top of the page, should I ignore them?

          Thanks

          Show
          Ankit Agarwal added a comment - Hi Andrew, These warnings do not appear on master. And if I was not clear earlier these are PHP(not moodle form) warnings displayed on top of the page, should I ignore them? Thanks
          Hide
          Andrew Davis added a comment -

          ah. Ill take another look.

          Show
          Andrew Davis added a comment - ah. Ill take another look.
          Hide
          Andrew Davis added a comment - - edited

          Yeah, I think we can ignore them. It looks like that at some point or other the eval class has had error suppression turned on in master. Its not directly related to this issue.

          In lib/mathslib.php at line 53 master has
          $this->_em->suppress_errors = true; // no PHP errors!
          while 2.2 stable has
          $this->_em->suppress_errors = !debugging('', DEBUG_DEVELOPER);

          Show
          Andrew Davis added a comment - - edited Yeah, I think we can ignore them. It looks like that at some point or other the eval class has had error suppression turned on in master. Its not directly related to this issue. In lib/mathslib.php at line 53 master has $this->_em->suppress_errors = true; // no PHP errors! while 2.2 stable has $this->_em->suppress_errors = !debugging('', DEBUG_DEVELOPER);
          Hide
          Ankit Agarwal added a comment -

          Okay Andrew,
          I entered 1.23456 as the grade and it was displayed as below:-
          1.23 1.24 for x and =round([[x]],3)
          1.23 1.23 for x and =round([[x]],2)

          Seems we round the result to two digits after decimal point to display.
          (1.23456 -> 1.235 ->1.24)
          (1.23456 -> 1.2346 -> 1.23)

          Seems fine to me, after you confirm I can pass the test.
          Thanks

          Show
          Ankit Agarwal added a comment - Okay Andrew, I entered 1.23456 as the grade and it was displayed as below:- 1.23 1.24 for x and =round([ [x] ],3) 1.23 1.23 for x and =round([ [x] ],2) Seems we round the result to two digits after decimal point to display. (1.23456 -> 1.235 ->1.24) (1.23456 -> 1.2346 -> 1.23) Seems fine to me, after you confirm I can pass the test. Thanks
          Hide
          Andrew Davis added a comment -

          Theres a course setting that can also reduce decimal places which you can see /grade/edit/settings/index.php?id=2 <= course id.

          imo, can pass this.

          Show
          Andrew Davis added a comment - Theres a course setting that can also reduce decimal places which you can see /grade/edit/settings/index.php?id=2 <= course id. imo, can pass this.
          Hide
          Ankit Agarwal added a comment -

          Thanks for the clarification.
          Passing!

          Show
          Ankit Agarwal added a comment - Thanks for the clarification. Passing!
          Hide
          Andrew Davis added a comment -

          Thankyou for your patience

          Show
          Andrew Davis added a comment - Thankyou for your patience
          Hide
          Eloy Lafuente (stronk7) added a comment -

          We could celebrate it today... but better if we perform a bigger party after releasing Moodle 2.3.

          Print this message and come to Perth that day, it's valid for one beer, wine, coke or... water, as you wish.

          Many thanks for your collaboration!

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - We could celebrate it today... but better if we perform a bigger party after releasing Moodle 2.3. Print this message and come to Perth that day, it's valid for one beer, wine, coke or... water, as you wish. Many thanks for your collaboration! Ciao

            People

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

              Dates

              • Created:
                Updated:
                Resolved: