Make WordPress Core

Opened 11 years ago

Closed 8 years ago

Last modified 7 years ago

#22229 closed enhancement (duplicate)

Plurals in JavaScript

Reported by: nacin's profile nacin Owned by: swissspidy's profile swissspidy
Milestone: Priority: low
Severity: normal Version:
Component: I18N Keywords: has-patch dev-feedback needs-refresh
Focuses: Cc:

Description

This is something koopersmith needs in the media modal, and I've seen a few other recent use cases.

Attachments (7)

22229.diff (2.0 KB) - added by nacin 11 years ago.
22229.2.diff (2.0 KB) - added by nacin 11 years ago.
Alternate script loader syntax
js-plurals.diff (10.1 KB) - added by nbachiyski 11 years ago.
22229.3.diff (11.2 KB) - added by nbachiyski 10 years ago.
22229.4.diff (11.2 KB) - added by ericlewis 9 years ago.
22229.5.diff (13.6 KB) - added by ericlewis 9 years ago.
22229-unittests.diff (1.4 KB) - added by MikeHansenMe 9 years ago.
see #30284

Download all attachments as: .zip

Change History (60)

@nacin
11 years ago

@nacin
11 years ago

Alternate script loader syntax

#1 @scribu
11 years ago

  • Type changed from defect (bug) to enhancement

Why is wp_js_i18n_plural() even necessary? What does it do?

#2 @nacin
11 years ago

In WordPress, we register plural strings in a POT file like so:

_n( '%d string', '%d strings' );

English has two forms: a singular form (n = 1) and a plural form for n != 1. Other languages have anywhere from one (like Japanese) to four forms (like Slovenian). I'm sure you are familiar with Romanian's three forms.

Using _n() alone is not sufficient. You also have to pass the number of items, so the proper translation is chosen for the language to which you are translating. So, _n( '%d string', '%d strings', $number_of_strings );. And, you will get back a string with %d (or %s, if the number could be sufficiently large enough to possibly need comma separators with number_format_i18n()). This is why we then sprintf() the result. So:

printf( _n( '%d string', '%d strings', $number_of_strings ), $number_of_strings );

Or:

printf( _n( '%s string', '%s strings', $number_of_strings ), number_format_i18n( $number_of_strings ) );

For JavaScript, we obviously do not have access to core's i18n libraries. So we pass strings via the script-loader. For example, postL10n.publish is __( 'Publish' );. But in order to translate a plural, we need to know the count of items (n), which then gets passed to an expression to determine the plural form to be used based.

What wp_js_i18n_plural() does is provide a function in JS that can be called with the number of items, because these numbers are going to be dynamic. It does this by using the same plural expression (which takes n and returns the plural to use) and an array of all of the plural forms.

So rather than exampleL10n.numItems being '%d items', it can be this:

exampleL10n.numItems = function(n) {
	var i = (n != 1),
		t = {"0":"%d item","1":"%d items"};
	if (typeof i === 'boolean') i = i ? 1 : 0;
	return i > 1 ? t[1] : t[i];
}

And then instead of invoking exampleL10n.numItems, you invoke exampleL10n.numItems(5), and get back %d items'. Or, if called with (1)`, you'd get back '%d item'.

The patch needs some work. One, we need to decide whether the JS function should do the sprintf automatically (my example does not; the patch does). Two, some of this logic needs to make its way back into the pomo classes, as right now it is dependent on Gettext_Translations to function.

#3 follow-up: @scribu
11 years ago

Ok, so instead of generating a new JS function for each string, how about we make a single generic JS function that gets run for all the strings? Something like the following:

wp.i18n._n(exampleL10n.numItems, 5);

#4 @scribu
11 years ago

This would work, because the plural expression is the same for all the strings in a particular language.

We could even keep the pretty syntax, but implement the JS function generator in JS, rather than PHP. This would make the code both cleaner and more compact.

#5 @nbachiyski
11 years ago

