Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Right to Left, Accounting/Currency Wizard, and Php Nightly #3571

Closed
1 of 8 tasks
oleibman opened this issue May 18, 2023 · 2 comments · Fixed by #3640
Closed
1 of 8 tasks

Right to Left, Accounting/Currency Wizard, and Php Nightly #3571

oleibman opened this issue May 18, 2023 · 2 comments · Fixed by #3640

Comments

@oleibman
Copy link
Collaborator

oleibman commented May 18, 2023

This is:

- [ ] a bug report
- [ ] a feature request
- [ ] **not** a usage question (ask them on https://stackoverflow.com/questions/tagged/phpspreadsheet or https://gitter.im/PHPOffice/PhpSpreadsheet)

What is the expected behavior?

@MarkBaker, I'm not sure what is correct. Some tests in Style/NumberFormat/Wizard/Accounting and Currency are failing in Windows using Php nightly. I do not see any failure indication on Github using nightly, which indicates that Windows and Linux aren't returning the same result, which is very peculiar. I do not have access to a Linux system with Php nightly. At any rate, on Windows nightly, character RLM (right to left marker) is placed at the beginning of the style string; on Windows (all other releases) and Linux, there is no RLM. The code below generates the following spreadsheet for all but Windows using Php nightly:
image
I realize that it's very early for this kind of report. It is really easy to change the test to strip the RLM so that it will work on Windows nightly; and only slightly more complicated to change the source code. Mostly I am curious as to whether the expected result should be that depicted in the image above, or is the image in current behavior below what a user might expect.

What is the current behavior?

Here is the result of the code below for Windows using Php nightly:
image

What are the steps to reproduce?

Please provide a Minimal, Complete, and Verifiable example of code that exhibits the issue without relying on an external Excel file or a web server:

<?php

require __DIR__ . '/vendor/autoload.php';

use PhpOffice\PhpSpreadsheet\Spreadsheet;
use PhpOffice\PhpSpreadsheet\Style\NumberFormat\Wizard\Accounting;
use PhpOffice\PhpSpreadsheet\Writer\Xlsx;

        $currencyCode = 'د.ب‎';
        $locale = 'ar-BH';
        $wizard = new Accounting($currencyCode);
        $wizard->setLocale($locale);
        $spreadsheet = new Spreadsheet();
        $sheet = $spreadsheet->getActiveSheet();
        $sheet->setCellValue('A1', 5);
        $sheet->getStyle('A1')->getNumberFormat()->setFormatCode((string) $wizard);
        $writer = new Xlsx($spreadsheet);
        $oufil = 'rlmtest2.xlsx';
        $writer->save($oufil);

If this is an issue with reading a specific spreadsheet file, then it may be appropriate to provide a sample file that demonstrates the problem; but please keep it as small as possible, and sanitize any confidential information before uploading.

What features do you think are causing the issue

  • Reader
  • Writer
  • Styles
  • Data Validations
  • Formula Calculations
  • Charts
  • AutoFilter
  • Form Elements

Does an issue affect all spreadsheet file formats? If not, which formats are affected?

Tested only with Xlsx. It would not surprise me if other formats are affected. The code in question is not dependent on the spreadsheet format.

Which versions of PhpSpreadsheet and PHP are affected?

All PhpSpreadsheet versions. Php nightly on Windows.

@oleibman
Copy link
Collaborator Author

I now know where the discrepancy occurs:

        $locale = 'ar-BH';
        $fmt = new \NumberFormatter($locale, \NumberFormatter::CURRENCY);
        echo 'PHP_VERSION is ', PHP_VERSION, ' format is ', $fmt->getPattern(), "\n";

No PhpSpreadsheet code involved. Windows Php8.3 puts the RLM at the front of $fmt->getPattern(); no earlier Php release does so, and it is possible that Php8.3 on Linux also doesn't add it (seems likely from Github result but no way for me to test).

It's also worth noting that the test cases for ar-BH use an (invisible) LRM at the end of their currency strings (Arabic letter Dal followed by period followed by Arabic letter Beh followed by LRM, and, of course, allow for the possibility that I've swapped the letters in that string). I don't think the LRM is relevant to the problem at hand.

@oleibman
Copy link
Collaborator Author

oleibman commented Jul 7, 2023

Asking the question at Php, it turns out that the inclusion of RLM is part of ICU Version 72.1, which is clearly used in my Windows build; the build being used in the Github tests must be using an earlier version. So it will affect the Github tests eventually, and we should probably fix it sooner rather than later.

oleibman added a commit to oleibman/PhpSpreadsheet that referenced this issue Jul 9, 2023
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);
```
oleibman added a commit that referenced this issue Jul 24, 2023
…#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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging a pull request may close this issue.

1 participant