[2.3] Added currency locale data, form type and validator#6566
[2.3] Added currency locale data, form type and validator#6566mvrhov wants to merge 1 commit intosymfony:masterfrom mvrhov:currency
Conversation
|
Big 👍 for this. Thanks @mvrhov! |
|
BTW: I have no idea why the php 5.3.x seg'faults when running tests on Travis. |
There was a problem hiding this comment.
It should probably be:
'choices' => Locale::getDisplayCurrencies(Locale::getDefault()),(use Symfony's Locale consistently)
There was a problem hiding this comment.
All other types: Country, Language, Locale, Date, Money.. also use the one from main ns.
There was a problem hiding this comment.
In that case I won't argue ;)
I still think we should use Symfony's Locale class. However, it should be used everywhere consistently (so if we're about to fix it, we should fix it in other types as well).
There was a problem hiding this comment.
@jakzal It doesn't really matter which of the two you use. If you think it is better, you can create a PR that fixes appearances of \Locale::get|setDefault to Symfony's equivalent.
|
@fabpot: Any chances of having this in 2.2? I really need this in my next project and I would be a shame if I had to maintain a 2.2 fork just because of this feature |
|
👍 |
There was a problem hiding this comment.
Well the truth is that the file is almost identical to other locale based validators. So basically I'm the author of the few lines that are different.
There was a problem hiding this comment.
I think it's ok that you are the author.
To me the author is the one who had the idea/designed it conceptually or contributed large parts of the code.
There was a problem hiding this comment.
Still you have edited the file, so you should at least add yourself as author (there can be more than one author).
|
Big 👍 |
|
Looks good apart from a few minor issues! |
There was a problem hiding this comment.
Sounds strange. Why not getCurrencyNames?
There was a problem hiding this comment.
Ah I see it's because \Locale also uses getDisplay....
|
This has been implemented as a part of #7386 |
|
Ok, I'll reopen and rebase this later today. |
|
This is now rebased |
There was a problem hiding this comment.
@bschussek All form types are without it. do you still want me to add it?
There was a problem hiding this comment.
bad examples are still bad examples :)
|
Thank you @mvrhov! :) 👍 |
|
Please let me know before merging so I can cherry pick this on master and do a clean PR. |
|
Looks good :) Not sure what you mean with your last comment, but do whatever you feel is necessary! |
Bug fix: no
Feature addition: yes
Backwards compatibility break: no
Symfony2 tests pass: yes
License of the code: MIT
Documentation PR: symfony/symfony-docs#2105