Nacin, I like the idea.

A couple of notes on the implementation:

  • I would hide the big if in a Translations method like translate_entry_or_return_entry(), which instead of false returns the entry itself. This way you won't need the special case and the code will be easier to follow.
  • The default number of plurals and the default expression do not belong to this part of the code. We're dealing with JS here, not with gettext specifics. I have two ideas around this problem:
    1. Make nplurals_and_expression_from_header() and parenthesize_plural_exression() static. Then just call Gettext_Translations::nplurals_and_expression_from_header( $mo->get_header( 'Plural-Forms' ) );. Or better, create an instance method of Gettext_Translations called nplurals_and_expression(), which uses the header from the instance.
    2. We can create a NOOP_Gettext_Translations, which extends Gettext_Translations and all the functionality, except actually translating is still there. The note about nplurals_and_expression() still applies.
  • I agree with scribu that we shouldn't create a new function each time. I think we should keep the pretty syntax of numItems(5) (it's pretty and I don't need to remember any other function names) and we should have a function generator, building a single function and just varying on singular/plural.
Last edited 11 years ago by nbachiyski (previous) (diff)

#6 in reply to: ↑ 3 ; follow-up: @nacin
11 years ago

Replying to scribu:

Ok, so instead of generating a new JS function for each string, how about we make a single generic JS function that gets run for all the strings? Something like the following:

wp.i18n._n(exampleL10n.numItems, 5);

I've been playing around with different syntaxes but couldn't get it just right. This looks interesting, though I agree with nikolay that the pretty function syntax is quite nice. The patch was definitely written as a temporary rush-plurals-into-JS solution, not an API that would stand the test of time.

The main thing I couldn't figure out was where wp.i18n would live. A new file that is a dependency of common.js? If we print the plural expression through script-loader, we'd have to then eval() it, because it's an expression, not a string. So, we probably have to print it via some other means.

I'm curious what you see the role of NOOP_Gettext_Translations as. That would be used in place of NOOP_Translations, I imagine?

#7 follow-up: @scribu
11 years ago

I'm curious what you see the role of NOOP_Gettext_Translations as.

I don't think we've met. :)

The main thing I couldn't figure out was where wp.i18n would live. A new file that is a dependency of common.js?

It could be an inline script, as in the current patch, except it would be spit out only once (possibly in the header) and then used for all the strings.

#8 in reply to: ↑ 7 @nacin
11 years ago

Replying to scribu:

I'm curious what you see the role of NOOP_Gettext_Translations as.

I don't think we've met. :)

Sorry, that was in reply to nbachiyski.

#9 in reply to: ↑ 6 @nbachiyski
11 years ago

Replying to nacin:


I'm curious what you see the role of NOOP_Gettext_Translations as. That would be used in place of NOOP_Translations, I imagine?

Yes, it would contain gettext-specific methods, hence the name. Doesn't matter much, since I doubt somebody ever used this class outside of core.

#10 @nbachiyski
11 years ago

  • Owner set to nbachiyski
  • Status changed from new to accepted

#11 @nbachiyski
11 years ago

On our way back from the Summit we sat together with duck_ and thought a bit about that and wrote some of the code in planes and airports.

Architecture

