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.

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

Fix plural messages not falling back to key. #10911

Merged
merged 2 commits into from Jul 18, 2017
Merged

Fix plural messages not falling back to key. #10911

merged 2 commits into from Jul 18, 2017

Conversation

markstory
Copy link
Member

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

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
@codecov-io
Copy link

codecov-io commented Jul 15, 2017

Codecov Report

Merging #10911 into master will decrease coverage by 0.3%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #10911      +/-   ##
============================================
- Coverage     94.94%   94.63%   -0.31%     
+ Complexity    12138    12130       -8     
============================================
  Files           422      422              
  Lines         30170    31107     +937     
============================================
+ Hits          28645    29439     +794     
- Misses         1525     1668     +143
Impacted Files Coverage Δ Complexity Δ
src/I18n/Formatter/SprintfFormatter.php 100% <100%> (ø) 1 <0> (-8) ⬇️
src/I18n/Translator.php 95.91% <100%> (+1.04%) 23 <6> (+6) ⬆️
src/I18n/Formatter/IcuFormatter.php 71.42% <100%> (-12.58%) 5 <0> (-6)
src/Database/Statement/MysqlStatement.php 63.63% <0%> (-23.87%) 1% <0%> (ø)
src/Database/Schema/BaseSchema.php 83.33% <0%> (-4.55%) 16% <0%> (ø)
src/Database/Expression/BetweenExpression.php 85.71% <0%> (-1.79%) 13% <0%> (ø)
src/I18n/PluralRules.php 96.36% <0%> (-1.79%) 75% <0%> (ø)
src/Event/EventList.php 84% <0%> (-1.72%) 11% <0%> (ø)
src/TestSuite/EmailAssertTrait.php 71.42% <0%> (-1.65%) 26% <0%> (ø)
src/Database/Statement/StatementDecorator.php 96.36% <0%> (-1.64%) 25% <0%> (ø)
... and 295 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1205789...3091666. Read the comment docs.

return $key;
}
if (is_string($message['_context'][$context]) &&
strlen($message['_context'][$context]) === 0
Copy link
Member

Choose a reason for hiding this comment

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

This can be written as

if ($message['_context'][$context] === '')

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch!

@markstory markstory merged commit 6a3a70d into master Jul 18, 2017
@markstory markstory deleted the issue-10900 branch July 18, 2017 01:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants