The current regex used in _locale_parse_js_file (file core/modules/locale/locale.module) doesn't allow to format plural string that doesn't contains @count in it.

The current regex looks like this:

'~
    [^\w]Drupal\s*\.\s*formatPlural\s*  # match "Drupal.formatPlural" with whitespace
    \(                                  # match "(" argument list start
    \s*.+?\s*,\s*                       # match count argument
    (' . LOCALE_JS_STRING . ')\s*,\s*   # match singular string argument
    (                             # capture plural string argument
      (?:                         # non-capturing group to repeat string pieces
        (?:
          \'                      # match start of single-quoted string
          (?:\\\\\'|[^\'])*       # match any character except unescaped single-quote
          @count                  # match "@count"
          (?:\\\\\'|[^\'])*       # match any character except unescaped single-quote
          \'                      # match end of single-quoted string
          |
          "                       # match start of double-quoted string
          (?:\\\\"|[^"])*         # match any character except unescaped double-quote
          @count                  # match "@count"
          (?:\\\\"|[^"])*         # match any character except unescaped double-quote
          "                       # match end of double-quoted string
        )
        (?:\s*\+\s*)?             # match "+" with possible whitespace, for str concat
      )+                          # match multiple because we supports concatenating strs
    )\s*                          # end capturing of plural string argument
    (?:,\s*' . LOCALE_JS_OBJECT . '\s*          # optionally capture string args
      (?:,\s*' . LOCALE_JS_OBJECT_CONTEXT . '\s*)?  # optionally capture context
    )?
    [,\)]
    ~sx';

With this regex, this JavaScript code will be correctly matched and translations will be available:

Drupal.formatPlural(quantity, "element", "@count elements");

But this JavaScript code will not be matched and translations will not be available:

Drupal.formatPlural(quantity, "element", "elements");

Proposed resolution: avoid having to have @count in the plural argument by modifying the regex like this:

'~
    [^\w]Drupal\s*\.\s*formatPlural\s*  # match "Drupal.formatPlural" with whitespace
    \(                                  # match "(" argument list start
    \s*.+?\s*,\s*                       # match count argument
    (' . LOCALE_JS_STRING . ')\s*,\s*   # match singular string argument
    (                             # capture plural string argument
      (?:                         # non-capturing group to repeat string pieces
        (?:
          \'(?:\\\\\'|[^\'])*\'   # match single-quoted string
          |
          "(?:\\\\"|[^"])*"  # match double-quoted string
        )
        (?:\s*\+\s*)?             # match "+" with possible whitespace, for str concat
      )+                          # match multiple because we supports concatenating strs
    )\s*                          # end capturing of plural string argument
    (?:,\s*' . LOCALE_JS_OBJECT . '\s*          # optionally capture string args
      (?:,\s*' . LOCALE_JS_OBJECT_CONTEXT . '\s*)?  # optionally capture context
    )?
    [,\)]
    ~sx';
CommentFileSizeAuthor
#22 3004226-22.patch4.21 KBalexpott
#22 18-22-interdiff.txt1.46 KBalexpott
#18 core-update_javascript_locale_regex-3004226-17-8.7-8.x.patch4.18 KBElendev
#18 core-update_javascript_locale_regex-3004226-17-8.6.x.patch4.18 KBElendev
#12 core-update_javascript_locale_regex_tests_only-3004226-11-8.6.x.patch2.62 KBElendev
#12 core-update_javascript_locale_regex-3004226-11-8.6.x.patch4.18 KBElendev
#12 core-update_javascript_locale_regex-3004226-11-8.7-8.x.patch4.18 KBElendev
#9 core-update_javascript_locale_regex-3004226-8-8.7-8.x.patch4.18 KBElendev
#9 core-update_javascript_locale_regex-3004226-8-8.6.x.patch4.18 KBElendev
#7 core-update_javascript_locale_regex-3004226-6-8.6.x.patch4.19 KBElendev
#7 core-update_javascript_locale_regex-3004226-6-8.7.x.patch4.19 KBElendev
#7 core-update_javascript_locale_regex-3004226-6-8.8.x.patch4.19 KBElendev
#3 core-update_javascript_locale_regex-3004226-1-8.7.x.patch1.55 KBElendev
#2 core-update_javascript_locale_regex-3004226-1-8.6.x.patch1.55 KBElendev
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Elendev created an issue. See original summary.

Elendev’s picture

Status: Active » Needs review
amermod’s picture

Version: 8.6.x-dev » 8.7.x-dev
Status: Needs review » Reviewed & tested by the community

Exactly what I need !

Tested locally, it works fine.

Go!

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

Thanks for filing this bug report. Bug fixing is very valuable. However in order to commit a bug fix we need an automated to test to prove that we've fixed the bug and ensure that we don't break it again in the future. For more information about writing tests in Drupal 8 see the following links:

  1. https://www.drupal.org/docs/8/testing
  2. https://api.drupal.org/api/drupal/core%21core.api.php/group/testing/8.7.x

I think maybe we need to expand what is already in \Drupal\Tests\locale\Functional\LocaleJavascriptTranslationTest

Elendev’s picture

FileSize
4.19 KB
4.19 KB
4.19 KB

Thanks for your feedback @alexpott, I've added tests to cover this specific case.

alexpott’s picture

  1. Thanks for writing the test! It's a good idea to try to run them locally before submitting at patch - see core/tests/README.md
  2. +++ b/core/modules/locale/tests/src/Functional/LocaleJavascriptTranslationTest.php
    @@ -85,6 +85,9 @@ public function testFileParsing() {
    +
    +        "No count argument plural - singular" => '',
    +        "No count argument plural - plural" => '',
    

    No need for the additional line and this should be something like:

    "No count argument plural - singular{$etx}No count argument plural - plural" => 'No count argument'
    
  3. There's not need to submit the patch against every possible testing combination - and as far as I can see the patch is applicable to both 8.8.x and 8.7.x so one patch will suffice.
Elendev’s picture

Thank you for your advice, I've been able to execute the test locally and I've fixed the issue.

Elendev’s picture

Status: Needs work » Needs review
alexpott’s picture

@Elendev thanks - this is looking good. One more thing that is useful is a test only patch that shows the expected fail. You'll need to upload #9 again so if it gets to rtbc the latest patch is the good one but test only patch help to show people your test is testing what you say it is.

Elendev’s picture

I've added core-update_javascript_locale_regex_tests_only-3004226-11-8.6.x.patch that contains only the tests without the fix (only for 8.6.x, it doesn't seems useful to do the same for 8.7.x or 8.8.x) and I've re-uploaded the fixes for all the branches.

Status: Needs review » Needs work
Elendev’s picture

Status: Needs work » Needs review
gbaudoin’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs tests

Thanks ! It makes French translations easier. Running on 8.6

gbaudoin’s picture

Status: Reviewed & tested by the community » Needs work
Elendev’s picture

@alexpott Am I really supposed to provide a non-working patch with tests but not the fixed feature ? Everytime the status are changed, the test robot mark the issue as `Needs works`.
I'll re-upload the working patches only to avoid having this problem.

Elendev’s picture

Status: Needs work » Reviewed & tested by the community

alexpott’s picture

@Elendev if the test-only patch had been the first patch you uploaded on #12 the rtbc retester would not have set it back to needs work. I'm sorry I should have made this bit of arcane knowledge a bit clearer when I requested the test only patch.

alexpott’s picture

The es6 file to js building resulted in a change... it's tricky making JS changes atm you need to run yarn run build:js from the core folder to ensure the .js is exactly as it should be.

Also 8.6.x is closed to bugfixes now so we only need to the 8.7.x / 8.8.x patch

Anyway as this is about coding standards I've made the changes...

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed 58146e4827 to 8.8.x and d7ca039811 to 8.7.x. Thanks!

  • alexpott committed 58146e4 on 8.8.x
    Issue #3004226 by Elendev, alexpott: Allow formatPlural's plural string...

  • alexpott committed d7ca039 on 8.7.x
    Issue #3004226 by Elendev, alexpott: Allow formatPlural's plural string...
Elendev’s picture

Thanks !

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.