remove eval from locale.module

A short summary of some things we talked about with @chx earlier:

- eval() is evil
- locale module is one of the very few places in core still using eval(), PHP.module is another one (so unlike the patch name, we are not eliminating the last one)
- to avoid using eval, @chx proposes we precompute a lookup table of results based on the formula
- @chx looked at the formulas in l10n_pconfig module and the plural form wiki at http://translate.sourceforge.net/wiki/l10n/pluralforms and concluded that we can keep most lookup tables pretty small (if you look at the formulas they repeat at most after 100, but most repeat patterns below that)
- therefore we can apply a lookup table and if we don't find the value in there, we can mod the number and try again, which should overall be much simpler than eval()-ing the formula all the time
- no performance tests so far to prove it actually performs faster

(Note I just summarized our discussion, was not involved in the making of the algorithm/patch).

This is being looked at the extended D8MI sprint in Denver in context of making it simple to provide examples for a UI for plural translation. (#1452188: New UI for string translation). Also discussed yesterday with some people vs. making Drupal 8 better support hiphop.

The hard coded plural processing as in Symphonys PluralizationRules (https://github.com/symfony/Translation/blob/master/PluralizationRules.php) can be partial solution for known languages. However for unknown languages we (may) need the flexibility a populated plural table can provide.

I have not tested the suggested patch but if we can (a) cover all operators supported (b) use a prepopulated table for lookup, then we can avoid both hardcoding some formulas and needing to compute for others, since we can do it all dynamic but still pretty fast. That was the conclusion of our discussion about the above with chx.

Suppose we should provide a predefined formula|rule for languages in standard_language_list() and use serialized tables as chx suggested for other languages.

Why maintain two ways in parallel?

In #1570346: Let Symfony Translation component handle language messages we discuss symfony Translation component which has PluralizationRules.

I like the idea of hardcoded rules but PluralizationRules allows for custom rules.

Can't we use that instead?

Added some links from here and other places into Issue Summary.

We need to re-roll this anyway, so I suggests to add serialized values for standard_language_list() and inherit them in locale_translation_plurals variable, also it's good question about how to convert it's value to CMI?

This is definitely not closed :)

1. Symfony has hard coded pluralization rules with support for custom.
2. We do have a PluralForms parser and Symfony does not have yet.

When we switch to Symfony Translation component we need implement a callable when we use https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Tra...

On http://localize.drupal.org/translate/languages we support for 104 languages.

Symfony has 95 langcodes in PluralizationRules.php

chx: I know you are pretty busy. Do you have any intermediate work on this that might help someone else help you? Or maybe this is ok for someone else to pick up?

OK. :)

This looks challenging, and next step might just be a reroll.

We should not have eval() in Drupal core like this.

Porting ...

Also put on sprint then.

This is an port of the #0 patch. It needs more work to get evaluatePlural() functioning.

Bad patch in #21. Corrected version here, still needs work.

Working patch without the compression suggested in #17.
Needs tests.

This should pass the test. Additional tests are not required, the existing tests have sufficient coverage IMHO.

