From fb7b2ed219c687dac0944081667042e56268ad7a Mon Sep 17 00:00:00 2001 From: oleibman <10341515+oleibman@users.noreply.github.com> Date: Sun, 23 Jul 2023 20:51:23 -0700 Subject: [PATCH] Php 8.3 Problem - RLM Added to NumberFormatter Currency - Minor Break (#3640) * Php 8.3 Problem - RLM Added to NumberFormatter Currency - Minor Break Fix #3571. This isn't truly a Php8.3 problem - it all depends on the version of ICU with which Php was linked. ICU 72.1 adds an RLM (right-to-left mark) character in some circumstances when creating a currency format. This broke some tests for the Currency and Accounting wizards, and can result in a difference in the appearance of some spreadsheet cells. This PR changes code to strip out the RLM or not depending on a new property. The new property could default to true (so end-users will not see any change no matter what release of ICU is used), or false. For the latter, users might see a break, but my assumption is that the ICU developers have good reasons for their change, and it's probably best to go along with it. If users wish to retain the existing behavior, they can do so by adding the following code before setting the wizard's locale: ```php $wizard->setStripLeadingRLM(true); ``` * Eliminate 2 Dead Statements Correctly flagged by Scrutinizer. --- .../Style/NumberFormat/Wizard/Accounting.php | 14 ++++-- .../Style/NumberFormat/Wizard/Currency.php | 17 ++++++- .../Style/NumberFormat/Wizard/Locale.php | 6 ++- .../NumberFormat/Wizard/AccountingTest.php | 44 ++++++++++++++++-- .../NumberFormat/Wizard/CurrencyTest.php | 45 +++++++++++++++++-- 5 files changed, 110 insertions(+), 16 deletions(-) diff --git a/src/PhpSpreadsheet/Style/NumberFormat/Wizard/Accounting.php b/src/PhpSpreadsheet/Style/NumberFormat/Wizard/Accounting.php index c30028601..c14d38532 100644 --- a/src/PhpSpreadsheet/Style/NumberFormat/Wizard/Accounting.php +++ b/src/PhpSpreadsheet/Style/NumberFormat/Wizard/Accounting.php @@ -28,7 +28,8 @@ class Accounting extends Currency bool $thousandsSeparator = true, bool $currencySymbolPosition = self::LEADING_SYMBOL, bool $currencySymbolSpacing = self::SYMBOL_WITHOUT_SPACING, - ?string $locale = null + ?string $locale = null, + bool $stripLeadingRLM = self::DEFAULT_STRIP_LEADING_RLM ) { $this->setCurrencyCode($currencyCode); $this->setThousandsSeparator($thousandsSeparator); @@ -36,6 +37,7 @@ class Accounting extends Currency $this->setCurrencySymbolPosition($currencySymbolPosition); $this->setCurrencySymbolSpacing($currencySymbolSpacing); $this->setLocale($locale); + $this->stripLeadingRLM = $stripLeadingRLM; } /** @@ -44,16 +46,20 @@ class Accounting extends Currency protected function getLocaleFormat(): string { if (version_compare(PHP_VERSION, '7.4.1', '<')) { + // @codeCoverageIgnoreStart throw new Exception('The Intl extension does not support Accounting Formats below PHP 7.4.1'); + // @codeCoverageIgnoreEnd } - if ($this->icuVersion() < 53.0) { + if (self::icuVersion() < 53.0) { + // @codeCoverageIgnoreStart throw new Exception('The Intl extension does not support Accounting Formats without ICU 53'); + // @codeCoverageIgnoreEnd } // Scrutinizer does not recognize CURRENCY_ACCOUNTING $formatter = new Locale($this->fullLocale, NumberFormatter::CURRENCY_ACCOUNTING); - $mask = $formatter->format(); + $mask = $formatter->format($this->stripLeadingRLM); if ($this->decimals === 0) { $mask = (string) preg_replace('/\.0+/miu', '', $mask); } @@ -61,7 +67,7 @@ class Accounting extends Currency return str_replace('¤', $this->formatCurrencyCode(), $mask); } - private function icuVersion(): float + public static function icuVersion(): float { [$major, $minor] = explode('.', INTL_ICU_VERSION); diff --git a/src/PhpSpreadsheet/Style/NumberFormat/Wizard/Currency.php b/src/PhpSpreadsheet/Style/NumberFormat/Wizard/Currency.php index 2fcd10838..37e99484f 100644 --- a/src/PhpSpreadsheet/Style/NumberFormat/Wizard/Currency.php +++ b/src/PhpSpreadsheet/Style/NumberFormat/Wizard/Currency.php @@ -21,6 +21,10 @@ class Currency extends Number protected bool $currencySymbolSpacing = self::SYMBOL_WITHOUT_SPACING; + protected const DEFAULT_STRIP_LEADING_RLM = false; + + protected bool $stripLeadingRLM = self::DEFAULT_STRIP_LEADING_RLM; + /** * @param string $currencyCode the currency symbol or code to display for this mask * @param int $decimals number of decimal places to display, in the range 0-30 @@ -33,6 +37,8 @@ class Currency extends Number * If provided, Locale values must be a valid formatted locale string (e.g. 'en-GB', 'fr', uz-Arab-AF). * Note that setting a locale will override any other settings defined in this class * other than the currency code; or decimals (unless the decimals value is set to 0). + * @param bool $stripLeadingRLM remove leading RLM added with + * ICU 72.1+. * * @throws Exception If a provided locale code is not a valid format */ @@ -42,7 +48,8 @@ class Currency extends Number bool $thousandsSeparator = true, bool $currencySymbolPosition = self::LEADING_SYMBOL, bool $currencySymbolSpacing = self::SYMBOL_WITHOUT_SPACING, - ?string $locale = null + ?string $locale = null, + bool $stripLeadingRLM = self::DEFAULT_STRIP_LEADING_RLM ) { $this->setCurrencyCode($currencyCode); $this->setThousandsSeparator($thousandsSeparator); @@ -50,6 +57,7 @@ class Currency extends Number $this->setCurrencySymbolPosition($currencySymbolPosition); $this->setCurrencySymbolSpacing($currencySymbolSpacing); $this->setLocale($locale); + $this->stripLeadingRLM = $stripLeadingRLM; } public function setCurrencyCode(string $currencyCode): void @@ -67,10 +75,15 @@ class Currency extends Number $this->currencySymbolSpacing = $currencySymbolSpacing; } + public function setStripLeadingRLM(bool $stripLeadingRLM): void + { + $this->stripLeadingRLM = $stripLeadingRLM; + } + protected function getLocaleFormat(): string { $formatter = new Locale($this->fullLocale, NumberFormatter::CURRENCY); - $mask = $formatter->format(); + $mask = $formatter->format($this->stripLeadingRLM); if ($this->decimals === 0) { $mask = (string) preg_replace('/\.0+/miu', '', $mask); } diff --git a/src/PhpSpreadsheet/Style/NumberFormat/Wizard/Locale.php b/src/PhpSpreadsheet/Style/NumberFormat/Wizard/Locale.php index 9f0336274..ad4ba44f6 100644 --- a/src/PhpSpreadsheet/Style/NumberFormat/Wizard/Locale.php +++ b/src/PhpSpreadsheet/Style/NumberFormat/Wizard/Locale.php @@ -30,8 +30,10 @@ final class Locale } } - public function format(): string + public function format(bool $stripRlm = true): string { - return $this->formatter->getPattern(); + $str = $this->formatter->getPattern(); + + return ($stripRlm && substr($str, 0, 3) === "\xe2\x80\x8f") ? substr($str, 3) : $str; } } diff --git a/tests/PhpSpreadsheetTests/Style/NumberFormat/Wizard/AccountingTest.php b/tests/PhpSpreadsheetTests/Style/NumberFormat/Wizard/AccountingTest.php index 5d3812151..269ecdcdc 100644 --- a/tests/PhpSpreadsheetTests/Style/NumberFormat/Wizard/AccountingTest.php +++ b/tests/PhpSpreadsheetTests/Style/NumberFormat/Wizard/AccountingTest.php @@ -44,19 +44,25 @@ class AccountingTest extends TestCase public function testAccountingLocale( string $expectedResult, string $currencyCode, - string $locale + string $locale, + ?bool $stripRLM = null ): void { if (class_exists(NumberFormatter::class) === false) { self::markTestSkipped('Intl extension is not available'); } $wizard = new Accounting($currencyCode); + if ($stripRLM !== null) { + $wizard->setStripLeadingRLM($stripRLM); + } $wizard->setLocale($locale); self::assertSame($expectedResult, (string) $wizard); } public static function providerAccountingLocale(): array { + // \u{a0} is non-breaking space + // \u{200e} is LRM (left-to-right mark) return [ ["[\$€-fy-NL]\u{a0}#,##0.00;([\$€-fy-NL]\u{a0}#,##0.00)", '€', 'fy-NL'], ["[\$€-nl-NL]\u{a0}#,##0.00;([\$€-nl-NL]\u{a0}#,##0.00)", '€', 'nl-NL'], @@ -66,29 +72,59 @@ class AccountingTest extends TestCase ['[$$-en-CA]#,##0.00;([$$-en-CA]#,##0.00)', '$', 'en-ca'], ["#,##0.00\u{a0}[\$\$-fr-CA];(#,##0.00\u{a0}[\$\$-fr-CA])", '$', 'fr-ca'], ['[$¥-ja-JP]#,##0;([$¥-ja-JP]#,##0)', '¥', 'ja-JP'], // No decimals - ["#,##0.000\u{a0}[\$د.ب‎-ar-BH]", 'د.ب‎', 'ar-BH'], // 3 decimals + ["#,##0.000\u{a0}[\$د.ب\u{200e}-ar-BH]", "د.ب\u{200e}", 'ar-BH', true], // 3 decimals ]; } + public function testIcu721(): void + { + if (class_exists(NumberFormatter::class) === false) { + self::markTestSkipped('Intl extension is not available'); + } + + $currencyCode = "د.ب\u{200e}"; + $locale = 'ar-BH'; + $wizardFalse = new Accounting($currencyCode); + $wizardFalse->setStripLeadingRLM(false); + $wizardFalse->setLocale($locale); + $stringFalse = (string) $wizardFalse; + $wizardTrue = new Accounting($currencyCode); + $wizardTrue->setStripLeadingRLM(true); + $wizardTrue->setLocale($locale); + $stringTrue = (string) $wizardTrue; + $version = Accounting::icuVersion(); + if ($version < 72.1) { + self::assertSame($stringFalse, $stringTrue); + } else { + self::assertSame("\u{200f}$stringTrue", $stringFalse); + } + } + /** * @dataProvider providerAccountingLocaleNoDecimals */ public function testAccountingLocaleNoDecimals( string $expectedResult, string $currencyCode, - string $locale + string $locale, + ?bool $stripRLM = null ): void { if (class_exists(NumberFormatter::class) === false) { self::markTestSkipped('Intl extension is not available'); } $wizard = new Accounting($currencyCode, 0); + if ($stripRLM !== null) { + $wizard->setStripLeadingRLM($stripRLM); + } $wizard->setLocale($locale); self::assertSame($expectedResult, (string) $wizard); } public static function providerAccountingLocaleNoDecimals(): array { + // \u{a0} is non-breaking space + // \u{200e} is LRM (left-to-right mark) return [ ["[\$€-fy-NL]\u{a0}#,##0;([\$€-fy-NL]\u{a0}#,##0)", '€', 'fy-NL'], ["[\$€-nl-NL]\u{a0}#,##0;([\$€-nl-NL]\u{a0}#,##0)", '€', 'nl-NL'], @@ -98,7 +134,7 @@ class AccountingTest extends TestCase ['[$$-en-CA]#,##0;([$$-en-CA]#,##0)', '$', 'en-ca'], ["#,##0\u{a0}[\$\$-fr-CA];(#,##0\u{a0}[\$\$-fr-CA])", '$', 'fr-ca'], ['[$¥-ja-JP]#,##0;([$¥-ja-JP]#,##0)', '¥', 'ja-JP'], // No decimals to truncate - ["#,##0\u{a0}[\$د.ب‎-ar-BH]", 'د.ب‎', 'ar-BH'], // 3 decimals truncated to none + ["#,##0\u{a0}[\$د.ب\u{200e}-ar-BH]", "د.ب\u{200e}", 'ar-BH', true], // 3 decimals truncated to none ]; } diff --git a/tests/PhpSpreadsheetTests/Style/NumberFormat/Wizard/CurrencyTest.php b/tests/PhpSpreadsheetTests/Style/NumberFormat/Wizard/CurrencyTest.php index 308713adf..a84d5566d 100644 --- a/tests/PhpSpreadsheetTests/Style/NumberFormat/Wizard/CurrencyTest.php +++ b/tests/PhpSpreadsheetTests/Style/NumberFormat/Wizard/CurrencyTest.php @@ -4,6 +4,7 @@ namespace PhpOffice\PhpSpreadsheetTests\Style\NumberFormat\Wizard; use NumberFormatter; use PhpOffice\PhpSpreadsheet\Exception; +use PhpOffice\PhpSpreadsheet\Style\NumberFormat\Wizard\Accounting; use PhpOffice\PhpSpreadsheet\Style\NumberFormat\Wizard\Currency; use PhpOffice\PhpSpreadsheet\Style\NumberFormat\Wizard\Number; use PHPUnit\Framework\TestCase; @@ -43,19 +44,25 @@ class CurrencyTest extends TestCase public function testCurrencyLocale( string $expectedResult, string $currencyCode, - string $locale + string $locale, + ?bool $stripRLM = null ): void { if (class_exists(NumberFormatter::class) === false) { self::markTestSkipped('Intl extension is not available'); } $wizard = new Currency($currencyCode); + if ($stripRLM !== null) { + $wizard->setStripLeadingRLM($stripRLM); + } $wizard->setLocale($locale); self::assertSame($expectedResult, (string) $wizard); } public static function providerCurrencyLocale(): array { + // \u{a0} is non-breaking space + // \u{200e} is LRM (left-to-right mark) return [ ["[\$€-fy-NL]\u{a0}#,##0.00;[\$€-fy-NL]\u{a0}#,##0.00-", '€', 'fy-NL'], // Trailing negative ["[\$€-nl-NL]\u{a0}#,##0.00;[\$€-nl-NL]\u{a0}-#,##0.00", '€', 'nl-NL'], // Sign between currency and value @@ -65,29 +72,59 @@ class CurrencyTest extends TestCase ['[$$-en-CA]#,##0.00', '$', 'en-ca'], ["#,##0.00\u{a0}[\$\$-fr-CA]", '$', 'fr-ca'], // Trailing currency code ['[$¥-ja-JP]#,##0', '¥', 'ja-JP'], // No decimals - ["#,##0.000\u{a0}[\$د.ب‎-ar-BH]", 'د.ب‎', 'ar-BH'], // 3 decimals + ["#,##0.000\u{a0}[\$د.ب\u{200e}-ar-BH]", "د.ب\u{200e}", 'ar-BH', true], // 3 decimals ]; } + public function testIcu721(): void + { + if (class_exists(NumberFormatter::class) === false) { + self::markTestSkipped('Intl extension is not available'); + } + + $currencyCode = "د.ب\u{200e}"; + $locale = 'ar-BH'; + $wizardFalse = new Currency($currencyCode); + $wizardFalse->setStripLeadingRLM(false); + $wizardFalse->setLocale($locale); + $stringFalse = (string) $wizardFalse; + $wizardTrue = new Currency($currencyCode); + $wizardTrue->setStripLeadingRLM(true); + $wizardTrue->setLocale($locale); + $stringTrue = (string) $wizardTrue; + $version = Accounting::icuVersion(); + if ($version < 72.1) { + self::assertSame($stringFalse, $stringTrue); + } else { + self::assertSame("\u{200f}$stringTrue", $stringFalse); + } + } + /** * @dataProvider providerCurrencyLocaleNoDecimals */ public function testCurrencyLocaleNoDecimals( string $expectedResult, string $currencyCode, - string $locale + string $locale, + ?bool $stripRLM = null ): void { if (class_exists(NumberFormatter::class) === false) { self::markTestSkipped('Intl extension is not available'); } $wizard = new Currency($currencyCode, 0); + if ($stripRLM !== null) { + $wizard->setStripLeadingRLM($stripRLM); + } $wizard->setLocale($locale); self::assertSame($expectedResult, (string) $wizard); } public static function providerCurrencyLocaleNoDecimals(): array { + // \u{a0} is non-breaking space + // \u{200e} is LRM (left-to-right mark) return [ ["[\$€-fy-NL]\u{a0}#,##0;[\$€-fy-NL]\u{a0}#,##0-", '€', 'fy-NL'], // Trailing negative ["[\$€-nl-NL]\u{a0}#,##0;[\$€-nl-NL]\u{a0}-#,##0", '€', 'nl-NL'], // Sign between currency and value @@ -97,7 +134,7 @@ class CurrencyTest extends TestCase ['[$$-en-CA]#,##0', '$', 'en-ca'], ["#,##0\u{a0}[\$\$-fr-CA]", '$', 'fr-ca'], // Trailing currency code ['[$¥-ja-JP]#,##0', '¥', 'ja-JP'], // No decimals to truncate - ["#,##0\u{a0}[\$د.ب‎-ar-BH]", 'د.ب‎', 'ar-BH'], // 3 decimals truncated to none + ["#,##0\u{a0}[\$د.ب\u{200e}-ar-BH]", "د.ب\u{200e}", 'ar-BH', true], // 3 decimals truncated to none ]; }