We have a couple of pieces:

  • Factory JS function. wp.i18n.make_plural(translations, domain). Returns a function, which accepts a number and returns the proper plural form. It needs the domain, because different domains, even using the same language, can be using different plural rules for some weird reason. Also, this easily allows us to have untranslated domains without introducing a special factory function for English.
  • Around the factory we need two structures for plural information (number of plurals and the ternary operator logic). These are wp.i18n.domains_plural_info and wp.i18n.english_plural_info. We need the latter, so that we can handle untranslated strings within a domain.
  • We need a way to output the make_plural() calls in the localization object for a script. Here it gets tricky.
    • First, we would like to hide it in a function, because the Nacin's l10n_print_after hack is very error-prone, because it's long and not obvious, makes it hard to add more than one plural, and includes the object's name, which we will for sure forget to update when we rename the object. Let's call this function _n_js(). I added it to makepot.php.
    • Then, we need to assign the literal JavaScript, not a string to the key in the localization object. This proved hard, because we run json_encode() on the whole localization object and we don't have a way to tell it not to encode some values.
    • Enter WP_JS_Literal. This is a small class (+ a convenience function), which represents a literal JS value, which shouldn't be encoded. The easiest way to use it is to do:
      wp_localize_script( 'handle', array( 'name' => 'string', 'code' => wp_js_make_literal( 'f()' ) ), 'l10n' )
      l10n = {
      	name: 'string',
      	code: f()
      }
      
  • The next step is how to output the make_plural definition only once, without many hacks. Since the JS literal is essentially code, w can allow it to have a dependancy. Then, when used in localizing a script, we just add this dependancy to its parent script.
  • But we don't want another JS file, just for a couple lines of code, we want it inline. So, with a small change to the logic of printing scripts, we can make scripts with source of inline to only add its data and not try to load its src.

You can find tests in [UT1115] and [UT1116].

#12 @nacin
11 years ago

  • Type changed from enhancement to task (blessed)

#13 @nbachiyski
11 years ago

  • Keywords has-patch added

#14 @nacin
11 years ago

  • Priority changed from normal to low

This looks great. It's also quite a bit of extra API. Holding off here until we figure out exactly what we need plurals for in 3.5.

#15 @nacin
11 years ago

  • Milestone changed from 3.5 to Future Release
  • Type changed from task (blessed) to enhancement

We can pull this back in 3.5 if we have an urgent need.

#16 @pavelevap
11 years ago

There is for example string "selected". But in Czech it is 1 "zvolen", 2 "zvoleny" a 5 "zvoleno". We really need plurals in WP 3.5.

#17 @pavelevap
11 years ago

Is it possible to add plurals to string "selected"? Or change it to "%s selected" to allow playing with order?

#18 @pavelevap
11 years ago

Nice, my problem with "selected" string will be partially solved here: #22749

#19 @knutsp
11 years ago

Sorry, but lack of plurals is almost a blocker, at least seen from a translators view.

#20 @nacin
11 years ago

In 23075:

Media: Use '%d selected' for the selection string, and offer a comment to translators to help them find a workable solution with this would-be plural string. fixes #22749. see #22229.

#21 @nbachiyski
11 years ago

How about we get this early in 3.6?

#22 @nbachiyski
11 years ago

Ping. Let's commit this.

#23 @adamsilverstein
11 years ago

  • Cc adamsilverstein@… added

#24 @johnbillion
11 years ago

  • Cc johnbillion added

#25 @nacin
10 years ago

In 25711:

Remove tests for code still in development, see #22229. If accidentally run, they produce fatal errors.

props pauldewouters, no_fear_inc.

#26 @nbachiyski
10 years ago

Here is the previous patch, but with the tests included (and working on current trunk).

@nbachiyski
10 years ago

This ticket was mentioned in IRC in #wordpress-dev by helen. View the logs.


9 years ago

#28 @johnbillion
9 years ago

  • Keywords dev-feedback added
  • Milestone changed from Future Release to 4.1

#29 @ocean90
9 years ago

  • Keywords needs-refresh added

Latest patch needs a lot of docs, and lines like $this->add_data( $handle, 'data', 'dudu' ); should be removed.

@ericlewis
9 years ago

#30 @ericlewis
9 years ago

22229.4.diff applies spotlessly to trunk and removes testing.

#31 @nacin
9 years ago

It's been a few years, so I'd like to revisit 22229.4.diff with nikolay and see if it's the right approach. At the time I remember feeling like it was a bit over-engineered. It's definitely on the right track, though.

This ticket was mentioned in IRC in #wordpress-dev by ericandrewlewis. View the logs.


9 years ago

#33 @nbachiyski
9 years ago