+++ b/core/lib/Drupal/Component/Gettext/PoHeader.php
@@ -417,4 +417,102 @@ private function tokenizeFormula($formula) {
+  /**
+   * Evaluate the plural element stack using a plural value.
+   *
+   * Using an element stack, which represents a plural formula, we calculate
+   * which plural string should be used for a given plural value.

Can we have an example evaluation. The code is now hard to grasp.

+++ b/core/modules/locale/lib/Drupal/locale/PoDatabaseWriter.php
@@ -178,7 +178,7 @@ function setHeader(PoHeader $header) {
+          'formula' =>  $formula,

whitespace (2 spaces)

Added more comments (#27, #29), added a limiter to prevent endless loops (#30), added the compression logic (#17) and a bit of extra test cases.
Finally I've added code for the upgrade. But this still fails and I did not manage to find the cause of it:

@@ -475,10 +476,14 @@ function update_prepare_d8_language() {
     $javascript = array();
     $prefixes = array();
     $domains = array();
+    $header = new PoHeader;
     foreach ($languages as $language) {
+      $plural_forms = 'nplurals=' . $language->plurals . '; plural=' . $language->formula . ';';
+      $parsed = $header->parsePluralForms($plural_forms);
+      list($nplurals, $formula) = $parsed;
       $plurals[$language->language] = array(
-        'plurals' => $language->plurals,
-        'formula' => $language->formula,
+        'plurals' => $nplurals,
+        'formula' => $formula,
       );

Removed the surplus type casting.
Upgrade test still fails with:

Drupal\Core\Database\DatabaseExceptionWrapper: SQLSTATE[42S02]: Base table or view not found: 1146 Table 'drupal8.simpletest12875language' doesn't exist: SELECT * FROM {language} ORDER BY weight ASC, name ASC; Array ( ) in Drupal\Core\Database\Connection->query() (line 571 of /Users/erik/www/drupal8/core/lib/Drupal/Core/Database/Connection.php).
Drupal\Core\Database\Connection->query('SELECT * FROM {language} ORDER BY weight ASC, name ASC', Array, Array)
db_query('SELECT * FROM {language} ORDER BY weight ASC, name ASC')
language_list()
Drupal\language\HttpKernel\PathProcessorLanguage->__construct(Object, Object, Object)
ReflectionClass->newInstanceArgs(Array)
Symfony\Component\DependencyInjection\ContainerBuilder->createService(Object, 'path_processor_language')
Symfony\Component\DependencyInjection\ContainerBuilder->get('path_processor_language', 1)
Symfony\Component\DependencyInjection\ContainerBuilder->resolveServices(Object)
Symfony\Component\DependencyInjection\ContainerBuilder->resolveServices(Array)
Symfony\Component\DependencyInjection\ContainerBuilder->callMethod(Object, Array)
Symfony\Component\DependencyInjection\ContainerBuilder->createService(Object, 'path_processor_manager')
Symfony\Component\DependencyInjection\ContainerBuilder->get('path_processor_manager', 1)
Symfony\Component\DependencyInjection\ContainerBuilder->resolveServices(Object)
Symfony\Component\DependencyInjection\ContainerBuilder->resolveServices(Array)
Symfony\Component\DependencyInjection\ContainerBuilder->createService(Object, 'url_generator')
Symfony\Component\DependencyInjection\ContainerBuilder->get('url_generator')
Drupal\simpletest\WebTestBase->prepareRequestForGenerator()
Drupal\simpletest\WebTestBase->rebuildContainer()
Drupal\system\Tests\Upgrade\UpgradePathTestBase->getUpdatePhp()
Drupal\system\Tests\Upgrade\UpgradePathTestBase->performUpgrade()
Drupal\contact\Tests\ContactUpgradePathTest->testContactUpgrade()
Drupal\simpletest\TestBase->run()
_simpletest_batch_operation(Array, '21', Array)
call_user_func_array('_simpletest_batch_operation', Array)
_batch_process()
_batch_do()
_batch_page()
system_batch_page()
call_user_func_array('system_batch_page', Array)
Drupal\Core\EventSubscriber\{closure}()
call_user_func_array(Object, Array)
Symfony\Component\HttpKernel\HttpKernel->handleRaw(Object, 1)
Symfony\Component\HttpKernel\HttpKernel->handle(Object, 1, 1)
Drupal\Core\HttpKernel->handle(Object, 1, 1)
Symfony\Component\HttpKernel\Kernel->handle(Object)
drupal_handle_request()
 

This seems *not* to be caused by the changes in core/includes/update.inc.

After full reboot and running an upgrade test with this patch applied to HEAD the the same upgrade test passes. I guess it's too late for humans, lets see what the bot thinks.

Tests fail in a different manner locally. Not nice, give it another spin.

#34: the_last_eval-1273968-34.patch queued for re-testing.

This brings us one step closer. The test now (only) fails with The plural formula could not be parsed. .

I think that the problem is that we have 'nplurals=2; plural=($n>1);' with $n instead of n in the UpgradeTest.
I am having problems with testing this locally, but could be green.

+++ b/core/lib/Drupal/Component/Gettext/PoHeader.phpundefined
@@ -302,6 +301,9 @@ private function parseArithmetic($string) {
+      elseif ($current_token == "$") {
+        // Do nothing, we will process next n (we have found $n).
+      }

No! .po files will not have a $ in the number. The *database* data has it. The **upgrade** should remove the $ and then parse it.

Oops... working on that then.

Oops... working on that then.

This should address the issue, but I'm having some issues with Upgrade tests in my system. Let's wait for testbot.

chx noted in irc that we could filter out more values from the more $plurals array. This is what I came up with for parsePluralForms():

  $element_stack = $this->parseArithmetic($plural);
  $default = 0;
  if ($element_stack !== FALSE) {
    for ($i = 0; $i <= 199; $i++) {
      $plurals[$i] = $this->evaluatePlural($element_stack, $i);
    }
    $default = $plurals[$i-1];
    $plurals = array_filter($plurals, function ($value) use ($default) {
      return ($value != $default);
    });
    $plurals['default'] = $default;

    return array($nplurals, $plurals);
  }

Instead of truncating values from the end of the array, this uses array_filter to remove all occurrences of $default from the array.

Let's if chx + Sutharsan suggestion works...

Yes it works :)
Out of curiosity I tried to filter on a few other positions ($plurals[0], $plurals[1], $plurals[189] .. $plurals[198]) but none gave a better result. On average the compression is 90%. Because I have used many exotic plural formula's for the test, in practice the compression may go up to 99%.

@webflo and I looked at this and we found no problems. I'm also very happy with the set of people who looked at this and worked together :) Yay!

Committed to 8.x. Thanks!

Thanks!
Upon suggestion of chx, I have create a follow-up issue for additional unit test.

The follow-up issue is #2031441: Add unit test for PoHeader::evaluatePlural

Removing sprint tag.

Attempt to remove tag again.

This did not cover JavaScript unfortunately. See #2043953: Singular/plural strings don't work in JavaScript anymore.

I've rolled this back and am closing the other issue as duplicate. We can commit this again along with the js fix.

Because #2043953: Singular/plural strings don't work in JavaScript anymore was critical
Feel free to bump back.

I'm not sure @catch considers having an eval() in Drupal's codebase critical, he rolled this patch back to manage the critical queue by trading a critical bug for a major one. That is rather keep an eval() in vs. having a critical about a non-complete implementation of it. I don't necessarily agree that the rollback was the best technique forward to get the eval removed and JS properly fixed, but I don't need to :)

Cross-post.

afaik there's no API change here, so I'd commit a patch to 8.x, I'm not going to play status pong though.

I hate eval() but I don't consider it a release blocker no (especially since we could backport that change anyway).

@catch: it is an API change in terms of how the language object is structured, which having no methods exposes its stuff via public properties. That is why the JS code broke.

@Gabor: Ok technically that's an API change but do you know anywhere other than the js that uses formula?

At least the l10n_server, potx and l10n_pconfig modules rely on the formula in some way (needing the string itself in l10n_pconfig and potx and the num of plurals in l10n_server). Which in fact points to a problem in the above patch that it does not keep storing that information additionally to the computed array.

tag

Updated issue summary.

Rerolled the #53 patch.

Nevermind :/ A proper reroll, this time.

This patch makes JS use the plural "formula" array instead of the old formula. In the translated js file the formula gets replaced by an array of indexes. Drupal.formatPlural() used this array to determine the right index.

I have seen it working with English plural strings in JS, but could not get translated strings. It probably needs the right JS to to demonstrate plural translation.

@Sutharsan: @Sweetchuck wanted to take this on on Drupal sprint weekend and found several other bugs in JS translation to make JS translation work in the first place... Those are now fixed in #2182265: Drupal.t() placeholder substitution doesn't always work correctly (backport part of the Javascript file translation fixes from Drupal 8), so this can now be adapted to work with JS too from a prior working JS state (as opposed to several bugs there :)

This patch is not finished.
The JS Drupal.t() is not works with the english language, but works with every other language.

@sweetchuck, I tried to review the issue but found it very hard to check your work. Your patch combines too many things in one: (unrelated) changes due to the reroll, code style changes and finally the actual functional code changes. As you did not provide an interdiff, I can not judge the patch. Could you create a new patch, with interdiff. Ideally I would like to see 1. (first) a patch of the #75 rerolled; 2. a patch with only functional changes including an interdiff and lastly a separate follow-up issue with the code style changes and the non-essential comment changes.
I do realise that this is a lot of extra work, but the #78 patch is not in a state that I can review it and therefore holds the issue up.

the_last_eval-1273968-80-75-reroll.patch
Rerolled version of patch #75.

the_last_eval-1273968-80-78-clean.patch
Simplified version of patch #78. The code comments and coding standards modifications has been removed.

the_last_eval-1273968-80-interdiff-75-78.txt
This is the interdiff between the the_last_eval-1273968-80-75-reroll and the_last_eval-1273968-80-78-clean.patch

I've reviewed the interdiff:

1. +++ b/core/includes/common.inc
   @@ -2073,7 +2073,12 @@ function _drupal_add_js($data = NULL, $options = NULL) {
   -          $javascript['settings']['data'][] = array('path' => $path);
   +          $javascript['settings']['data'][] = array(
   +            'path' => $path,
   +            'locale' => array(
   +              'pluralDelimiter' => LOCALE_PLURAL_DELIMITER,
   +            ),
   +          );
   

   This duplicates #2224691: JavaScript Drupal.t and Drupal.formatPlural only works for non-english languages.

2. +++ b/core/includes/update.inc
   @@ -15,7 +15,6 @@
   -use Drupal\Component\Gettext\PoHeader;
   

   This change seems not to be related to this patch, it is probably code clean-up. If so, remove it from the patch.

3. +++ b/core/misc/drupal.js
   @@ -363,26 +363,20 @@ if (window.jQuery) {
      Drupal.formatPlural = function (count, singular, plural, args, options) {
        args = args || {};
        args['@count'] = count;
   +
   +    var pluralDelimiter = drupalSettings.locale.pluralDelimiter,
   +      translations = Drupal.t(singular + pluralDelimiter + plural, args, options).split(pluralDelimiter),
   

   This is duplicate code of #2224691: JavaScript Drupal.t and Drupal.formatPlural only works for non-english languages. Remove it.

4. +++ b/core/misc/drupal.js
   @@ -363,26 +363,20 @@ if (window.jQuery) {
   -    else {
   -      index = (args['@count'] === 1) ? 0 : 1;
   +    else if (args['@count'] !== 1) {
   +      index = 1;
   

   This is not a functional change, but a personal preference in code style. Please remove to avoid clutter in the patch.

5. +++ b/core/modules/locale/locale.module
   @@ -340,7 +340,6 @@ function locale_get_plural($count, $langcode = NULL) {
          // find exceptions (1, 2, etc).
          $index = $count > 199 ? 100 + ($count % 100) : $count;
          $plural_indexes[$langcode][$count] = isset($plural_formulas[$langcode]['formula'][$index]) ? $plural_formulas[$langcode]['formula'][$index] : $plural_formulas[$langcode]['formula']['default'];
   -
        }
   

   More code clean-up.

6. +++ b/core/modules/locale/locale.module
   @@ -340,7 +340,6 @@ function locale_get_plural($count, $langcode = NULL) {
   @@ -1299,19 +1298,16 @@ function _locale_rebuild_js($langcode = NULL) {
   
   @@ -1299,19 +1298,16 @@ function _locale_rebuild_js($langcode = NULL) {
      $data_hash = NULL;
      $data = $status = '';
      if (!empty($translations)) {
   -    $data = "Drupal.locale = { ";
   +    $data = array(
   +      'strings' => $translations,
   +    );
   +
   

   This is duplicate code of #2224691: JavaScript Drupal.t and Drupal.formatPlural only works for non-english languages. Remove it, or drop that issue.

7. +++ b/core/modules/locale/locale.module
   @@ -1372,6 +1368,7 @@ function _locale_rebuild_js($langcode = NULL) {
   +
        case 'rebuilt':
   

   More code clean-up. Please remove.

+++ b/core/misc/drupal.js
@@ -363,26 +363,20 @@ if (window.jQuery) {
+      translations = Drupal.t(singular + pluralDelimiter + plural, args, options).split(pluralDelimiter),

+++ b/core/modules/locale/locale.module
@@ -1299,19 +1298,16 @@ function _locale_rebuild_js($langcode = NULL) {
+    $data = 'Drupal.locale = ' . Json::encode($data) . ';';

Am I correct to understand that this line is the only functional change in this patch? The other changes are 1. covered by #2224691: JavaScript Drupal.t and Drupal.formatPlural only works for non-english languages, 2. usage of Json::encode() instead of custom encoding and 3. the above commented non-essential changes.

We need to decide what to do with #2224691: JavaScript Drupal.t and Drupal.formatPlural only works for non-english languages. Those changes are completely included in the #80 patch. We either drop the first issue or remove its code from the patch. Dropping #2224691: JavaScript Drupal.t and Drupal.formatPlural only works for non-english languages simplifies the workflow and we need the combined code to fix the (plural) translation in English. And that is what we try to achieve after this issue got reverted.

Jay, #2224691: JavaScript Drupal.t and Drupal.formatPlural only works for non-english languages got committed.
@Sweetchuck, Will you re-roll and update the patch as we discussed yesterday?

Tagging for D8MI sprint board.

Yes. I will reroll this patch today. After the "D8 Field API - Fields reborn" session

Reroll the patch #80 the_last_eval-1273968-80-78-clean.patch

The code looks Ok to me. Since javascript is not tested by the testbot, this patch now needs a manual test. Alternatively we could use a simple javascript script and us use that in a manual test.

How to test manually

The Drupal.formatPlural() is called only twice in Drupal core.
core/modules/comment/js/node-new-comments-link.js
core/modules/contextual/js/contextual.toolbar.js
Not easy to trigger it manually. Therefore I wrote a small script to call theDrupal.formatPlural() as way as I want.

1. Enable the interface translation module
2. Add the Polish language on admin/config/regional/language page
3. We need some translated text, so download the translation of the Date module http://ftp.drupal.org/files/translations/7.x/date/date-7.x-2.7.pl.po
4. Import the downloaded PO file on admin/config/regional/translate/import page.
5. Add this two line to the core/themes/bartik/bartik.info.yml file
   

   scripts:
     - js/bartik.format_plural_tester.js
   

6. Create the file "core/themes/bartik/js/bartik.format_plural_tester.js" with the following content:
   

   Drupal.behaviors.bartikFormatPluralTester = {
     attach: function (context, settings) {
       var
         args = {},
         options = {},
         t = '';
   
       t = Drupal.formatPlural(1, 'Do not translate this text.', 'Do not translate this texts.', {}, {});
       console.log('Untranslated singular -> ' + t);
   
       t = Drupal.formatPlural(2, 'Do not translate this text.', 'Do not translate this texts.', {}, {});
       console.log('Untranslated plural -> ' + t);
   
       for (var i = 1; i < 120; i++) {
         t = Drupal.formatPlural(i, 'every week', 'every @count weeks', args, options);
         console.log(i + ' -> ' + t);
       }
     }
   };
   

7. Open the console window of your browser
8. Visit the front page with English interface language
9. Visit the front page with Polish interface language

Of course you can use any other language not just the Polish language.

Updated the issue summary with a general problem description and added details to the manual test.

I have ran the manual test,but it fails. This is the output:

Untranslated singular -> Some untranslated singular.
Untranslated plural -> Some untranslated plural text.
1 -> every week
2 -> every 2 weeks
3 -> every 3 weeks
4 -> every 4 weeks
5 -> undefined
6 -> undefined
...
21 -> undefined
22 -> every 22 weeks
23 -> every 23 weeks
24 -> every 24 weeks
25 -> undefined
...

It looks like the default is not being applied.

The problem was in the test. It used a translated string, but the string was not in the JavaScript translation file. The source string is now include in the code and used in the Drupal.t() function, further and a more commonly used source string is being used in the test.

Ignore #93 - #95 comments, they are about old patches. The patch in #85 is the latest and still applies.

I did the manual test, and I got all strings translated.

Some untranslated singular text.
Some untranslated plural text.
1 час 
2 часа
3 часа
4 часа
5[2] часова
6[2] часова
7[2] часова
8[2] часова
9[2] часова
10[2] часова
11[2] часова
12[2] часова
13[2] часова
14[2] часова
15[2] часова 

@botanic_spark, thanks for testing.
+1 for RTBC

Committed 0ac4b91 and pushed to 8.x. Thanks!

diff --git a/core/lib/Drupal/Component/Gettext/PoHeader.php b/core/lib/Drupal/Component/Gettext/PoHeader.php
index 5088104..3136f2e 100644
--- a/core/lib/Drupal/Component/Gettext/PoHeader.php
+++ b/core/lib/Drupal/Component/Gettext/PoHeader.php
@@ -228,7 +228,6 @@ function parsePluralForms($pluralforms) {
     // For data compression we store the last position the array value
     // changes and store it as default.
     $element_stack = $this->parseArithmetic($plural);
-    $default = 0;
     if ($element_stack !== FALSE) {
       for ($i = 0; $i <= 199; $i++) {
         $plurals[$i] = $this->evaluatePlural($element_stack, $i);

Removed unused local variable on commit.

Yay, thanks all!

Since the arithmetic plural formula is now evaluated and then discarded, the original plural formula is no longer available. This is a problem when creating interface translation exports in .po files. This currently blocks #2611298: Port potx to Drupal 8

Can you have a look at the following issue and help determine the proposed resolution? #2882617: String version of plural formula is not available, exported .po files contain an incorrect default