Remove calls to old CLDR #12999

Quetzacoalt91 · 2019-03-21T17:53:12Z

Questions	Answers
Branch?	develop
Description?	The CLDR feature has been replaced in the core. This PR removes the old classes.
Type?	improvement
Category?	CO
BC breaks?	Yes, `Tools::getCldr()`, and all classes in `PrestaShop\PrestaShop\Core\Cldr`
Deprecations?	Nope
Fixed ticket?	Fixes #10052 and #12659
How to test?	Prices are displayed on the front office properly

This change is

classes/Tools.php

src/PrestaShopBundle/Resources/config/services/core/grid/doctrine_query_builder.yml

src/PrestaShopBundle/Resources/config/services/adapter/data_provider.yml

classes/Currency.php

classes/LocalizationPack.php

src/Core/Grid/Query/CurrencyQueryBuilder.php

src/Core/Localization/Currency.php

jolelievre · 2019-03-29T10:33:58Z

src/PrestaShopBundle/Resources/config/services/core/form/choice_provider.yml

@@ -158,7 +158,7 @@ services:
  prestashop.core.form.choice_provider.currency_name_by_iso_code:
    class: 'PrestaShop\PrestaShop\Core\Form\ChoiceProvider\CurrencyNameByIsoCodeChoiceProvider'
    arguments:
-      - '@=service("prestashop.core.cldr.repository").getAllCurrencies()'
+      - '@=service("prestashop.core.localization.cldr.locale_repository").getLocale(service("prestashop.adapter.legacy.context").getContext().language.getLocale()).getAllCurrencies()'


wow!! 🤪
I get the logic but it's quite a long expression, do you think there might a service that could do this more easily? Although I don't have the solution, it's a question? ^^

Well, I also thought about a dedicated service, but I wonder if it's a good idea.
As we rely on a context attribute (language), I do not want to see a service initialized too early and see it empty.

Yes I see the problem, maybe we would need a service with the legacy context, which would have getContextLocale getContextCurrency (rough idea..) which would be resolved on the fly
But anyway it's not the purpose of this PR

tomlev

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @eternoendless, @jolelievre, and @PierreRambaud)

src/Core/Localization/Specification/Number.php, line 332 at r10 (raw file):

Previously, eternoendless (Pablo Borowicz) wrote…

Why not add it in its constructor then?

that's was before, it makes CLDR reference and prestahop currency coupled. I think it is better to get price spec from the reference THEN override the precision.

Else we have to pass a not-required precision to construct the spec

eternoendless

Reviewed 1 of 1 files at r15.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @jolelievre and @PierreRambaud)

marionf · 2019-04-04T07:40:38Z

@mbadrani Could you launch CLDR E2E tests ?

eternoendless

Reviewed 5 of 25 files at r16, 3 of 8 files at r17, 5 of 9 files at r18, 5 of 6 files at r19, 4 of 4 files at r20, 1 of 3 files at r22, 1 of 1 files at r23, 1 of 1 files at r24.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @jolelievre, @mbadrani, and @PierreRambaud)

eternoendless · 2019-04-05T07:59:25Z

🎉 Thank you @Quetzacoalt91 and @tomlev!

prestonBot added develop Branch Improvement Type: Improvement labels Mar 21, 2019

Quetzacoalt91 force-pushed the remove-old-cldr branch 6 times, most recently from 0cf74fe to 0a0cdaa Compare March 25, 2019 15:00

Quetzacoalt91 marked this pull request as ready for review March 25, 2019 15:02

Quetzacoalt91 closed this Mar 25, 2019

Quetzacoalt91 reopened this Mar 25, 2019

Quetzacoalt91 force-pushed the remove-old-cldr branch 2 times, most recently from 98a4990 to 6731d77 Compare March 25, 2019 16:23

PierreRambaud reviewed Mar 26, 2019

View reviewed changes

classes/Tools.php Show resolved Hide resolved

PierreRambaud reviewed Mar 26, 2019

View reviewed changes

src/PrestaShopBundle/Resources/config/services/core/grid/doctrine_query_builder.yml Outdated Show resolved Hide resolved

PierreRambaud reviewed Mar 26, 2019

