WordPress.org

Make WordPress Core

Opened 5 years ago

Last modified 10 days ago

#13651 new defect (bug)

Problem with plural form in comments_number

Reported by: pavelevap Owned by: nbachiyski
Milestone: Future Release Priority: high
Severity: major Version: 3.0
Component: I18N Keywords: has-patch 4.3-early
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 (7)

13651.diff (2.5 KB) - added by zeo 4 years ago.
13651.2.diff (566 bytes) - added by markmcwilliams 4 years ago.
fixes the single comment number
comments-number-i18n.diff (17.1 KB) - added by nbachiyski 4 years ago.
comments-number-i18n-custom-parsing.diff (17.7 KB) - added by nbachiyski 4 years ago.
comments-number-i18n.2.diff (14.4 KB) - added by SergeyBiryukov 4 years ago.
comments-number-i18n-custom-parsing.2.diff (17.5 KB) - added by nbachiyski 4 years ago.
13651.3.diff (4.8 KB) - added by SergeyBiryukov 3 months ago.

Download all attachments as: .zip

Change History (61)

comment:1 @nacin5 years ago

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

comment:2 @wojtek.szkutnik5 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.

comment:3 @nacin5 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.

comment:4 @pavelevap5 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?

comment:5 follow-up: @nacin4 years ago

  • Milestone changed from Awaiting Triage to 3.1

@zeo4 years ago

comment:6 follow-up: @nbachiyski4 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

comment:7 in reply to: ↑ 5 @nbachiyski4 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.

comment:8 follow-up: @nacin4 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.

@markmcwilliams4 years ago

fixes the single comment number

comment:9 in reply to: ↑ 8 @markmcwilliams4 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?

comment:10 @nbachiyski4 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') );

comment:11 @nacin4 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.

comment:12 @nacin4 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.

comment:13 @nacin4 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.

comment:14 @nacin4 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.

comment:15 @nbachiyski4 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.

comment:16 @nbachiyski4 years ago

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

comment:18 follow-up: @nbachiyski4 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.

comment:19 follow-up: @ABTOP4 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).

comment:20 @SergeyBiryukov4 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 is probably should 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;

comment:21 @SergeyBiryukov4 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 4 years ago by SergeyBiryukov (previous) (diff)

comment:22 in reply to: ↑ 19 @SergeyBiryukov4 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.

comment:23 in reply to: ↑ 18 @SergeyBiryukov4 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.

comment:24 @nbachiyski4 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.

comment:25 follow-up: @SergeyBiryukov4 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, than 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.

comment:26 follow-up: @nbachiyski4 years ago

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

Any other objections/comments before committing it?

comment:27 in reply to: ↑ 26 ; follow-up: @westi4 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.

comment:28 in reply to: ↑ 27 @nbachiyski4 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.

comment:29 in reply to: ↑ 6 @westi4 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.

comment:30 @westi4 years ago

fix properly in 3.2 rather

comment:31 @nacin4 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.

comment:32 in reply to: ↑ description @SergeyBiryukov4 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).

comment:33 @pavelevap4 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)...

comment:34 @nacin4 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.

comment:35 @nacin4 years ago

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

comment:36 in reply to: ↑ 25 @SergeyBiryukov4 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.

comment:37 @holizz4 years ago

  • Cc tom@… added

comment:38 @pavelevap3 years ago

Bump. Any chance for WP 3.4?

comment:39 @SergeyBiryukov3 years ago

  • Milestone changed from Future Release to 3.4

comment:40 @jkudish3 years ago

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

comment:41 @nacin3 years ago

  • Milestone changed from 3.4 to Future Release

We're not ready yet for this one.

comment:42 @grapplerulrich3 years ago

Any chance of this being added to 3.5?

comment:43 @toscho2 years ago

  • Cc info@… added

comment:44 @lancewillett2 years ago

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

A related ticket came up for Twenty Thirteen: #23963

comment:45 @obenland2 years ago

  • Keywords 3.7-early added

comment:46 @wonderboymusic21 months ago

  • Milestone changed from Future Release to 3.7

these are all marked 3.7-early

comment:47 @wonderboymusic19 months ago

  • Keywords punt added

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

comment:48 @johnbillion19 months ago

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

comment:49 @pavelevap15 months ago

Any chance for 3.9?

comment:50 @slackbot3 months ago

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

@SergeyBiryukov3 months ago

comment:51 @SergeyBiryukov3 months 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 3 months ago by SergeyBiryukov (previous) (diff)

comment:52 @pavelevap3 months 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 :-)

comment:53 @slackbot6 weeks ago

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

comment:54 @DrewAPicture6 weeks 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 10 days ago by SergeyBiryukov (previous) (diff)
Note: See TracTickets for help on using tickets.