At contributor day we sat with Eric Lewis and went through the patch. To make the decision whether to commit it or to simplify it, here are some of the design trade-offs we were facing in the form of two implementation options:

Option 0. (this patch)

  • A developer needs to only one function and to use it at wp_localize_script calls: key => _n_js( singular, plural, domain )
  • Using the plurals from JavaScript is incredibly easy, just call the value with the number as an argument to get the proper pluralized translation: ScriptData.pluralValue(5)
  • Allows anybody to add “inline” scripts and styles without a source file. Think document.getElementById( 'username' ).focus();. I am not sure how useful this will be in the future, may be YAGNI, outside of this case.
  • Allows anybody to add any literal JavaScript as part of the wp_localize_script array: key => wp_js_make_literal( text, deps ). See this post for a use-case. Also remember l10n_print_after? ;-)

Option 1. (possible simpler implementation)

  • Adding a plural via wp_localize_script is still simple: wp_localize_script calls: key => _n_js( singular, plural, domain )
  • Instead of adding a general method for adding literal JS inside wp_localize_script, we could make _n_js return a special structure and then developers in JS could call a special function, let’s say: wp.i18n.plural( ScriptData.pluralValue ). I think this is much worse developer experience, because they need to remember even more functions.
  • In order to have the wp.i18n.plural function loaded, we have two choices: either the developer adds the dependence manually, or wp_localize_script goes through its values and searches for special cues that a return value is the special plural structure, returned by _n_js(). This is what my patch does anyway, so we either significantly worse the developer experience, or face similar implementation complexity as the patch above.
  • We can avoid supporting “inline” scripts/styles by hacking the JS we need as direct add_data() calls.
  • We can avoid supporting specifying dependencies by just adding them manually after the add_data() calls.

Summary: the trade-off here are between the experience of the developer using the code and some extra complexity of the implementation.

@ericlewis
9 years ago

#34 @ericlewis
9 years ago

attachment:22229.5.diff is on the way towards documenting everything.

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


9 years ago

#36 @ocean90
9 years ago

  • Milestone changed from 4.1 to Future Release

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


9 years ago

#38 @boonebgorges
9 years ago

In 30517:

Remove skipped tests for unimplemented JS plural functions.

The tests have been added as a patch for the original ticket, #22229.

Props MikeHansenMe.
See #22229.

#39 @knutsp
9 years ago

What is left here @boonebgorges or @nbachiyski ?

#40 @nbachiyski
9 years ago

@knutsp, the lead devs need to make a decision about how/if do we implement this. I have explained the design trade-offs as I see them in https://core.trac.wordpress.org/ticket/22229#comment:33.

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


8 years ago

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


8 years ago

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


8 years ago

#45 @ocean90
8 years ago

  • Milestone changed from Future Release to 4.6

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


8 years ago

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


8 years ago

#48 @ocean90
8 years ago

  • Owner changed from nbachiyski to swissspidy
  • Status changed from accepted to assigned

See #20491.

#49 @nbachiyski
8 years ago

Yay :) @swissspidy are you planning to go with a similar approach or a totally custom JS-in-gettext one?

#50 @swissspidy
8 years ago

@nbachiyski The patch on #20491 is actually quite close to what I had in mind. Leveraging the learnings from this ticket combined with the powers of Jed, we could solve this using mostly JavaScript I think. Let me wrap my head around it properly though :)

#51 @swissspidy
8 years ago

  • Milestone 4.6 deleted
  • Resolution set to duplicate
  • Status changed from assigned to closed

We're focusing on #20491 right now to implement proper JavaScript internationalization.

This ticket was mentioned in Slack in #buddypress by r-a-y. View the logs.


7 years ago

#53 @johnbillion
7 years ago

In 40451:

Remove skipped tests for unimplemented JS plural functions.

See #22229.

Merges [30517] to the 4.0 branch.

See #40463

Note: See TracTickets for help on using tickets.