Fixed inconsistent string handling in SUM() implementations, issue #3652

[betterphp]

Fix #3652.

This is:

• a bugfix
• a new feature
• refactoring
• additional unit tests

Checklist:

• Changes are covered by unit tests
  • Changes are covered by existing unit tests
  • New unit tests have been added
• Code style is respected
• Commit message explains why the change is made (see https://github.com/erlang/otp/wiki/Writing-good-commit-messages)
• CHANGELOG.md contains a short summary of the change and a link to the pull request if applicable
• Documentation is updated as necessary

Why this change is needed?

To resolve inconsistent results from the two SUM() implementations. Issue #3652

[oleibman]

Thank you for pointing out the problem. You are correct that the code should be more consistent between sumIgnoringStrings and sumErroringStrings. However, as they are currently coded, sumIgnoringStrings is more correct - you should be changing sumErroringStrings to look more like it rather than vice versa. The logic in sumErroringStrings to cast the value to int was really intended only for null and null-string arguments, and, frankly, seems misguided. Null is accounted for later on, and I don't think null-string is acceptable as a literal in the SUM function, just as with any other string literal. I think both the empty test and the cast are unneeded, and the code should be simplified to:

            if (is_numeric($arg)) {
                $returnValue += $arg;

All string numeric arguments will automatically be cast to float or int as appropriate.

Once you have made that change, there is no longer any reason to change RefErrorTest, so you should restore it to its original form. However, your observation makes it clear that we have not included a test for a string which represents a floating point value in the SUM tests (tests/data/Calculation/MathTrig/SUM and SUMLITERAL). Please add such a test to each of those, and also please add a test for null string to each.

[betterphp]

It looks like there are some test cases for floats already (e.g. https://github.com/PHPOffice/PhpSpreadsheet/blob/master/tests/data/Calculation/MathTrig/SUMLITERALS.php#L11) - unless I'm misunderstanding what you mean?

Calling Sum::sumErroringStrings() directly seems to return a different result compared to using the =SUM() formula. Would it be worth adding tests that call the function directly maybe? That's our current use case and how I found this bug.

[oleibman]

The existing tests for float supply the literal as a float, not as a string (which is the bug you uncovered). So, we have:

[39.1, 5.7, true, 30, 2.4],

But we need to add a test for

[39.1, '5.7', true, 30, 2.4],

As to your question, can you give me an example where SUM yields a different result than calling sumerroringstrings directly?

[betterphp]

Hey, sorry I've been away for a few weeks - thanks for the extra commits @oleibman and for merging :)