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.Problem/Motivation
JavaScript string translation, and particularly the discovery of the translatable strings, was designed for the D5/D6/D7 asset handling system's lowest common denominator, which was: drupal_add_js(). This function allowed you to add any JS asset to be loaded at any time during the request.
Consequently, the system to discover translatable strings had to be equally dynamic: 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 JS translation file.
In Drupal 8, that has changed: all assets must be defined in asset libraries, and to have an asset be present in the HTML/loaded by the browser, you must "attach asset libraries". (Already supported and recommended in Drupal 7, but not enforced; as of Drupal 8, this is the only supported mechanism.)
Proposed resolution
In Drupal 8, we can generate the translation files (one per language) all at once, without having to do that iterative building! That will mean, after a cache clear or a deployment:
fewer I/O (no need to first build a translation file for the strings on page A, then expand it for the additional strings discovered in page B, etc.)
no need to update the entry in the State system in sync with 1, hence avoiding race conditions/stampedes
_locale_rebuild_js() and friends can go away.
| Comment | File | Size | Author |
|---|
Issue fork drupal-2607376
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes
Comments
Comment #6
catchComment #9
fagoThis is definitely a good idea and would help to resolve bugs like #2968031: [PP-1] Race condition with locale javascript translation generation.
Comment #10
fagoWe ran into another, similar issue - see #3034990: Race condition when removing old JS translation files.
Comment #11
maximpodorov CreditAttribution: maximpodorov commentedComment #17
gappleI think because
hook_js_altercan modify the files added to the page when it is rendered, it may still be necessary to check for additional files not defined in library definitions and update the translation file if necessary. Having an initial build that parses all files listed in library definitions would still reduce rebuilds and race conditions.Comment #19
gappleA couple things about this MR feel a little hacky (e.g. relying on
AssetResolver::getJsAssets()to callhook_js_alterwhich parses for source strings), but it's a functional start without requiring a lot of refactoring.There's plenty of room for optimization - for example
AssetResolverdoes dependency resolution on the list of libraries, which isn't necessary since the list is know to contain every defined library.A couple warnings pop up during rebuild because of dependency resolution when certain modules are not enabled:
quickedit.inPlaceEditor.formattedTextwith a dependency onquickedit/quickedit. If Quickedit module is not enabled though,LibraryDiscoveryParser::buildByExtension()will throw a warning (it also assumes it's a theme since it's not in the module list...)translation.jsfile incore/ckeditor5.translationsis intended to be removed byckeditor5_js_alter(), but if CKEditor 5 module is not enabled, it gets passed to_locale_parse_js_fileand throws a file not found warningComment #20
gappleoops. fix title.
Comment #21
catchThe editor/quickedit bug will be fixed by #3267258: Remove Quick Edit support from editor.module.
This seems like a ckeditor bug, we should move that file into ckeditor5 itself I would think.
Yeah I think we'll want to factor out some logic into something that both the locale_rebuild() and locale_js_alter() can use. I was hoping we could remove locale_js_alter() altogether in this issue, but the ability to add arbitrary js files from there probably means we can't and will have to keep both mechanisms around.
Comment #24
smustgrave CreditAttribution: smustgrave at Mobomo commentedThis issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request as a guide.
At this time we will need an MR for D10
Comment #26
joelstein CreditAttribution: joelstein commentedRerolling the patch.
Comment #27
_utsavsharma CreditAttribution: _utsavsharma at OpenSense Labs for DrupalFit commentedFixed failures in #26.
Comment #32
alexpottHiding all the patches and other MR to leave the 11.x MR easy to find as that is where we need to start.