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

Replaced Tools::displayPrice deprecated in favor of Locale::formatPrice() #15448

Merged
merged 7 commits into from Oct 22, 2019

Conversation

@Progi1984
Copy link
Contributor

Progi1984 commented Sep 6, 2019

Questions Answers
Branch? develop
Description? Refactoring for a deprecated method
Type? refacto
Category? CO
BC breaks? no
Deprecations? no
Fixed ticket? Fixes #15427
How to test?

This change is Reviewable

@Progi1984 Progi1984 requested a review from PrestaShop/prestashop-core-developers Sep 6, 2019
@matks

This comment has been minimized.

Copy link
Contributor

matks commented Sep 9, 2019

@Progi1984 When a PR modifies several folders, we go for CO 😉(instead of Category? | FO / BO / TE)
The changelog requires we choose only one category

@LedCloud

This comment has been minimized.

Copy link
Contributor

LedCloud commented Sep 23, 2019

Why not to rewite only Tools::displayPrice?

old function still might be used by some external modules.

@jolelievre

This comment has been minimized.

Copy link
Contributor

jolelievre commented Sep 23, 2019

@LedCloud Tools::displayPrice has been deprecated because it doesn't respect the expected architecture any more. A deprecated function is meant to be removed in the next major version.

So although we did modify the function so that it calls the new expected way to display price (so that it still works for now), this method should not be used in any module nor in the core. That's why we must, at the minimum, show the example and remove its uses from the core code.

@PierreRambaud

This comment has been minimized.

Copy link
Contributor

PierreRambaud commented Sep 23, 2019

@Progi1984 Don't forget to squash your commit to prevent:
image

@Progi1984 Progi1984 force-pushed the Progi1984:issue15427 branch 5 times, most recently from c3715f9 to 45ed9ea Oct 4, 2019
@Progi1984 Progi1984 added the WIP label Oct 7, 2019
composer.json Outdated Show resolved Hide resolved
@Progi1984 Progi1984 force-pushed the Progi1984:issue15427 branch 2 times, most recently from 5e1df97 to c5ef817 Oct 8, 2019
@Progi1984 Progi1984 removed the WIP label Oct 8, 2019
@Progi1984 Progi1984 requested review from PrestaShop/prestashop-core-developers and PierreRambaud Oct 8, 2019
@Progi1984 Progi1984 added this to the 1.7.7.0 milestone Oct 8, 2019
@jolelievre

This comment has been minimized.

Copy link
Contributor

jolelievre commented Oct 14, 2019

@Progi1984 some conflicts to fix on this one ^^

@Progi1984 Progi1984 force-pushed the Progi1984:issue15427 branch 2 times, most recently from 87becfc to 7959413 Oct 17, 2019
@Progi1984

This comment has been minimized.

Copy link
Contributor Author

Progi1984 commented Oct 17, 2019

@jolelievre Fixed. Thanks

Copy link
Contributor

jolelievre left a comment

Sorry I read the whole PR and found out some things that could be improved ^^
Especially you inject prestashop.core.localization.locale.repository in many services whereas what you actually need is a Locale so I think it would be very handy to have a repository or a service able to return the Locale object linked to the context so you can inject it directly in services when you need it

@Progi1984

This comment has been minimized.

Copy link
Contributor Author

Progi1984 commented Oct 18, 2019

@jolelievre I fixed your feedback. Could you check ?

Copy link
Contributor

jolelievre left a comment

Hi, sorry a new idea of improvement to avoid creating too many Currency instances everwhere (see in the comment about adding a Currency::getIsoCodeById method)

About the injection issue, I saw your modif but the thing is you still inject the repository plus a string, whereas just the Local object should enough I don't think a service to get this object exist but I thought maybe we can use symfony services and factory to our advantage, I'm not sure if the syntax is correct but something like:

prestashop.core.localization.locale.context_locale:
        factory: ['@prestashop.core.localization.locale.repository', 'getLocale']
        class: PrestaShop\PrestaShop\Core\Localization\Locale
        arguments: ["@=service("prestashop.adapter.legacy.context").getContext().language.getLocale()"]

This way you could directly inject @prestashop.core.localization.locale.context_locale in the handlers What do you think?
This way we keep the handler constructor cleaner (only a Locale to inject), no need to create a new service, and we can also easily change the "service" in the future to get rid of the Context

classes/controller/AdminController.php Outdated Show resolved Hide resolved
classes/order/OrderHistory.php Outdated Show resolved Hide resolved
controllers/admin/AdminOrdersController.php Outdated Show resolved Hide resolved
@jolelievre

This comment has been minimized.

Copy link
Contributor

jolelievre commented Oct 18, 2019

Oh and I didn't comment them all but there are a few useless Tools::ps_round still present, you can easily find them searching them in the "Files changed" section

@Progi1984 Progi1984 force-pushed the Progi1984:issue15427 branch 2 times, most recently from 4cc805d to b6119f9 Oct 21, 2019
@Progi1984 Progi1984 requested a review from jolelievre Oct 21, 2019
Copy link
Contributor

jolelievre left a comment

Almost there 😅

classes/Currency.php Outdated Show resolved Hide resolved
src/Adapter/CombinationDataProvider.php Show resolved Hide resolved
@@ -273,7 +289,7 @@ private function getCustomerOrders(Customer $customer)
}
return new OrdersInformation(
Tools::displayPrice($totalSpent),
$this->locale->formatPrice($totalSpent, $this->context->getContext()->currency->iso_code),

This comment has been minimized.

Copy link
@jolelievre

jolelievre Oct 21, 2019

Contributor

I'm wondering if we shouldn't use the order currency I know it changes the initial behaviour, but maybe it was forgotten?

This comment has been minimized.

Copy link
@Progi1984

Progi1984 Oct 22, 2019

Author Contributor

I don't think it's a good idea. It is completely out of the scope of this PR.

This comment has been minimized.

Copy link
@Progi1984

Progi1984 Oct 22, 2019

Author Contributor

May be you could create a dedicated issue ?

This comment has been minimized.

Copy link
@matthieu-rolland

matthieu-rolland Oct 22, 2019

Contributor

I agree with @Progi1984 , if the QA team finds an issue related to this the whole PR could be blocked for a while...

By the way are there cases when the order currency is different than the context currency ?

This comment has been minimized.

Copy link
@jolelievre

jolelievre Oct 22, 2019

Contributor

Yes, I know it's not in this PR scope But it makes me wonder it would be worth investigating on the expected result here
And yes I think in some cases the currency can be different between the order and the context, especially in an Order history If it's the order you are purchasing then it should be the same logically

src/Adapter/Kpi/ShoppingCartTotalKpi.php Outdated Show resolved Hide resolved
src/Adapter/Product/AdminProductWrapper.php Outdated Show resolved Hide resolved
@Progi1984 Progi1984 force-pushed the Progi1984:issue15427 branch from 04a33b5 to 2aebc23 Oct 22, 2019
@Progi1984 Progi1984 force-pushed the Progi1984:issue15427 branch from 933a97e to d7b0b8e Oct 22, 2019
Copy link
Contributor

jolelievre left a comment

Congratulations!!! 🎉
Nice work!

Copy link
Contributor

matthieu-rolland left a comment

LGTM, well done

@Progi1984 Progi1984 self-assigned this Oct 22, 2019
@atomiix atomiix merged commit 25b3304 into PrestaShop:develop Oct 22, 2019
2 checks passed
2 checks passed
PrettyCI Code formatting
Details
Travis CI - Pull Request Build Passed
Details
@Robin-Fischer-PS

This comment has been minimized.

Copy link

Robin-Fischer-PS commented Oct 22, 2019

Hello guys ! Nice try avoiding the QA 😛

#16068

(if it's not from this PR, blame @matks , he's the one who told me that)

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.