From 61b1f9c36be9b87e8ef5040cca41f93ac35396c2 Mon Sep 17 00:00:00 2001 From: Gerhard Schlager Date: Tue, 5 Nov 2019 02:15:44 +0100 Subject: [PATCH] FEATURE: Load translation overrides without JS `eval` --- .../initializers/localization.js.es6 | 23 +++---- app/controllers/application_controller.rb | 1 - app/controllers/extra_locales_controller.rb | 61 +++++++++-------- app/helpers/application_helper.rb | 9 ++- app/models/translation_override.rb | 1 + app/views/layouts/application.html.erb | 5 +- app/views/wizard/index.html.erb | 5 +- config/initializers/100-i18n.rb | 5 +- lib/freedom_patches/translate_accelerator.rb | 8 +-- lib/js_locale_helper.rb | 33 +++++++++ .../translate_accelerator_spec.rb | 21 ------ spec/helpers/application_helper_spec.rb | 17 +++-- .../requests/extra_locales_controller_spec.rb | 68 ++++++++++++++++++- .../initializers/localization-test.js.es6 | 11 +-- 14 files changed, 181 insertions(+), 87 deletions(-) diff --git a/app/assets/javascripts/discourse/initializers/localization.js.es6 b/app/assets/javascripts/discourse/initializers/localization.js.es6 index fc9eee1536e..cbe50e53d50 100644 --- a/app/assets/javascripts/discourse/initializers/localization.js.es6 +++ b/app/assets/javascripts/discourse/initializers/localization.js.es6 @@ -1,5 +1,3 @@ -import PreloadStore from "preload-store"; - export default { name: "localization", after: "inject-objects", @@ -21,20 +19,9 @@ export default { } // Merge any overrides into our object - const overrides = PreloadStore.get("translationOverrides") || {}; + const overrides = I18n._overrides || {}; Object.keys(overrides).forEach(k => { const v = overrides[k]; - - // Special case: Message format keys are functions - if (/_MF$/.test(k)) { - k = k.replace(/^[a-z_]*js\./, ""); - I18n._compiledMFs[k] = new Function( - "transKey", - `return (${v})(transKey);` - ); - return; - } - k = k.replace("admin_js", "js"); const segs = k.split("."); @@ -52,6 +39,14 @@ export default { } }); + const mfOverrides = I18n._mfOverrides || {}; + Object.keys(mfOverrides).forEach(k => { + const v = mfOverrides[k]; + + k = k.replace(/^[a-z_]*js\./, ""); + I18n._compiledMFs[k] = v; + }); + bootbox.addLocale(I18n.currentLocale(), { OK: I18n.t("composer.modal_ok"), CANCEL: I18n.t("composer.modal_cancel"), diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 1cd6bd39e50..3c19910598f 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -520,7 +520,6 @@ class ApplicationController < ActionController::Base store_preloaded("customHTML", custom_html_json) store_preloaded("banner", banner_json) store_preloaded("customEmoji", custom_emoji) - store_preloaded("translationOverrides", I18n.client_overrides_json(I18n.locale)) end def preload_current_user_data diff --git a/app/controllers/extra_locales_controller.rb b/app/controllers/extra_locales_controller.rb index 43560c8660c..4bee4449764 100644 --- a/app/controllers/extra_locales_controller.rb +++ b/app/controllers/extra_locales_controller.rb @@ -8,57 +8,64 @@ class ExtraLocalesController < ApplicationController :redirect_to_login_if_required, :verify_authenticity_token + OVERRIDES_BUNDLE ||= 'overrides' + def show bundle = params[:bundle] - raise Discourse::InvalidAccess.new if bundle !~ /^(admin|wizard)$/ || !current_user&.staff? + raise Discourse::InvalidAccess.new if !valid_bundle?(bundle) if params[:v]&.size == 32 hash = ExtraLocalesController.bundle_js_hash(bundle) - immutable_for(24.hours) if hash == params[:v] + immutable_for(1.year) if hash == params[:v] end render plain: ExtraLocalesController.bundle_js(bundle), content_type: "application/javascript" end def self.bundle_js_hash(bundle) - @bundle_js_hash ||= {} - @bundle_js_hash["#{bundle}_#{I18n.locale}"] ||= Digest::MD5.hexdigest(bundle_js(bundle)) + if bundle == OVERRIDES_BUNDLE + site = RailsMultisite::ConnectionManagement.current_db + + @by_site ||= {} + @by_site[site] ||= {} + @by_site[site][I18n.locale] ||= begin + js = bundle_js(bundle) + js.present? ? Digest::MD5.hexdigest(js) : nil + end + else + @bundle_js_hash ||= {} + @bundle_js_hash["#{bundle}_#{I18n.locale}"] ||= Digest::MD5.hexdigest(bundle_js(bundle)) + end end def self.url(bundle) - if Rails.env == "production" - "#{Discourse.base_uri}/extra-locales/#{bundle}?v=#{bundle_js_hash(bundle)}" - else - "#{Discourse.base_uri}/extra-locales/#{bundle}" - end + "#{Discourse.base_uri}/extra-locales/#{bundle}?v=#{bundle_js_hash(bundle)}" + end + + def self.client_overrides_exist? + bundle_js_hash(OVERRIDES_BUNDLE).present? end def self.bundle_js(bundle) locale_str = I18n.locale.to_s bundle_str = "#{bundle}_js" - translations = JsLocaleHelper.translations_for(locale_str) - - translations.keys.each do |l| - translations[l].keys.each do |k| - bundle_translations = translations[l].delete(k) - translations[l].deep_merge!(bundle_translations) if k == bundle_str - end + if bundle == OVERRIDES_BUNDLE + JsLocaleHelper.output_client_overrides(locale_str) + else + JsLocaleHelper.output_extra_locales(bundle_str, locale_str) end + end - js = "" + def self.clear_cache! + site = RailsMultisite::ConnectionManagement.current_db + @by_site&.delete(site) + end - if translations.present? - js = <<~JS.squish - (function() { - if (window.I18n) { - window.I18n.extras = #{translations.to_json}; - } - })(); - JS - end + private - js + def valid_bundle?(bundle) + bundle == OVERRIDES_BUNDLE || (bundle =~ /^(admin|wizard)$/ && current_user&.staff?) end end diff --git a/app/helpers/application_helper.rb b/app/helpers/application_helper.rb index 96531a9a187..f4ef9660d67 100644 --- a/app/helpers/application_helper.rb +++ b/app/helpers/application_helper.rb @@ -93,9 +93,14 @@ module ApplicationHelper def preload_script(script) path = script_asset_path(script) + preload_script_url(path) + end -" -".html_safe + def preload_script_url(url) + <<~HTML.html_safe + + + HTML end def discourse_csrf_tags diff --git a/app/models/translation_override.rb b/app/models/translation_override.rb index 95e4183589b..bef0c3eb1df 100644 --- a/app/models/translation_override.rb +++ b/app/models/translation_override.rb @@ -39,6 +39,7 @@ class TranslationOverride < ActiveRecord::Base def self.i18n_changed(keys) I18n.reload! + ExtraLocalesController.clear_cache! MessageBus.publish('/i18n-flush', refresh: true) keys.flatten.each do |key| diff --git a/app/views/layouts/application.html.erb b/app/views/layouts/application.html.erb index fdebcba6f25..3340017d75c 100644 --- a/app/views/layouts/application.html.erb +++ b/app/views/layouts/application.html.erb @@ -20,6 +20,9 @@ <%= build_plugin_html 'server:before-script-load' %> <%= preload_script "locales/#{I18n.locale}" %> + <%- if ExtraLocalesController.client_overrides_exist? %> + <%= preload_script_url ExtraLocalesController.url('overrides') %> + <%- end %> <%= preload_script "ember_jquery" %> <%= preload_script "preload-store" %> <%= preload_script "vendor" %> @@ -29,7 +32,7 @@ <%= preload_script file %> <%- end %> <%- if staff? %> - + <%= preload_script_url ExtraLocalesController.url('admin') %> <%= preload_script "admin" %> <%- end %> diff --git a/app/views/wizard/index.html.erb b/app/views/wizard/index.html.erb index 9ccc5e3b15a..382f68b4336 100644 --- a/app/views/wizard/index.html.erb +++ b/app/views/wizard/index.html.erb @@ -3,9 +3,12 @@ <%= discourse_stylesheet_link_tag :wizard, theme_ids: nil %> <%= preload_script 'ember_jquery' %> <%= preload_script "locales/#{I18n.locale}" %> + <%- if ExtraLocalesController.client_overrides_exist? %> + <%= preload_script_url ExtraLocalesController.url('overrides') %> + <%- end %> <%= preload_script 'wizard-vendor' %> <%= preload_script 'wizard-application' %> - + <%= preload_script_url ExtraLocalesController.url('wizard') %> <%= csrf_meta_tags %> <%= render partial: "layouts/head" %> diff --git a/config/initializers/100-i18n.rb b/config/initializers/100-i18n.rb index d714de498d7..10749264a41 100644 --- a/config/initializers/100-i18n.rb +++ b/config/initializers/100-i18n.rb @@ -12,5 +12,8 @@ I18n.reload! I18n.init_accelerator! unless Rails.env.test? - MessageBus.subscribe("/i18n-flush") { I18n.reload! } + MessageBus.subscribe("/i18n-flush") do + I18n.reload! + ExtraLocalesController.clear_cache! + end end diff --git a/lib/freedom_patches/translate_accelerator.rb b/lib/freedom_patches/translate_accelerator.rb index b2a0f51f867..26dff3594c9 100644 --- a/lib/freedom_patches/translate_accelerator.rb +++ b/lib/freedom_patches/translate_accelerator.rb @@ -137,9 +137,10 @@ module I18n def overrides_by_locale(locale) return unless @overrides_enabled - return {} if GlobalSetting.skip_db? + execute_reload if @requires_reload + site = RailsMultisite::ConnectionManagement.current_db by_site = @overrides_by_site[site] @@ -170,11 +171,6 @@ module I18n end end - def client_overrides_json(locale) - client_json = (overrides_by_locale(locale) || {}).select { |k, _| k[/^(admin_js|js)\./] } - MultiJson.dump(client_json) - end - def translate(*args) execute_reload if @requires_reload diff --git a/lib/js_locale_helper.rb b/lib/js_locale_helper.rb index 798a67fe87b..99d344ef8c9 100644 --- a/lib/js_locale_helper.rb +++ b/lib/js_locale_helper.rb @@ -159,6 +159,39 @@ module JsLocaleHelper result end + def self.output_client_overrides(locale) + translations = (I18n.overrides_by_locale(locale) || {}).select { |k, _| k[/^(admin_js|js)\./] } + return "" if translations.blank? + + message_formats = {} + + translations.delete_if do |key, value| + if key.to_s.end_with?("_MF") + message_formats[key] = value + end + end + + message_formats = message_formats.map { |k, v| "#{k.inspect}: #{v}" }.join(", ") + + <<~JS + I18n._mfOverrides = {#{message_formats}}; + I18n._overrides = #{translations.to_json}; + JS + end + + def self.output_extra_locales(bundle, locale) + translations = translations_for(locale) + + translations.keys.each do |l| + translations[l].keys.each do |k| + bundle_translations = translations[l].delete(k) + translations[l].deep_merge!(bundle_translations) if k == bundle + end + end + + translations.present? ? "I18n.extras = #{translations.to_json};" : "" + end + MOMENT_LOCALE_MAPPING ||= { "hy" => "hy-am", "en" => "en-gb" diff --git a/spec/components/freedom_patches/translate_accelerator_spec.rb b/spec/components/freedom_patches/translate_accelerator_spec.rb index fc12655881d..93875648643 100644 --- a/spec/components/freedom_patches/translate_accelerator_spec.rb +++ b/spec/components/freedom_patches/translate_accelerator_spec.rb @@ -204,26 +204,5 @@ describe "translate accelerator" do override_translation('en', 'fish', 'fake fish') expect(Fish.model_name.human).to eq('Fish') end - - describe "client json" do - it "is empty by default" do - expect(I18n.client_overrides_json('en')).to eq('{}') - end - - it "doesn't return server overrides" do - override_translation('en', 'foo', 'bar') - expect(I18n.client_overrides_json('en')).to eq('{}') - end - - it "returns client overrides" do - override_translation('en', 'js.foo', 'bar') - override_translation('en', 'admin_js.beep', 'boop') - json = ::JSON.parse(I18n.client_overrides_json('en')) - - expect(json).to be_present - expect(json['js.foo']).to eq('bar') - expect(json['admin_js.beep']).to eq('boop') - end - end end end diff --git a/spec/helpers/application_helper_spec.rb b/spec/helpers/application_helper_spec.rb index 20af731aaad..c03af37509b 100644 --- a/spec/helpers/application_helper_spec.rb +++ b/spec/helpers/application_helper_spec.rb @@ -6,13 +6,20 @@ require 'rails_helper' describe ApplicationHelper do describe "preload_script" do + def preload_link(url) + <<~HTML + + + HTML + end + it "provides brotli links to brotli cdn" do set_cdn_url "https://awesome.com" helper.request.env["HTTP_ACCEPT_ENCODING"] = 'br' link = helper.preload_script('application') - expect(link).to eq("\n") + expect(link).to eq(preload_link("https://awesome.com/brotli_asset/application.js")) end context "with s3 CDN" do @@ -45,26 +52,26 @@ describe ApplicationHelper do helper.request.env["HTTP_ACCEPT_ENCODING"] = 'br' link = helper.preload_script('application') - expect(link).to eq("\n") + expect(link).to eq(preload_link("https://s3cdn.com/assets/application.br.js")) end it "gives s3 cdn if asset host is not set" do link = helper.preload_script('application') - expect(link).to eq("\n") + expect(link).to eq(preload_link("https://s3cdn.com/assets/application.js")) end it "can fall back to gzip compression" do helper.request.env["HTTP_ACCEPT_ENCODING"] = 'gzip' link = helper.preload_script('application') - expect(link).to eq("\n") + expect(link).to eq(preload_link("https://s3cdn.com/assets/application.gz.js")) end it "gives s3 cdn even if asset host is set" do set_cdn_url "https://awesome.com" link = helper.preload_script('application') - expect(link).to eq("\n") + expect(link).to eq(preload_link("https://s3cdn.com/assets/application.js")) end end end diff --git a/spec/requests/extra_locales_controller_spec.rb b/spec/requests/extra_locales_controller_spec.rb index 551dceda3df..7c12b1fcfdb 100644 --- a/spec/requests/extra_locales_controller_spec.rb +++ b/spec/requests/extra_locales_controller_spec.rb @@ -28,16 +28,16 @@ describe ExtraLocalesController do let(:moderator) { Fabricate(:moderator) } before { sign_in(moderator) } - it "caches for 24 hours if version is provided and it matches current hash" do + it "caches for 1 year if version is provided and it matches current hash" do get "/extra-locales/admin", params: { v: ExtraLocalesController.bundle_js_hash('admin') } expect(response.status).to eq(200) - expect(response.headers["Cache-Control"]).to eq("max-age=86400, public, immutable") + expect(response.headers["Cache-Control"]).to eq("max-age=31556952, public, immutable") end it "does not cache at all if version is invalid" do get "/extra-locales/admin", params: { v: 'a' * 32 } expect(response.status).to eq(200) - expect(response.headers["Cache-Control"]).not_to eq("max-age=86400, public, immutable") + expect(response.headers["Cache-Control"]).not_to include("max-age", "public", "immutable") end context "with plugin" do @@ -67,6 +67,47 @@ describe ExtraLocalesController do end end end + + context "overridden translations" do + after { I18n.reload! } + + it "works for anonymous users" do + TranslationOverride.upsert!(I18n.locale, 'js.some_key', 'client-side translation') + + get "/extra-locales/overrides", params: { v: ExtraLocalesController.bundle_js_hash('overrides') } + expect(response.status).to eq(200) + expect(response.headers["Cache-Control"]).to eq("max-age=31556952, public, immutable") + end + + it "returns nothing when there are not overridden translations" do + get "/extra-locales/overrides" + expect(response.status).to eq(200) + expect(response.body).to be_empty + end + + context "with translations" do + it "returns the correct translations" do + TranslationOverride.upsert!(I18n.locale, 'js.some_key', 'client-side translation') + TranslationOverride.upsert!(I18n.locale, 'js.client_MF', '{NUM_RESULTS, plural, one {1 result} other {many} }') + TranslationOverride.upsert!(I18n.locale, 'admin_js.another_key', 'admin client js') + TranslationOverride.upsert!(I18n.locale, 'server.some_key', 'server-side translation') + TranslationOverride.upsert!(I18n.locale, 'server.some_MF', '{NUM_RESULTS, plural, one {1 result} other {many} }') + + get "/extra-locales/overrides" + expect(response.status).to eq(200) + expect(response.body).to_not include("server.some_key", "server.some_MF") + + ctx = MiniRacer::Context.new + ctx.eval("I18n = {};") + ctx.eval(response.body) + + expect(ctx.eval('typeof I18n._mfOverrides["js.client_MF"]')).to eq("function") + expect(ctx.eval('I18n._overrides["js.some_key"]')).to eq("client-side translation") + expect(ctx.eval('I18n._overrides["js.client_MF"] === undefined')).to eq(true) + expect(ctx.eval('I18n._overrides["admin_js.another_key"]')).to eq("admin client js") + end + end + end end describe ".bundle_js_hash" do @@ -95,4 +136,25 @@ describe ExtraLocalesController do expect(ExtraLocalesController.bundle_js_hash("wizard")).to eq(expected_hash_de) end end + + describe ".client_overrides_exist?" do + after do + I18n.reload! + ExtraLocalesController.clear_cache! + end + + it "returns false if there are no client-side translation overrides" do + expect(ExtraLocalesController.client_overrides_exist?).to eq(false) + + TranslationOverride.upsert!(I18n.locale, 'server.some_key', 'server-side translation') + expect(ExtraLocalesController.client_overrides_exist?).to eq(false) + end + + it "returns true if there are client-side translation overrides" do + expect(ExtraLocalesController.client_overrides_exist?).to eq(false) + + TranslationOverride.upsert!(I18n.locale, 'js.some_key', 'client-side translation') + expect(ExtraLocalesController.client_overrides_exist?).to eq(true) + end + end end diff --git a/test/javascripts/initializers/localization-test.js.es6 b/test/javascripts/initializers/localization-test.js.es6 index 222953482fc..7182e6d8f4d 100644 --- a/test/javascripts/initializers/localization-test.js.es6 +++ b/test/javascripts/initializers/localization-test.js.es6 @@ -1,9 +1,9 @@ -import PreloadStore from "preload-store"; import LocalizationInitializer from "discourse/initializers/localization"; QUnit.module("initializer:localization", { _locale: I18n.locale, _translations: I18n.translations, + _overrides: I18n._overrides, beforeEach() { I18n.locale = "fr"; @@ -31,14 +31,15 @@ QUnit.module("initializer:localization", { afterEach() { I18n.locale = this._locale; I18n.translations = this._translations; + I18n._overrides = this._overrides; } }); QUnit.test("translation overrides", function(assert) { - PreloadStore.store("translationOverrides", { + I18n._overrides = { "js.composer.reply": "WAT", "js.topic.reply.help": "foobar" - }); + }; LocalizationInitializer.initialize(this.registry); assert.equal( @@ -56,10 +57,10 @@ QUnit.test("translation overrides", function(assert) { QUnit.test( "skip translation override if parent node is not an object", function(assert) { - PreloadStore.store("translationOverrides", { + I18n._overrides = { "js.composer.reply": "WAT", "js.composer.reply.help": "foobar" - }); + }; LocalizationInitializer.initialize(this.registry); assert.equal(I18n.t("composer.reply.help"), "[fr.composer.reply.help]");