View reviewed changes

src/PrestaShopBundle/Resources/config/services/adapter/data_provider.yml Outdated Show resolved Hide resolved

Quetzacoalt91 force-pushed the remove-old-cldr branch 5 times, most recently from b840f5a to 719c77d Compare March 27, 2019 12:31

This was referenced Mar 27, 2019

fix currency install by getting some properties from CLDR reference #13060

Closed

Call CLDR update only if the class exists PrestaShop/autoupgrade#293

Merged

tomlev force-pushed the remove-old-cldr branch from bd14bdb to 5e90fc2 Compare March 29, 2019 09:40

jolelievre reviewed Mar 29, 2019

View reviewed changes

jolelievre previously approved these changes Mar 29, 2019

View reviewed changes

jolelievre added the Waiting for QA Status: action required, waiting for test feedback label Mar 29, 2019

Quetzacoalt91 removed the Waiting for QA Status: action required, waiting for test feedback label Mar 29, 2019

Quetzacoalt91 dismissed jolelievre’s stale review via fcd0b9d March 29, 2019 15:40

Quetzacoalt91 force-pushed the remove-old-cldr branch from fcd0b9d to aa49f04 Compare March 29, 2019 15:44

tomlev reviewed Apr 3, 2019

View reviewed changes

eternoendless previously approved these changes Apr 3, 2019

View reviewed changes

eternoendless added the Waiting for QA Status: action required, waiting for test feedback label Apr 3, 2019

marionf assigned mbadrani Apr 4, 2019

marionf added QA ✔️ Status: check done, code approved and removed Waiting for QA Status: action required, waiting for test feedback labels Apr 4, 2019

Quetzacoalt91 and others added 17 commits April 4, 2019 17:23

Remove old CLDR calls

4ecffb5

Remove old CLDR

d1c3bec

Update composer.lock without icanboogie lib

cb68ce6

Remove connexions between codebase & old CLDR

fa92cfe

fix currency install by getting some properties from CLDR reference

d360a88

Make kernel available in CLI install & tests

173205a

CS fixer and code review changes

7cc6813

fix cldr tests

c970c97

remove test double

4794451

Remove parentheses around single value parameter

0f5a4dc

Remove wrong data in database from unexpected modules

8e8ceb6

Do not erase existing format value of currency

7c1f5c6

Add deprecation notice in classes/Tools.php

a0c005a

add phpdoc

4d36dc7

old code removed - restored previous

0b28a8c

Apply CS fixer

38ea6d3

set setMaxFractionDigits as deprecated

c18de89

tomlev dismissed eternoendless’s stale review via c18de89 April 4, 2019 15:24

tomlev force-pushed the remove-old-cldr branch from 8e8cac0 to c18de89 Compare April 4, 2019 15:24

eternoendless approved these changes Apr 5, 2019

View reviewed changes

eternoendless merged commit 1085d7f into PrestaShop:develop Apr 5, 2019

eternoendless added the BC break Type: Introduces a backwards-incompatible break label Apr 12, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove calls to old CLDR #12999

Remove calls to old CLDR #12999

Quetzacoalt91 commented Mar 21, 2019 •

edited by colinegin

jolelievre Mar 29, 2019

Quetzacoalt91 Mar 29, 2019

jolelievre Mar 29, 2019

tomlev left a comment

eternoendless left a comment

marionf commented Apr 4, 2019

eternoendless left a comment

eternoendless commented Apr 5, 2019

Remove calls to old CLDR #12999

Remove calls to old CLDR #12999

Conversation

Quetzacoalt91 commented Mar 21, 2019 • edited by colinegin

jolelievre Mar 29, 2019

Choose a reason for hiding this comment

Quetzacoalt91 Mar 29, 2019

Choose a reason for hiding this comment

jolelievre Mar 29, 2019

Choose a reason for hiding this comment

tomlev left a comment

Choose a reason for hiding this comment

eternoendless left a comment

Choose a reason for hiding this comment

marionf commented Apr 4, 2019

eternoendless left a comment

Choose a reason for hiding this comment

eternoendless commented Apr 5, 2019

Quetzacoalt91 commented Mar 21, 2019 •

edited by colinegin