Make WordPress Core

Opened 14 years ago

Closed 8 years ago

Last modified 8 years ago

#13651 closed defect (bug) (fixed)

Problem with plural form in comments_number

Reported by: pavelevap's profile pavelevap Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: 4.6 Priority: high
Severity: major Version: 3.0
Component: I18N Keywords: has-patch commit
Focuses: Cc:

Description

Hi, there is problem with comments_number in WordPress (and also Twentyten theme). There is possibility to use 0, 1 or more comments. But for Czech there are also two plural forms: 0 (žádný komentář), 1 (1 komentář), 2-4 (komentáře) and 5 and more (komentářů).

Function comments_number does not support it, also in Twentyten it is not possible...

Attachments (9)

13651.diff (2.5 KB) - added by zeo 13 years ago.
13651.2.diff (566 bytes) - added by markmcwilliams 13 years ago.
fixes the single comment number
comments-number-i18n.diff (17.1 KB) - added by nbachiyski 13 years ago.
comments-number-i18n-custom-parsing.diff (17.7 KB) - added by nbachiyski 13 years ago.
comments-number-i18n.2.diff (14.4 KB) - added by SergeyBiryukov 13 years ago.
comments-number-i18n-custom-parsing.2.diff (17.5 KB) - added by nbachiyski 13 years ago.
13651.3.diff (4.8 KB) - added by SergeyBiryukov 9 years ago.
13651.4.diff (4.9 KB) - added by SergeyBiryukov 8 years ago.
13651.5.diff (4.9 KB) - added by SergeyBiryukov 8 years ago.

Download all attachments as: .zip

Change History (81)

#1 @nacin
14 years ago

  • Component changed from General to i18n
  • Owner set to nbachiyski

#2 @wojtek.szkutnik
14 years ago

The problem is a bit more complex - in Polish, which is a language very similiar to Czech, not only 2-4 have the form "komentarze", but also every number larger than 20, with the last digit 2,3 or 4.

#3 @nacin
14 years ago

  • Milestone changed from 3.0 to 3.1

Per [14358] we're no longer using this in Twenty Ten.

Moving to 3.1 where we can change how we're handling this, probably by deprecating the function.

#4 @pavelevap
14 years ago

This problem still appears in loop.php:

<span class="comments-link"><?php comments_popup_link( __( 'Leave a comment', 'twentyten' ), __( '1 Comment', 'twentyten' ), __( '% Comments', 'twentyten' ) ); ?></span>

There is no possibility to have two plural forms, only one or more. It should be fixed as soon as possible...

BTW: What is the difference between TwentyTen 1.0 in trunk and 1.0.2 in Themes repository?

#5 follow-up: @nacin
13 years ago

  • Milestone changed from Awaiting Triage to 3.1

@zeo
13 years ago

#6 follow-up: @nbachiyski
13 years ago

(In [16614]) Make comments_popup_link() calls in Twenty Ten respect languages with multiple plural forms until we add such support in comments_number(). See #13651, props zeo

#7 in reply to: ↑ 5 @nbachiyski
13 years ago

  • Milestone changed from 3.1 to Future Release

It is too late to change or deprecate comments_number().

That's why I only committed a slightly modified version of zeo's patch, which fixes the problem for Twenty Ten.

#8 follow-up: @nacin
13 years ago

  • Milestone changed from Future Release to 3.1
  • Priority changed from normal to high

I would rather go ahead and replace the function rather than put that kind of logic in the default theme. Whenever we need to work around a deficiency in core, you know there's a problem.

@markmcwilliams
13 years ago

fixes the single comment number

#9 in reply to: ↑ 8 @markmcwilliams
13 years ago

Replying to nacin:

I would rather go ahead and replace the function rather than put that kind of logic in the default theme. Whenever we need to work around a deficiency in core, you know there's a problem.

Would tend to agree with Andrew here, I thought bugs were allowed to be made, it's not exactly an enhancement, more of a bug nobody had really found! :P Anyway, I uploaded a patch in case anything wasn't made, it updates the only comment from being % Comment to 1 Comment if we're rolling with that?

#10 @nbachiyski
13 years ago

I had do deprecate both comments_number() and comments_popup_link(), because both used the pseudo-plural style.

They are replaced by comments_number_text(), comments_number_link() and their get_* counterparts.

Here are example usages:

comments_number_text( __( 'Zero Comments' ), _n_noop( 'One Comment', '%s Comments' ) );	
comments_number_link( __( 'Zero Comments', 'twentyten' ), _n_noop( 'One Comment', '%s Comments', 'twentyten' ), __( 'Comments are as off as your as my sense of humour' ), array( 'class' => 'my-css-class') );

