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

[Question] How to get the currency code and symbol of an AbstractPrice? #941

Closed
Vinai opened this issue Jan 9, 2015 · 6 comments
Closed
Assignees
Labels
improvement Issue: Ready for Work Gate 4. Acknowledged. Issue is added to backlog and ready for development

Comments

@Vinai
Copy link
Contributor

Vinai commented Jan 9, 2015

During website implementations I often had to access currency information like the currency symbol of a price (e.g. for custom API resources, or when rendering a price on a graphical banner).

For that purpose the currency information for a given AbstractPrice instance should be made accessable for the current locale.
Currently this information is hidden 4 levels away:

  • Pricing\Price\AbstractPrice has a private Magento\Directory\Model\PriceCurrency
  • PriceCurrency gives access to Magento\Directory\Model\Currency via the public getCurrency()
  • Currency has a private Magento\Framework\Locale\Currency
  • Locale\Currency uses a CurrencyFactory to instantiate a Magento\Framework\Currency which is exposed via getCurrency() but requires a currency code as an argument.
  • Magento\Framework\Currency finally extends Zend_Currency and thus implements getSymbol()

Is there already a good way to access that information, for example in a price template?

If not, I think the cleanest way to access the data would be via getter methods on the AbstractPrice instances.
Ideally a method getCurrencySymbol would be added to the Magento\Framework\Pricing\Price\PriceInterface and implemented in AbstractPrice.

Would that be something you generally agree with?

@vkorotun
Copy link
Contributor

vkorotun commented Jan 9, 2015

hey @Vinai, sure generally we agree :)
I think that we'd better do not target PriceInterface since PriceInterface is a "data" interface and should provide only unique information of some particular price of some particular product.
We need to find some utility/service interface to extend. And the best candidate I see so far is \Magento\Framework\Pricing\PriceCurrencyInterface. We can make it available from templates and we may add method getCurrencySymbol to it.

Changes required:

  • \Magento\Framework\Pricing\Render\AmountRenderInterface add "getPriceCurrency" method
  • \Magento\Framework\Pricing\Render\AdjustmentRenderInterface add "getPriceCurrency" method
    (or introduce a shared interface for both to extend, since there are couple methods already they may share)
    and finally
  • \Magento\Framework\Pricing\PriceCurrencyInterface add "getCurrencySymbol" method, to do not enforce people calling a long chain of methods.

Is this sound like a good high level plan?

@Vinai
Copy link
Contributor Author

Vinai commented Jan 9, 2015

Good plan :)
I didn't want to suggest forcing people to call a long chain of methods, what I wanted to say was also that the intefaces would need to be extended somehow if there is no method already.

But your reply brings up another question, if you don't mind me asking.
The Magento\Framework\Pricing\Price\AbstractPrice class defines 2 public methods that are not part of the PriceInterface (getProduct and getQuantity).
What is the policy in regards to that?
If it is okay to add additional methods to abstract classes then why not simply add the getCurrencySymbol() method to Magento\Framework\Pricing\Price\AbstractPrice?

Generally I thin it might be problematic to expose methods that are not part of the interface.

@vkorotun
Copy link
Contributor

Good question.
I believe these two in particular are public by accident :)
In general, nothing wrong with having some public methods available in class, which are not described in implemented interface(s), so those could be available only for "internal" module classes, but as I said in this particular case we should make them protected, since there are no calls from outside of this class and it's successors. Since we need to add a method which should be available for external clients also (templates could be easily overloaded by any other module/theme) this is not our case.
I'll proceed with opening ticket.
Thank you, Vinai.

@vkorotun
Copy link
Contributor

Related ticket has been added to the backlog (MAGETWO-32756).
Thank you, for helping us making the product better :)

@vkorotun vkorotun added Issue: Ready for Work Gate 4. Acknowledged. Issue is added to backlog and ready for development and removed question labels Jan 15, 2015
@Vinai
Copy link
Contributor Author

Vinai commented Jan 15, 2015

Thanks!

@susantadas
Copy link

I want currency Symbol using currency code in magento 2. so any one help me?

magento-team pushed a commit that referenced this issue Apr 11, 2017
magento-engcom-team added a commit that referenced this issue Oct 2, 2019
 - Merge Pull Request magento/graphql-ce#941 from magento/graphql-ce:GraphQl-912-CustomerInput-improvements
 - Merged commits:
   1. 984ef45
   2. eb29ce7
   3. 8247d57
   4. f9ff771
   5. 30e8127
   6. be95cfa
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement Issue: Ready for Work Gate 4. Acknowledged. Issue is added to backlog and ready for development
Projects
None yet
Development

No branches or pull requests

6 participants