Skip to content

Commit

Permalink
Fix for Issue 2042 (SUM Partially Broken) (#2045)
Browse files Browse the repository at this point in the history
As issue #2042 documents, SUM behaves differently with invalid strings depending on whether they come from a cell or are used as literals in the formula. SUM is not alone in this regard; COUNTA is another function within this behavior, and the solution to this one is modeled on COUNTA. New tests are added for SUM, and the resulting tests are duplicated to confirm correct behavior for both cells and literals.

Samples 16 (CSV), 17 (Html), and 21 (PDF) were adversely affected by this problem. 17 and 21 were immediately fixed, but 16 had another problem - Excel was not interpreting the UTF8 currency symbols correctly, even though the file was saved with a BOM. After some experimenting, it appears that the `sep=;` line generated by setExcelCompatibility(true) causes Excel to mis-handle the file. This seems like a bug - there is apparently no way to save a UTF-8 CSV with non-ASCII characters which specifies a non-standard separator which Excel will open correctly. I don't know if this is a recent change or if it is just the case that nobody noticed this problem till now. So, I changed Sample 16 to use setUseBom rather than setExcelCompatibility, which solved its problem. I then added new tests for setExcelCompatibility, with documentation of this problem.
  • Loading branch information
oleibman authored May 3, 2021
1 parent fd14da1 commit 346bad1
Show file tree
Hide file tree
Showing 6 changed files with 92 additions and 4 deletions.
3 changes: 2 additions & 1 deletion samples/Basic/16_Csv.php
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,8 @@
$filenameCSV = $helper->getFilename(__FILE__, 'csv');
/** @var \PhpOffice\PhpSpreadsheet\Writer\Csv $writerCSV */
$writerCSV = new CsvWriter($spreadsheetFromCSV);
$writerCSV->setExcelCompatibility(true);
//$writerCSV->setExcelCompatibility(true);
$writerCSV->setUseBom(true); // because of non-ASCII chars

$callStartTime = microtime(true);
$writerCSV->save($filenameCSV);
Expand Down
8 changes: 6 additions & 2 deletions src/PhpSpreadsheet/Calculation/MathTrig/Sum.php
Original file line number Diff line number Diff line change
Expand Up @@ -51,16 +51,20 @@ public static function funcSumNoStrings(...$args)
{
$returnValue = 0;
// Loop through the arguments
foreach (Functions::flattenArray($args) as $arg) {
$aArgs = Functions::flattenArrayIndexed($args);
foreach ($aArgs as $k => $arg) {
// Is it a numeric value?
if (is_numeric($arg) || empty($arg)) {
if (is_string($arg)) {
$arg = (int) $arg;
}
$returnValue += $arg;
} elseif (is_bool($arg)) {
$returnValue += (int) $arg;
} elseif (Functions::isError($arg)) {
return $arg;
} else {
// ignore non-numerics from cell, but fail as literals (except null)
} elseif ($arg !== null && !Functions::isCellValue($k)) {
return Functions::VALUE();
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,4 +26,22 @@ public function providerSUM(): array
{
return require 'tests/data/Calculation/MathTrig/SUM.php';
}

/**
* @dataProvider providerSUMLiterals
*
* @param mixed $expectedResult
*/
public function testSUMLiterals($expectedResult, string $args): void
{
$sheet = $this->sheet;
$sheet->getCell('B1')->setValue("=SUM($args)");
$result = $sheet->getCell('B1')->getCalculatedValue();
self::assertEqualsWithDelta($expectedResult, $result, 1E-12);
}

public function providerSUMLiterals(): array
{
return require 'tests/data/Calculation/MathTrig/SUMLITERALS.php';
}
}
49 changes: 49 additions & 0 deletions tests/PhpSpreadsheetTests/Writer/Csv/CsvExcelCompatibilityTest.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
<?php

namespace PhpOffice\PhpSpreadsheetTests\Writer\Csv;

use PhpOffice\PhpSpreadsheet\Reader\Csv as CsvReader;
use PhpOffice\PhpSpreadsheet\Shared\File;
use PhpOffice\PhpSpreadsheet\Spreadsheet;
use PhpOffice\PhpSpreadsheet\Writer\Csv as CsvWriter;
use PhpOffice\PhpSpreadsheetTests\Functional;

class CsvExcelCompatibilityTest extends Functional\AbstractFunctional
{
// Excel seems to have changed with how they handle this.
// In particular, it does not recognize UTF-8 non-ASCII characters
// if a file is written with ExcelCompatibility on.
// The initial 'sep=;' line seems to confuse it, even though
// it has a BOM. The Unix "file" command also indicates a difference
// when the sep line is or is not included:
// UTF-8 Unicode (with BOM) text, with CRLF line terminators
// vs CSV text (without sep line, with or without BOM)
// So, this test has no UTF-8 yet while more research is conducted.
public function testExcelCompatibility(): void
{
$spreadsheet = new Spreadsheet();
$sheet = $spreadsheet->getActiveSheet();
$sheet->setCellValue('A1', '1');
$sheet->setCellValue('B1', '2');
$sheet->setCellValue('C1', '3');
$sheet->setCellValue('A2', '4');
$sheet->setCellValue('B2', '5');
$sheet->setCellValue('C2', '6');
$writer = new CsvWriter($spreadsheet);
$writer->setExcelCompatibility(true);
self::assertSame('', $writer->getOutputEncoding());
$filename = File::temporaryFilename();
$writer->save($filename);
$reader = new CsvReader();
$spreadsheet2 = $reader->load($filename);
$contents = file_get_contents($filename);
unlink($filename);
self::assertEquals(1, $spreadsheet2->getActiveSheet()->getCell('A1')->getValue());
self::assertEquals(6, $spreadsheet2->getActiveSheet()->getCell('C2')->getValue());
self::assertStringContainsString(CsvReader::UTF8_BOM, $contents);
self::assertStringContainsString("\r\n", $contents);
self::assertStringContainsString('sep=;', $contents);
self::assertStringContainsString('"1";"2";"3"', $contents);
self::assertStringContainsString('"4";"5";"6"', $contents);
}
}
6 changes: 5 additions & 1 deletion tests/data/Calculation/MathTrig/SUM.php
Original file line number Diff line number Diff line change
Expand Up @@ -4,5 +4,9 @@
[50, 5, 15, 30],
[52, 5, 15, 30, 2],
[53.1, 5.7, 15, 30, 2.4],
['#VALUE!', 5.7, 'X', 30, 2.4], // error here conflicts with SUMIF
[52.1, 5.7, '14', 30, 2.4],
[38.1, 5.7, 'X', 30, 2.4], // error if entered in formula, but not in cell
[38.1, 5.7, null, 30, 2.4],
[38.1, 5.7, false, 30, 2.4],
[39.1, 5.7, true, 30, 2.4],
];
12 changes: 12 additions & 0 deletions tests/data/Calculation/MathTrig/SUMLITERALS.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
<?php

return [
[50, '5, 15, 30'],
[52, '5, 15, 30, 2'],
[53.1, '5.7, 15, 30, 2.4'],
[52.1, '5.7, "14", 30, 2.4'],
['#VALUE!', '5.7, "X", 30, 2.4'], // error if entered in formula, but not in cell
[38.1, '5.7, , 30, 2.4'],
[38.1, '5.7, false, 30, 2.4'],
[39.1, '5.7, true, 30, 2.4'],
];

0 comments on commit 346bad1

Please sign in to comment.