Javascript translations are loaded in the wrong order due to missing dependencies

Better title if I understand the problem right.

With test coverage for this (its absence is totally understandable), this would have been prevented. Will need tests to prevent regressions like this.

Quick-fix: Add scope footer in locale_js_alter() or change the default in drupal_js_defaults. I think its fine to change the default because drupal_js_defaults is only used by locale module.

This is not a solution; it will only fix it in what has become the typical case. drupal.js could be either in the header or the footer, depending on where it's needed. If some JS declares it must live in the header, and it depends on drupal.js, then drupal.js will be in the footer.

More Advanced solution: Add a new library locale/drupal.locale.translation with proper dependencies and replace the path to the generated js file during runtime in hook_js_alter or hook_library_info_alter. Remove drupal_js_defaults from core.

This is the correct solution :)

This does not work yet, but I need feedback if it is the right direction.

Re: #8: Obvious error: if this is cached, the library is generated only for one language. Anyway it does not work even for one language.

Via @webflo: hook_library_alter looks a better place for this. See #2378263: hook_library_alter() must be manually invoked by users of LibraryDiscovery, and has no test coverage too.

Looks very clever. Still needs tests :)

@Wim: This will add js to all pages when visiting in a non-English language, even if anonymous. Is that acceptable?

I'm formulating a response. I have at least one viable solution already. Assigning to me to prevent duplicate work.

WIP of the test. I will wait for @Wim feedback before continuing on this.

While replying to this, I ended up with a very rambling reply. So… added some titles to group things :)

Patch review

+++ b/modules/locale/locale.module
@@ -1304,3 +1307,10 @@ function locale_translation_language_table($form_element) {
+function locale_page_attachments(array &$attachments) {
+  $attachments['#attached']['library'][] = 'locale/drupal.locale.translation';
+}

This is wrong; even on pages where no JS is necessary, this will still load the JS translation library, plus core/drupal. (EDIT: as penyaskito remarked in #17 — I'd already started writing this comment before that :P)

Hence I think this approach won't work after all.

Idea #1: each translation file injected into the library that contains the strings to be translated

But… I have a much better idea … I think — it's late here and I'm hungry :P. Here it is. All assets are in libraries now. Including JS assets. Therefore, it is now possible for the locale module to implement hook_library_alter()… and just inject its translation file in there! Except that it breaks down completely when you realize libraries are cached globally, but doing this would require them to be cached per language. That could be solvable.

It would be very elegant though.

That won't work, because the locale module is designed to generate a single JS translation file.

Idea #2: fix the patch in #14 by fixing wrong assumptions from the past

The root cause of the patch in #14 not working is that the library containing the translations declares a dependency on core/drupal. But that dependency is actually a lie; what really is going on is that the translation library is a conditional dependency of core/drupal, i.e. core/drupal must depend on locale/translations, locale/translations must not depend on core/drupal! After all, it's drupal.js that uses the translations, the translations don't use drupal.js.

Basically, that is all that we need to do.

There is one silly complication: weights. We've been working to get rid of them, but it's not yet finished. See #1945262: Replace custom weights with dependencies in library declarations; introduce "before" and "after" for conditional ordering. Sadly, drupal.js is one of the few JS assets still using weights. The dependency of core/drupal on locale/translations should be enough, but drupal.js's custom weight messes with that. So for now, until #1945262 is fixed, we have to add one LoC to adopt the weight of drupal.js and decrease it a little bit, to ensure the translation file is loaded first.

This is what the attached patch does!

Simplification/performance enhancement: no more need to track JS being discovered, we can generate it all at once, because we can list all JS assets with certainty

I realized that the way JS translation is currently designed, is basically: detect all JS files on the fly, track which ones we've already seen, if we encounter one we haven't seen yet, rebuild the translation file. This was indeed necessary in all prior versions of Drupal, and in Drupal 8… until https://www.drupal.org/node/2379475 (as of that moment, Drupal 8 used asset libraries solely).
So… now it's possible to generate the translation files all at once, without having to do that iterative building! That will mean fewer I/O after a cache clear or a deployment.

But that is out of scope for this issue. Opened #2421323: Parse js files from library definitions during rebuild to minimize variable_set() calls for that.

Wim: was the prior title incorrect? "JS translations are broken" could be for a whole set of reasons. I tried to spell out the reason so that its more evident from the commit message and its easier to search for it later. Reasons for why JS translation was broken before include: (1) the JS translation file was not added to the page at all (2) the source JS files were not properly parsed (3) Drupal was parsing non-Drupal JS files from 3rd party libs that it should not, etc.

Fixed test failures and improved title per #24. (EDIT: and yes, the title was arguably incorrect, but only because of my patch: it was correct from HEAD's POV, but since my patch flips the order around, the previous issue title contradicted what the patch does. Hopefully the new title is sufficiently specific?)

The tests written in #19 should be merged into this patch (great job, penyaskito!).

Fix looks good. Needs tests merged.

HA! So the problem with the test was that the translations were created via API in our test, and the refresh of the locale translations are done in translate form submit handler... Changed the test for translating via drupalPostForm turns it green.

Also re-added the locale JS placeholder file, I guess @Wim forgot it and wasn't removed on purpose.

facepalm, this is green locally.

OK, let's remove xpath and simplify the assertion. This is more in line with AttachedAssetsTest.

Also re-added the locale JS placeholder file, I guess @Wim forgot it and wasn't removed on purpose.

Actually, it was removed on purpose; it's not necessary :) Could you remove it again?

Test looks great! Nitpicks:

1. +++ b/core/modules/locale/src/Tests/LocaleJavascriptTranslationTest.php
   @@ -93,4 +94,62 @@ public function testFileParsing() {
   +   * Ensure that the translations js is added after its dependencies.
   ...
   +    // Loading the Translate page should refresh the translations js and import
   ...
   +    // Assert translations js is included before drupal.js.
   

   s/translations js/translation JS/

   Also: the first cited line contradicts the last cited line.

2. +++ b/core/modules/locale/src/Tests/LocaleJavascriptTranslationTest.php
   @@ -93,4 +94,62 @@ public function testFileParsing() {
   +    // Translate a string in locale.admin.js to langcode.
   

   "translate to langcode" -> typo? Not sure what that means.

3. +++ b/core/modules/locale/src/Tests/LocaleJavascriptTranslationTest.php
   @@ -93,4 +94,62 @@ public function testFileParsing() {
   +    // We need the generated hash in the js filename.
   

   s/js/JS/

Removed the placeholder file and attended other #33 remarks.
Also removed an unnecessary request in the test. I think this is as simple as it can be right now.

Having a only-test patch was a no-sense. The JS files were in the correct order, the problem was that translations assumed that Drupal JS object already existed. Now we are storing in the window object.

+++ b/core/modules/locale/src/Tests/LocaleJavascriptTranslationTest.php
@@ -135,20 +135,15 @@ public function testLocaleTranslationJsDependencies() {
-    // Loading the Translate page should refresh the translations js and import
-    // the sources.
-    $this->drupalGet($prefix . '/admin/config/regional/translate');

Oh, nice, one less GET :)

Assigning to nod_ for review, since he oversees how drupal.js works and this is closely related to that, it feels like he's almost the only person who can RTBC this.

Since the dependencies are kind of backwards if we use Drupal.locale, let's get rid of Drupal.locale and use drupalTranslations everywhere. Needs to be added in .eslintrc as a global too.

Add drupalTranslations to the closure like it's done with drupalSettings.

I'm going to work on that.-

Argh, forgot to rebase my branch against 8.0.x

Forgotten .eslintrc as requested in #36.

Should LOCALE_PLURAL_DELIMITER and drupalSettings.locale.pluralDelimiter also be moved to locale module?

Other than that, this looks great IMO.

Assigning to nod_ again, since I can't RTBC this (I've worked too much on this patch).

I'm at the sprint before DrupalCon Latino, doing 'beta evaluation' work, and would like to edit the 'Problem/Motivation' section to be more comprehensible to those not involved with the issue. Could you provide some more clarification about 'Javascript translation is currently broken'? Is the issue that a) certain JS files/functions are not loading properly, or b) translation of javascript strings doesn't function correctly or c) something else? Thanks!

As the title indicates: the order in which files are loaded in HEAD is wrong, hence Drupal.t() doesn't find the translated strings.

Mark: thanks! :)

I dont understand this in the issue summary:
"This worked fine because both js files where is the same scope and drupal/drupal has a weight of -18."

No change record is needed, as there does not appear to be any backwards-compatibility-breaking API changes, based on looking at https://www.drupal.org/files/issues/js_translations-2419897-48.patch and not seeing any changes to function signatures.

#55 @YesCT: I'm far from being an expert in the area but will try to clarify.

We have three different concepts in JS inclusion: JS file weights, scope (header, footer...) and the library dependency system.
First one of them is going to be removed on #1945262: Replace custom weights with dependencies in library declarations; introduce "before" and "after" for conditional ordering as we should use components with explicit dependencies.

"This worked fine because both js files where is the same scope and drupal/drupal has a weight of -18."

Same scope: both are in header/footer/wherever. Weights: drupal.js has a weight of -18 specified on core.libraries.yml, so it was included before the translations, which has nothing specified so its weight 0. Lower weight comes first.

+++ b/core/modules/locale/locale.module
@@ -537,6 +555,15 @@ function locale_library_info_alter(array &$libraries, $module) {
+  if ($module === 'core' && isset($libraries['drupal'])) {
+    $libraries['drupal']['dependencies'][] = 'locale/translations';
+  }

This makes the precedence of locale/translation to be calculated with the new library dependency system instead of the weight one, which hopefully will be removed before 8.0.0. We do this dinamically instead of in core.libraries.yml because it should only be done if locale is enabled.

Hope this helps.

So #49–#62 was a big distraction for the IS. I've clarified the IS further.

Let's get back on topic now. Repeating the last on-topic comment, #49:

Should LOCALE_PLURAL_DELIMITER and drupalSettings.locale.pluralDelimiter also be moved to locale module?

Other than that, this looks great IMO.

Assigning to nod_ again, since I can't RTBC this (I've worked too much on this patch).

Hate to fail a patch on that but there is an eslint error, extra space. Also when there are no translations, the whole thing crash.

if (drupalTranslations.strings && drupalTranslations.strings[options.context] && drupalTranslations.strings[options.context][str]) {
    

…


    if (drupalTranslations.pluralFormula) {

Need to add a couple of typeof drupalTranslations !== 'undefined' in drupal.js.

Once that's fixed, RTBC for me.

when there are no translations, the whole thing crash sounds bad :/ in what way?

drupal.js has a js error because drupalTranslations is not defined and we're trying to access a member of that object directly, without checking if it exist. Very good way to test no-js version of a page.

Thanks @nod_, I learnt something new with that :-)

Fixed @nod_ comments at #64. Double-checked with eslint that everything is OK.
I implicitly understand that #63 can be discussed in a follow-up issue.

@penyaskito: The updates look good. I don't think that moving half of the things out of Drupal.locale and leaving some things there is a good idea. Should move the plural delimiter too here AFAIS.

rgrep LOCALE_PLURAL_DELIMITER . and there are several references in core libs and system module. So I don't think we should block this bugfix on that.

drupalSettings.locale.pluralDelimiter is needed in English sites too. So I guess this needs to stay at drupal.js (without locale namespace).

#72: only in core/lib/Drupal/Core/StringTranslation/TranslationManager.php, really. The single occurrence in system.module is precisely the one that should be moved to locale.module. But indeed, because of English sites, that's easier said than done.

There is also Drupal\Component\Gettext, which uses this. So if we move the definition, we need to move that to locale too? I am not sure Gábor agrees with that.

Sorry for not being clear: I agree, we should not block this issue on the ideal place for the plural delimiter.

So attached a patch that moves drupalSettings.locale.pluralDelimiter to drupalSettings.pluralDelimiter now that we removed Drupal.locale.

For the backend part we will create a different issue, as it shouldn't block a major.

#76 interdiff looks good. Patch looks good. I'll leave RTBC'ing this to Gábor — I did too much work on this issue.

Looks good to me.

This issue addresses a major bug and is allowed per https://www.drupal.org/core/beta-changes. Committed bda2066 and pushed to 8.0.x. Thanks!

Superb, thanks @WimLeers, @penyaskito, @nod_, @webflo!

did the issue #76 mentions we need get created?

@YesCT: No, AFAIR it wasn't :(
I'll try to do that tomorrow if noone beats me to do it.

Tomorrow is always too late. This already happened on #2570107: Make format_plural() return a PluralTranslatableString object to remove reliance on a static, unpredictable safe list