[ticket/10345] Add a system to allow multiple plural forms #363

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
2 participants
@p

p reviewed Sep 10, 2011
View changes

phpBB/includes/session.php
{
+ // This is basically just some lazy backwords compatible stuff.

This comment has been minimized.

Show comment Hide comment
@p

p Sep 10, 2011

Contributor

backwards

@p

p Sep 10, 2011

Contributor

backwards

This comment has been minimized.

Show comment Hide comment
@nickvergessen

nickvergessen Sep 10, 2011

Contributor

Fixed in new pr

@nickvergessen

nickvergessen Sep 10, 2011

Contributor

Fixed in new pr

@nickvergessen

This comment has been minimized.

Show comment Hide comment
@nickvergessen

nickvergessen Sep 10, 2011

Contributor

Pulled the wrong branch,
see phpbb#364 for correct solution

Contributor

nickvergessen commented Sep 10, 2011

Pulled the wrong branch,
see phpbb#364 for correct solution

+ * Determine which plural form we should use.
+ * For some languages this is not as simple as for English.
+ */
+ function get_plural_form($number, $force_rule = false)

This comment has been minimized.

Show comment Hide comment
@p

p Sep 10, 2011

Contributor

Parameters should be documented in the docblock.

Apparently there is a return value also, which should be documented as well.

@p

p Sep 10, 2011

Contributor

Parameters should be documented in the docblock.

Apparently there is a return value also, which should be documented as well.

+ case 0:
+ /**
+ * Families: Asian (Chinese, Japanese, Korean, Vietnamese), Persian, Turkic/Altaic (Turkish), Thai, Lao
+ * 1 - everything: 1, 2, ...

This comment has been minimized.

Show comment Hide comment
@p

p Sep 10, 2011

Contributor

This switch statement needs proper attribution, both for credit and so that we know whom to blame if they got any part of it wrong.

@p

p Sep 10, 2011

Contributor

This switch statement needs proper attribution, both for credit and so that we know whom to blame if they got any part of it wrong.

+
+ // Default to english system
+ $plural_rule = ($force_rule !== false) ? $force_rule : ((isset($this->lang['PLURAL_RULE'])) ? $this->lang['PLURAL_RULE'] : 1);
+ $plural_rule = max(0, min($plural_rule, 15));

This comment has been minimized.

Show comment Hide comment
@p

p Sep 10, 2011

Contributor

Unless there is an actual reason for this clamping, invalid input should be rejected.

The min part is in particular highly questionable, you're going to silently alter plural rule 16 into "icelandic".

@p

p Sep 10, 2011

Contributor

Unless there is an actual reason for this clamping, invalid input should be rejected.

The min part is in particular highly questionable, you're going to silently alter plural rule 16 into "icelandic".

+ return 0;
+ }
+
+ // Default to english system

This comment has been minimized.

Show comment Hide comment
@p

p Sep 10, 2011

Contributor

I can't see what $plural_rule is actually set to on github due to it cutting off the later part of that line, but in common.php below you default rule to 1 which is not English.

Also, if "English" stays it should be capitalized.

Adding a constant for the default rule seems like it might be useful.

@p

p Sep 10, 2011

Contributor

I can't see what $plural_rule is actually set to on github due to it cutting off the later part of that line, but in common.php below you default rule to 1 which is not English.

Also, if "English" stays it should be capitalized.

Adding a constant for the default rule seems like it might be useful.

This comment has been minimized.

Show comment Hide comment
@nickvergessen

nickvergessen Sep 10, 2011

Contributor

English is Rule #1 as Mozilla starts counting with 0,
see wiki article for details: http://wiki.phpbb.com/Plural_Rules#Rule_.231

@nickvergessen

nickvergessen Sep 10, 2011

Contributor

English is Rule #1 as Mozilla starts counting with 0,
see wiki article for details: http://wiki.phpbb.com/Plural_Rules#Rule_.231

This comment has been minimized.

Show comment Hide comment
@p

p Sep 10, 2011

Contributor

I think I misread this, default is 1 which is English.

Does everything work as expected if language pack provides no plural forms (i.e. any of the currently existing ones) and we treat it as English instead of no plurals (0)?

@p

p Sep 10, 2011

Contributor

I think I misread this, default is 1 which is English.

Does everything work as expected if language pack provides no plural forms (i.e. any of the currently existing ones) and we treat it as English instead of no plurals (0)?

This comment has been minimized.

Show comment Hide comment
@nickvergessen

nickvergessen Sep 10, 2011

Contributor

The code acts like it does now, the only thing that is a must is, that the lang-key is an array with at least one element.
The only thing I changed is to determinate which "plural"-case we use.

@nickvergessen

nickvergessen Sep 10, 2011

Contributor

The code acts like it does now, the only thing that is a must is, that the lang-key is an array with at least one element.
The only thing I changed is to determinate which "plural"-case we use.

@@ -54,5 +54,23 @@ class phpbb_user_lang_test extends phpbb_test_case
// Bug PHPBB3-9949
$this->assertEquals($user->lang('ARRY', 1, 2), '1 post');
$this->assertEquals($user->lang('ARRY', 1, 's', 2), '1 post');
+
+ // Bug PHPBB3-10345

This comment has been minimized.

Show comment Hide comment
@p

p Sep 10, 2011

Contributor

s/bug/ticket/ ?

@p

p Sep 10, 2011

Contributor

s/bug/ticket/ ?

This comment has been minimized.

Show comment Hide comment
@nickvergessen

nickvergessen Sep 10, 2011

Contributor

Shall i change the bug in the previous line as well?

@nickvergessen

nickvergessen Sep 10, 2011

Contributor

Shall i change the bug in the previous line as well?

This comment has been minimized.

Show comment Hide comment
@p

p Sep 10, 2011

Contributor

I read the diff before reading the ticket, and it looked like a new feature as opposed to a bug fix, thus my comment. But the ticket is actually a bug. But we now use "ticket" in commit messages for all kinds of tickets. I would say go with whichever makes more sense to you. Changing bug to ticket on 9949 is ok with me.

@p

p Sep 10, 2011

Contributor

I read the diff before reading the ticket, and it looked like a new feature as opposed to a bug fix, thus my comment. But the ticket is actually a bug. But we now use "ticket" in commit messages for all kinds of tickets. I would say go with whichever makes more sense to you. Changing bug to ticket on 9949 is ok with me.

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