Skip to content

Commit

Permalink
Fix plural messages not falling back to key.
Browse files Browse the repository at this point in the history 
Plural messages without a translated value did not correctly fallback to
the message key as singular messages do. This caused undesirable
behavior when working with plural messages.

I've shifted the plural form resolution into the Translator. Now that we
have a custom translator class this logic is better suited here instead
of having duplicate logic in each message formatter. Interestingly our
formatters had divergent logic for handling plurals which could cause
some tedious to fix issues for app developers.

I've not unset the `_singular` key in the Translator as userland formatters could be
relying on those keys being passed in.

Refs #10900
  • Loading branch information
@markstory
markstory committed Jul 15, 2017
1 parent 1205789 commit 31033e9
Show file tree
Hide file tree
Showing 6 changed files with 73 additions and 184 deletions.
22 changes: 1 addition & 21 deletions src/I18n/Formatter/IcuFormatter.php
View file
Expand Up @@ -30,34 +30,14 @@ class IcuFormatter implements FormatterInterface
* Returns a string with all passed variables interpolated into the original
* message. Variables are interpolated using the MessageFormatter class.
*
* If an array is passed in `$message`, it will trigger the plural selection
* routine. Plural forms are selected depending on the locale and the `_count`
* key passed in `$vars`.
*
* @param string $locale The locale in which the message is presented.
* @param string|array $message The message to be translated
* @param array $vars The list of values to interpolate in the message
* @return string The formatted message
*/
public function format($locale, $message, array $vars)
{
$isString = is_string($message);
if ($isString && isset($vars['_singular'])) {
$message = [$vars['_singular'], $message];
unset($vars['_singular']);
$isString = false;
}

if ($isString) {
return $this->_formatMessage($locale, $message, $vars);
}

if (!is_string($message)) {
$count = isset($vars['_count']) ? $vars['_count'] : 0;
unset($vars['_count'], $vars['_singular']);
$form = PluralRules::calculate($locale, $count);
$message = isset($message[$form]) ? $message[$form] : (string)end($message);
}
unset($vars['_singular'], $vars['_count']);

return $this->_formatMessage($locale, $message, $vars);
}
Expand Down
30 changes: 1 addition & 29 deletions src/I18n/Formatter/SprintfFormatter.php
View file
Expand Up @@ -28,42 +28,14 @@ class SprintfFormatter implements FormatterInterface
* Returns a string with all passed variables interpolated into the original
* message. Variables are interpolated using the sprintf format.
*
* If an array is passed in `$message`, it will trigger the plural selection
* routine. Plural forms are selected depending on the locale and the `_count`
* key passed in `$vars`.
*
* @param string $locale The locale in which the message is presented.
* @param string|array $message The message to be translated
* @param array $vars The list of values to interpolate in the message
* @return string The formatted message
*/
public function format($locale, $message, array $vars)
{
if (is_string($message) && isset($vars['_singular'])) {
$message = [$vars['_singular'], $message];
unset($vars['_singular']);
}

if (is_string($message)) {
return vsprintf($message, $vars);
}

if (isset($vars['_context']) && isset($message['_context'])) {
$message = $message['_context'][$vars['_context']];
unset($vars['_context']);
}

// Assume first context when no context key was passed
if (isset($message['_context'])) {
$message = current($message['_context']);
}

if (!is_string($message)) {
$count = isset($vars['_count']) ? $vars['_count'] : 0;
unset($vars['_singular']);
$form = PluralRules::calculate($locale, $count);
$message = $message[$form];
}
unset($vars['_singular']);

return vsprintf($message, $vars);
}
Expand Down
68 changes: 50 additions & 18 deletions src/I18n/Translator.php
View file
Expand Up @@ -23,6 +23,7 @@
use Aura\Intl\FormatterInterface;
use Aura\Intl\Package;
use Aura\Intl\TranslatorInterface;
use Cake\I18n\PluralRules;

