[Bug] Percentages stored as strings are not converted during formula calculations as they are in Excel

[fdjohnston]

This is:

- [x] a bug report
- [ ] a feature request
- [x] **not** a usage question (ask them on https://stackoverflow.com/questions/tagged/phpspreadsheet or https://gitter.im/PHPOffice/PhpSpreadsheet)

What is the expected behavior?

When converting a spreadsheet to an array using toArray(), formulas that reference a cell that contains a percentage stored as a string should still calculate, like they do in Excel.

What is the current behavior?

PHPSpreadsheet returns a #VALUE! error instead of converting the string to a float.

What are the steps to reproduce?

<?php

require __DIR__ . '/vendor/autoload.php';

//Create reader
$xlsxReader = \PhpOffice\PhpSpreadsheet\IOFactory::createReader('Xlsx');
$xlsxReader->setReadDataOnly(true);

//Load worksheet
$workbook = $xlsxReader->load('Book1.xlsx');
$worksheet = $workbook->getSheetByName('Sheet1');

//Convert to array
$data = $worksheet->toArray(null, true, false, false);

var_dump($data);
die();

Output:

array(2) {
  [0]=>
  array(2) {
    [0]=>
    string(7) "#VALUE!"
    [1]=>
    NULL
  }
  [1]=>
  array(2) {
    [0]=>
    int(100)
    [1]=>
    string(2) "2%"
  }
}

Expected output:

array(2) {
  [0]=>
    [0]=>
    float(2)
    [1]=>
    NULL
  }
  [1]=>
  array(2) {
    [0]=>
    int(100)
    [1]=>
    string(2) "2%"
  }
}

I have attached an XLSX test file.
Book1.xlsx

What features do you think are causing the issue

• Reader
• Writer
• Styles
• Data Validations
• Formula Calculations
• Charts
• AutoFilter
• Form Elements

The validateBinaryOperand() function in src/PhpSpreadsheet/Calculation/Calculation.php already supports converting fractions stored as strings to numbers. I think adding a function here to convert percentages stored as strings to numbers could solve this problem.

Does an issue affect all spreadsheet file formats? If not, which formats are affected?

Only tested on XLSX files, but based on my reading of the code this will likely affect all spreadsheet formats.

Which versions of PhpSpreadsheet and PHP are affected?

PHPSpreadsheet 1.25.2

I would be happy to submit a PR to address this issue if the maintainers agree it is a bug and should be fixed.

[MarkBaker]

Yes, this is a "bug", but it's a lot more complicated once you start to look at it in detail.

This applies to a lot more than just percentages: Excel also behaves that way with strings containing currencies, dates/times, fractions, etc. And for float values, it's locale-aware. Nor is it completely consistent. And it's also incredibly complicated, and a performance overhead when it's necessary to test every string value encountered in a calculation to see if it could match an Excel number format, and to convert it to a number if it can be converted.

It's also not something that occurs frequently, so I made a decision many years ago that I wouldn't try implementing the logic for it: given the complexity, and the more frequent requests to improve performance, add new features, improve handline for Excel functions, add charts and tables, support more file formats, etc, so those changes have always had priority.
In 16 years of PHPExcel and PhpSpreadsheet, you're the first person to notice.

[MarkBaker]

Feel free to submit a PR, but the logic for handling this in validateBinaryOperand() should probably be factored out into a dedicated helper class (perhaps in Calculation\Engine), the logic for convertToNumberIfFraction() moved out from the Shared String Helper, which then makes it easier to build on that in the future.

[fdjohnston]

Thanks for the feedback, and all your work on PHPSpreadsheet. I didn't consider the other cases as this is the particular case that I was running into. I couldn't figure out why the values I was getting out of toArray() weren't lining up with what I was seeing in the spreadsheet. One of the rare times where Excel "magic" actually helps instead of randomly breaking things.
I wasn't trying to be critical when calling it a bug; I guess feature request might have been better. This is my first time trying to contribute to this project and I was trying to follow the Contribution guidelines, perhaps a little too carefully.

In the patch I've written I added a convertToNumberIfPercent() function to the shared string helper and chained it in the same elseif with convertToNumberIfFraction(). Probably not the ideal solution but it works for this case.

I'll submit the PR and if you have some thoughts on how it could be improved or refactored I'd love to hear them.