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

New CLDR implementation #10455

Merged
merged 52 commits into from Feb 21, 2019

Conversation

@tomlev
Copy link
Member

commented Sep 17, 2018

Questions Answers
Branch? develop
Description? restore CLDR feature with fixes on BO and FO
Type? new feature
Category? CO
BC breaks? The Currency ObjectModel is now multilingual
Deprecations? Tools::displayPrice() & Tools::displayNumber()
Fixed ticket? Fixes #11019, Fixes #11558
How to test? install and test with multiple languages, especially prices diplay. Install new language. Test by switching language / currency to be sure price display are correct

This change is Reviewable

@tomlev tomlev changed the title CLDR fixes restore CLDR and fix it Sep 17, 2018
classes/Tools.php Outdated Show resolved Hide resolved
@jolelievre

This comment has been minimized.

Copy link
Contributor

commented Sep 17, 2018

@tomlev huge work! took me a while to read it all, I lost my motivation when I got to the tests part sorry ^^ A few comments for you in the review

@tomlev tomlev force-pushed the tomlev:cldr-take-3 branch from d10635d to e46de08 Feb 15, 2019
@marionf

This comment has been minimized.

Copy link
Contributor

commented Feb 19, 2019

@mbadrani @ansar21mallouli
Can we launch the full test suit on this PR ?

@ansar21mallouli

This comment has been minimized.

Copy link
Contributor

commented Feb 21, 2019

@marionf @mbadrani It'is fine for the campaign full, there is no new regression.
Thanks!

@marionf marionf added QA ✔️ and removed waiting for QA labels Feb 21, 2019
@PierreRambaud PierreRambaud dismissed eternoendless’s stale review Feb 21, 2019

Because it was one month ago, and everything must be ok now

@PierreRambaud

This comment has been minimized.

Copy link
Contributor

commented Feb 21, 2019

Waiting for @eternoendless

Copy link
Member

left a comment

There are still some minor problems but we can merge this as long as those issues are addressed before freeze.
This PR won't resolve #10052 because Icanboogie CLDR (the previous library) is still present and used at some points.

Reviewed 6 of 87 files at r26, 80 of 95 files at r27.
Reviewable status: all files reviewed, 85 unresolved discussions (waiting on @jolelievre, @marionf, @MatShir, @mickaelandrieu, @PierreRambaud, @Quetzacoalt91, and @tomlev)


classes/Tools.php, line 834 at r27 (raw file):

    {
        @trigger_error(
            'Tools::displayNumber() is deprecated since version 1.7.5.0. '

This must be changed to 1.7.6.0


src/Core/Localization/CLDR/Reader.php, line 563 at r27 (raw file):

                }

                if (!empty($fractionsData)) {

I guess this could be the else of the previous if


tests-legacy/Integration/Core/Localization/LocaleUsageTest.php, line 319 at r27 (raw file):

                'currencyCode' => 'INR',
                'formattedPrice' => '12,34,568.12₹',
                //'nativeFormattedPricey' => '১২,৩৪,৫৬৮.১২₹',

This would be the result when using intl?

@PierreRambaud PierreRambaud merged commit dd85299 into PrestaShop:develop Feb 21, 2019
1 of 2 checks passed
1 of 2 checks passed
code-review/reviewable 85 discussions left (jolelievre, marionf, MatShir, mickaelandrieu, PierreRambaud, Quetzacoalt91, tomlev)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@PierreRambaud

This comment has been minimized.

Copy link
Contributor

commented Feb 21, 2019

Thanks @tomlev

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.