#11 @nacin
13 years ago

Looks good. Still feels a bit ugly, Nikolay and I agreed, so if anyone has any ideas, that'd be helpful.

Let's make sure that array( 'One Comment', '%s Comments') ) also works, as _n_noop() now returns an associative array.

Non-i18n examples:

comments_number_text( 'Zero Comments', array( 'One Comment', '%s Comments' ) );
comments_number_link( 'Zero Comments', array( 'One Comment', '%s Comments' ), 'Comments off.' );

I wish we could sanely add these functions directly to the pot generation, which would allow for comments_number_text( 'Zero Comments', 'One Comment', '%s Comments' ), then we could translate that in parts. The _n_noop() makes little sense to most -- it's an extremely rare function to use currently, outside of core.

Thoughts? String / array of two strings / string will trip a lot of people up. And i18n makes it very confusing beyond that.

#12 @nacin
13 years ago

Also, I don't like the changes to _n_noop() and _nx_noop() here -- we should still leave 0/1/2 indices for back compat.

#13 @nacin
13 years ago

(In [16726]) Fix comment number in Twenty Ten when there is one comment. Due to how these values are then passed to comments_popup_link(). We need to fix this. see #13651.

#14 @nacin
13 years ago

  • Priority changed from high to highest omg bbq
  • Severity changed from normal to major

We can't leave all this, in its current cryptic state, in the default theme.

#15 @nbachiyski
13 years ago

I played a lot with different variants. Here are some:

  • We can't put arrays in 2010, since most people will copy/paste it.
  • I like the idea of tying these functions into POT generation. This will need custom work, because we don't have a way to tell xgettext that the first argument is a standalone string and the second/third are plurals of a different one. In general it's good to have a custom POT generator, because we are already bending the xgettext capabilities. I will see what I can do.
  • We could try to think of a better name for _n_noop(). Nothing comes to mind, though.

#16 @nbachiyski
13 years ago

For the record, I started a a simple xgettext replacement in php, and I should have something by tomorrow.

#18 follow-up: @nbachiyski
13 years ago

Getting very close. Now I have makepot-tokenizer.php which doesn't depend on xgettext and produces the same set of strings as its xgettext counterpart.

Here is a proof-of-concept patch, which is incomplete (missing some comments and context version of comments_number_link()), but shows off the new way of calling the functions in the theme:

<span class="comments-link"><?php comments_number_link( 'Leave a comment', '1 Comment', '%s Comments', 'twentyten' ); ?></span>

Short and sweet.

The final leap is to add some functionality to the custom parser, to recognize two set of strings in one function call and to deploy it to the .org cluster, so that translators will get the new POT. ETA: Sunday evening or Monday morning.

#19 follow-up: @ABTOP
13 years ago

If it's any help, here's some insight into the complexity of this problem.

But the problem is actually way wider and potentially applicable to any noun (i.e. days, users, articles, categories etc.) not just comments.

Here's someRussians' attempts to deal with 1/2-4/5+ schema (Russian, Ukrainian, Serbian, Croatian).

#20 @SergeyBiryukov
13 years ago

Replying to wojtek.szkutnik:

The problem is a bit more complex - in Polish, which is a language very similiar to Czech, not only 2-4 have the form "komentarze", but also every number larger than 20, with the last digit 2,3 or 4.

This should probably be fixed by using the right Plural-Forms string in the .po file (from ABTOP's link above):

Plural-Forms: nplurals=3; plural=n==1 ? 0 : n%10>=2 && n%10<=4 && (n%100<10 || n%100>=20) ? 1 : 2;
Last edited 8 years ago by SergeyBiryukov (previous) (diff)

#21 @SergeyBiryukov
13 years ago

I'd like to suggest an approach based on Nikolay's first patch which seems more intuitive to me:

  • Doesn't require changes to _n_noop() or POT generation.
  • Keeps the usual logic of using domains only for i18n functions.
  • Therefore works with Poedit when updating from sources.

Example usage:

<span class="comments-link"><?php comments_number_link( __( 'Leave a comment', 'twentyten' ), _n( '1 Comment', '%s Comments', get_comments_number(), 'twentyten' ) ); ?></span>

The similar string is used in comments.php, which I consider as a valid example:

<h3 id="comments-title"><?php
printf( _n( 'One Response to %2$s', '%1$s Responses to %2$s', get_comments_number(), 'twentyten' ),
number_format_i18n( get_comments_number() ), '<em>' . get_the_title() . '</em>' );
?></h3>
Last edited 13 years ago by SergeyBiryukov (previous) (diff)

#22 in reply to: ↑ 19 @SergeyBiryukov
13 years ago

Replying to ABTOP:

If it's any help, here's some insight into the complexity of this problem.

But the problem is actually way wider and potentially applicable to any noun (i.e. days, users, articles, categories etc.) not just comments.

The problem is not that plural forms are not supported, it's that comments_popup_link() and comments_number() didn't use them correctly.

#23 in reply to: ↑ 18 @SergeyBiryukov
13 years ago

Replying to nbachiyski:

Getting very close. Now I have makepot-tokenizer.php which doesn't depend on xgettext and produces the same set of strings as its xgettext counterpart.

I guess comments-number-i18n.2.diff is too late considering all the hard work you have put into the new tokenizer, but I'd still love to get some feedback for learning purposes, if possible.

#24 @nbachiyski
13 years ago

The previous efforts put into something should affect our judgement of what's the best solution.

Sergey, compare:

comments_number_link( __( 'Leave a comment', 'twentyten' ), _n( '1 Comment', '%s Comments', get_comments_number(), 'twentyten' ) );

with:

comments_number_link( 'Leave a comment',  '1 Comment', '%s Comments', 'twentyten' );

The only drawback of the second variant is that the POT generation has to be done through our custom tools. This affects very few people and they (including you) should just start using makepot.php. It's not rocket science.

#25 follow-up: @SergeyBiryukov
13 years ago

The second example is obviously shorter.

This whole new concept of incapsulating i18n functions at first seemed odd to me for this specific function. If there will be others, then maybe I just need to get accustomed to it.

The only remaining concern is that I know some folks who rely on Poedit's “Update from sources” feature. Perhaps we should create docs for theme developers and translators about makepot.php.

Last edited 8 years ago by SergeyBiryukov (previous) (diff)

#26 follow-up: @nbachiyski
13 years ago

All the POT-generating infrastructure is in place and running.

Any other objections/comments before committing it?

#27 in reply to: ↑ 26 ; follow-up: @westi
13 years ago

Replying to nbachiyski:

All the POT-generating infrastructure is in place and running.

Any other objections/comments before committing it?

I assume you are referring to comments-number-i18n-custom-parsing.diff

I'm a little wary of reworking this code so close to release - I understand the desire to support the translation here better but I'm not sure this is the right point in the cycle to make these changes.

Other feedback:

  • _get_comments_number_translated_format should probably just be part of _get_comments_number_text_helper as it is only ever called from there as far as I can tell
  • I'm not a big fan of deprecating comments_number and comments_popup_link without trying to make them use the new functions if possible?
  • Need full phpdoc on the new functions
  • Not sure of the point of double apply_filters calls when they don't provide different contexts.
  • get_comments_number_link code for post_password_required is broken.

#28 in reply to: ↑ 27 @nbachiyski
13 years ago

Replying to westi:

  • _get_comments_number_translated_format should probably just be part of _get_comments_number_text_helper as it is only ever called from there as far as I can tell

It used to be used in another place. Why do you think it should be in the other functions? It does something specific and two short functions are easier to read than one bigger.

  • I'm not a big fan of deprecating comments_number and comments_popup_link without trying to make them use the new functions if possible?

I left them like this, exactly to prevent subtle changes of current functionality this late in the cycle. Apart from that I agree we should make them use the new ones.

Also, we should probably not deprecate them for now. Just introduce the new ones and use them in 2010. As you said, it's pretty late in the cycle.

  • Need full phpdoc on the new functions

Yes, I am on it.

  • Not sure of the point of double apply_filters calls when they don't provide different contexts.

Because usually these filters match the function name. In the documentation only comments_number_text will be advertised, but the old one should work too, for backwards compatibility.

  • get_comments_number_link code for post_password_required is broken.

What do you mean by broken? WFM.

It had an extra if-clause, which I fixed in the second patch.

#29 in reply to: ↑ 6 @westi
13 years ago

Replying to nbachiyski:

(In [16614]) Make comments_popup_link() calls in Twenty Ten respect languages with multiple plural forms until we add such support in comments_number(). See #13651, props zeo

I think we should probably revert this for 3.1 and fix properly in 3.1

These functions have been broken in this respect for a long time and we shouldn't been hacking around the brokenness in the Theme which is meant to serve as an example.

Instead we should fix this properly in the next cycle.

#30 @westi
13 years ago

fix properly in 3.2 rather

#31 @nacin
13 years ago

Per IRC I agree. I'm choosing readability over proper L10n, but it's only proper L10n in Twenty Ten. Other themes will need to continue with the same hack.

Suggest revert on [16614] and [16726] and handle 3.2-early.

#32 in reply to: ↑ description @SergeyBiryukov
13 years ago

Replying to pavelevap:

There is possibility to use 0, 1 or more comments. But for Czech there are also two plural forms: 0 (žádný komentář), 1 (1 komentář), 2-4 (komentáře) and 5 and more (komentářů).

Is “Komentářů (2)” or “Komentářů: 2” a valid sentence in Czech? Perhaps it can be a workaround until 3.2-early. I had been using similar strings in Russian package for a long time when correct plural forms were not available (e.g. in importers and in this particular case).

#33 @pavelevap
13 years ago

SergeyBiryukov: Yes, we use this sentence hack, but it is not the best way. Everyone understand it, but it is not the right solution.

I wrote also small plugin based on comments_number filter (checking number of comments and using str_replace), but it is also not the best solution.

It is common question in many Czech support forums, so I wanted to bring to light this problem (several years old)...

#34 @nacin
13 years ago

(In [17111]) Revert [16614] and [16614]. Deal with plural form issues in early 3.2 and remove the hacks from Twenty Ten. see #13651.

#35 @nacin
13 years ago

  • Keywords 3.2-early early added
  • Milestone changed from 3.1 to Future Release
  • Priority changed from highest omg bbq to high

#36 in reply to: ↑ 25 @SergeyBiryukov
13 years ago

Replying to SergeyBiryukov:

Perhaps we should create docs for theme developers and translators about makepot.php.

For the record: I've just found out that it's already documented. OK, then.

#37 @holizz
13 years ago

  • Cc tom@… added

#38 @pavelevap
12 years ago

Bump. Any chance for WP 3.4?

#39 @SergeyBiryukov
12 years ago

  • Milestone changed from Future Release to 3.4

#40 @jkudish
12 years ago

  • Keywords has-patch added; 3.2-early early removed

#41 @nacin
12 years ago

  • Milestone changed from 3.4 to Future Release

We're not ready yet for this one.

#42 @grapplerulrich
12 years ago

Any chance of this being added to 3.5?

#43 @toscho
11 years ago

  • Cc info@… added

#44 @lancewillett
11 years ago

What's the status of this one? 3.7 early now?

A related ticket came up for Twenty Thirteen: #23963

#45 @obenland
11 years ago

  • Keywords 3.7-early added

#46 @wonderboymusic
11 years ago

  • Milestone changed from Future Release to 3.7

these are all marked 3.7-early

#47 @wonderboymusic
11 years ago

  • Keywords punt added

Unless someone is full-throatedly behind this, will punt soon

#48 @johnbillion
10 years ago

  • Keywords 3.7-early punt removed
  • Milestone changed from 3.7 to Future Release

#49 @pavelevap
10 years ago

Any chance for 3.9?

This ticket was mentioned in Slack in #core by sergeybiryukov. View the logs.


9 years ago

#51 follow-up: @SergeyBiryukov
9 years ago

  • Milestone changed from Future Release to 4.2

Since this only affects a limited number of locales, I'd like to suggest what seems to be the least obtrusive fix here.

comments_popup_link() is used in each and every theme, so deprecating it is probably not an option. Instead, we could introduce a switch similar to the one we have for Open Sans font.

When enabled, the switch would override the theme's pseudo-plural '% Comments' string with a correct form of _n( '%s Comment', '%s Comments', $number ).

Only the actual word would be replaced, leaving surrounding tags and numbers as is. This works correctly with the standard '% Comments' string, as well as '%' or '<b>%</b> Replies' strings in Twenty Eleven. If the string does not contain a % placeholder, it would be left as is.

If the theme wants to use an i18n-friendly string for comment number, it should either rely on core strings (for Twenty Fourteen and Twenty Fifteen, simply removing the last two parameters would be enough), or use _n() for the $more parameter, like we do in comments_popup_link() now (see [31401] and [31402]).

13651.3.diff includes the proposed fix and unit tests with different strings from bundled themes.

This should eliminate the need for plugins like Multilingual Comments Number or Russify Comments Number, which were created solely to fix this issue. WordPress should not require a plugin just to display the number of comments correctly.

Last edited 9 years ago by SergeyBiryukov (previous) (diff)

#52 @pavelevap
9 years ago

Nice! I am not sure about right solution, in this case there will be new pluralized string used only by some locales? So there should be probably also some translator comment to explain this string. But I am really happy that this issue is moving :-)

This ticket was mentioned in Slack in #core by drew. View the logs.


9 years ago

#54 @DrewAPicture
9 years ago

  • Keywords 4.3-early added
  • Milestone changed from 4.2 to Future Release

Seems like the suggestion from @SergeyBiryukov is sound and we've got 13651.3.diff to implement it.

Unfortunately, I feel like we'd need more time to actually test it, and dropping this in on the eve of 4.2 beta 2 is probably not the greatest idea.

Last edited 9 years ago by SergeyBiryukov (previous) (diff)

#55 @obenland
9 years ago

  • Keywords 4.3-early removed
  • Milestone changed from Future Release to 4.3

This ticket was mentioned in Slack in #core by helen. View the logs.


9 years ago

#57 @helen
9 years ago

  • Milestone changed from 4.3 to Future Release

No real movement pre-4.3-beta, punting. @SergeyBiryukov - if you want to come back to this before beta tomorrow, it's still all yours.

#58 in reply to: ↑ 51 @SergeyBiryukov
8 years ago

Replying to SergeyBiryukov:

comments_popup_link() is used in each and every theme, so deprecating it is probably not an option.

Looks like we got away with deprecating wp_title() in #31078, so we might as well introduce a better alternative for comments_popup_link() too. See also comment:20:ticket:17763.

Still, 13651.3.diff seems worth trying, probably in early 4.5.

#59 @SergeyBiryukov
8 years ago

[35536] sets the correct default string, but we should still account for custom strings provided by themes.

Refreshed in 13651.4.diff.

This ticket was mentioned in Slack in #core-i18n by sergey. View the logs.


8 years ago

#61 @ocean90
8 years ago

  • Milestone changed from Future Release to 4.6
  • Owner changed from nbachiyski to ocean90
  • Status changed from new to reviewing

#62 follow-up: @ocean90
8 years ago

13651.4.diff looks good so far. I suggest some changes:

  • Move the $output = str_replace( '%', number_format_i18n( $number ), $more ); part into the declension condition.
  • The data provider for the test should include a case for "Hello % world" to test the screen-reader-text removal. Something like '% Comments<span class="screen-reader-text"> on Hello % world!</span>'.
  • The on/off flag needs a translator comment with the "Do not translate into your own language." hint, similar to wp_maybe_decline_date().
  • In test_get_comments_number_text_declension_with_custom_args() there is one space to much in the remove_filter() call.

#63 @ocean90
8 years ago

  • Owner changed from ocean90 to SergeyBiryukov

This ticket was mentioned in Slack in #core-i18n by ocean90. View the logs.


8 years ago

#65 in reply to: ↑ 62 @SergeyBiryukov
8 years ago

  • Keywords commit added

Replying to ocean90:

13651.4.diff looks good so far. I suggest some changes

Thanks for the review! 13651.5.diff addresses all the suggestions.

Created #37103 for a related "Hello % world!" issue.

#66 @ocean90
8 years ago

@SergeyBiryukov Is there anything missing which prevents a commit here?

This ticket was mentioned in Slack in #core-i18n by ocean90. View the logs.


8 years ago

#68 @SergeyBiryukov
8 years ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

In 37987:

I18N: Introduce an on/off switch for locales where comment number needs to be declined.

When enabled, the switch would override the theme's pseudo-plural '% Comments' string with a correct form of _n( '%s Comment', '%s Comments', $number ).

Historically, comments_popup_link() and get_comments_number_text() did not support plural forms and used a pseudo-plural style instead, so some locales were forced to come up with workarounds to display the number of comments in their language correctly.

This change should make those functions more i18n-friendly.

Fixes #13651.

#69 @SergeyBiryukov
8 years ago

In 37997:

Unit Tests: Add description for data_get_comments_number_text_declension().

See #13651.

#70 follow-up: @pavelevap
8 years ago

@SergeyBiryukov: We have first bug report with this fix :-(

Function: comments_popup_link("Komentáře (0)", "Komentáře (1)", "Komentáře (%)");

Strange results:

0 – Komentáře (0)
1 – Komentáře (1)
3 – 3 komentáře
9 – 9 komentářů

This ticket was mentioned in Slack in #core-i18n by pavelevap. View the logs.


8 years ago

#72 in reply to: ↑ 70 @SergeyBiryukov
8 years ago

Replying to pavelevap:

We have first bug report with this fix :-(

Thanks, created #37789 for that.

Note: See TracTickets for help on using tickets.