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

Extract currency bundle #1490

Merged
merged 11 commits into from
Jun 30, 2014

Conversation

pjedrzejewski
Copy link
Member

Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Fixed tickets -
License MIT
Doc PR -

Money should be only about money processing, especially that we use it in other bundles. I thought that extracting a CurrencyBundle and renaming the Money component to Currency makes sense.

Especially that we need more stuff, the base CurrencyContextInterface now lives in the component etc.

Services for updating exchange rates will follow soon, I need to do few tweaks.

Quick summary, cause I'm tired tonight:

  • Money bundle cares only about money formatting and forms.
  • ExchangeRate renamed to Currency and moved to CurrencyBundle.
  • Introduced CurrencyProviderInterface and appropriate service.
  • Money component renamed to Currency.

@mrbase
Copy link

mrbase commented May 12, 2014

Do the CurrencyBundle also allow for fixed prices ?
A lot of shops I know of would not allow prices based on calculated prices, and would rather go for a fixed price pr. currency and a fallback currency for countries "outside the box"

@pjedrzejewski
Copy link
Member Author

@mrbase Yes, I have a currency based pricing calculator in a separate branch. It allows you to define the price per currency on the products.

@mrbase
Copy link

mrbase commented May 12, 2014

cool, thanks for the quick answer :)

@stloyd
Copy link
Contributor

stloyd commented May 12, 2014

TBH. I don't see any real point in leaving the MoneyBundle, it's now so cut out that it's totally useless, it hold one Twig helper & one Form class...

Of course I like the overall rename, as it's more accurate to what it does.

@stloyd
Copy link
Contributor

stloyd commented May 12, 2014

Additionally can you leave those "services for updating exchange rates" for another PR? This would not pollute this PR, as it's big enough already IMO.

@pjedrzejewski
Copy link
Member Author

@stloyd

  1. The thing is that as we store the amounts in integers, we need a form type shared by all bundles operating on money. As you see, CurrencyBundle cleanly extends the MoneyBundle sylius_money form type to get the currency from the context, instead of container parameter.
  2. Yep, I will leave it for separate PR!

@jekill
Copy link

jekill commented Jun 16, 2014

This PR must be fix my problem with russian prices.
To resolve that in my fork I have extracted format pattern to parameters.
@pjedrzejewski how about to extract NumberFormatter to services?

@jekill
Copy link

jekill commented Jun 19, 2014

@pjedrzejewski very often in our projects we have several representations of one price.
For example:

"1020 руб."
"1020,00 руб."
"1020"
"One thousand twenty roubles " 
"1 020 р."
"1 020 ₽"
...

I think it would be useful to define price formats in the config file and optionally pass their codes to sylius_price.

{{ product.price|sylius_price('RUR','short') }}
{{ product.price|sylius_price('RUR','full') }}
{{ product.price|sylius_price('RUR','words') }}
....

What do you think about this idea?

@pjedrzejewski
Copy link
Member Author

@jekill I think this is excellent idea and should be relatively easy to implement, but I will leave this for another PR, cause this is one is big enough and took long to finish.

@pjedrzejewski pjedrzejewski changed the title [WIP] Extract currency bundle Extract currency bundle Jun 28, 2014
@pjedrzejewski pjedrzejewski mentioned this pull request Jun 30, 2014
pjedrzejewski pushed a commit that referenced this pull request Jun 30, 2014
@pjedrzejewski pjedrzejewski merged commit 72c8c28 into Sylius:master Jun 30, 2014
@stloyd stloyd mentioned this pull request Jul 8, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement Minor issues and PRs improving the current solutions (optimizations, typo fixes, etc.).
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants