Also
4. Add a check that $answer is numeric before running the above loop. Eval() seems like something of a wild card to me, and I wonder if we can really guarantee it'll never return anything weird? An infinite loop makes for a bad user experience

You are right, this is odd.

I was hoping that a judicious cast to (float) would fix the -0 issue, but it doesn't:

echo '<pre>';
$a = '-0';
echo (float) $a, "\n";
$a = (float) $a;
echo ">=: ", $a >= 0, "\n";
echo ">: ", $a > 0, "\n";
echo '</pre>';

outputs -0, true, false.

So, I think I prefer your option 1.

For the infinite loop, we need to handle 0 as a special case.

Are you intending to do a patch for this, or do I need to?

I didn't realize this before: php is correct that float(-0) == float(0) in IEEE floating point. See http://en.wikipedia.org/wiki/Negative_zero#Comparisons

Sure, I'll make a patch.

Here's a fix for 1,2, and 4.

For 4. I'm just checking if the result is numeric or not. (i.e. if it's a numeric answer or an error message, or null.) This will prevent the infinite loop if for some reason the eval() fails.

The error handling in this qtype could use a lot of work!

substitute_variables_and_eval can return '*'. In that case, is your change in qtype_calculated_calculate_answer the right thing? (I can't work it out now.) This qtype could also use a lot more PHPdoc comments to explain what these functions are supposed to do!

These changes feel to me like the sort of thing that should have unit tests. It would also help document the expected behaviour of these functions. Do you have time to add some?

Thanks. P.S. I do remember I owe you a code-review of MDL-35717.

What happens with * is that qtype_calculated_calculate_answer() returns NaN, but then comment_on_datasetitems() turns around and overwrites the answer with *. None of the code below the is_numeric() in qtype_calculated_calculate_answer() makes any sense for $answer = * (or anything non-numeric), which is why I just have it return NaN. I chose NaN because if the formula has an error then at least NaN means it won't match any student response.

I think this fixes the immediate problem we're seeing. I've got other projects right now that are taking my time, so I can't really get into it beyond this fix.

(Oh, and the other caller of qtype_calculated_calculate_answer() is in multichoice_comment_on_datasetitems(), but that's apparently never called. Maybe it's obsoleted by the calculatedmulti qtype?)

Right, I understand now. Thanks for explaining. We can look at cleaning up the obsolete code, and adding more unit tests later.

Can you re-base and cherry-pick to stable branches, then I will submit this for integration. Thanks.

Hopefully I did the re-base and cherry-picking right. Does anything else need to be done for integration?

2.1 is not supported any more (apart from security fixes) so your only mistake was to cherry pick to that branch.

I hope the testing instructions I wrote are OK.

Ah, and I forgot to write testing instructions. Thanks for your help on this, Tim.

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

I've not seen NAN used before so I checked it was a php predefined constant: http://php.net/manual/en/math.constants.php

Thanks Matt, i've integrated this to master, 24, 23 and 22.

Yes, I had to check that too. Perhaps I should have added a comment at that point? Oh well.

I tested this yesterday but was unable to pass it due to a problem with tracker. Passing.

Many thanks for your effort, the whole Moodle Community will be enjoying your great solutions starting now!

Closing, ciao