[Work in progress] I18n icu #917

Merged
merged 21 commits into from Oct 16, 2013

Conversation

Projects
None yet
6 participants
@samdark
Member

samdark commented Sep 28, 2013

  • Make tests pass on PHP 5.3 and PHP 5.4
  • add some more test for compex and edge cases
  • fix Yii code to use params w/o { and } in Yii::t.
- $message = $this->applyPluralRules($message, $params[0], $language);
- if (!isset($params['{n}'])) {
- $params['{n}'] = $params[0];
+ if (class_exists('MessageFormatter', false) && preg_match('~{\s*[\d\w]+\s*,~u', $message)) {

This comment has been minimized.

@qiangxue

qiangxue Sep 28, 2013

Member

Does it mean that every message translation will go through MessageFormatter? Will this cause performance issue?

@qiangxue

qiangxue Sep 28, 2013

Member

Does it mean that every message translation will go through MessageFormatter? Will this cause performance issue?

This comment has been minimized.

@samdark

samdark Sep 28, 2013

Member

No. There's regexp that checks if it's something more than just plain interpolation and only then calls MessageFormatter.

@samdark

samdark Sep 28, 2013

Member

No. There's regexp that checks if it's something more than just plain interpolation and only then calls MessageFormatter.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Sep 28, 2013

Coverage Status

Coverage increased (+0.07%) when pulling dafbeda on i18n-icu into b873f9f on master.

Coverage Status

Coverage increased (+0.07%) when pulling dafbeda on i18n-icu into b873f9f on master.

+```php
+$username = 'Alexander';
+echo \Yii::t('app', 'Hello, {username}!', array(
+ 'username' => $username,

This comment has been minimized.

@qiangxue

qiangxue Sep 28, 2013

Member

Is it {username} or username? If the latter, then we should fix all other similar usage in Yii.

@qiangxue

qiangxue Sep 28, 2013

Member

Is it {username} or username? If the latter, then we should fix all other similar usage in Yii.

This comment has been minimized.

@samdark

samdark Sep 28, 2013

Member

Latter.

@samdark

samdark Sep 28, 2013

Member

Latter.

docs/guide/i18n.md
+
+> **Tip**: When messages are extracted and passed to translator, he sees strings only. For the code above extracted message will be
+> "Balance: {0}". It's not recommended to use positional placeholders except when there's only one and message context is
+> clear as above.

This comment has been minimized.

@qiangxue

qiangxue Sep 28, 2013

Member

Why is it only recommended for a single parameter? What will happen if there are multiple parameters?

@qiangxue

qiangxue Sep 28, 2013

Member

Why is it only recommended for a single parameter? What will happen if there are multiple parameters?

This comment has been minimized.

@samdark

samdark Sep 28, 2013

Member

Technically nothing. It's just hard to get context for translator if he has no access to code.

@samdark

samdark Sep 28, 2013

Member

Technically nothing. It's just hard to get context for translator if he has no access to code.

framework/yii/i18n/MessageFormatter.php
+ /**
+ * Replace named placeholders with numeric placeholders.
+ *
+ * @param string $pattern The pattern string to relace things into.

This comment has been minimized.

@creocoder

creocoder Sep 28, 2013

Contributor

relace => replace

@creocoder

creocoder Sep 28, 2013

Contributor

relace => replace

@samdark samdark referenced this pull request in pradosoft/prado Sep 29, 2013

Open

Update I18n support #439

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Sep 29, 2013

Coverage Status

Coverage decreased (-0.04%) when pulling abc0df6 on i18n-icu into b873f9f on master.

Coverage Status

Coverage decreased (-0.04%) when pulling abc0df6 on i18n-icu into b873f9f on master.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Sep 29, 2013

Coverage Status

Coverage decreased (-0.11%) when pulling abc0df6 on i18n-icu into b873f9f on master.

Coverage Status

Coverage decreased (-0.11%) when pulling abc0df6 on i18n-icu into b873f9f on master.

+ * @param array $args The array of values to insert into the format string.
+ * @return string The pattern string with placeholders replaced.
+ */
+ private static function replaceNamedArguments($pattern, $args)

This comment has been minimized.

@samdark

samdark Sep 30, 2013

Member

Need help with this one. See broken tests with PHP 5.3 or 5.4.

@samdark

samdark Sep 30, 2013

Member

Need help with this one. See broken tests with PHP 5.3 or 5.4.

This comment has been minimized.

@cebe

cebe Oct 16, 2013

Member

Came up with a solution to go through the pattern string and replace all relevant parts but leave out others. (3h work) Tests are passing :)

@cebe

cebe Oct 16, 2013

Member

Came up with a solution to go through the pattern string and replace all relevant parts but leave out others. (3h work) Tests are passing :)

@samdark samdark referenced this pull request in auraphp/Aura.Intl Oct 10, 2013

Closed

Select format isn't supported by IntlFormatter #6

- $params['{n}'] = $params[0];
+ if (class_exists('MessageFormatter', false) && preg_match('~{\s*[\d\w]+\s*,~u', $message)) {
+ $formatter = new MessageFormatter($language, $message);
+ if ($formatter === null) {

This comment has been minimized.

@cebe

cebe Oct 15, 2013

Member

how can $formatter be null here?

@cebe

cebe Oct 15, 2013

Member

how can $formatter be null here?

This comment has been minimized.

@samdark

samdark Oct 15, 2013

Member

It's null if pattern is incorrect and can't be consumed by ICU.

@samdark

samdark Oct 15, 2013

Member

It's null if pattern is incorrect and can't be consumed by ICU.

This comment has been minimized.

@cebe

cebe Oct 15, 2013

Member

Also if this class is our extended version of MessageFormatter? Am I right that this is only possible with PHP internal classes?

@cebe

cebe Oct 15, 2013

Member

Also if this class is our extended version of MessageFormatter? Am I right that this is only possible with PHP internal classes?

This comment has been minimized.

@samdark

samdark Oct 15, 2013

Member

Not sure about it. Try with '' as a message.

@samdark

samdark Oct 15, 2013

Member

Not sure about it. Try with '' as a message.

This comment has been minimized.

@cebe

cebe Oct 16, 2013

Member

verified. works.

@cebe

cebe Oct 16, 2013

Member

verified. works.

framework/yii/i18n/MessageFormatter.php
+ {
+ return (
+ !defined('INTL_ICU_VERSION') ||
+ version_compare(INTL_ICU_VERSION, '48.0.0', '<') ||

This comment has been minimized.

@cebe

cebe Oct 16, 2013

Member

Need to check whether 48 is correct here. On my system I get 4.8.1.1 when doing echo INTL_ICU_VERSION . "\n";
PHP Version is 5.3.10-1ubuntu3.8

@cebe

cebe Oct 16, 2013

Member

Need to check whether 48 is correct here. On my system I get 4.8.1.1 when doing echo INTL_ICU_VERSION . "\n";
PHP Version is 5.3.10-1ubuntu3.8

This comment has been minimized.

@samdark

samdark Oct 16, 2013

Member

On Windows:

  • 50.1.2 on 5.5
  • 49.1.2 on 5.3
  • 49.1.2 on 5.4
@samdark

samdark Oct 16, 2013

Member

On Windows:

  • 50.1.2 on 5.5
  • 49.1.2 on 5.3
  • 49.1.2 on 5.4

This comment has been minimized.

@iJackUA

iJackUA Oct 16, 2013

Contributor

also confirm on Ubuntu PHP Version => 5.5.3-1ubuntu2
php -r "echo INTL_ICU_VERSION;"

4.8.1.1

@iJackUA

iJackUA Oct 16, 2013

Contributor

also confirm on Ubuntu PHP Version => 5.5.3-1ubuntu2
php -r "echo INTL_ICU_VERSION;"

4.8.1.1

This comment has been minimized.

@cebe

cebe Oct 16, 2013

Member

@samdark how is the intl version relevant for the fix?

@cebe

cebe Oct 16, 2013

Member

@samdark how is the intl version relevant for the fix?

This comment has been minimized.

@cebe

cebe Oct 16, 2013

Member

looks like it is save to compare the number without dots:

4.8.1.1 -> 4811
50.1.2 -> 5012
49.1.2 -> 4912
@cebe

cebe Oct 16, 2013

Member

looks like it is save to compare the number without dots:

4.8.1.1 -> 4811
50.1.2 -> 5012
49.1.2 -> 4912

This comment has been minimized.

@cebe

cebe Oct 29, 2013

Member

Found the reason for this. They changed version numbering after 4.8 release: http://site.icu-project.org/download

@cebe

cebe Oct 29, 2013

Member

Found the reason for this. They changed version numbering after 4.8 release: http://site.icu-project.org/download

@ghost ghost assigned cebe Oct 16, 2013

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Oct 16, 2013

Coverage Status

Coverage increased (+0.91%) when pulling 150b936 on i18n-icu into b873f9f on master.

Coverage Status

Coverage increased (+0.91%) when pulling 150b936 on i18n-icu into b873f9f on master.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Oct 16, 2013

Coverage Status

Coverage increased (+0.97%) when pulling 2eb5abb on i18n-icu into b873f9f on master.

Coverage Status

Coverage increased (+0.97%) when pulling 2eb5abb on i18n-icu into b873f9f on master.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Oct 16, 2013

Coverage Status

Coverage increased (+1.24%) when pulling bbcee32 on i18n-icu into b873f9f on master.

Coverage Status

Coverage increased (+1.24%) when pulling bbcee32 on i18n-icu into b873f9f on master.

@samdark

This comment has been minimized.

Show comment
Hide comment
@samdark

samdark Oct 16, 2013

Member

Should we return $message here?

Should we return $message here?

@samdark

This comment has been minimized.

Show comment
Hide comment
@samdark

samdark Oct 16, 2013

Member

Should we return $message here?

Should we return $message here?

This comment has been minimized.

Show comment
Hide comment
@qiangxue

qiangxue Oct 16, 2013

Member

I think in both cases we should return $message since we already log the error message.
BTW, Yii::error() should be used.

Member

qiangxue replied Oct 16, 2013

I think in both cases we should return $message since we already log the error message.
BTW, Yii::error() should be used.

@qiangxue

This comment has been minimized.

Show comment
Hide comment
@qiangxue

qiangxue Oct 16, 2013

Member

What if $params is 0 or an empty string?

What if $params is 0 or an empty string?

This comment has been minimized.

Show comment
Hide comment
@samdark

samdark Oct 16, 2013

Member

0 is valid for number formatter. Should be check for null here, I think.

Member

samdark replied Oct 16, 2013

0 is valid for number formatter. Should be check for null here, I think.

This comment has been minimized.

Show comment
Hide comment
@qiangxue

qiangxue Oct 16, 2013

Member

null or empty array?

Member

qiangxue replied Oct 16, 2013

null or empty array?

This comment has been minimized.

Show comment
Hide comment
@cebe

cebe Oct 16, 2013

Member

oh yeah you are right. will fix this.

Member

cebe replied Oct 16, 2013

oh yeah you are right. will fix this.

This comment has been minimized.

Show comment
Hide comment
@samdark

samdark Oct 16, 2013

Member

Empty array is better.

Member

samdark replied Oct 16, 2013

Empty array is better.

@qiangxue

This comment has been minimized.

Show comment
Hide comment
@qiangxue

qiangxue Oct 16, 2013

Member

How can $formatter be null? Sorry for asking this again. Your previous answer is gone.

How can $formatter be null? Sorry for asking this again. Your previous answer is gone.

This comment has been minimized.

Show comment
Hide comment
@qiangxue

qiangxue Oct 16, 2013

Member

Have you tested that it may return null? I never know new xyz could be null...

Member

qiangxue replied Oct 16, 2013

Have you tested that it may return null? I never know new xyz could be null...

This comment has been minimized.

Show comment
Hide comment
@cebe

cebe Oct 16, 2013

Member

php internal class can do that. With plain php you can not make it null after you created it. really weird behaior though.
I have added a unit test that verifies it :)
I also did not believe it ;)

Member

cebe replied Oct 16, 2013

php internal class can do that. With plain php you can not make it null after you created it. really weird behaior though.
I have added a unit test that verifies it :)
I also did not believe it ;)

This comment has been minimized.

Show comment
Hide comment
@qiangxue

qiangxue Oct 16, 2013

Member

I see. Good to know that.

Member

qiangxue replied Oct 16, 2013

I see. Good to know that.

@qiangxue

This comment has been minimized.

Show comment
Hide comment
@qiangxue

qiangxue Oct 16, 2013

Member

Need to adjust existing Yii::t() uses because the parameters no longer need curly brackets in their names.

Need to adjust existing Yii::t() uses because the parameters no longer need curly brackets in their names.

This comment has been minimized.

Show comment
Hide comment
@cebe

cebe Oct 16, 2013

Member

will do.

Member

cebe replied Oct 16, 2013

will do.

This comment has been minimized.

Show comment
Hide comment
@cebe

cebe Oct 16, 2013

Member

The validators are currently using a simlar style but this is not given to Yii::t. it uses strtr directly.
Would be good to have icu formatting available there too. what do you think?

Member

cebe replied Oct 16, 2013

The validators are currently using a simlar style but this is not given to Yii::t. it uses strtr directly.
Would be good to have icu formatting available there too. what do you think?

This comment has been minimized.

Show comment
Hide comment
@samdark

samdark Oct 16, 2013

Member

Absolutely.

Member

samdark replied Oct 16, 2013

Absolutely.

This comment has been minimized.

Show comment
Hide comment
@cebe

cebe Oct 16, 2013

Member

Same thing is with the BaseListView and gridview

Member

cebe replied Oct 16, 2013

Same thing is with the BaseListView and gridview

cebe added some commits Oct 16, 2013

Merge branch 'master' into i18n-icu
* master: (103 commits)
  fixed broken test after whitespace changes in view
  Removed the extra EOLs.
  I add new line in methods that render code in the head and for body
  GII update button style
  GII create object button style [skip ci]
  Fixes #980: Changed the default way of generating action URLs for ActionColumn.
  View default value for $params
  adjusted cubrid version in schema quote
  removed cubrid env from tavis.yml
  simplified cubrid db install on travis
  fixed cubrid schema test for pdo type
  fixed validator test break
  added cubrid specific pdo type casting
  Updated FileValidator tests
  Better AR connection init in tests
  Removed @codeCoverageIgnore
  no xss for attribute error messages that contain {value}
  Clientvalidation {value} was not what has been validated
  moved getPdoType() to Schema.
  optimized datepick js code.
  ...
@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Oct 16, 2013

Coverage Status

Coverage increased (+0.51%) when pulling 677893e on i18n-icu into e574f10 on master.

Coverage Status

Coverage increased (+0.51%) when pulling 677893e on i18n-icu into e574f10 on master.

@samdark

This comment has been minimized.

Show comment
Hide comment
@samdark

samdark Oct 16, 2013

Member

Found a bug in PHP 5.5 in progress: https://bugs.php.net/bug.php?id=65920 Our wrapper fixes it.

Member

samdark commented Oct 16, 2013

Found a bug in PHP 5.5 in progress: https://bugs.php.net/bug.php?id=65920 Our wrapper fixes it.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Oct 16, 2013

Coverage Status

Coverage increased (+0.53%) when pulling da8f179 on i18n-icu into e574f10 on master.

Coverage Status

Coverage increased (+0.53%) when pulling da8f179 on i18n-icu into e574f10 on master.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Oct 16, 2013

Coverage Status

Coverage increased (+0.53%) when pulling da8f179 on i18n-icu into e574f10 on master.

Coverage Status

Coverage increased (+0.53%) when pulling da8f179 on i18n-icu into e574f10 on master.

@cebe

This comment has been minimized.

Show comment
Hide comment
@cebe

cebe Oct 16, 2013

Member

this could then be moved to the methods directly. avoid one method call.

this could then be moved to the methods directly. avoid one method call.

This comment has been minimized.

Show comment
Hide comment
@samdark

samdark Oct 16, 2013

Member

Done.

Member

samdark replied Oct 16, 2013

Done.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Oct 16, 2013

Coverage Status

Coverage increased (+0.52%) when pulling bf7a084 on i18n-icu into e574f10 on master.

Coverage Status

Coverage increased (+0.52%) when pulling bf7a084 on i18n-icu into e574f10 on master.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Oct 16, 2013

Coverage Status

Coverage increased (+0.58%) when pulling 71e2dcc on i18n-icu into e574f10 on master.

Coverage Status

Coverage increased (+0.58%) when pulling 71e2dcc on i18n-icu into e574f10 on master.

samdark added a commit that referenced this pull request Oct 16, 2013

Merge pull request #917 from yiisoft/i18n-icu
[Work in progress] I18n icu

@samdark samdark merged commit 3667e03 into master Oct 16, 2013

1 check was pending

default The Travis CI build is in progress
Details

@samdark samdark deleted the i18n-icu branch Oct 16, 2013

@samdark

This comment has been minimized.

Show comment
Hide comment
@samdark

samdark Oct 16, 2013

Member

@cebe thanks a lot for handling ICU expressions grammar. Merged.

Member

samdark commented Oct 16, 2013

@cebe thanks a lot for handling ICU expressions grammar. Merged.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Oct 16, 2013

Coverage Status

Coverage remained the same when pulling 6b65ef5 on i18n-icu into e574f10 on master.

Coverage Status

Coverage remained the same when pulling 6b65ef5 on i18n-icu into e574f10 on master.

@cebe

This comment has been minimized.

Show comment
Hide comment
@cebe

cebe Oct 16, 2013

Member

My Pleasure :)

Member

cebe commented Oct 16, 2013

My Pleasure :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment