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.

Sign up for GitHub

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

Jump to bottom

[Intl] Form Money type / decimal type without intl installed don't work #8588

Closed
wants to merge 3 commits into from
Closed

[Intl] Form Money type / decimal type without intl installed don't work #8588

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
39053b6
Stub: Form Money type / decimal type without intl installed don't work
Jul 26, 2013
c348d85
Formatting and comments
Jul 27, 2013
5caed4f
Formatting
Jul 27, 2013
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
19 changes: 19 additions & 0 deletions src/Symfony/Component/Intl/NumberFormatter/NumberFormatter.php
View file
Open in desktop
Expand Up @@ -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

switch ($attr) {
case self::DECIMAL_SEPARATOR_SYMBOL:
return '.';
case self::GROUPING_SEPARATOR_SYMBOL:
return ',';
case self::DIGIT_SYMBOL:
return '#';
case self::CURRENCY_SYMBOL:
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.

;
}

// 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.

http://www.icu-project.org/apiref/icu4c/classDecimalFormat.html#details scroll down to "Special Pattern Characters"

// this function is to be consiered as not implemented
Copy link
Contributor

Choose a reason for hiding this comment

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

consiered -> considered

throw new MethodNotImplementedException(__METHOD__);
Copy link
Member

Choose a reason for hiding this comment

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

This method is implemented, so it should not throw this exception. the php documentation says the method returns false on error (such as unknown attribute)

Copy link
Author

Choose a reason for hiding this comment

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

No, it is not really implemented, it should still be considered as not implemented by application writers.

To return false would be wrong, since it would reflect an incomplete and in those cases a wrong implementation.

The function is partly implemented just to support symfonys own internal use of it when the PHP 'intl' extension is not there.

Copy link
Member

Choose a reason for hiding this comment

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

Well, does it support all valid symbols or no ?

Being implemented only for en is not a reason to mark it as not implemented. None of the stub classes will ever be implemented for other locales

Copy link
Author

Choose a reason for hiding this comment

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

Den 08. aug. 2013 16:06, skrev Christophe Coevoet:

In src/Symfony/Component/Intl/NumberFormatter/NumberFormatter.php:

  •    // when the "intl" extension is not installed (Issue #6045)

  •    switch($attr) {

  •    case self::DECIMAL_SEPARATOR_SYMBOL:

  •        return '.';

  •    case self::GROUPING_SEPARATOR_SYMBOL:

  •        return ',';

  •    case self::DIGIT_SYMBOL:

  •        return '#';

  •    case self::CURRENCY_SYMBOL:

  •        return '$';

  •    case self::EXPONENTIAL_SYMBOL:

  •        return 'E';

  •    default:

  •        ;

  •    }
    • throw new MethodNotImplementedException(METHOD);

Well, does it support all valid symbols or no ?

No, the current implementation does not support all valid symbols for
the "en" locale.

Being implemented only for |en| is not a reason to mark it as not
implemented. None of the stub classes will ever be implemented for
other locales

I would agree with that.

Terje Bråten.

}

Expand Down