Moodle

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

Details

  • Type: Bug Bug
  • Status: Closed 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 from Repository:
  • Pull Master Branch:
    MDL-29173_calc
  • Pull Master Diff URL:

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

is duplicated by

Bug - A problem which impairs or prevents Moodle from functioning correctly. MDL-31668 Gradebook: Round function gives 'Invalid formula error' when editing a calculation

  • Minor - Minor loss of function where workaround is possible
  • Closed - The issue is considered finished, the resolution is correct. Issues which are closed should not be reopened.
testing discovered

Bug - A problem which impairs or prevents Moodle from functioning correctly. MDL-33084 strict standards warnings when using calculated grade items

  • Minor - Minor loss of function where workaround is possible
  • Closed - The issue is considered finished, the resolution is correct. Issues which are closed should not be reopened.

Task - A task that needs to be completed. Tasks usually refer to work that must be done outside the product. MDL-33112 Assess floor() and ceil() support in calculated grade items

  • Minor - Minor loss of function where workaround is possible
  • Open - The issue is open and ready for the assignee to start work on it.

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
Andrew Davis added a comment -

Good spotting. Fixed.

Show
Andrew Davis added a comment - Good spotting. Fixed.
Hide
Permalink
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
Permalink
Andrew Davis added a comment -

Updated testing instructions. Submitting for integration.

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

That looks correct.

Show
Andrew Davis added a comment - That looks correct.
Hide
Permalink
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
Permalink
Andrew Davis added a comment -

ah. Ill take another look.

Show
Andrew Davis added a comment - ah. Ill take another look.
Hide
Permalink
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
Permalink
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
Permalink
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
Permalink
Ankit Agarwal added a comment -

Thanks for the clarification.
Passing!

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

Thankyou for your patience

Show
Andrew Davis added a comment - Thankyou for your patience
Hide
Permalink
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

Vote (2)
Watch (4)

Dates

  • Created:
    Updated:
    Resolved: