Skip to content

[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

Closed
wants to merge 3 commits into from
Closed

Conversation

TerjeBr
Copy link

@TerjeBr 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:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cases should be indented

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, done.

@stof
Copy link
Member

stof commented Jul 27, 2013

This should be tested

@TerjeBr
Copy link
Author

TerjeBr commented Jul 27, 2013

It would be great if someone else would step up and do some testing of this.

return '$';
case self::EXPONENTIAL_SYMBOL:
return 'E';
default:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is useless.

Copy link
Author

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.

@kristoffeys
Copy link

This doesn't seem to fix the issue. After testing this i recieved:
The Symfony\Component\Intl\NumberFormatter\NumberFormatter::parse() method's argument $position behavior is not implemented. Please install the "intl" extension for full localization capabilities.

@@ -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)
Copy link
Member

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

Copy link
Author

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.

Copy link
Member

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

Copy link
Author

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

@TerjeBr
Copy link
Author

TerjeBr commented Aug 8, 2013

@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
Copy link
Member

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) ?

Copy link
Author

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.

Copy link
Member

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 ?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. I do not know which symbol to return for many of those constants.
  2. 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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fabpot
Copy link
Member

fabpot commented Sep 22, 2013

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.

@fabpot fabpot closed this Sep 22, 2013
fabpot added a commit that referenced this pull request Sep 22, 2013
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants