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

Add support for NumberFormatter symbol parameter #72

Closed
ste93cry opened this issue Jul 16, 2015 · 10 comments
Closed

Add support for NumberFormatter symbol parameter #72

ste93cry opened this issue Jul 16, 2015 · 10 comments

Comments

@ste93cry
Copy link
Contributor

Currently there is no way to change symbol attributes using the number_format_* methods in Twig templates. This feature can be useful in various cases, for example when the user wants to change the thousand separator or the decimal separator. OroCRM properly implemented this feature in their LocaleBundle, would it be possible to add it to SonataIntlBundle too?

@soullivaneuh
Copy link
Member

Feel free to make a PR. 👍

With concrete samples, before / after etc...

@ste93cry
Copy link
Contributor Author

I think this PR will be break compatibility for sure because it introduces a new parameter in all Twig filters. Is this ok? We could add the parameter to the end of the list instead of before the locale argument, but it wouldn't be so nice.

public function formatDecimal($number, array $attributes = array(), array $textAttributes = array(), $locale = null)

would be

public function formatDecimal($number, array $attributes = array(), array $textAttributes = array(), symbolAttributes = array(), $locale = null)

@soullivaneuh
Copy link
Member

Yes I understand the local.

Maybe we could find another way to keep bc like a new method with old one deprecation.

If not, this feature will be for next major version.

In any case, any PR is welcome.

@ste93cry
Copy link
Contributor Author

I will try to work on a PR (I already overridden a few classes to support this feature in my project) when I have some spare time, but I'm not an expert of PRs so be patient. About using a new method, I'm not sure it would be a good solution because there would be two filters for each formatter that differs only in parameters. I saw Symfony couting the number of args in some cases to maintain BC break while attending a new major version. Maybe we could do the same?

@soullivaneuh
Copy link
Member

I saw Symfony couting the number of args in some cases to maintain BC break while attending a new major version. Maybe we could do the same?

I'm curious. Could you provide a code sample link of this?

@ste93cry
Copy link
Contributor Author

Here it is. As you can see from the comment, they use the func_num_args function to support different function signatures and keep the compatibility with older versions.

@soullivaneuh
Copy link
Member

Seems to be a good alternative, let's try it.

@ste93cry
Copy link
Contributor Author

I've submitted a PR, although I noticed a few things that I have to fix before merging. Btw, if you want to do a code review and tell me what you think about how I tried to manage the BC it would be great

@OskarStark
Copy link
Member

@ste93cry would be good if you could prefix your PR with [WIP] if you work on it

thank you! 👍

@ste93cry ste93cry changed the title Add support for NumberFormatter symbol parameter [WIP] Add support for NumberFormatter symbol parameter Sep 30, 2015
@ste93cry ste93cry changed the title [WIP] Add support for NumberFormatter symbol parameter Add support for NumberFormatter symbol parameter Sep 30, 2015
@ste93cry
Copy link
Contributor Author

ste93cry commented Nov 8, 2015

@rande I don't want to sound rude, but can you please check my PR and eventually approve/disapprove it?

@rande rande closed this as completed in #77 Nov 26, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants