-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[Intl] Form Money type / decimal type without intl installed don't work #8588
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
Conversation
TerjeBr
commented
Jul 26, 2013
Q | A |
---|---|
Bug fix? | yes |
New feature? | no |
BC breaks? | no |
Deprecations? | no |
Tests pass? | yes |
Fixed tickets | #6045 |
License | MIT |
// Just a minimum implementation to make the "en" locale work | ||
// when the "intl" extension is not installed (Issue #6045) | ||
switch($attr) { | ||
case self::DECIMAL_SEPARATOR_SYMBOL: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cases should be indented
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, done.
This should be tested |
It would be great if someone else would step up and do some testing of this. |
return '$'; | ||
case self::EXPONENTIAL_SYMBOL: | ||
return 'E'; | ||
default: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is useless.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just being explicit that we do nothing in the default case. Some like that coding style, some don't.
This doesn't seem to fix the issue. After testing this i recieved: |
@@ -458,6 +458,25 @@ public function getPattern() | |||
*/ | |||
public function getSymbol($attr) | |||
{ | |||
// Need a minimum implementation to make the "en" locale work | |||
// when the "intl" extension is not installed (Issue #6045) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would remove this comment entirely. The whole stub intl is about providing a minimal implementation for the en locale. There is no need to comment it in this particular method
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It explains why this method is not left as just a throw new MethodNotImplementedException.
But if you do not like this comment, I can remove it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove it as it is the same than for other methods
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, will do that on next update
@quietkillah I do not know when what you mentioned here #8588 (comment) happend, but at least this PR is an improvement. |
; | ||
} | ||
|
||
// Apart from what is implemented above |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what are the missing symbols to be able to consider the implementation as complete (and so being able to properly return false
for any other input as done in the C implementation) ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You find them on lines 96-113 in the same file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couldn't we implement it then ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- I do not know which symbol to return for many of those constants.
- I thought this was to be only a minimum implementation to just support the use of the function in other parts of symfony. If an application programmer wants to use the function with the full range of possible values, he or she should install the php "intl" module.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
http://www.icu-project.org/apiref/icu4c/classDecimalFormat.html#details scroll down to "Special Pattern Characters"
I'm going to take over this PR as it should be done in the 2.2 branch instead and apparently, some more stubs are needed. |
This PR was merged into the 2.2 branch. Discussion ---------- [Locale] added some more stubs for the number formatter | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #8588, #6045 | License | MIT | Doc PR | n/a I've used this snippet of code to populate the default values for the en locale: ```php for ($style = 0; $style <= 8; $style++) { $f = new \NumberFormatter('en', $style); echo 'array('; for ($i = 0; $i <= 17; $i++) { echo "'".$f->getSymbol($i)."', "; } echo "),\n"; } ``` Commits ------- 3108c71 [Locale] added support for the position argument to NumberFormatter::parse() 0774c79 [Locale] added some more stubs for the number formatter