From 86a87e093bc2808836e08eb731012f25c2b899f1 Mon Sep 17 00:00:00 2001 From: oleibman <10341515+oleibman@users.noreply.github.com> Date: Sat, 14 Dec 2024 12:40:56 -0800 Subject: [PATCH 1/7] Unexpected Charset Possible for Currency Symbol We do not recommend it, but users can call the Php function `setlocale` and that might affect the character used as currency symbol. A problem arises when the caller to setlocale does not specify a character set - in that case, Php will attempt to return its `localeconv()` values in a single-byte character set rather than UTF-8. This is particularly problematic for currency symbols. PhpSpreadsheet till now has accepted such a character, and that can lead to corrupt spreadsheets. It is changed to validate the currency symbol as UTF-8, and fall back to a different choice if not (e.g. EUR rather than 0x80, which is how the euro symbol is depicted in Win-1252). An additional problem arises because Linux systems seem to return the alternate symbol with a trailing blank, but Windows systems do not. To allow callers to get a consistent result, a parameter is added to `getCurrencyCode` which will trim or not (default) the currency code. --- src/PhpSpreadsheet/Shared/StringHelper.php | 59 ++++++++----------- .../Reader/Csv/CsvNumberFormatLocaleTest.php | 10 ++-- .../Shared/StringHelperLocaleTest.php | 56 ++++++++++++++++++ 3 files changed, 87 insertions(+), 38 deletions(-) create mode 100644 tests/PhpSpreadsheetTests/Shared/StringHelperLocaleTest.php diff --git a/src/PhpSpreadsheet/Shared/StringHelper.php b/src/PhpSpreadsheet/Shared/StringHelper.php index aac3836ce4..a16b1cafc0 100644 --- a/src/PhpSpreadsheet/Shared/StringHelper.php +++ b/src/PhpSpreadsheet/Shared/StringHelper.php @@ -19,17 +19,17 @@ class StringHelper /** * Decimal separator. */ - private static ?string $decimalSeparator; + private static ?string $decimalSeparator = null; /** * Thousands separator. */ - private static ?string $thousandsSeparator; + private static ?string $thousandsSeparator = null; /** * Currency code. */ - private static ?string $currencyCode; + private static ?string $currencyCode = null; /** * Is iconv extension avalable? @@ -511,6 +511,23 @@ public static function strCaseReverse(string $textValue): string return implode('', $characters); } + private static function useAlt(string $altValue, string $default, bool $trimAlt): string + { + return ($trimAlt ? trim($altValue) : $altValue) ?: $default; + } + + private static function getLocaleValue(string $key, string $altKey, string $default, bool $trimAlt = false): string + { + $localeconv = localeconv(); + $rslt = $localeconv[$key]; + // win-1252 implements Euro as 0x80 plus other symbols + if (preg_match('//u', $rslt) !== 1) { + $rslt = ''; + } + + return $rslt ?: self::useAlt($localeconv[$altKey], $default, $trimAlt); + } + /** * Get the decimal separator. If it has not yet been set explicitly, try to obtain number * formatting information from locale. @@ -518,14 +535,7 @@ public static function strCaseReverse(string $textValue): string public static function getDecimalSeparator(): string { if (!isset(self::$decimalSeparator)) { - $localeconv = localeconv(); - self::$decimalSeparator = ($localeconv['decimal_point'] != '') - ? $localeconv['decimal_point'] : $localeconv['mon_decimal_point']; - - if (self::$decimalSeparator == '') { - // Default to . - self::$decimalSeparator = '.'; - } + self::$decimalSeparator = self::getLocaleValue('decimal_point', 'mon_decimal_point', '.'); } return self::$decimalSeparator; @@ -549,14 +559,7 @@ public static function setDecimalSeparator(?string $separator): void public static function getThousandsSeparator(): string { if (!isset(self::$thousandsSeparator)) { - $localeconv = localeconv(); - self::$thousandsSeparator = ($localeconv['thousands_sep'] != '') - ? $localeconv['thousands_sep'] : $localeconv['mon_thousands_sep']; - - if (self::$thousandsSeparator == '') { - // Default to . - self::$thousandsSeparator = ','; - } + self::$thousandsSeparator = self::getLocaleValue('thousands_sep', 'mon_thousands_sep', ','); } return self::$thousandsSeparator; @@ -577,22 +580,10 @@ public static function setThousandsSeparator(?string $separator): void * Get the currency code. If it has not yet been set explicitly, try to obtain the * symbol information from locale. */ - public static function getCurrencyCode(): string + public static function getCurrencyCode(bool $trimAlt = false): string { - if (!empty(self::$currencyCode)) { - return self::$currencyCode; - } - self::$currencyCode = '$'; - $localeconv = localeconv(); - if (!empty($localeconv['currency_symbol'])) { - self::$currencyCode = $localeconv['currency_symbol']; - - return self::$currencyCode; - } - if (!empty($localeconv['int_curr_symbol'])) { - self::$currencyCode = $localeconv['int_curr_symbol']; - - return self::$currencyCode; + if (!isset(self::$currencyCode)) { + self::$currencyCode = self::getLocaleValue('currency_symbol', 'int_curr_symbol', '$', $trimAlt); } return self::$currencyCode; diff --git a/tests/PhpSpreadsheetTests/Reader/Csv/CsvNumberFormatLocaleTest.php b/tests/PhpSpreadsheetTests/Reader/Csv/CsvNumberFormatLocaleTest.php index 513576b19b..086d780f53 100644 --- a/tests/PhpSpreadsheetTests/Reader/Csv/CsvNumberFormatLocaleTest.php +++ b/tests/PhpSpreadsheetTests/Reader/Csv/CsvNumberFormatLocaleTest.php @@ -6,8 +6,12 @@ use PhpOffice\PhpSpreadsheet\Cell\DataType; use PhpOffice\PhpSpreadsheet\Reader\Csv; +use PHPUnit\Framework\Attributes; +use PHPUnit\Framework\Attributes\DataProvider; use PHPUnit\Framework\TestCase; +// separate processes due to setLocale +#[Attributes\RunTestsInSeparateProcesses] class CsvNumberFormatLocaleTest extends TestCase { private bool $localeAdjusted; @@ -44,8 +48,7 @@ protected function tearDown(): void } } - #[\PHPUnit\Framework\Attributes\DataProvider('providerNumberFormatNoConversionTest')] - #[\PHPUnit\Framework\Attributes\RunInSeparateProcess] + #[DataProvider('providerNumberFormatNoConversionTest')] public function testNumberFormatNoConversion(mixed $expectedValue, string $expectedFormat, string $cellAddress): void { if (!$this->localeAdjusted) { @@ -85,8 +88,7 @@ public static function providerNumberFormatNoConversionTest(): array ]; } - #[\PHPUnit\Framework\Attributes\DataProvider('providerNumberValueConversionTest')] - #[\PHPUnit\Framework\Attributes\RunInSeparateProcess] + #[DataProvider('providerNumberValueConversionTest')] public function testNumberValueConversion(mixed $expectedValue, string $cellAddress): void { if (!$this->localeAdjusted) { diff --git a/tests/PhpSpreadsheetTests/Shared/StringHelperLocaleTest.php b/tests/PhpSpreadsheetTests/Shared/StringHelperLocaleTest.php new file mode 100644 index 0000000000..22d103aa20 --- /dev/null +++ b/tests/PhpSpreadsheetTests/Shared/StringHelperLocaleTest.php @@ -0,0 +1,56 @@ +currentLocale = setlocale(LC_ALL, '0'); + } + + protected function tearDown(): void + { + if (is_string($this->currentLocale)) { + setlocale(LC_ALL, $this->currentLocale); + } + StringHelper::setCurrencyCode(null); + } + + public function testCurrency(): void + { + if (!setlocale(LC_ALL, 'de_DE.UTF-8', 'deu_deu.utf8')) { + self::markTestSkipped('Unable to set German UTF8 locale for testing.'); + } + $result = StringHelper::getCurrencyCode(); + self::assertSame('€', $result); + if (!setlocale(LC_ALL, 'en_us')) { + self::markTestSkipped('Unable to set US locale for testing.'); + } + $result = StringHelper::getCurrencyCode(); + self::assertSame('€', $result, 'result persists despite locale change'); + StringHelper::setCurrencyCode(null); + $result = StringHelper::getCurrencyCode(); + self::assertSame('$', $result, 'locale now used'); + StringHelper::setCurrencyCode(null); + if (!setlocale(LC_ALL, 'deu_deu', 'de_DE')) { + self::markTestSkipped('Unable to set German single-byte locale for testing.'); + } + // Seems like Linux returns trailing blank, Win doesn't + $result = StringHelper::getCurrencyCode(true); // trim if alt symbol is used + self::assertSame('EUR', $result, 'non-UTF8 result ignored'); + } +} From ea9af88e49f360654e6220a2520259d304cb7a8d Mon Sep 17 00:00:00 2001 From: oleibman <10341515+oleibman@users.noreply.github.com> Date: Sun, 22 Dec 2024 23:45:07 -0800 Subject: [PATCH 2/7] Add Language Packs to Ubuntu Images --- .github/workflows/main.yml | 3 +++ 1 file changed, 3 insertions(+) diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index 8fb4f81978..01cbf16dc9 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -25,6 +25,9 @@ jobs: - name: Checkout uses: actions/checkout@v4 + - name: Install locales + run: sudo apt-get install -y language-pack-fr language-pack-de + - name: Setup PHP, with composer and extensions uses: shivammathur/setup-php@v2 with: From f4c576b9d4c896a3148a390c85100d7c23f768cf Mon Sep 17 00:00:00 2001 From: oleibman <10341515+oleibman@users.noreply.github.com> Date: Sun, 22 Dec 2024 23:55:04 -0800 Subject: [PATCH 3/7] Add English Language Pack --- .github/workflows/main.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index 01cbf16dc9..72148bd081 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -26,7 +26,7 @@ jobs: uses: actions/checkout@v4 - name: Install locales - run: sudo apt-get install -y language-pack-fr language-pack-de + run: sudo apt-get install -y language-pack-fr language-pack-de language-pack-en - name: Setup PHP, with composer and extensions uses: shivammathur/setup-php@v2 From 81440e711e74ac3eed9c5085cfdb0845ff39a332 Mon Sep 17 00:00:00 2001 From: oleibman <10341515+oleibman@users.noreply.github.com> Date: Mon, 23 Dec 2024 00:12:04 -0800 Subject: [PATCH 4/7] Use en_ca Rather than en_us Installing language-pack-en does not install en_US???? --- tests/PhpSpreadsheetTests/Shared/StringHelperLocaleTest.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/PhpSpreadsheetTests/Shared/StringHelperLocaleTest.php b/tests/PhpSpreadsheetTests/Shared/StringHelperLocaleTest.php index 22d103aa20..b9eaf9b7a8 100644 --- a/tests/PhpSpreadsheetTests/Shared/StringHelperLocaleTest.php +++ b/tests/PhpSpreadsheetTests/Shared/StringHelperLocaleTest.php @@ -37,8 +37,8 @@ public function testCurrency(): void } $result = StringHelper::getCurrencyCode(); self::assertSame('€', $result); - if (!setlocale(LC_ALL, 'en_us')) { - self::markTestSkipped('Unable to set US locale for testing.'); + if (!setlocale(LC_ALL, 'en_ca', 'en_CA')) { + self::markTestSkipped('Unable to set en_ca locale for testing.'); } $result = StringHelper::getCurrencyCode(); self::assertSame('€', $result, 'result persists despite locale change'); From eed339fb9c06b2f838197389441cf46bb3ad78e5 Mon Sep 17 00:00:00 2001 From: oleibman <10341515+oleibman@users.noreply.github.com> Date: Mon, 23 Dec 2024 00:20:16 -0800 Subject: [PATCH 5/7] Try Default Locale Rather than English --- .github/workflows/main.yml | 2 +- tests/PhpSpreadsheetTests/Shared/StringHelperLocaleTest.php | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index 72148bd081..01cbf16dc9 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -26,7 +26,7 @@ jobs: uses: actions/checkout@v4 - name: Install locales - run: sudo apt-get install -y language-pack-fr language-pack-de language-pack-en + run: sudo apt-get install -y language-pack-fr language-pack-de - name: Setup PHP, with composer and extensions uses: shivammathur/setup-php@v2 diff --git a/tests/PhpSpreadsheetTests/Shared/StringHelperLocaleTest.php b/tests/PhpSpreadsheetTests/Shared/StringHelperLocaleTest.php index b9eaf9b7a8..505005cb41 100644 --- a/tests/PhpSpreadsheetTests/Shared/StringHelperLocaleTest.php +++ b/tests/PhpSpreadsheetTests/Shared/StringHelperLocaleTest.php @@ -37,8 +37,8 @@ public function testCurrency(): void } $result = StringHelper::getCurrencyCode(); self::assertSame('€', $result); - if (!setlocale(LC_ALL, 'en_ca', 'en_CA')) { - self::markTestSkipped('Unable to set en_ca locale for testing.'); + if (!setlocale(LC_ALL, $this->currentLocale)) { + self::markTestSkipped('Unable to restore default locale.'); } $result = StringHelper::getCurrencyCode(); self::assertSame('€', $result, 'result persists despite locale change'); From 12b201b0a4e17c4f83180b9e4691e840383f994d Mon Sep 17 00:00:00 2001 From: oleibman <10341515+oleibman@users.noreply.github.com> Date: Mon, 23 Dec 2024 02:49:00 -0800 Subject: [PATCH 6/7] Keep Trying --- .github/workflows/main.yml | 3 +++ tests/PhpSpreadsheetTests/Shared/StringHelperLocaleTest.php | 5 ++--- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index 01cbf16dc9..d640a0b8e8 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -28,6 +28,9 @@ jobs: - name: Install locales run: sudo apt-get install -y language-pack-fr language-pack-de + - name: Install single-byte locale + run: sudo sed -i -e 's/# de_DE@euro/de_DE@euro/g' /etc/locale.gen && sudo locale-gen de_DE@euro + - name: Setup PHP, with composer and extensions uses: shivammathur/setup-php@v2 with: diff --git a/tests/PhpSpreadsheetTests/Shared/StringHelperLocaleTest.php b/tests/PhpSpreadsheetTests/Shared/StringHelperLocaleTest.php index 505005cb41..c4178f53ab 100644 --- a/tests/PhpSpreadsheetTests/Shared/StringHelperLocaleTest.php +++ b/tests/PhpSpreadsheetTests/Shared/StringHelperLocaleTest.php @@ -32,7 +32,7 @@ protected function tearDown(): void public function testCurrency(): void { - if (!setlocale(LC_ALL, 'de_DE.UTF-8', 'deu_deu.utf8')) { + if ($this->currentLocale === false || !setlocale(LC_ALL, 'de_DE.UTF-8', 'deu_deu.utf8')) { self::markTestSkipped('Unable to set German UTF8 locale for testing.'); } $result = StringHelper::getCurrencyCode(); @@ -46,10 +46,9 @@ public function testCurrency(): void $result = StringHelper::getCurrencyCode(); self::assertSame('$', $result, 'locale now used'); StringHelper::setCurrencyCode(null); - if (!setlocale(LC_ALL, 'deu_deu', 'de_DE')) { + if (!setlocale(LC_ALL, 'deu_deu', 'de_DE@euro')) { self::markTestSkipped('Unable to set German single-byte locale for testing.'); } - // Seems like Linux returns trailing blank, Win doesn't $result = StringHelper::getCurrencyCode(true); // trim if alt symbol is used self::assertSame('EUR', $result, 'non-UTF8 result ignored'); } From b8f2aa97c2230f2fb9c1006c10d67780ac7c8cb7 Mon Sep 17 00:00:00 2001 From: oleibman <10341515+oleibman@users.noreply.github.com> Date: Mon, 23 Dec 2024 08:53:31 -0800 Subject: [PATCH 7/7] Update CHANGELOG.md --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index f567e658c6..f975b47750 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -26,6 +26,7 @@ and this project adheres to [Semantic Versioning](https://semver.org). ### Fixed - More context options may be needed for http(s) image. [Php issue 17121](https://github.com/php/php-src/issues/17121) [PR #4276](https://github.com/PHPOffice/PhpSpreadsheet/pull/4276) +- Avoid unexpected charset in currency symbol. [PR #4279](https://github.com/PHPOffice/PhpSpreadsheet/pull/4279) ## 2024-12-08 - 3.6.0