Xls Reader Handle MACCENTRALEUROPE With or Without Hyphen (#2213)

* Xls Reader Handle MACCENTRALEUROPE With or Without Hyphen Fixes issue #549 and SpartnerNL/Laravel-Excel#989 (which is the source of the new test file). Some systems accept MACCENTRALEUROPE as the name for the appropriate encoding, and some accept MAC-CENTRALEUROPE. I fortunately have access to at least one of each type, and have run the tests on each. CodePage.php has an array of translations from codepage number to string. I now allow the value to itself be an array; if so, the code will test each in turn to see if it can be used in iconv. I did not go fishing for other similar problems. If such show up, they can be dealt with in the same manner as this one. I don't really expect others, since this is a problem not merely for Xls, but, even then, it applies only to BIFF5 and earlier. I also moved XlsTest from Reader to Reader/Xls. * Cache Successful Result For Future Use Per suggestion from @MarkBaker
PHPOffice · Jul 12, 2021 · 8729a68 · 8729a68
1 parent 075cecd
commit 8729a68
Show file tree

Hide file tree

Showing 6 changed files with 76 additions and 21 deletions.
diff --git a/phpstan-baseline.neon b/phpstan-baseline.neon
@@ -3490,11 +3490,6 @@ parameters:
 			count: 1
 			path: src/PhpSpreadsheet/Settings.php
 
-		-
-			message: "#^Property PhpOffice\\\\PhpSpreadsheet\\\\Shared\\\\CodePage\\:\\:\\$pageArray has no typehint specified\\.$#"
-			count: 1
-			path: src/PhpSpreadsheet/Shared/CodePage.php
-
 		-
 			message: "#^Parameter \\#1 \\$dateValue of static method PhpOffice\\\\PhpSpreadsheet\\\\Shared\\\\Date\\:\\:timestampToExcel\\(\\) expects int, float\\|int\\|string given\\.$#"
 			count: 1
@@ -6525,16 +6520,6 @@ parameters:
 			count: 1
 			path: tests/PhpSpreadsheetTests/Reader/Security/XmlScannerTest.php
 
-		-
-			message: "#^Cannot call method getFormattedValue\\(\\) on PhpOffice\\\\PhpSpreadsheet\\\\Cell\\\\Cell\\|null\\.$#"
-			count: 1
-			path: tests/PhpSpreadsheetTests/Reader/XlsTest.php
-
-		-
-			message: "#^Cannot call method getCoordinate\\(\\) on PhpOffice\\\\PhpSpreadsheet\\\\Cell\\\\Cell\\|null\\.$#"
-			count: 1
-			path: tests/PhpSpreadsheetTests/Reader/XlsTest.php
-
 		-
 			message: "#^Method PhpOffice\\\\PhpSpreadsheetTests\\\\Reader\\\\Xlsx\\\\AutoFilterTest\\:\\:getWorksheetInstance\\(\\) has no return typehint specified\\.$#"
 			count: 1

diff --git a/src/PhpSpreadsheet/Shared/CodePage.php b/src/PhpSpreadsheet/Shared/CodePage.php
@@ -8,6 +8,7 @@ class CodePage
 {
     public const DEFAULT_CODE_PAGE = 'CP1252';
 
+    /** @var array */
     private static $pageArray = [
         0 => 'CP1252', //    CodePage is not always correctly set when the xls file was saved by Apple's Numbers program
         367 => 'ASCII', //    ASCII
@@ -56,7 +57,7 @@ class CodePage
         10010 => 'MACROMANIA', //    Macintosh Romania
         10017 => 'MACUKRAINE', //    Macintosh Ukraine
         10021 => 'MACTHAI', //    Macintosh Thai
-        10029 => 'MACCENTRALEUROPE', //    Macintosh Central Europe
+        10029 => ['MACCENTRALEUROPE', 'MAC-CENTRALEUROPE'], //    Macintosh Central Europe
         10079 => 'MACICELAND', //    Macintosh Icelandic
         10081 => 'MACTURKISH', //    Macintosh Turkish
         10082 => 'MACCROATIAN', //    Macintosh Croatian
@@ -65,6 +66,7 @@ class CodePage
         //32769 => 'unsupported', //    ANSI Latin I (BIFF2-BIFF3)
         65000 => 'UTF-7', //    Unicode (UTF-7)
         65001 => 'UTF-8', //    Unicode (UTF-8)
+        99999 => ['unsupported'], //    Unicode (UTF-8)
     ];
 
     public static function validate(string $codePage): bool
@@ -83,7 +85,20 @@ public static function validate(string $codePage): bool
     public static function numberToName(int $codePage): string
     {
         if (array_key_exists($codePage, self::$pageArray)) {
-            return self::$pageArray[$codePage];
+            $value = self::$pageArray[$codePage];
+            if (is_array($value)) {
+                foreach ($value as $encoding) {
+                    if (@iconv('UTF-8', $encoding, ' ') !== false) {
+                        self::$pageArray[$codePage] = $encoding;
+
+                        return $encoding;
+                    }
+                }
+
+                throw new PhpSpreadsheetException("Code page $codePage not implemented on this system.");
+            } else {
+                return $value;
+            }
         }
         if ($codePage == 720 || $codePage == 32769) {
             throw new PhpSpreadsheetException("Code page $codePage not supported."); //    OEM Arabic

diff --git a/tests/PhpSpreadsheetTests/Reader/XlsTest.php → ...hpSpreadsheetTests/Reader/Xls/XlsTest.php b/tests/PhpSpreadsheetTests/Reader/XlsTest.php → ...hpSpreadsheetTests/Reader/Xls/XlsTest.php
@@ -1,8 +1,10 @@
 <?php
 
-namespace PhpOffice\PhpSpreadsheetTests\Reader;
+namespace PhpOffice\PhpSpreadsheetTests\Reader\Xls;
 
+use PhpOffice\PhpSpreadsheet\Cell\Cell;
 use PhpOffice\PhpSpreadsheet\Reader\Xls;
+use PhpOffice\PhpSpreadsheet\Shared\CodePage;
 use PhpOffice\PhpSpreadsheetTests\Functional\AbstractFunctional;
 
 class XlsTest extends AbstractFunctional
@@ -16,6 +18,7 @@ public function testLoadXlsSample(): void
         $reader = new Xls();
         $spreadsheet = $reader->load($filename);
         self::assertEquals('Title', $spreadsheet->getSheet(0)->getCell('A1')->getValue());
+        $spreadsheet->disconnectWorksheets();
     }
 
     /**
@@ -42,6 +45,8 @@ public function testLoadXlsBug1505(): void
         self::assertEquals($row, $newrow);
         self::assertEquals($sheet->getCell('A1')->getFormattedValue(), $newsheet->getCell('A1')->getFormattedValue());
         self::assertEquals($sheet->getCell("$col$row")->getFormattedValue(), $newsheet->getCell("$col$row")->getFormattedValue());
+        $spreadsheet->disconnectWorksheets();
+        $newspreadsheet->disconnectWorksheets();
     }
 
     /**
@@ -71,11 +76,49 @@ public function testLoadXlsBug1592(): void
         $rowIterator = $sheet->getRowIterator();
 
         foreach ($rowIterator as $row) {
-            foreach ($row->getCellIterator() as $cell) {
+            foreach ($row->getCellIterator() as $cellx) {
+                /** @var Cell */
+                $cell = $cellx;
                 $valOld = $cell->getFormattedValue();
                 $valNew = $newsheet->getCell($cell->getCoordinate())->getFormattedValue();
                 self::assertEquals($valOld, $valNew);
             }
         }
+        $spreadsheet->disconnectWorksheets();
+        $newspreadsheet->disconnectWorksheets();
+    }
+
+    /**
+     * Test load Xls file with MACCENTRALEUROPE encoding, which is implemented
+     * as MAC-CENTRALEUROPE on some systems. Issue #549.
+     */
+    public function testLoadMacCentralEurope(): void
+    {
+        $codePages = CodePage::getEncodings();
+        self::assertIsArray($codePages[10029]);
+        $filename = 'tests/data/Reader/XLS/maccentraleurope.xls';
+        $reader = new Xls();
+        // When no fix applied, spreadsheet fails to load on some systems
+        $spreadsheet = $reader->load($filename);
+        $sheet = $spreadsheet->getActiveSheet();
+        self::assertSame('Ładowność', $sheet->getCell('I1')->getValue());
+        $spreadsheet->disconnectWorksheets();
+    }
+
+    /**
+     * First test changes array entry in CodePage.
+     * This test confirms new that new entry is okay.
+     */
+    public function testLoadMacCentralEurope2(): void
+    {
+        $codePages = CodePage::getEncodings();
+        self::assertIsString($codePages[10029]);
+        $filename = 'tests/data/Reader/XLS/maccentraleurope.xls';
+        $reader = new Xls();
+        // When no fix applied, spreadsheet fails to load on some systems
+        $spreadsheet = $reader->load($filename);
+        $sheet = $spreadsheet->getActiveSheet();
+        self::assertSame('Ładowność', $sheet->getCell('I1')->getValue());
+        $spreadsheet->disconnectWorksheets();
     }
 }
