Skip to content
This repository has been archived by the owner on Nov 2, 2020. It is now read-only.

Added new filters localizednumber and localizedcurrency on Intl Extension #55

Closed
wants to merge 2 commits into from
Closed

Added new filters localizednumber and localizedcurrency on Intl Extension #55

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9794578
Added new filters localizednumber and localizedcurrency on Intl exten…
Garfield-fr Apr 30, 2012
4d137ff
Rename getNumberFormatter to twig_get_number_formatter
Garfield-fr May 1, 2012
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 52 additions & 2 deletions lib/Twig/Extensions/Extension/Intl.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ class Twig_Extensions_Extension_Intl extends Twig_Extension
{
public function __construct()
{
if (!class_exists('IntlDateFormatter')) {
if (!class_exists('IntlDateFormatter') || !class_exists('NumberFormatter')) {
throw new RuntimeException('The intl extension is needed to use intl-based filters.');
}
}
Expand All @@ -26,7 +26,9 @@ public function __construct()
public function getFilters()
{
return array(
'localizeddate' => new Twig_Filter_Function('twig_localized_date_filter'),
'localizeddate' => new Twig_Filter_Function('twig_localized_date_filter'),
'localizednumber' => new Twig_Filter_Function('twig_localized_number_filter'),
'localizedcurrency' => new Twig_Filter_Function('twig_localized_currency_filter'),
Copy link
Member

Choose a reason for hiding this comment

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

@fabpot should we rename them to localized_currency and so on ? It would be more readable

Copy link
Contributor

Choose a reason for hiding this comment

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

Can't we rename them with the same name we had in symfony 1.x: format_currency, format_date...?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why would you like to rename this ? I use the same syntax of the first filter "localizeddate".

In Twig, we are "number_format" on Twig Core Extension. It's not a good idea.

Copy link
Member

Choose a reason for hiding this comment

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

@hhamon keeping localized in the name is a good idea IMO. Twig already has filters to format date and numbers in the core. The difference here is that these filters are using intl

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@stof localizeddate is allready in this extension. It's not my add.

Copy link
Member

Choose a reason for hiding this comment

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

and I know it is the same syntax than the previous filter (added by myself following a discussion in the Twig issue tracker). But I find these names quite unreadable, which is why I suggest renaming them

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we can go for something like l10n_date?

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with @hhamon l10n prefix is shorter

Copy link
Contributor

Choose a reason for hiding this comment

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

Why not simply intl_date, intl_number, intl_currency? The advantages are obivous: Shorter and in reference to the intl Extension.

);
}

Expand Down Expand Up @@ -68,3 +70,51 @@ function twig_localized_date_filter($date, $dateFormat = 'medium', $timeFormat =

return $formatter->format($date->getTimestamp());
}

function twig_localized_number_filter($number, $style = 'decimal', $format = 'default', $currency = null, $locale = null )
Copy link
Member

Choose a reason for hiding this comment

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

unused argument $currency

Copy link
Member

Choose a reason for hiding this comment

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

and extra space before the closing brace

{
$formatter = getNumberFormatter(
$locale !== null ? $locale : Locale::getDefault(),
Copy link
Member

Choose a reason for hiding this comment

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

you can simplify it as twig_get_number_formatter($locale, $style) as twig_get_number_formatter already handles the default

$style
);

$formatValues = array(
'default' => NumberFormatter::TYPE_DEFAULT,
'int32' => NumberFormatter::TYPE_INT32,
'int64' => NumberFormatter::TYPE_INT64,
'double' => NumberFormatter::TYPE_DOUBLE,
'currency' => NumberFormatter::TYPE_CURRENCY,
);

return $formatter->format(
$number,
Copy link
Member

Choose a reason for hiding this comment

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

you should keep it on 1 line

$formatValues[$format]);
Copy link
Member

Choose a reason for hiding this comment

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

you should add some error handling for invalid formats

}

function twig_localized_currency_filter($number, $currency = null, $locale = null)
{
$formatter = getNumberFormatter(
$locale !== null ? $locale : Locale::getDefault(),
Copy link
Member

Choose a reason for hiding this comment

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

same here

'currency'
);

return $formatter->formatCurrency($number, $currency);
}

function getNumberFormatter($locale, $style)
Copy link
Member

Choose a reason for hiding this comment

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

all functions defined by twig are prefixed by twig_ to avoid conflicts. Please do it here too (and _twig_ may be better here as it is only an internal function, to be consistent with Twig itself)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK. I change this.

{
$styleValues = array(
'decimal' => NumberFormatter::DECIMAL,
'currency' => NumberFormatter::CURRENCY,
'percent' => NumberFormatter::PERCENT,
'scientific' => NumberFormatter::SCIENTIFIC,
'spellout' => NumberFormatter::SPELLOUT,
'ordinal' => NumberFormatter::ORDINAL,
'duration' => NumberFormatter::DURATION,
);

return NumberFormatter::create(
$locale !== null ? $locale : Locale::getDefault(),
$styleValues[$style]
Copy link
Member

Choose a reason for hiding this comment

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

you should handle invalid styles

);
}