From 1853aaac793e704a7d42cb36a12fdb9229f77db6 Mon Sep 17 00:00:00 2001 From: Mikkel Paulson Date: Fri, 14 Jul 2017 04:53:13 -0400 Subject: [PATCH] Add option to suppress validation of sheet titles (#186) Add option to suppress validation of sheet titles Based on a "lowest common denominator" approach to compatibility, we will continue to enforce a 31-character limit for sheet titles. However, this limit should not be enforced when loading an existing file. Added a new optional parameter to Worksheet::setTitle() and Worksheet::setCodeName() to suppress validation and massaging, based on the premise that existing files should be given a best-effort approach to loading and parsing. Unfortunately, it's not possible with the current architecture to prevent users from making use of this functionality, aside from with a strongly-worded warning. Added test coverage. I didn't see any existing unit tests of the Worksheet class, so I created a new test to cover these methods. Fixes #176 --- CHANGELOG.md | 1 + src/PhpSpreadsheet/Reader/Gnumeric.php | 2 +- src/PhpSpreadsheet/Reader/Html.php | 2 +- src/PhpSpreadsheet/Reader/Ods.php | 2 +- src/PhpSpreadsheet/Reader/Xls.php | 2 +- src/PhpSpreadsheet/Reader/Xlsx.php | 4 +- src/PhpSpreadsheet/Reader/Xml.php | 2 +- src/PhpSpreadsheet/Worksheet.php | 116 +++++++++--------- tests/PhpSpreadsheetTests/WorksheetTest.php | 126 ++++++++++++++++++++ 9 files changed, 195 insertions(+), 62 deletions(-) create mode 100644 tests/PhpSpreadsheetTests/WorksheetTest.php diff --git a/CHANGELOG.md b/CHANGELOG.md index 8f2e317c..1a1f8773 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -23,6 +23,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/). - Fix to getCell() method when cell reference includes a worksheet reference - @MarkBaker - Ignore inlineStr type if formula element exists - @ncrypthic [#570](https://github.com/PHPOffice/PHPExcel/issues/570) - Excel 2007 Reader freezes because of conditional formatting - @rentalhost [#575](https://github.com/PHPOffice/PHPExcel/issues/575) +- Readers will now parse files containing worksheet titles over 31 characters [#176](https://github.com/PHPOffice/PhpSpreadsheet/pull/176) ### General diff --git a/src/PhpSpreadsheet/Reader/Gnumeric.php b/src/PhpSpreadsheet/Reader/Gnumeric.php index 313fcf00..e1bef14f 100644 --- a/src/PhpSpreadsheet/Reader/Gnumeric.php +++ b/src/PhpSpreadsheet/Reader/Gnumeric.php @@ -351,7 +351,7 @@ class Gnumeric extends BaseReader implements IReader // Use false for $updateFormulaCellReferences to prevent adjustment of worksheet references in formula // cells... during the load, all formulae should be correct, and we're simply bringing the worksheet // name in line with the formula, not the reverse - $spreadsheet->getActiveSheet()->setTitle($worksheetName, false); + $spreadsheet->getActiveSheet()->setTitle($worksheetName, false, false); if ((!$this->readDataOnly) && (isset($sheet->PrintInformation))) { if (isset($sheet->PrintInformation->Margins)) { diff --git a/src/PhpSpreadsheet/Reader/Html.php b/src/PhpSpreadsheet/Reader/Html.php index f77ef7a8..67dfa184 100644 --- a/src/PhpSpreadsheet/Reader/Html.php +++ b/src/PhpSpreadsheet/Reader/Html.php @@ -317,7 +317,7 @@ class Html extends BaseReader implements IReader break; case 'title': $this->processDomElement($child, $sheet, $row, $column, $cellContent); - $sheet->setTitle($cellContent); + $sheet->setTitle($cellContent, true, false); $cellContent = ''; break; case 'span': diff --git a/src/PhpSpreadsheet/Reader/Ods.php b/src/PhpSpreadsheet/Reader/Ods.php index e82d2d4e..d454a86d 100644 --- a/src/PhpSpreadsheet/Reader/Ods.php +++ b/src/PhpSpreadsheet/Reader/Ods.php @@ -440,7 +440,7 @@ class Ods extends BaseReader implements IReader // Use false for $updateFormulaCellReferences to prevent adjustment of worksheet references in // formula cells... during the load, all formulae should be correct, and we're simply // bringing the worksheet name in line with the formula, not the reverse - $spreadsheet->getActiveSheet()->setTitle($worksheetName, false); + $spreadsheet->getActiveSheet()->setTitle($worksheetName, false, false); } // Go through every child of table element diff --git a/src/PhpSpreadsheet/Reader/Xls.php b/src/PhpSpreadsheet/Reader/Xls.php index a7fe48e4..45798fbe 100644 --- a/src/PhpSpreadsheet/Reader/Xls.php +++ b/src/PhpSpreadsheet/Reader/Xls.php @@ -808,7 +808,7 @@ class Xls extends BaseReader implements IReader // Use false for $updateFormulaCellReferences to prevent adjustment of worksheet references in formula // cells... during the load, all formulae should be correct, and we're simply bringing the worksheet // name in line with the formula, not the reverse - $this->phpSheet->setTitle($sheet['name'], false); + $this->phpSheet->setTitle($sheet['name'], false, false); $this->phpSheet->setSheetState($sheet['sheetState']); $this->pos = $sheet['offset']; diff --git a/src/PhpSpreadsheet/Reader/Xlsx.php b/src/PhpSpreadsheet/Reader/Xlsx.php index 3d39016d..57af0a58 100644 --- a/src/PhpSpreadsheet/Reader/Xlsx.php +++ b/src/PhpSpreadsheet/Reader/Xlsx.php @@ -697,7 +697,7 @@ class Xlsx extends BaseReader implements IReader // references in formula cells... during the load, all formulae should be correct, // and we're simply bringing the worksheet name in line with the formula, not the // reverse - $docSheet->setTitle((string) $eleSheet['name'], false); + $docSheet->setTitle((string) $eleSheet['name'], false, false); $fileWorksheet = $worksheets[(string) self::getArrayItem($eleSheet->attributes('http://schemas.openxmlformats.org/officeDocument/2006/relationships'), 'id')]; $xmlSheet = simplexml_load_string( //~ http://schemas.openxmlformats.org/spreadsheetml/2006/main" @@ -766,7 +766,7 @@ class Xlsx extends BaseReader implements IReader } } if (isset($xmlSheet->sheetPr) && isset($xmlSheet->sheetPr['codeName'])) { - $docSheet->setCodeName((string) $xmlSheet->sheetPr['codeName']); + $docSheet->setCodeName((string) $xmlSheet->sheetPr['codeName'], false); } if (isset($xmlSheet->sheetPr) && isset($xmlSheet->sheetPr->outlinePr)) { if (isset($xmlSheet->sheetPr->outlinePr['summaryRight']) && diff --git a/src/PhpSpreadsheet/Reader/Xml.php b/src/PhpSpreadsheet/Reader/Xml.php index d7aff065..7c445fee 100644 --- a/src/PhpSpreadsheet/Reader/Xml.php +++ b/src/PhpSpreadsheet/Reader/Xml.php @@ -552,7 +552,7 @@ class Xml extends BaseReader implements IReader // Use false for $updateFormulaCellReferences to prevent adjustment of worksheet references in // formula cells... during the load, all formulae should be correct, and we're simply bringing // the worksheet name in line with the formula, not the reverse - $spreadsheet->getActiveSheet()->setTitle($worksheetName, false); + $spreadsheet->getActiveSheet()->setTitle($worksheetName, false, false); } $columnID = 'A'; diff --git a/src/PhpSpreadsheet/Worksheet.php b/src/PhpSpreadsheet/Worksheet.php index b2278e7d..8cca3606 100644 --- a/src/PhpSpreadsheet/Worksheet.php +++ b/src/PhpSpreadsheet/Worksheet.php @@ -825,52 +825,54 @@ class Worksheet implements IComparable * Set title. * * @param string $pValue String containing the dimension of this worksheet - * @param string $updateFormulaCellReferences boolean Flag indicating whether cell references in formulae should + * @param bool $updateFormulaCellReferences Flag indicating whether cell references in formulae should * be updated to reflect the new sheet name. * This should be left as the default true, unless you are * certain that no formula cells on any worksheet contain * references to this worksheet + * @param bool $validate False to skip validation of new title. WARNING: This should only be set + * at parse time (by Readers), where titles can be assumed to be valid. * * @return Worksheet */ - public function setTitle($pValue, $updateFormulaCellReferences = true) + public function setTitle($pValue, $updateFormulaCellReferences = true, $validate = true) { // Is this a 'rename' or not? if ($this->getTitle() == $pValue) { return $this; } - // Syntax check - self::checkSheetTitle($pValue); - // Old title $oldTitle = $this->getTitle(); - if ($this->parent) { - // Is there already such sheet name? - if ($this->parent->sheetNameExists($pValue)) { - // Use name, but append with lowest possible integer + if ($validate) { + // Syntax check + self::checkSheetTitle($pValue); - if (Shared\StringHelper::countCharacters($pValue) > 29) { - $pValue = Shared\StringHelper::substring($pValue, 0, 29); - } - $i = 1; - while ($this->parent->sheetNameExists($pValue . ' ' . $i)) { - ++$i; - if ($i == 10) { - if (Shared\StringHelper::countCharacters($pValue) > 28) { - $pValue = Shared\StringHelper::substring($pValue, 0, 28); - } - } elseif ($i == 100) { - if (Shared\StringHelper::countCharacters($pValue) > 27) { - $pValue = Shared\StringHelper::substring($pValue, 0, 27); + if ($this->parent) { + // Is there already such sheet name? + if ($this->parent->sheetNameExists($pValue)) { + // Use name, but append with lowest possible integer + + if (Shared\StringHelper::countCharacters($pValue) > 29) { + $pValue = Shared\StringHelper::substring($pValue, 0, 29); + } + $i = 1; + while ($this->parent->sheetNameExists($pValue . ' ' . $i)) { + ++$i; + if ($i == 10) { + if (Shared\StringHelper::countCharacters($pValue) > 28) { + $pValue = Shared\StringHelper::substring($pValue, 0, 28); + } + } elseif ($i == 100) { + if (Shared\StringHelper::countCharacters($pValue) > 27) { + $pValue = Shared\StringHelper::substring($pValue, 0, 27); + } } } + + $pValue .= " $i"; } - - $altTitle = $pValue . ' ' . $i; - - return $this->setTitle($altTitle, $updateFormulaCellReferences); } } @@ -2976,51 +2978,55 @@ class Worksheet implements IComparable /** * Define the code name of the sheet. * - * @param null|string Same rule as Title minus space not allowed (but, like Excel, change silently space to underscore) - * @param null|mixed $pValue + * @param string $pValue Same rule as Title minus space not allowed (but, like Excel, change + * silently space to underscore) + * @param bool $validate False to skip validation of new title. WARNING: This should only be set + * at parse time (by Readers), where titles can be assumed to be valid. * * @throws Exception * * @return objWorksheet */ - public function setCodeName($pValue) + public function setCodeName($pValue, $validate = true) { // Is this a 'rename' or not? if ($this->getCodeName() == $pValue) { return $this; } - $pValue = str_replace(' ', '_', $pValue); //Excel does this automatically without flinching, we are doing the same - // Syntax check - // throw an exception if not valid - self::checkSheetCodeName($pValue); - // We use the same code that setTitle to find a valid codeName else not using a space (Excel don't like) but a '_' + if ($validate) { + $pValue = str_replace(' ', '_', $pValue); //Excel does this automatically without flinching, we are doing the same - if ($this->getParent()) { - // Is there already such sheet name? - if ($this->getParent()->sheetCodeNameExists($pValue)) { - // Use name, but append with lowest possible integer + // Syntax check + // throw an exception if not valid + self::checkSheetCodeName($pValue); - if (Shared\StringHelper::countCharacters($pValue) > 29) { - $pValue = Shared\StringHelper::substring($pValue, 0, 29); - } - $i = 1; - while ($this->getParent()->sheetCodeNameExists($pValue . '_' . $i)) { - ++$i; - if ($i == 10) { - if (Shared\StringHelper::countCharacters($pValue) > 28) { - $pValue = Shared\StringHelper::substring($pValue, 0, 28); - } - } elseif ($i == 100) { - if (Shared\StringHelper::countCharacters($pValue) > 27) { - $pValue = Shared\StringHelper::substring($pValue, 0, 27); + // We use the same code that setTitle to find a valid codeName else not using a space (Excel don't like) but a '_' + + if ($this->getParent()) { + // Is there already such sheet name? + if ($this->getParent()->sheetCodeNameExists($pValue)) { + // Use name, but append with lowest possible integer + + if (Shared\StringHelper::countCharacters($pValue) > 29) { + $pValue = Shared\StringHelper::substring($pValue, 0, 29); + } + $i = 1; + while ($this->getParent()->sheetCodeNameExists($pValue . '_' . $i)) { + ++$i; + if ($i == 10) { + if (Shared\StringHelper::countCharacters($pValue) > 28) { + $pValue = Shared\StringHelper::substring($pValue, 0, 28); + } + } elseif ($i == 100) { + if (Shared\StringHelper::countCharacters($pValue) > 27) { + $pValue = Shared\StringHelper::substring($pValue, 0, 27); + } } } - } - $pValue = $pValue . '_' . $i; // ok, we have a valid name - //codeName is'nt used in formula : no need to call for an update - //return $this->setTitle($altTitle, $updateFormulaCellReferences); + $pValue = $pValue . '_' . $i; // ok, we have a valid name + } } } diff --git a/tests/PhpSpreadsheetTests/WorksheetTest.php b/tests/PhpSpreadsheetTests/WorksheetTest.php new file mode 100644 index 00000000..1a7389b0 --- /dev/null +++ b/tests/PhpSpreadsheetTests/WorksheetTest.php @@ -0,0 +1,126 @@ +setTitle($testTitle); + $this->assertSame($testTitle, $worksheet->getTitle()); + } + + public function setTitleInvalidProvider() + { + return [ + [str_repeat('a', 32), 'Maximum 31 characters allowed in sheet title.'], + ['invalid*title', 'Invalid character found in sheet title'], + ]; + } + + /** + * @param string $title + * @param string $expectMessage + * @dataProvider setTitleInvalidProvider + */ + public function testSetTitleInvalid($title, $expectMessage) + { + // First, test setting title with validation disabled -- should be successful + $worksheet = new Worksheet(); + $worksheet->setTitle($title, true, false); + + // Next, test again with validation enabled -- this time we should fail + $worksheet = new Worksheet(); + $this->expectException(\Exception::class); + $this->expectExceptionMessage($expectMessage); + $worksheet->setTitle($title); + } + + public function testSetTitleDuplicate() + { + // Create a Spreadsheet with three Worksheets (the first is created automatically) + $spreadsheet = new Spreadsheet(); + $spreadsheet->createSheet(); + $spreadsheet->createSheet(); + + // Set unique title -- should be unchanged + $sheet = $spreadsheet->getSheet(0); + $sheet->setTitle('Test Title'); + $this->assertSame('Test Title', $sheet->getTitle()); + + // Set duplicate title -- should have numeric suffix appended + $sheet = $spreadsheet->getSheet(1); + $sheet->setTitle('Test Title'); + $this->assertSame('Test Title 1', $sheet->getTitle()); + + // Set duplicate title with validation disabled -- should be unchanged + $sheet = $spreadsheet->getSheet(2); + $sheet->setTitle('Test Title', true, false); + $this->assertSame('Test Title', $sheet->getTitle()); + } + + public function testSetCodeName() + { + $testCodeName = str_repeat('a', 31); + + $worksheet = new Worksheet(); + $worksheet->setCodeName($testCodeName); + $this->assertSame($testCodeName, $worksheet->getCodeName()); + } + + public function setCodeNameInvalidProvider() + { + return [ + [str_repeat('a', 32), 'Maximum 31 characters allowed in sheet code name.'], + ['invalid*code*name', 'Invalid character found in sheet code name'], + ]; + } + + /** + * @param string $codeName + * @param string $expectMessage + * @dataProvider setCodeNameInvalidProvider + */ + public function testSetCodeNameInvalid($codeName, $expectMessage) + { + // First, test setting code name with validation disabled -- should be successful + $worksheet = new Worksheet(); + $worksheet->setCodeName($codeName, false); + + // Next, test again with validation enabled -- this time we should fail + $worksheet = new Worksheet(); + $this->expectException(\Exception::class); + $this->expectExceptionMessage($expectMessage); + $worksheet->setCodeName($codeName); + } + + public function testSetCodeNameDuplicate() + { + // Create a Spreadsheet with three Worksheets (the first is created automatically) + $spreadsheet = new Spreadsheet(); + $spreadsheet->createSheet(); + $spreadsheet->createSheet(); + + // Set unique code name -- should be massaged to Snake_Case + $sheet = $spreadsheet->getSheet(0); + $sheet->setCodeName('Test Code Name'); + $this->assertSame('Test_Code_Name', $sheet->getCodeName()); + + // Set duplicate code name -- should be massaged and have numeric suffix appended + $sheet = $spreadsheet->getSheet(1); + $sheet->setCodeName('Test Code Name'); + $this->assertSame('Test_Code_Name_1', $sheet->getCodeName()); + + // Set duplicate code name with validation disabled -- should be unchanged, and unmassaged + $sheet = $spreadsheet->getSheet(2); + $sheet->setCodeName('Test Code Name', false); + $this->assertSame('Test Code Name', $sheet->getCodeName()); + } +}