fix: change translation logic to handle plurals better #49852
Conversation
Signed-off-by: SebastianKrupinski <krupinskis05@gmail.com>
SebastianKrupinski
requested review from
st3iny and
tcitworld
as code owners
|
/backport to stable30 |
|
Code added in 30, do not backport further. |
| [$startDate] | ||
| ), | ||
| ['minute', false] => $this->l10n->n( | ||
| 'In a minute on %1$s for the entire day', |
There was a problem hiding this comment.
| 'In a minute on %1$s for the entire day', | |
| 'In %n minute on %1$s for the entire day', |
If you use a plural, you must also use the %n placeholder.
Same for the rest of the changes :)
https://docs.nextcloud.com/server/latest/developer_manual/basics/front-end/l10n.html#correct-plurals
There was a problem hiding this comment.
See below
| return match ([$occurring['scale'], !empty($endTime)]) { | ||
| ['past', false] => $this->l10n->n( | ||
| 'In the past on %1$s for the entire day', | ||
| '', |
There was a problem hiding this comment.
This seems problematic. If the plural function of the user language considers $occurring['interval'] a plural then the string will be empty. If there is no number in the translation string just use t instead of n.
There was a problem hiding this comment.
This seems to work correctly as is...
Singular
Plural
There was a problem hiding this comment.
$occurring['interval'] = is a time interval (0+ integer) it will never be empty.
| return match ([$occurrenceIn['scale'], !empty($occurrence2), !empty($occurrence3)]) { | ||
| ['past', false, false] => $this->l10n->n( | ||
| 'In the past on %1$s', | ||
| '', |
There was a problem hiding this comment.
See comment above
| // In 2 minutes/hours/days/weeks/months/years on July 1, 2024 | ||
| // In 2 minutes/hours/days/weeks/months/years on July 1, 2024 then on July 3, 2024 | ||
| // In 2 minutes/hours/days/weeks/months/years on July 1, 2024 then on July 3, 2024 and July 5, 2024 | ||
| return match ([$occurrenceIn['scale'], !empty($occurrence2), !empty($occurrence3)]) { |
There was a problem hiding this comment.
would a null or empty string comparison work too to replace empty?
There was a problem hiding this comment.
Yes, a isset() will work also.
There was a problem hiding this comment.
Please use an explicit null and empty string comparison. Isset has unexpecte side effects too https://dev.to/aleksikauppila/using-isset-and-empty-hurts-your-code-aaa
Summary
Checklist