Skip to content

Commit

Permalink
Php 8.3 Problem - RLM Added to NumberFormatter Currency - Minor Break
Browse files Browse the repository at this point in the history 
Fix PHPOffice#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(false);
```
  • Loading branch information
@oleibman
oleibman committed Jul 9, 2023
1 parent ea4f0a2 commit 0753ad5
Show file tree
Hide file tree
Showing 5 changed files with 112 additions and 16 deletions.
14 changes: 10 additions & 4 deletions src/PhpSpreadsheet/Style/NumberFormat/Wizard/Accounting.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,14 +28,16 @@ public function __construct(
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);
$this->setDecimals($decimals);
$this->setCurrencySymbolPosition($currencySymbolPosition);
$this->setCurrencySymbolSpacing($currencySymbolSpacing);
$this->setLocale($locale);
$this->stripLeadingRLM = $stripLeadingRLM;
}

/**
Expand All @@ -44,24 +46,28 @@ public function __construct(
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);
}

return str_replace('¤', $this->formatCurrencyCode(), $mask);
}

private function icuVersion(): float
public static function icuVersion(): float
{
[$major, $minor] = explode('.', INTL_ICU_VERSION);

Expand Down
17 changes: 15 additions & 2 deletions src/PhpSpreadsheet/Style/NumberFormat/Wizard/Currency.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
*/
Expand All @@ -42,14 +48,16 @@ public function __construct(
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);
$this->setDecimals($decimals);
$this->setCurrencySymbolPosition($currencySymbolPosition);
$this->setCurrencySymbolSpacing($currencySymbolSpacing);
$this->setLocale($locale);
$this->stripLeadingRLM = $stripLeadingRLM;
}

public function setCurrencyCode(string $currencyCode): void
Expand All @@ -67,10 +75,15 @@ public function setCurrencySymbolSpacing(bool $currencySymbolSpacing = self::SYM
$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);
}
Expand Down
6 changes: 4 additions & 2 deletions src/PhpSpreadsheet/Style/NumberFormat/Wizard/Locale.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,10 @@ public function __construct(?string $locale, int $style)
}
}

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;
}
}
45 changes: 41 additions & 4 deletions tests/PhpSpreadsheetTests/Style/NumberFormat/Wizard/AccountingTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -44,19 +44,25 @@ public static function providerAccounting(): array
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'],
Expand All @@ -66,29 +72,60 @@ public static function providerAccountingLocale(): array
['[$$-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';
$expected = "#,##0.000\u{a0}[\$د.ب\u{200e}-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'],
Expand All @@ -98,7 +135,7 @@ public static function providerAccountingLocaleNoDecimals(): array
['[$$-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
];
}

Expand Down
46 changes: 42 additions & 4 deletions tests/PhpSpreadsheetTests/Style/NumberFormat/Wizard/CurrencyTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

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;
Expand Down Expand Up @@ -43,19 +44,25 @@ public static function providerCurrency(): array
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
Expand All @@ -65,29 +72,60 @@ public static function providerCurrencyLocale(): array
['[$$-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';
$expected = "#,##0.000\u{a0}[\$د.ب\u{200e}-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
Expand All @@ -97,7 +135,7 @@ public static function providerCurrencyLocaleNoDecimals(): array
['[$$-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
];
}

Expand Down

0 comments on commit 0753ad5

Please sign in to comment.