Skip to content

Commit fe969f5

Browse files
author
Mark Baker
authored
Merge pull request #2627 from PHPOffice/Improve-Translation-Separator-Conflict
Resolve `translateSeparator()` method to handle separators cleanly Some refactoring of the Ods Reader classes to eliminate code duplication
2 parents e6047cf + b31d42c commit fe969f5

File tree

9 files changed

+169
-127
lines changed

9 files changed

+169
-127
lines changed

‎CHANGELOG.md

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@ and this project adheres to [Semantic Versioning](https://semver.org).
1515

1616
- Gnumeric Reader now loads number formatting for cells.
1717
- Gnumeric Reader now correctly identifies selected worksheet.
18+
- Some Refactoring of the Ods Reader, moving all formula and address translation from Ods to Excel into a separate class to eliminate code duplication and ensure consistency.
1819

1920
### Deprecated
2021

@@ -27,6 +28,12 @@ and this project adheres to [Semantic Versioning](https://semver.org).
2728
### Fixed
2829

2930
- Fixed behaviour of XLSX font style vertical align settings
31+
- Resolved formula translations to handle separators (row and column) for array functions as well as for function argument separators; and cleanly handle nesting levels.
32+
33+
Note that this method is used when translating Excel functions between en and other locale languages, as well as when converting formulae between different spreadsheet formats (e.g. Ods to Excel).
34+
35+
Nor is this a perfect solution, as there may still be issues when function calls have array arguments that themselves contain function calls; but it's still better than the current logic.
36+
3037

3138
## 1.22.0 - 2022-02-18
3239

‎phpstan-baseline.neon

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -162,7 +162,7 @@ parameters:
162162

163163
-
164164
message: "#^Parameter \\#3 \\$formula of static method PhpOffice\\\\PhpSpreadsheet\\\\Calculation\\\\Calculation\\:\\:translateSeparator\\(\\) expects string, string\\|null given\\.$#"
165-
count: 2
165+
count: 1
166166
path: src/PhpSpreadsheet/Calculation/Calculation.php
167167

168168
-

‎src/PhpSpreadsheet/Calculation/Calculation.php

Lines changed: 59 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -50,8 +50,10 @@ class Calculation
5050
const RETURN_ARRAY_AS_VALUE = 'value';
5151
const RETURN_ARRAY_AS_ARRAY = 'array';
5252

53-
const FORMULA_OPEN_FUNCTION_BRACE = '{';
54-
const FORMULA_CLOSE_FUNCTION_BRACE = '}';
53+
const FORMULA_OPEN_FUNCTION_BRACE = '(';
54+
const FORMULA_CLOSE_FUNCTION_BRACE = ')';
55+
const FORMULA_OPEN_MATRIX_BRACE = '{';
56+
const FORMULA_CLOSE_MATRIX_BRACE = '}';
5557
const FORMULA_STRING_QUOTE = '"';
5658

5759
private static $returnArrayAsType = self::RETURN_ARRAY_AS_VALUE;
@@ -3084,30 +3086,28 @@ public function setLocale(string $locale)
30843086
return false;
30853087
}
30863088

3087-
/**
3088-
* @param string $fromSeparator
3089-
* @param string $toSeparator
3090-
* @param string $formula
3091-
* @param bool $inBraces
3092-
*
3093-
* @return string
3094-
*/
3095-
public static function translateSeparator($fromSeparator, $toSeparator, $formula, &$inBraces)
3096-
{
3089+
public static function translateSeparator(
3090+
string $fromSeparator,
3091+
string $toSeparator,
3092+
string $formula,
3093+
int &$inBracesLevel,
3094+
string $openBrace = self::FORMULA_OPEN_FUNCTION_BRACE,
3095+
string $closeBrace = self::FORMULA_CLOSE_FUNCTION_BRACE
3096+
): string {
30973097
$strlen = mb_strlen($formula);
30983098
for ($i = 0; $i < $strlen; ++$i) {
30993099
$chr = mb_substr($formula, $i, 1);
31003100
switch ($chr) {
3101-
case self::FORMULA_OPEN_FUNCTION_BRACE:
3102-
$inBraces = true;
3101+
case $openBrace:
3102+
++$inBracesLevel;
31033103

31043104
break;
3105-
case self::FORMULA_CLOSE_FUNCTION_BRACE:
3106-
$inBraces = false;
3105+
case $closeBrace:
3106+
--$inBracesLevel;
31073107

31083108
break;
31093109
case $fromSeparator:
3110-
if (!$inBraces) {
3110+
if ($inBracesLevel > 0) {
31113111
$formula = mb_substr($formula, 0, $i) . $toSeparator . mb_substr($formula, $i + 1);
31123112
}
31133113
}
@@ -3116,40 +3116,55 @@ public static function translateSeparator($fromSeparator, $toSeparator, $formula
31163116
return $formula;
31173117
}
31183118

3119-
/**
3120-
* @param string[] $from
3121-
* @param string[] $to
3122-
* @param string $formula
3123-
* @param string $fromSeparator
3124-
* @param string $toSeparator
3125-
*
3126-
* @return string
3127-
*/
3128-
private static function translateFormula(array $from, array $to, $formula, $fromSeparator, $toSeparator)
3119+
private static function translateFormulaBlock(
3120+
array $from,
3121+
array $to,
3122+
string $formula,
3123+
int &$inFunctionBracesLevel,
3124+
int &$inMatrixBracesLevel,
3125+
string $fromSeparator,
3126+
string $toSeparator
3127+
): string {
3128+
// Function Names
3129+
$formula = preg_replace($from, $to, $formula);
3130+
3131+
// Temporarily adjust matrix separators so that they won't be confused with function arguments
3132+
$formula = self::translateSeparator(';', '|', $formula, $inMatrixBracesLevel, self::FORMULA_OPEN_MATRIX_BRACE, self::FORMULA_CLOSE_MATRIX_BRACE);
3133+
$formula = self::translateSeparator(',', '!', $formula, $inMatrixBracesLevel, self::FORMULA_OPEN_MATRIX_BRACE, self::FORMULA_CLOSE_MATRIX_BRACE);
3134+
// Function Argument Separators
3135+
$formula = self::translateSeparator($fromSeparator, $toSeparator, $formula, $inFunctionBracesLevel);
3136+
// Restore matrix separators
3137+
$formula = self::translateSeparator('|', ';', $formula, $inMatrixBracesLevel, self::FORMULA_OPEN_MATRIX_BRACE, self::FORMULA_CLOSE_MATRIX_BRACE);
3138+
$formula = self::translateSeparator('!', ',', $formula, $inMatrixBracesLevel, self::FORMULA_OPEN_MATRIX_BRACE, self::FORMULA_CLOSE_MATRIX_BRACE);
3139+
3140+
return $formula;
3141+
}
3142+
3143+
private static function translateFormula(array $from, array $to, string $formula, string $fromSeparator, string $toSeparator): string
31293144
{
3130-
// Convert any Excel function names to the required language
3145+
// Convert any Excel function names and constant names to the required language;
3146+
// and adjust function argument separators
31313147
if (self::$localeLanguage !== 'en_us') {
3132-
$inBraces = false;
3133-
// If there is the possibility of braces within a quoted string, then we don't treat those as matrix indicators
3148+
$inFunctionBracesLevel = 0;
3149+
$inMatrixBracesLevel = 0;
3150+
// If there is the possibility of separators within a quoted string, then we treat them as literals
31343151
if (strpos($formula, self::FORMULA_STRING_QUOTE) !== false) {
3135-
// So instead we skip replacing in any quoted strings by only replacing in every other array element after we've exploded
3136-
// the formula
3152+
// So instead we skip replacing in any quoted strings by only replacing in every other array element
3153+
// after we've exploded the formula
31373154
$temp = explode(self::FORMULA_STRING_QUOTE, $formula);
31383155
$i = false;
31393156
foreach ($temp as &$value) {
3140-
// Only count/replace in alternating array entries
3157+
// Only adjust in alternating array entries
31413158
if ($i = !$i) {
3142-
$value = preg_replace($from, $to, $value);
3143-
$value = self::translateSeparator($fromSeparator, $toSeparator, $value, $inBraces);
3159+
$value = self::translateFormulaBlock($from, $to, $value, $inFunctionBracesLevel, $inMatrixBracesLevel, $fromSeparator, $toSeparator);
31443160
}
31453161
}
31463162
unset($value);
31473163
// Then rebuild the formula string
31483164
$formula = implode(self::FORMULA_STRING_QUOTE, $temp);
31493165
} else {
31503166
// If there's no quoted strings, then we do a simple count/replace
3151-
$formula = preg_replace($from, $to, $formula);
3152-
$formula = self::translateSeparator($fromSeparator, $toSeparator, $formula, $inBraces);
3167+
$formula = self::translateFormulaBlock($from, $to, $formula, $inFunctionBracesLevel, $inMatrixBracesLevel, $fromSeparator, $toSeparator);
31533168
}
31543169
}
31553170

@@ -3162,6 +3177,7 @@ private static function translateFormula(array $from, array $to, $formula, $from
31623177

31633178
public function _translateFormulaToLocale($formula)
31643179
{
3180+
// Build list of function names and constants for translation
31653181
if (self::$functionReplaceFromExcel === null) {
31663182
self::$functionReplaceFromExcel = [];
31673183
foreach (array_keys(self::$localeFunctions) as $excelFunctionName) {
@@ -3798,11 +3814,11 @@ private function showTypeDetails($value)
37983814
*/
37993815
private function convertMatrixReferences($formula)
38003816
{
3801-
static $matrixReplaceFrom = [self::FORMULA_OPEN_FUNCTION_BRACE, ';', self::FORMULA_CLOSE_FUNCTION_BRACE];
3817+
static $matrixReplaceFrom = [self::FORMULA_OPEN_MATRIX_BRACE, ';', self::FORMULA_CLOSE_MATRIX_BRACE];
38023818
static $matrixReplaceTo = ['MKMATRIX(MKMATRIX(', '),MKMATRIX(', '))'];
38033819

38043820
// Convert any Excel matrix references to the MKMATRIX() function
3805-
if (strpos($formula, self::FORMULA_OPEN_FUNCTION_BRACE) !== false) {
3821+
if (strpos($formula, self::FORMULA_OPEN_MATRIX_BRACE) !== false) {
38063822
// If there is the possibility of braces within a quoted string, then we don't treat those as matrix indicators
38073823
if (strpos($formula, self::FORMULA_STRING_QUOTE) !== false) {
38083824
// So instead we skip replacing in any quoted strings by only replacing in every other array element after we've exploded
@@ -3814,8 +3830,8 @@ private function convertMatrixReferences($formula)
38143830
foreach ($temp as &$value) {
38153831
// Only count/replace in alternating array entries
38163832
if ($i = !$i) {
3817-
$openCount += substr_count($value, self::FORMULA_OPEN_FUNCTION_BRACE);
3818-
$closeCount += substr_count($value, self::FORMULA_CLOSE_FUNCTION_BRACE);
3833+
$openCount += substr_count($value, self::FORMULA_OPEN_MATRIX_BRACE);
3834+
$closeCount += substr_count($value, self::FORMULA_CLOSE_MATRIX_BRACE);
38193835
$value = str_replace($matrixReplaceFrom, $matrixReplaceTo, $value);
38203836
}
38213837
}
@@ -3824,8 +3840,8 @@ private function convertMatrixReferences($formula)
38243840
$formula = implode(self::FORMULA_STRING_QUOTE, $temp);
38253841
} else {
38263842
// If there's no quoted strings, then we do a simple count/replace
3827-
$openCount = substr_count($formula, self::FORMULA_OPEN_FUNCTION_BRACE);
3828-
$closeCount = substr_count($formula, self::FORMULA_CLOSE_FUNCTION_BRACE);
3843+
$openCount = substr_count($formula, self::FORMULA_OPEN_MATRIX_BRACE);
3844+
$closeCount = substr_count($formula, self::FORMULA_CLOSE_MATRIX_BRACE);
38293845
$formula = str_replace($matrixReplaceFrom, $matrixReplaceTo, $formula);
38303846
}
38313847
// Trap for mismatched braces and trigger an appropriate error

‎src/PhpSpreadsheet/Reader/Ods.php

Lines changed: 34 additions & 51 deletions
Original file line numberDiff line numberDiff line change
@@ -6,11 +6,11 @@
66
use DOMDocument;
77
use DOMElement;
88
use DOMNode;
9-
use PhpOffice\PhpSpreadsheet\Calculation\Calculation;
109
use PhpOffice\PhpSpreadsheet\Cell\Coordinate;
1110
use PhpOffice\PhpSpreadsheet\Cell\DataType;
1211
use PhpOffice\PhpSpreadsheet\Reader\Ods\AutoFilter;
1312
use PhpOffice\PhpSpreadsheet\Reader\Ods\DefinedNames;
13+
use PhpOffice\PhpSpreadsheet\Reader\Ods\FormulaTranslator;
1414
use PhpOffice\PhpSpreadsheet\Reader\Ods\PageSettings;
1515
use PhpOffice\PhpSpreadsheet\Reader\Ods\Properties as DocumentProperties;
1616
use PhpOffice\PhpSpreadsheet\Reader\Security\XmlScanner;
@@ -512,7 +512,7 @@ public function loadIntoExisting($filename, Spreadsheet $spreadsheet)
512512
if ($hasCalculatedValue) {
513513
$type = DataType::TYPE_FORMULA;
514514
$cellDataFormula = substr($cellDataFormula, strpos($cellDataFormula, ':=') + 1);
515-
$cellDataFormula = $this->convertToExcelFormulaValue($cellDataFormula);
515+
$cellDataFormula = FormulaTranslator::convertToExcelFormulaValue($cellDataFormula);
516516
}
517517

518518
if ($cellData->hasAttributeNS($tableNs, 'number-columns-repeated')) {
@@ -568,31 +568,7 @@ public function loadIntoExisting($filename, Spreadsheet $spreadsheet)
568568
}
569569

570570
// Merged cells
571-
if (
572-
$cellData->hasAttributeNS($tableNs, 'number-columns-spanned')
573-
|| $cellData->hasAttributeNS($tableNs, 'number-rows-spanned')
574-
) {
575-
if (($type !== DataType::TYPE_NULL) || (!$this->readDataOnly)) {
576-
$columnTo = $columnID;
577-
578-
if ($cellData->hasAttributeNS($tableNs, 'number-columns-spanned')) {
579-
$columnIndex = Coordinate::columnIndexFromString($columnID);
580-
$columnIndex += (int) $cellData->getAttributeNS($tableNs, 'number-columns-spanned');
581-
$columnIndex -= 2;
582-
583-
$columnTo = Coordinate::stringFromColumnIndex($columnIndex + 1);
584-
}
585-
586-
$rowTo = $rowID;
587-
588-
if ($cellData->hasAttributeNS($tableNs, 'number-rows-spanned')) {
589-
$rowTo = $rowTo + (int) $cellData->getAttributeNS($tableNs, 'number-rows-spanned') - 1;
590-
}
591-
592-
$cellRange = $columnID . $rowID . ':' . $columnTo . $rowTo;
593-
$spreadsheet->getActiveSheet()->mergeCells($cellRange);
594-
}
595-
}
571+
$this->processMergedCells($cellData, $tableNs, $type, $columnID, $rowID, $spreadsheet);
596572

597573
++$columnID;
598574
}
@@ -735,31 +711,38 @@ private function parseRichText($is)
735711
return $value;
736712
}
737713

738-
private function convertToExcelFormulaValue(string $openOfficeFormula): string
739-
{
740-
$temp = explode('"', $openOfficeFormula);
741-
$tKey = false;
742-
foreach ($temp as &$value) {
743-
// Only replace in alternate array entries (i.e. non-quoted blocks)
744-
if ($tKey = !$tKey) {
745-
// Cell range reference in another sheet
746-
$value = preg_replace('/\[\$?([^\.]+)\.([^\.]+):\.([^\.]+)\]/miu', '$1!$2:$3', $value);
747-
// Cell reference in another sheet
748-
$value = preg_replace('/\[\$?([^\.]+)\.([^\.]+)\]/miu', '$1!$2', $value ?? '');
749-
// Cell range reference
750-
$value = preg_replace('/\[\.([^\.]+):\.([^\.]+)\]/miu', '$1:$2', $value ?? '');
751-
// Simple cell reference
752-
$value = preg_replace('/\[\.([^\.]+)\]/miu', '$1', $value ?? '');
753-
// Convert references to defined names/formulae
754-
$value = str_replace('$$', '', $value ?? '');
755-
756-
$value = Calculation::translateSeparator(';', ',', $value, $inBraces);
757-
}
758-
}
714+
private function processMergedCells(
715+
DOMElement $cellData,
716+
string $tableNs,
717+
string $type,
718+
string $columnID,
719+
int $rowID,
720+
Spreadsheet $spreadsheet
721+
): void {
722+
if (
723+
$cellData->hasAttributeNS($tableNs, 'number-columns-spanned')
724+
|| $cellData->hasAttributeNS($tableNs, 'number-rows-spanned')
725+
) {
726+
if (($type !== DataType::TYPE_NULL) || ($this->readDataOnly === false)) {
727+
$columnTo = $columnID;
728+
729+
if ($cellData->hasAttributeNS($tableNs, 'number-columns-spanned')) {
730+
$columnIndex = Coordinate::columnIndexFromString($columnID);
731+
$columnIndex += (int) $cellData->getAttributeNS($tableNs, 'number-columns-spanned');
732+
$columnIndex -= 2;
733+
734+
$columnTo = Coordinate::stringFromColumnIndex($columnIndex + 1);
735+
}
759736

760-
// Then rebuild the formula string
761-
$excelFormula = implode('"', $temp);
737+
$rowTo = $rowID;
762738

763-
return $excelFormula;
739+
if ($cellData->hasAttributeNS($tableNs, 'number-rows-spanned')) {
740+
$rowTo = $rowTo + (int) $cellData->getAttributeNS($tableNs, 'number-rows-spanned') - 1;
741+
}
742+
743+
$cellRange = $columnID . $rowID . ':' . $columnTo . $rowTo;
744+
$spreadsheet->getActiveSheet()->mergeCells($cellRange);
745+
}
746+
}
764747
}
765748
}

‎src/PhpSpreadsheet/Reader/Ods/AutoFilter.php

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@
55
use DOMElement;
66
use DOMNode;
77

8-
class AutoFilter extends BaseReader
8+
class AutoFilter extends BaseLoader
99
{
1010
public function read(DOMElement $workbookData): void
1111
{
@@ -20,7 +20,7 @@ protected function readAutoFilters(DOMElement $workbookData): void
2020
foreach ($autofilters->childNodes as $autofilter) {
2121
$autofilterRange = $this->getAttributeValue($autofilter, 'target-range-address');
2222
if ($autofilterRange !== null) {
23-
$baseAddress = $this->convertToExcelAddressValue($autofilterRange);
23+
$baseAddress = FormulaTranslator::convertToExcelAddressValue($autofilterRange);
2424
$this->spreadsheet->getActiveSheet()->setAutoFilter($baseAddress);
2525
}
2626
}

‎src/PhpSpreadsheet/Reader/Ods/BaseLoader.php

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,27 @@
1+
<?php
2+
3+
namespace PhpOffice\PhpSpreadsheet\Reader\Ods;
4+
5+
use DOMElement;
6+
use PhpOffice\PhpSpreadsheet\Spreadsheet;
7+
8+
abstract class BaseLoader
9+
{
10+
/**
11+
* @var Spreadsheet
12+
*/
13+
protected $spreadsheet;
14+
15+
/**
16+
* @var string
17+
*/
18+
protected $tableNs;
19+
20+
public function __construct(Spreadsheet $spreadsheet, string $tableNs)
21+
{
22+
$this->spreadsheet = $spreadsheet;
23+
$this->tableNs = $tableNs;
24+
}
25+
26+
abstract public function read(DOMElement $workbookData): void;
27+
}

0 commit comments

Comments
 (0)