/**
* Provides missing message behavior for CakePHP internal message formats.
Expand Down Expand Up @@ -126,25 +127,8 @@ public function translate($key, array $tokensValues = [])

// Check for missing/invalid context
if (isset($message['_context'])) {
$context = isset($tokensValues['_context']) ? $tokensValues['_context'] : null;
$message = $this->resolveContext($key, $message, $tokensValues);
unset($tokensValues['_context']);

// No or missing context, fallback to the key/first message
if ($context === null) {
if (isset($message['_context'][''])) {
$message = $message['_context'][''];
} else {
$message = current($message['_context']);
}
} elseif (!isset($message['_context'][$context])) {
$message = $key;
} elseif (is_string($message['_context'][$context]) &&
strlen($message['_context'][$context]) === 0
) {
$message = $key;
} else {
$message = $message['_context'][$context];
}
}

if (!$tokensValues) {
Expand All @@ -156,9 +140,57 @@ public function translate($key, array $tokensValues = [])
return $message;
}

// Singular message, but plural call
if (is_string($message) && isset($tokensValues['_singular'])) {
$message = [$tokensValues['_singular'], $message];
}

// Resolve plural form.
if (is_array($message)) {
$count = isset($tokensValues['_count']) ? $tokensValues['_count'] : 0;
$form = PluralRules::calculate($this->locale, $count);
$message = isset($message[$form]) ? $message[$form] : (string)end($message);
}

if (strlen($message) === 0) {
$message = $key;
}

return $this->formatter->format($this->locale, $message, $tokensValues);
}

/**
* Resolve a message's context structure.
*
* @param string $key The message key being handled.
* @param string|array $message The message content.
* @param array $vars The variables containing the `_context` key.
* @return string
*/
protected function resolveContext($key, $message, array $vars)
{
$context = isset($vars['_context']) ? $vars['_context'] : null;

// No or missing context, fallback to the key/first message
if ($context === null) {
if (isset($message['_context'][''])) {
return $message['_context'][''];
}

return current($message['_context']);
}
if (!isset($message['_context'][$context])) {
return $key;
}
if (is_string($message['_context'][$context]) &&
strlen($message['_context'][$context]) === 0
) {
return $key;
}

return $message['_context'][$context];
}

/**
* An object of type Package
*
Expand Down
36 changes: 0 additions & 36 deletions tests/TestCase/I18n/Formatter/IcuFormatterTest.php
View file
Expand Up @@ -50,26 +50,6 @@ public function testFormatSimple()
$this->assertEquals('1 Orange', $result);
}

/**
* Tests that plural forms can be selected using the PO file format plural forms
*
* @return void
*/
public function testFormatPlural()
{
$formatter = new IcuFormatter();
$messages = [
'{0} is 0',
'{0} is 1',
'{0} is 2',
'{0} is 3',
'{0} > 11'
];
$this->assertEquals('1 is 1', $formatter->format('ar', $messages, ['_count' => 1, 1]));
$this->assertEquals('2 is 2', $formatter->format('ar', $messages, ['_count' => 2, 2]));
$this->assertEquals('20 > 11', $formatter->format('ar', $messages, ['_count' => 20, 20]));
}

/**
* Tests that plurals can instead be selected using ICU's native selector
*
Expand Down Expand Up @@ -130,20 +110,4 @@ public function testBadMessageFormatPHP7()
$formatter = new IcuFormatter();
$formatter->format('en_US', '{crazy format', ['some', 'vars']);
}

/**
* Tests that it is possible to provide a singular fallback when passing a string message.
* This is useful for getting quick feedback on the code during development instead of
* having to provide all plural forms even for the default language
*
* @return void
*/
public function testSingularFallback()
{
$formatter = new IcuFormatter();
$singular = 'one thing';
$plural = 'many things';
$this->assertEquals($singular, $formatter->format('en_US', $plural, ['_count' => 1, '_singular' => $singular]));
$this->assertEquals($plural, $formatter->format('en_US', $plural, ['_count' => 2, '_singular' => $singular]));
}
}
79 changes: 0 additions & 79 deletions tests/TestCase/I18n/Formatter/SprintfFormatterTest.php
View file
Expand Up @@ -34,83 +34,4 @@ public function testFormatSimple()
$this->assertEquals('Hello José', $formatter->format('en_US', 'Hello %s', ['José']));
$this->assertEquals('1 Orange', $formatter->format('en_US', '%d %s', [1, 'Orange']));
}