diff --git a/tests/PhpSpreadsheetTests/Shared/CodePageTest.php b/tests/PhpSpreadsheetTests/Shared/CodePageTest.php
@@ -16,8 +16,15 @@ class CodePageTest extends TestCase
      */
     public function testCodePageNumberToName($expectedResult, $codePageIndex): void
     {
+        if ($expectedResult === 'exception') {
+            $this->expectException(Exception::class);
+        }
         $result = CodePage::numberToName($codePageIndex);
-        self::assertEquals($expectedResult, $result);
+        if (is_array($expectedResult)) {
+            self::assertContains($result, $expectedResult);
+        } else {
+            self::assertEquals($expectedResult, $result);
+        }
     }
 
     public function providerCodePage(): array

diff --git a/tests/data/Reader/XLS/maccentraleurope.xls b/tests/data/Reader/XLS/maccentraleurope.xls
diff --git a/tests/data/Shared/CodePage.php b/tests/data/Shared/CodePage.php
@@ -233,7 +233,7 @@
     ],
     // Macintosh Central Europe
     [
-        'MACCENTRALEUROPE',
+        ['MACCENTRALEUROPE', 'MAC-CENTRALEUROPE'],
         10029,
     ],
     // Macintosh Icelandic
@@ -271,4 +271,9 @@
         'UTF-8',
         65001,
     ],
+    // invalid
+    [
+        'exception',
+        99999,
+    ],
 ];