Skip to content

Conversation

@oleibman
Copy link
Collaborator

@oleibman oleibman commented Aug 6, 2024

Fix #4125. Currency and Accounting Wizards generate styles for ISO codes, but these are incorrect and cause a problem when Excel tries to open a spreadsheet containing these styles. Debugging that problem, other problems with Wizards came to light:

  • Currency Wizard should permit four different styles for negative numbers (as Excel does) - minus sign, minus sign and red font, paretheses, and parenthese and red font. It currently uses only minus sign.
  • Accounting Wizard should use parentheses for negative numbers (as Excel does). It currently uses minus sign.
  • Accounting Wizard should always use SYMBOL_WITH_SPACING (as Excel does). It currently permits the use of SYMBOL_WITHOUT_SPACING. What WITH_SPACING really does is to ensure decimal-point alignment among adjacent cells in a column with the same format.
  • Currency Wizard should always use SYMBOL_WITHOUT_SPACING (as Excel does). It currently permits the use of SYMBOL_WITH_SPACING.

I am correcting these problems by:

  • renaming Currency Wizard to CurrencyBase
  • adding a negative property with setter to it and its constructor.
  • adding a new Currency which extends CurrencyBase, always using SYMBOL_WITHOUT_SPACING when formatting.
  • having Accounting extend CurrencyBase rather than Currency, always using SYMBOL_WITH_SPACING and NEGATIVE_PARENS when formatting.
  • CurrencyBase can be used if the restrictions on Currency and Accounting are not desired (e.g. the suggested accounting constant from this unimplemented PR).

Excel does some funny stuff with these formats. In particular, it might try to guess if you have a particular Accounting format in mind. So the Accounting wizard for dollar sign generates a format which (a) matches FORMAT_ACCOUNTING_USD, and (b) Excel (correctly) interprets as an Accounting format for symbol $. On the other hand, the Accounting wizard for euro sign generates a format which (a) matches FORMAT_ACCOUNTING_EUR, but (b) Excel interprets as a custom code rather than an Accounting format. This in itself is not a particularly big deal, but it has made it impossible for me to see exactly what format Excel uses for trailing currency symbols for negative numbers. I can't get them to decimal-point align with positive numbers if I put any kind of space between the trailing parenthesis and the currency symbol, so I omit that. It doesn't look terrible, and it keeps everything aligned, but it might not be what people are used to.

I've also changed the formatting to use spaces rather than non-breaking spaces. They seem to work just fine, and the constants mentioned above use them rather than nbsp.

These changes broke some Wizard Accounting tests. However, I think those tests were checking the wrong thing. They checked the format code, rather than the results of formatting data using that format code. So they are slightly altered to make those checks instead.

Fix #4124. Currency formats that contain an ISO currency code which contains one of the characters used to recognize a date format (hmsdy), e.g. [$HUF], are being formatted by PhpSpreadsheet as dates rather than currencies. Code is changed to recognize open bracket followed by dollar sign followed by 3 Latin alphabetic characters followed by close bracket as a non-date.

This is:

  • a bugfix
  • a new feature
  • refactoring
  • additional unit tests

Checklist:

  • Changes are covered by unit tests
    • Changes are covered by existing unit tests
    • New unit tests have been added
  • Code style is respected
  • Commit message explains why the change is made (see https://github.com/erlang/otp/wiki/Writing-good-commit-messages)
  • CHANGELOG.md contains a short summary of the change and a link to the pull request if applicable
  • Documentation is updated as necessary

Why this change is needed?

Provide an explanation of why this change is needed, with links to any Issues (if appropriate).
If this is a bugfix or a new feature, and there are no existing Issues, then please also create an issue that will make it easier to track progress with this PR.

Fix PHPOffice#4125. Currency and Accounting Wizards generate styles for ISO codes, but these are incorrect and cause a problem when Excel tries to open a spreadsheet containing these styles. Debugging that problem, other problems with Wizards came to light:
- Currency Wizard should permit four different styles for negative numbers (as Excel does) - minus sign, minus sign and red font, paretheses, and parenthese and red font. It currently uses only minus sign.
- Accounting Wizard should use parentheses for negative numbers (as Excel does). It currently uses minus sign.
- Accounting Wizard should always use SYMBOL_WITH_SPACING (as Excel does). It currently permits the use of SYMBOL_WITHOUT_SPACING. What WITH_SPACING really does is to ensure decimal-point alignment among adjacent cells in a column with the same format.
- Currency Wizard should always use SYMBOL_WITHOUT_SPACING (as Excel does). It currently permits the use of SYMBOL_WITH_SPACING.

I am correcting these problems by:
- renaming Currency Wizard to CurrencyBase
- adding a `negative` property with setter to it and its constructor.
- adding a new Currency which extends CurrencyBase, always using SYMBOL_WITHOUT_SPACING when formatting.
- having Accounting extend CurrencyBase rather than Currency, always using SYMBOL_WITH_SPACING and NEGATIVE_PARENS when formatting.
- CurrencyBase can be used if the restrictions on Currency and Accounting are not desired (e.g. the suggested accounting constant from [this unimplemented PR](PHPOffice#1576)).

Excel does some funny stuff with these formats. In particular, it might try to guess if you have a particular Accounting format in mind. So the Accounting wizard for dollar sign generates a format which (a) matches FORMAT_ACCOUNTING_USD, and (b) Excel (correctly) interprets as an Accounting format for symbol $. On the other hand, the Accounting wizard for euro sign generates a format which (a) matches FORMAT_ACCOUNTING_EUR, but (b) Excel interprets as a custom code rather than an Accounting format. This in itself is not a particularly big deal, but it has made it impossible for me to see exactly what format Excel uses for trailing currency symbols for negative numbers. I can't get them to decimal-point align with positive numbers if I put any kind of space between the trailing parenthesis and the currency symbol, so I omit that. It doesn't look terrible, and it keeps everything aligned, but it might not be what people are used to.

I've also changed the formatting to use spaces rather than non-breaking spaces. They seem to work just fine, and the constants mentioned above use them rather than nbsp.

Fix PHPOffice#4124. Currency formats that contain an ISO currency code which contains one of the characters used to recognize a date format (hmsdy), e.g. [$HUF], are being formatted by PhpSpreadsheet as dates rather than currencies. Code is changed to recognize open bracket followed by dollar sign followed by 3 Latin alphabetic characters followed by close bracket as a non-date.
@oleibman
Copy link
Collaborator Author

oleibman commented Aug 6, 2024

Not concerned about Scrutinizer "complexity" message.

PHPCompatibility (versions check) erroneously flags the use of $this in enumerations. They fixed it in their development branch in October 2022. But they haven't had a release since 2019!
@oleibman oleibman added this pull request to the merge queue Aug 14, 2024
Merged via the queue into PHPOffice:master with commit f9837aa Aug 14, 2024
@oleibman oleibman deleted the issue4125 branch August 14, 2024 15:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

1 participant