/**
* Tests that plural forms are selected for the passed locale
*
* @return void
*/
public function testFormatPlural()
{
$formatter = new SprintfFormatter();
$messages = ['%d is 0', '%d is 1', '%d is 2', '%d is 3', '%d > 11'];
$this->assertEquals('1 is 1', $formatter->format('ar', $messages, ['_count' => 1]));
$this->assertEquals('2 is 2', $formatter->format('ar', $messages, ['_count' => 2]));
$this->assertEquals('20 > 11', $formatter->format('ar', $messages, ['_count' => 20]));
}

/**
* Tests that strings stored inside context namespaces can also be formatted
*
* @return void
*/
public function testFormatWithContext()
{
$messages = [
'simple' => [
'_context' => [
'context a' => 'Text "a" %s',
'context b' => 'Text "b" %s'
]
],
'complex' => [
'_context' => [
'context b' => [
0 => 'Only one',
1 => 'there are %d'
]
]
]
];

$formatter = new SprintfFormatter();
$this->assertEquals(
'Text "a" is good',
$formatter->format('en', $messages['simple'], ['_context' => 'context a', 'is good'])
);
$this->assertEquals(
'Text "b" is good',
$formatter->format('en', $messages['simple'], ['_context' => 'context b', 'is good'])
);
$this->assertEquals(
'Text "a" is good',
$formatter->format('en', $messages['simple'], ['is good'])
);

$this->assertEquals(
'Only one',
$formatter->format('en', $messages['complex'], ['_context' => 'context b', '_count' => 1])
);

$this->assertEquals(
'there are 2',
$formatter->format('en', $messages['complex'], ['_context' => 'context b', '_count' => 2])
);
}

/**
* Tests that it is possible to provide a singular fallback when passing a string message.
* This is useful for getting quick feedback on the code during development instead of
* having to provide all plural forms even for the default language
*
* @return void
*/
public function testSingularFallback()
{
$formatter = new SprintfFormatter();
$singular = 'one thing';
$plural = 'many things';
$this->assertEquals($singular, $formatter->format('en_US', $plural, ['_count' => 1, '_singular' => $singular]));
$this->assertEquals($plural, $formatter->format('en_US', $plural, ['_count' => 2, '_singular' => $singular]));
}
}
22 changes: 21 additions & 1 deletion tests/TestCase/I18n/I18nTest.php
View file
Expand Up @@ -280,7 +280,7 @@ public function testBasicTranslateFunctionsWithNullParam()
}

/**
* Tests the __() function on a plural key
* Tests the __() function on a plural key works
*
* @return void
*/
Expand Down Expand Up @@ -311,6 +311,18 @@ public function testBasicTranslatePluralFunction()
$this->assertEquals('1 red and blue are good', $result);
}

/**
* Tests the __n() function on singular keys
*
* @return void
*/
public function testBasicTranslatePluralFunctionSingularMessage()
{
I18n::defaultFormatter('sprintf');
$result = __n('No translation needed', 'not used', 1);
$this->assertEquals('No translation needed', $result);
}

/**
* Tests the __d() function
*
Expand All @@ -324,11 +336,13 @@ public function testBasicDomainFunction()
'Cow' => 'Le moo',
'The {0} is tasty' => 'The {0} is delicious',
'Average price {0}' => 'Price Average {0}',
'Unknown' => '',
]);

return $package;
});
$this->assertEquals('Le moo', __d('custom', 'Cow'));
$this->assertEquals('Unknown', __d('custom', 'Unknown'));

$result = __d('custom', 'The {0} is tasty', ['fruit']);
$this->assertEquals('The fruit is delicious', $result);
Expand All @@ -354,13 +368,19 @@ public function testBasicDomainPluralFunction()
'Cows' => [
'Le Moo',
'Les Moos'
],
'{0} years' => [
'',
''
]
]);

return $package;
});
$this->assertEquals('Le Moo', __dn('custom', 'Cow', 'Cows', 1));
$this->assertEquals('Les Moos', __dn('custom', 'Cow', 'Cows', 2));
$this->assertEquals('{0} years', __dn('custom', '{0} year', '{0} years', 1));
$this->assertEquals('{0} years', __dn('custom', '{0} year', '{0} years', 2));
}

/**
Expand Down

0 comments on commit 31033e9

Please sign in to comment.