Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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';
Comments
Comment #2
Elendev CreditAttribution: Elendev at Swissquote commentedComment #3
Elendev CreditAttribution: Elendev at Swissquote commentedComment #4
Elendev CreditAttribution: Elendev at Swissquote commentedComment #5
amermod CreditAttribution: amermod at Swissquote commentedExactly what I need !
Tested locally, it works fine.
Go!
Comment #6
alexpottThanks 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:
I think maybe we need to expand what is already in \Drupal\Tests\locale\Functional\LocaleJavascriptTranslationTest
Comment #7
Elendev CreditAttribution: Elendev at Swissquote commentedThanks for your feedback @alexpott, I've added tests to cover this specific case.
Comment #8
alexpottNo need for the additional line and this should be something like:
Comment #9
Elendev CreditAttribution: Elendev at Swissquote commentedThank you for your advice, I've been able to execute the test locally and I've fixed the issue.
Comment #10
Elendev CreditAttribution: Elendev at Swissquote commentedComment #11
alexpott@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.
Comment #12
Elendev CreditAttribution: Elendev at Swissquote commentedI'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.
Comment #14
Elendev CreditAttribution: Elendev at Swissquote commentedComment #15
gbaudoin CreditAttribution: gbaudoin at Swissquote commentedThanks ! It makes French translations easier. Running on 8.6
Comment #16
gbaudoin CreditAttribution: gbaudoin at Swissquote commentedComment #18
Elendev CreditAttribution: Elendev at Swissquote commented@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.
Comment #19
Elendev CreditAttribution: Elendev at Swissquote commentedComment #21
alexpott@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.
Comment #22
alexpottThe 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...
Comment #23
alexpottCommitted and pushed 58146e4827 to 8.8.x and d7ca039811 to 8.7.x. Thanks!
Comment #26
Elendev CreditAttribution: Elendev at Swissquote commentedThanks !