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

Handle Currencies Stored as Strings in Formulas #3189

Merged
merged 3 commits into from
Nov 25, 2022
Merged

Handle Currencies Stored as Strings in Formulas #3189

merged 3 commits into from
Nov 25, 2022

Conversation

fdjohnston
Copy link
Contributor

Added a function to the new FormattedNumber helper that will use the currency code pulled from localeconv by StringHelper::getCurrencyCode(). The currency code is preg_quoted and then dropped into a regexp that is modelled on the expression developed for convertToNumberIfPercent. This will allow locale independent operation. Unfortunately localeconv only provides information about standard currency formats and not accounting formats. This means that we can't say for sure whether or not a locale has an accounting format where the currency symbol shows up in a non-standard position (eg. left justified for some countries instead of appearing directly in front of the value). The regexp should handle these cases.

The primary issue with this approach is that the regexp will incorrectly match invalid currency formats for some locals as it checks for the symbol before and after the value.

A possible improvement would be to pull p_cs_precedes and n_cs_precedes from localeconv and using them to determine if the currency symbol should appear before or after the value for the current locale. Since white-space is ignored by the regexp, accounting formats should still work, as long as the accounting format doesn't involve moving the symbol to the opposite side of the value.

This is:

- [ ] a bugfix
- [x] 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
  • Documentation is updated as necessary

Why this change is needed?

See issue #3165

fdjohnston and others added 2 commits November 18, 2022 14:54
Added a function to the new `FormattedNumber` helper that will use the currency code pulled from `localeconv` by `StringHelper::getCurrencyCode()`.  The currency code is `preg_quoted` and then dropped into a regexp that is modelled on the expression developed for `convertToNumberIfPercent`.  This will allow locale independent operation.
Unfortunately `localeconv` only provides information about standard currency formats and not accounting formats.  This means that we can't say for sure whether or not a locale has an accounting format where the currency symbol shows up in a non-standard position (eg. left justified for some countries instead of appearing directly in front of the value).  The regexp should handle these cases.

The primary issue with this approach is that the regexp will incorrectly match invalid currency formats for some locals as it checks for the symbol before and after the value.

A possible improvement would be to pull `p_cs_precedes` and `n_cs_precedes` from `localeconv` and using them to determine if the currency symbol should appear before or after the value for the current locale.  Since white-space is ignored by the regexp, accounting formats should still work, as long as the accounting format doesn't involve moving the symbol to the opposite side of the value.
@MarkBaker
Copy link
Member

I've made a couple of small changes

  • Use of the unicode flag for the regexp, for currency codes that aren't a part of the standard ASCII character set
  • Some additional unit tests for positive sign
  • Handling of currency/accounting strings with a thousands separator (not locale-aware)

Thanks for your work on this

@MarkBaker MarkBaker merged commit bdb34b9 into PHPOffice:master Nov 25, 2022
@fdjohnston
Copy link
Contributor Author

Great improvements; thank you.

Happy to contribute to such a great project, and may I say I've really enjoyed going back and forth with you on this. I have definitely learned some new tricks with regular expressions.

If you ever decide to make the solution Locale aware or explore the PHP Intl option you mentioned over in #3165, and if you'd like a hand, I would be happy to try to help out.

MarkBaker added a commit that referenced this pull request Dec 21, 2022
### Added

- Extended flag options for the Reader `load()` and Writer `save()` methods
- Apply Row/Column limits (1048576 and XFD) in ReferenceHelper [PR #3213](#3213)
- Allow the creation of In-Memory Drawings from a string of binary image data, or from a stream. [PR #3157](#3157)
- Xlsx Reader support for Pivot Tables [PR #2829](#2829)
- Permit Date/Time Entered on Spreadsheet to be calculated as Float [Issue #1416](#1416) [PR #3121](#3121)

### Changed

- Nothing

### Deprecated

- Direct update of Calculation::suppressFormulaErrors is replaced with setter.
- Font public static variable defaultColumnWidths replaced with constant DEFAULT_COLUMN_WIDTHS.
- ExcelError public static variable errorCodes replaced with constant ERROR_CODES.
- NumberFormat constant FORMAT_DATE_YYYYMMDD2 replaced with existing identical FORMAT_DATE_YYYYMMDD.

### Removed

- Nothing

### Fixed

- Fixed handling for `_xlws` prefixed functions from Office365 [Issue #3245](#3245) [PR #3247](#3247)
- Conditionals formatting rules applied to rows/columns are removed [Issue #3184](#3184) [PR #3213](#3213)
- Treat strings containing currency or accounting values as floats in Calculation Engine operations [Issue #3165](#3165) [PR #3189](#3189)
- Treat strings containing percentage values as floats in Calculation Engine operations [Issue #3155](#3155) [PR #3156](#3156) and [PR #3164](#3164)
- Xlsx Reader Accept Palette of Fewer than 64 Colors [Issue #3093](#3093) [PR #3096](#3096)
- Use Locale-Independent Float Conversion for Xlsx Writer Custom Property [Issue #3095](#3095) [PR #3099](#3099)
- Allow setting AutoFilter range on a single cell or row [Issue #3102](#3102) [PR #3111](#3111)
- Xlsx Reader External Data Validations Flag Missing [Issue #2677](#2677) [PR #3078](#3078)
- Reduces extra memory usage on `__destruct()` calls [PR #3092](#3092)
- Additional properties for Trendlines [Issue #3011](#3011) [PR #3028](#3028)
- Calculation suppressFormulaErrors fix [Issue #1531](#1531) [PR #3092](#3092)
- Permit Date/Time Entered on Spreadsheet to be Calculated as Float [Issue #1416](#1416) [PR #3121](#3121)
- Incorrect Handling of Data Validation Formula Containing Ampersand [Issue #3145](#3145) [PR #3146](#3146)
- Xlsx Namespace Handling of Drawings, RowAndColumnAttributes, MergeCells [Issue #3138](#3138) [PR #3136](#3137)
- Generation3 Copy With Image in Footer [Issue #3126](#3126) [PR #3140](#3140)
- MATCH Function Problems with Int/Float Compare and Wildcards [Issue #3141](#3141) [PR #3142](#3142)
- Fix ODS Read Filter on number-columns-repeated cell [Issue #3148](#3148) [PR #3149](#3149)
- Problems Formatting Very Small and Very Large Numbers [Issue #3128](#3128) [PR #3152](#3152)
- XlsxWrite preserve line styles for y-axis, not just x-axis [PR #3163](#3163)
- Xlsx Namespace Handling of Drawings, RowAndColumnAttributes, MergeCells [Issue #3138](#3138) [PR #3137](#3137)
- More Detail for Cyclic Error Messages [Issue #3169](#3169) [PR #3170](#3170)
- Improved Documentation for Deprecations - many PRs [Issue #3162](#3162)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants