WordPress.org

Make WordPress Core

Opened 4 years ago

Last modified 3 months 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
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 (6)

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

Download all attachments as: .zip

Change History (55)

comment:1 nacin4 years ago

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

comment:2 wojtek.szkutnik4 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 nacin4 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 pavelevap4 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: nacin3 years ago

  • Milestone changed from Awaiting Triage to 3.1

zeo3 years ago

comment:6 follow-up: nbachiyski3 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 nbachiyski3 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: nacin3 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.

markmcwilliams3 years ago

fixes the single comment number

comment:9 in reply to: ↑ 8 markmcwilliams3 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 nbachiyski3 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 nacin3 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 nacin3 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 nacin3 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 nacin3 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 nbachiyski3 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 nbachiyski3 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: nbachiyski3 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: ABTOP3 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 SergeyBiryukov3 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 SergeyBiryukov3 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.

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>
Version 0, edited 3 years ago by SergeyBiryukov (next)

comment:22 in reply to: ↑ 19 SergeyBiryukov3 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 SergeyBiryukov3 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 nbachiyski3 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: SergeyBiryukov3 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: nbachiyski3 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: westi3 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 nbachiyski3 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 westi3 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 westi3 years ago

fix properly in 3.2 rather

comment:31 nacin3 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 SergeyBiryukov3 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 pavelevap3 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 nacin3 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 nacin3 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 SergeyBiryukov3 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 holizz3 years ago

  • Cc tom@… added

comment:38 pavelevap2 years ago

Bump. Any chance for WP 3.4?

comment:39 SergeyBiryukov2 years ago

  • Milestone changed from Future Release to 3.4

comment:40 jkudish2 years ago

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

comment:41 nacin2 years ago

  • Milestone changed from 3.4 to Future Release

We're not ready yet for this one.

comment:42 grapplerulrich19 months ago

Any chance of this being added to 3.5?

comment:43 toscho13 months ago

  • Cc info@… added

comment:44 lancewillett13 months ago

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

A related ticket came up for Twenty Thirteen: #23963

comment:45 obenland12 months ago

  • Keywords 3.7-early added

comment:46 wonderboymusic9 months ago

  • Milestone changed from Future Release to 3.7

these are all marked 3.7-early

comment:47 wonderboymusic7 months ago

  • Keywords punt added

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

comment:48 johnbillion7 months ago

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

comment:49 pavelevap3 months ago

Any chance for 3.9?

Note: See TracTickets for help on using tickets.