Skip to content

Conversation

@mimou78
Copy link
Contributor

@mimou78 mimou78 commented Nov 22, 2025

Description (*)

This PR fixes inconsistent currency formatting for several RTL locales such as ar_MA, ar_EG, and he_IL.
Historically, ICU versions behaved differently when rendering RTL currency formats, especially around:

  • symbol position (before vs after the number)
  • non-breaking space (\u00A0)
  • right-to-left markers (\u200F)
  • bidirectional ordering rules

Older Magento tests included conditional expectations to support multiple ICU versions.
However, recent ICU releases have stabilized currency symbol data for Arabic locales, meaning:

  • currency symbols (MAD → د.م.‏, EGP → ج.م.‏) have not changed
  • Magento does not rely on ICU currency patterns for these locales
  • Magento builds its currency output pattern internally (%s <symbol>)
  • the output for these locales is now consistent across PHP 8.2–8.4

This PR aligns unit tests with the actual stable behavior of Symfony Intl + Magento’s custom logic.
The fixes ensure consistent results across all PHP/ICU combinations used in CI and customer environments.


Why this change is needed

  • Several RTL currency tests failed depending on the PHP/ICU environment.
  • The expected format in older tests was based on historic ICU variations which no longer apply.
  • Magento itself does not delegate RTL symbol placement to ICU (it only uses the symbol).
  • Therefore, the previous conditional tests are unnecessary and misleading.

This PR provides stable, deterministic, cross-environment expectations.


What has been changed

  • Normalized expected currency formats for MAD, EGP, and he_IL.
  • Removed legacy ICU-dependent variations.
  • Ensured use of Unicode NBSP (\u00A0) and RTL mark (\u200F) where appropriate.
  • Updated tests to use the exact output produced by NumberFormatter + Magento logic.
  • Added clarifications for reviewers via simplified tests.

Manual testing scenarios (*)

  1. Set locale to ar_MA, load MAD, format value 3
    Expected: 3,00 د.م.‏ (or د.م.‏ 3,00 depending on formatter direction, but stable symbol)
  2. Set locale to ar_EG, load EGP, format value 3
    Expected: 3,00 ج.م.‏
  3. Set locale to he_IL, load USD, format value 9999
    Expected: RTL-correct $ position with NBSP.
  4. Run unit tests on PHP 8.2, 8.3, 8.4
    All tests must pass consistently.

Additional context for reviewers

Arabic currency symbols are stable in ICU:

  • MAD → د.م.‏
  • EGP → ج.م.‏

Magento uses its own pattern, not ICU’s RTL rules:

sprintf("%s <symbol>", $value)

So changes in ICU bidi algorithms do NOT affect the expected test results.
The updated tests reflect what Magento actually does, not historical ICU side effects.


Contribution checklist (*)

  • Meaningful description
  • Accurate commit messages
  • Updated & stable unit tests
  • Automated tests green

@m2-assistant
Copy link

m2-assistant bot commented Nov 22, 2025

Hi @mimou78. Thank you for your contribution!
Here are some useful tips on how you can test your changes using Magento test environment.
❗ Automated tests can be triggered manually with an appropriate comment:

  • @magento run all tests - run or re-run all required tests against the PR changes
  • @magento run <test-build(s)> - run or re-run specific test build(s)
    For example: @magento run Unit Tests

<test-build(s)> is a comma-separated list of build names.

Allowed build names are:
  1. Database Compare
  2. Functional Tests CE
  3. Functional Tests EE
  4. Functional Tests B2B
  5. Integration Tests
  6. Magento Health Index
  7. Sample Data Tests CE
  8. Sample Data Tests EE
  9. Sample Data Tests B2B
  10. Static Tests
  11. Unit Tests
  12. WebAPI Tests
  13. Semantic Version Checker

You can find more information about the builds here
ℹ️ Run only required test builds during development. Run all test builds before sending your pull request for review.


For more details, review the Code Contributions documentation.
Join Magento Community Engineering Slack and ask your questions in #github channel.

@mimou78
Copy link
Contributor Author

mimou78 commented Nov 22, 2025

@magento run all tests

@mimou78 mimou78 marked this pull request as draft November 22, 2025 16:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant