Convert percentages stored as strings to numerics in formula calculations

[fdjohnston]

This is:

- [x] 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
• Documentation is updated as necessary

Why this change is needed?

This is a pull request to address #3155

A new function was added that checks it a string contains a percentage, and if so, converts it to a numeric which can be used in formula calculations.

[MarkBaker]

Perhaps this check should be tightened up, because a string that contains only '%' will return a true here, even with no actual numeric value to convert. MS Excel will treat this as a string, and it won't be treated as 0.0%, but will give a #VALUE! error if used in an arithmetic formula.

[MarkBaker]

And MS Excel also treats a string containing a leading percentage sign (e.g. '%2.5') as a numeric percentage value in numeric calculations

[MarkBaker]

There may also be whitespace before a leadingg percentage sign, or after a trailing percentage sign

[fdjohnston]

Great feedback, thanks. Was only considering my particular case.
Just pushed a commit that switches to a regular expression. I've also attached an Excel sheet that tests a bunch of different permutations and combinations.
Let me know what you think.
Excel Test Cases.xlsx

Also just realized that I'm working on master on my fork instead of a new branch; sorry about that.

[MarkBaker]

There's a couple of little tricks that we can do with the regexp to extract the numeric value directly, rather than doing the str_replace() call.

~^(?: *-? *\% *(?<PrefixedValue>[0-9]+\.?[0-9*]*) *| *\-? *(?<PostfixedValue>[0-9]+\.?[0-9]*) *\% *)$~

To avoid capture of the complete string, I've set that as a non-capture group, because we're only interested in whether the % character appears, we don't need to actually capture it or any padding around it; and then I've set capture groups for the actual numeric part of the string. I've also named these groups (PrefixedValue and PostfixedValue) so that they can be referenced by name in the $matches array.

Then, if a match is identified, we can extract the numeric part directly by name, whether it's prefixed by the % or postfixed.

$poperand = (float) ($match['PostfixedValue'] ?? $match['PrefixedValue']) / 100.0;

The null coalescence identifies whether the number is prefixed or postfixed (using the named groups from $matches); and then does the division by 100 and casts the result to a float. There's no function call overhead; just PHP operations, so it should be more efficient if it's being called in a loop thatr checks a lot of cell values.

[fdjohnston]

Great trick with the regexp. I agree that eliminating the str_replace would be a big improvement.
Unfortunately the changes to the regexp break the negative percentage cases. I'll see if I can run with your idea and get it working for negative values.

[MarkBaker]

Ah, yes! I was being a bit to quick and casual with the regexp changes. If the capture group is expanded to encompass the sign as well, then we're potentially also capturing whitespace too, which would break other things.

One option would be to have additional pre/postfix capture groups for the sign, and a match there would trigger a * -1 operation

~^(?: *(?<PrefixedNegative>-?) *\% *(?<PrefixedValue>[0-9]+\.?[0-9*]*) *| *(?<PrefixedNegative>\-?) *(?<PostfixedValue>[0-9]+\.?[0-9]*) *\% *)$~

and then

$sign = $match['PostfixedNegative'] ?? $match['PrefixedNegative'];
$poperand *= ($sign === '-') ? -1.0 : 1.0;

So we're still using only operations, and no function calls.

Just to be bloody minded and awkward, MS Excel recognises a % prefix with the sign before or after the %, so %-2.5 and -%2.5 are both recognised as numeric values. Fortunately with a postfix %, the sign must be before the numeric part.
And, of course, the sign could be a + as well as a -; and the numeric part could be in scientific format, -2.5E-4%.

But if you get the basic negative working, I'll merge and then do some more tweaking myself to handle a few of those edge cases; and refactor it all into a separate helper.

I did say right from the start that this was a lot more complicated once you start to look at it in detail. ;-)

[fdjohnston]

You did warn me! I can't imagine what the code for Excel's formula parser must look like.
I'll take a crack at improving the regexp like you suggest.

Should we be concerned about any of the other spreadsheet applications? I'm focused entirely on Excel because it was the root of my issue, but I suppose it's worth considering how Open Office or Libre Office works? Won't that be a nightmare if they handle these various combinations of %, +, and - differently.... I don't have either installed at the moment but I'll download a copy tonight and see how they behave.

[MarkBaker]

Don't worry about other spreadsheet applications: once it's merged, I'll take your code and do some reworking for the additional Excel cases that I know about while I'm refactoring it all into a helper class, and I'll do a systematic check using Open/Libre Office, Gnumeric and even WPS while I'm doing that.

There are sometimes discrepancies between the different Office Suite spreadsheets, which we handle using the compatibility mode - mostly in the implementation of functions - but we treat Excel itself as the default. The worst is that they aren't consistent between versions, even MS Excel has made changes between versions. We don't cater for that; but we always strive for the latest version... and that means retesting everything whenever there's a new major release of Excel/LibreOffice/Gnumeric.

[fdjohnston]

Okay sounds good. I was able to get it working for combinations of +, -, and scientific notation by expanding on the approach you suggested.
I broke out all the different pieces of the final calculation because it was getting a bit difficult to follow as single line, owing to all the null coalescing checks. We could simplify it by moving all those checks inline.
Note that, as I mention in the commit, I did have to add the PREG_UNMATCHED_AS_NULL flag to the preg_match, otherwise I was getting '' back when a capture group wasn't found.
Let me know what you think.

[fdjohnston]

Updated my Excel sheet with various permutations.
Excel Test Cases.xlsx

[MarkBaker]

Of course, you don't need to use the pow() function at all, PHP has a power operator that can be used instead

But I'm sure that casting to float will convert a string like 1.23e-4 to a float value without needing that additional complexity, and that the regexp can be simplified to extract just sign and numeric value rather than sign, mantissa, exponent sign and exponent.
Also, E and e can both be used for scientific format, so the regexp needs to be case-insensitive with the i switch

[fdjohnston]

Yep, you're right. Thanks for your patience on this PR. Really appreciate your taking the time to point this stuff out.
I think this most recent push should take care of everything.

[MarkBaker]

Thank you for your work on this PR; it is appreciated.
I know that I migfht have been asking you to do more than you needed for your own use case, but that makes it more beneficial to other developers that may encounter the same issue

[fdjohnston]

Thanks for your patience working through the PR with me. I learned a couple new tricks with regular expressions. Happy to put in the extra work to improve PHPSpreadsheet. This is a fantastic library and I appreciate all the work that you and the other maintainers do to keep it up to date. Was happy to be able to contribute.