From d02eab498f9b5cd404cfb2751fbc685a110ab550 Mon Sep 17 00:00:00 2001 From: Victor Berchet Date: Tue, 17 Jan 2017 17:36:16 -0800 Subject: [PATCH] fix(compiler): [i18n] XMB/XTB placeholder names can contain only A-Z, 0-9, _n There are restrictions on the character set that can be used for xmb and xtb placeholder names. However because changing the placeholder names would change the message IDs it is not possible to add those restrictions to the names used internally. Then we have to map internal name to public names when generating an xmb file and back when translating using an xtb file. Note for implementors of `Serializer`: - When writing a file, the implementor should take care of converting the internal names to public names while visiting the message nodes - this is required because the original nodes are needed to compute the message ID. - When reading a file, the implementor does not need to take care of the mapping back to internal names as this is handled in the `I18nToHtmlVisitor` used by the `TranslationBundle`. fixes b/34339636 --- modules/@angular/compiler/src/i18n/digest.ts | 6 +- .../@angular/compiler/src/i18n/extractor.ts | 2 +- .../src/i18n/serializers/placeholder.ts | 12 +- .../src/i18n/serializers/serializer.ts | 24 +++- .../compiler/src/i18n/serializers/xliff.ts | 2 +- .../compiler/src/i18n/serializers/xmb.ts | 116 +++++++++++++++--- .../compiler/src/i18n/serializers/xtb.ts | 10 +- .../compiler/src/i18n/translation_bundle.ts | 38 ++++-- .../compiler/test/i18n/integration_spec.ts | 6 +- .../compiler/test/i18n/message_bundle_spec.ts | 2 +- 10 files changed, 175 insertions(+), 43 deletions(-) diff --git a/modules/@angular/compiler/src/i18n/digest.ts b/modules/@angular/compiler/src/i18n/digest.ts index 6bc2ff0dd4..e294cc88c9 100644 --- a/modules/@angular/compiler/src/i18n/digest.ts +++ b/modules/@angular/compiler/src/i18n/digest.ts @@ -13,9 +13,13 @@ export function digest(message: i18n.Message): string { } export function decimalDigest(message: i18n.Message): string { + if (message.id) { + return message.id; + } + const visitor = new _SerializerIgnoreIcuExpVisitor(); const parts = message.nodes.map(a => a.visit(visitor, null)); - return message.id || computeMsgId(parts.join(''), message.meaning); + return computeMsgId(parts.join(''), message.meaning); } /** diff --git a/modules/@angular/compiler/src/i18n/extractor.ts b/modules/@angular/compiler/src/i18n/extractor.ts index deeddf67fa..b3d6f7f7d6 100644 --- a/modules/@angular/compiler/src/i18n/extractor.ts +++ b/modules/@angular/compiler/src/i18n/extractor.ts @@ -52,7 +52,7 @@ export class Extractor { extract(rootFiles: string[]): Promise { const programSymbols = extractProgramSymbols(this.staticSymbolResolver, rootFiles, this.host); - const {ngModuleByPipeOrDirective, files, ngModules} = + const {files, ngModules} = analyzeAndValidateNgModules(programSymbols, this.host, this.metadataResolver); return Promise .all(ngModules.map( diff --git a/modules/@angular/compiler/src/i18n/serializers/placeholder.ts b/modules/@angular/compiler/src/i18n/serializers/placeholder.ts index 6de2423069..02fb3c96c7 100644 --- a/modules/@angular/compiler/src/i18n/serializers/placeholder.ts +++ b/modules/@angular/compiler/src/i18n/serializers/placeholder.ts @@ -111,8 +111,14 @@ export class PlaceholderRegistry { private _hashClosingTag(tag: string): string { return this._hashTag(`/${tag}`, {}, false); } private _generateUniqueName(base: string): string { - const next = this._placeHolderNameCounts[base]; - this._placeHolderNameCounts[base] = next ? next + 1 : 1; - return next ? `${base}_${next}` : base; + const seen = this._placeHolderNameCounts.hasOwnProperty(base); + if (!seen) { + this._placeHolderNameCounts[base] = 1; + return base; + } + + const id = this._placeHolderNameCounts[base]; + this._placeHolderNameCounts[base] = id + 1; + return `${base}_${id}`; } } diff --git a/modules/@angular/compiler/src/i18n/serializers/serializer.ts b/modules/@angular/compiler/src/i18n/serializers/serializer.ts index e39d8221e9..683d823408 100644 --- a/modules/@angular/compiler/src/i18n/serializers/serializer.ts +++ b/modules/@angular/compiler/src/i18n/serializers/serializer.ts @@ -8,10 +8,26 @@ import * as i18n from '../i18n_ast'; -export interface Serializer { - write(messages: i18n.Message[]): string; +export abstract class Serializer { + abstract write(messages: i18n.Message[]): string; - load(content: string, url: string): {[msgId: string]: i18n.Node[]}; + abstract load(content: string, url: string): {[msgId: string]: i18n.Node[]}; - digest(message: i18n.Message): string; + abstract digest(message: i18n.Message): string; + + // Creates a name mapper, see `PlaceholderMapper` + // Returning `null` means that no name mapping is used. + createNameMapper(message: i18n.Message): PlaceholderMapper { return null; } +} + +/** + * A `PlaceholderMapper` converts placeholder names from internal to serialized representation and + * back. + * + * It should be used for serialization format that put constraints on the placeholder names. + */ +export interface PlaceholderMapper { + toPublicName(internalName: string): string; + + toInternalName(publicName: string): string; } \ No newline at end of file diff --git a/modules/@angular/compiler/src/i18n/serializers/xliff.ts b/modules/@angular/compiler/src/i18n/serializers/xliff.ts index f30398157d..b4f91c8632 100644 --- a/modules/@angular/compiler/src/i18n/serializers/xliff.ts +++ b/modules/@angular/compiler/src/i18n/serializers/xliff.ts @@ -27,7 +27,7 @@ const _UNIT_TAG = 'trans-unit'; // http://docs.oasis-open.org/xliff/v1.2/os/xliff-core.html // http://docs.oasis-open.org/xliff/v1.2/xliff-profile-html/xliff-profile-html-1.2.html -export class Xliff implements Serializer { +export class Xliff extends Serializer { write(messages: i18n.Message[]): string { const visitor = new _WriteVisitor(); const visited: {[id: string]: boolean} = {}; diff --git a/modules/@angular/compiler/src/i18n/serializers/xmb.ts b/modules/@angular/compiler/src/i18n/serializers/xmb.ts index 92842931a0..b947c9cd99 100644 --- a/modules/@angular/compiler/src/i18n/serializers/xmb.ts +++ b/modules/@angular/compiler/src/i18n/serializers/xmb.ts @@ -9,7 +9,7 @@ import {decimalDigest} from '../digest'; import * as i18n from '../i18n_ast'; -import {Serializer} from './serializer'; +import {PlaceholderMapper, Serializer} from './serializer'; import * as xml from './xml_helper'; const _MESSAGES_TAG = 'messagebundle'; @@ -37,7 +37,7 @@ const _DOCTYPE = ` `; -export class Xmb implements Serializer { +export class Xmb extends Serializer { write(messages: i18n.Message[]): string { const exampleVisitor = new ExampleVisitor(); const visitor = new _Visitor(); @@ -51,6 +51,8 @@ export class Xmb implements Serializer { if (visited[id]) return; visited[id] = true; + const mapper = this.createNameMapper(message); + const attrs: {[k: string]: string} = {id}; if (message.description) { @@ -62,7 +64,8 @@ export class Xmb implements Serializer { } rootNode.children.push( - new xml.CR(2), new xml.Tag(_MESSAGE_TAG, attrs, visitor.serialize(message.nodes))); + new xml.CR(2), + new xml.Tag(_MESSAGE_TAG, attrs, visitor.serialize(message.nodes, {mapper}))); }); rootNode.children.push(new xml.CR()); @@ -82,22 +85,29 @@ export class Xmb implements Serializer { } digest(message: i18n.Message): string { return digest(message); } + + + createNameMapper(message: i18n.Message): PlaceholderMapper { + return new XmbPlaceholderMapper(message); + } } class _Visitor implements i18n.Visitor { - visitText(text: i18n.Text, context?: any): xml.Node[] { return [new xml.Text(text.value)]; } + visitText(text: i18n.Text, ctx: {mapper: PlaceholderMapper}): xml.Node[] { + return [new xml.Text(text.value)]; + } - visitContainer(container: i18n.Container, context?: any): xml.Node[] { + visitContainer(container: i18n.Container, ctx: any): xml.Node[] { const nodes: xml.Node[] = []; - container.children.forEach((node: i18n.Node) => nodes.push(...node.visit(this))); + container.children.forEach((node: i18n.Node) => nodes.push(...node.visit(this, ctx))); return nodes; } - visitIcu(icu: i18n.Icu, context?: any): xml.Node[] { + visitIcu(icu: i18n.Icu, ctx: {mapper: PlaceholderMapper}): xml.Node[] { const nodes = [new xml.Text(`{${icu.expressionPlaceholder}, ${icu.type}, `)]; Object.keys(icu.cases).forEach((c: string) => { - nodes.push(new xml.Text(`${c} {`), ...icu.cases[c].visit(this), new xml.Text(`} `)); + nodes.push(new xml.Text(`${c} {`), ...icu.cases[c].visit(this, ctx), new xml.Text(`} `)); }); nodes.push(new xml.Text(`}`)); @@ -105,30 +115,34 @@ class _Visitor implements i18n.Visitor { return nodes; } - visitTagPlaceholder(ph: i18n.TagPlaceholder, context?: any): xml.Node[] { + visitTagPlaceholder(ph: i18n.TagPlaceholder, ctx: {mapper: PlaceholderMapper}): xml.Node[] { const startEx = new xml.Tag(_EXEMPLE_TAG, {}, [new xml.Text(`<${ph.tag}>`)]); - const startTagPh = new xml.Tag(_PLACEHOLDER_TAG, {name: ph.startName}, [startEx]); + let name = ctx.mapper.toPublicName(ph.startName); + const startTagPh = new xml.Tag(_PLACEHOLDER_TAG, {name}, [startEx]); if (ph.isVoid) { // void tags have no children nor closing tags return [startTagPh]; } const closeEx = new xml.Tag(_EXEMPLE_TAG, {}, [new xml.Text(``)]); - const closeTagPh = new xml.Tag(_PLACEHOLDER_TAG, {name: ph.closeName}, [closeEx]); + name = ctx.mapper.toPublicName(ph.closeName); + const closeTagPh = new xml.Tag(_PLACEHOLDER_TAG, {name}, [closeEx]); - return [startTagPh, ...this.serialize(ph.children), closeTagPh]; + return [startTagPh, ...this.serialize(ph.children, ctx), closeTagPh]; } - visitPlaceholder(ph: i18n.Placeholder, context?: any): xml.Node[] { - return [new xml.Tag(_PLACEHOLDER_TAG, {name: ph.name})]; + visitPlaceholder(ph: i18n.Placeholder, ctx: {mapper: PlaceholderMapper}): xml.Node[] { + const name = ctx.mapper.toPublicName(ph.name); + return [new xml.Tag(_PLACEHOLDER_TAG, {name})]; } - visitIcuPlaceholder(ph: i18n.IcuPlaceholder, context?: any): xml.Node[] { - return [new xml.Tag(_PLACEHOLDER_TAG, {name: ph.name})]; + visitIcuPlaceholder(ph: i18n.IcuPlaceholder, ctx: {mapper: PlaceholderMapper}): xml.Node[] { + const name = ctx.mapper.toPublicName(ph.name); + return [new xml.Tag(_PLACEHOLDER_TAG, {name})]; } - serialize(nodes: i18n.Node[]): xml.Node[] { - return [].concat(...nodes.map(node => node.visit(this))); + serialize(nodes: i18n.Node[], ctx: {mapper: PlaceholderMapper}): xml.Node[] { + return [].concat(...nodes.map(node => node.visit(this, ctx))); } } @@ -158,3 +172,69 @@ class ExampleVisitor implements xml.IVisitor { visitDeclaration(decl: xml.Declaration): void {} visitDoctype(doctype: xml.Doctype): void {} } + +/** + * XMB/XTB placeholders can only contain A-Z, 0-9 and _ + * + * Because such restrictions do not exist on placeholder names generated locally, the + * `PlaceholderMapper` is used to convert internal names to XMB names when the XMB file is + * serialized and back from XTB to internal names when an XTB is loaded. + */ +export class XmbPlaceholderMapper implements PlaceholderMapper, i18n.Visitor { + private internalToXmb: {[k: string]: string} = {}; + private xmbToNextId: {[k: string]: number} = {}; + private xmbToInternal: {[k: string]: string} = {}; + + // create a mapping from the message + constructor(message: i18n.Message) { message.nodes.forEach(node => node.visit(this)); } + + toPublicName(internalName: string): string { + return this.internalToXmb.hasOwnProperty(internalName) ? this.internalToXmb[internalName] : + null; + } + + toInternalName(publicName: string): string { + return this.xmbToInternal.hasOwnProperty(publicName) ? this.xmbToInternal[publicName] : null; + } + + visitText(text: i18n.Text, ctx?: any): any { return null; } + + visitContainer(container: i18n.Container, ctx?: any): any { + container.children.forEach(child => child.visit(this)); + } + + visitIcu(icu: i18n.Icu, ctx?: any): any { + Object.keys(icu.cases).forEach(k => { icu.cases[k].visit(this); }); + } + + visitTagPlaceholder(ph: i18n.TagPlaceholder, ctx?: any): any { + this.addPlaceholder(ph.startName); + ph.children.forEach(child => child.visit(this)); + this.addPlaceholder(ph.closeName); + } + + visitPlaceholder(ph: i18n.Placeholder, ctx?: any): any { this.addPlaceholder(ph.name); } + + visitIcuPlaceholder(ph: i18n.IcuPlaceholder, ctx?: any): any { this.addPlaceholder(ph.name); } + + // XMB placeholders could only contains A-Z, 0-9 and _ + private addPlaceholder(internalName: string): void { + if (!internalName || this.internalToXmb.hasOwnProperty(internalName)) { + return; + } + + let xmbName = internalName.toUpperCase().replace(/[^A-Z0-9_]/g, '_'); + + if (this.xmbToInternal.hasOwnProperty(xmbName)) { + // Create a new XMB when it has already been used + const nextId = this.xmbToNextId[xmbName]; + this.xmbToNextId[xmbName] = nextId + 1; + xmbName = `${xmbName}_${nextId}`; + } else { + this.xmbToNextId[xmbName] = 1; + } + + this.internalToXmb[internalName] = xmbName; + this.xmbToInternal[xmbName] = internalName; + } +} diff --git a/modules/@angular/compiler/src/i18n/serializers/xtb.ts b/modules/@angular/compiler/src/i18n/serializers/xtb.ts index 5fee8229b4..7e333a46c7 100644 --- a/modules/@angular/compiler/src/i18n/serializers/xtb.ts +++ b/modules/@angular/compiler/src/i18n/serializers/xtb.ts @@ -11,14 +11,14 @@ import {XmlParser} from '../../ml_parser/xml_parser'; import * as i18n from '../i18n_ast'; import {I18nError} from '../parse_util'; -import {Serializer} from './serializer'; -import {digest} from './xmb'; +import {PlaceholderMapper, Serializer} from './serializer'; +import {XmbPlaceholderMapper, digest} from './xmb'; const _TRANSLATIONS_TAG = 'translationbundle'; const _TRANSLATION_TAG = 'translation'; const _PLACEHOLDER_TAG = 'ph'; -export class Xtb implements Serializer { +export class Xtb extends Serializer { write(messages: i18n.Message[]): string { throw new Error('Unsupported'); } load(content: string, url: string): {[msgId: string]: i18n.Node[]} { @@ -43,6 +43,10 @@ export class Xtb implements Serializer { } digest(message: i18n.Message): string { return digest(message); } + + createNameMapper(message: i18n.Message): PlaceholderMapper { + return new XmbPlaceholderMapper(message); + } } // Extract messages as xml nodes from the xtb file diff --git a/modules/@angular/compiler/src/i18n/translation_bundle.ts b/modules/@angular/compiler/src/i18n/translation_bundle.ts index c21cbbbb3e..56852395a4 100644 --- a/modules/@angular/compiler/src/i18n/translation_bundle.ts +++ b/modules/@angular/compiler/src/i18n/translation_bundle.ts @@ -11,7 +11,7 @@ import {HtmlParser} from '../ml_parser/html_parser'; import * as i18n from './i18n_ast'; import {I18nError} from './parse_util'; -import {Serializer} from './serializers/serializer'; +import {PlaceholderMapper, Serializer} from './serializers/serializer'; /** * A container for translated messages @@ -21,16 +21,20 @@ export class TranslationBundle { constructor( private _i18nNodesByMsgId: {[msgId: string]: i18n.Node[]} = {}, - public digest: (m: i18n.Message) => string) { - this._i18nToHtml = new I18nToHtmlVisitor(_i18nNodesByMsgId, digest); + public digest: (m: i18n.Message) => string, + public mapperFactory?: (m: i18n.Message) => PlaceholderMapper) { + this._i18nToHtml = new I18nToHtmlVisitor(_i18nNodesByMsgId, digest, mapperFactory); } + // Creates a `TranslationBundle` by parsing the given `content` with the `serializer`. static load(content: string, url: string, serializer: Serializer): TranslationBundle { const i18nNodesByMsgId = serializer.load(content, url); const digestFn = (m: i18n.Message) => serializer.digest(m); - return new TranslationBundle(i18nNodesByMsgId, digestFn); + const mapperFactory = (m: i18n.Message) => serializer.createNameMapper(m); + return new TranslationBundle(i18nNodesByMsgId, digestFn, mapperFactory); } + // Returns the translation as HTML nodes from the given source message. get(srcMsg: i18n.Message): html.Node[] { const html = this._i18nToHtml.convert(srcMsg); @@ -46,15 +50,17 @@ export class TranslationBundle { class I18nToHtmlVisitor implements i18n.Visitor { private _srcMsg: i18n.Message; - private _srcMsgStack: i18n.Message[] = []; + private _contextStack: {msg: i18n.Message, mapper: (name: string) => string}[] = []; private _errors: I18nError[] = []; + private _mapper: (name: string) => string; constructor( private _i18nNodesByMsgId: {[msgId: string]: i18n.Node[]} = {}, - private _digest: (m: i18n.Message) => string) {} + private _digest: (m: i18n.Message) => string, + private _mapperFactory: (m: i18n.Message) => PlaceholderMapper) {} convert(srcMsg: i18n.Message): {nodes: html.Node[], errors: I18nError[]} { - this._srcMsgStack.length = 0; + this._contextStack.length = 0; this._errors.length = 0; // i18n to text const text = this._convertToText(srcMsg); @@ -88,7 +94,7 @@ class I18nToHtmlVisitor implements i18n.Visitor { } visitPlaceholder(ph: i18n.Placeholder, context?: any): string { - const phName = ph.name; + const phName = this._mapper(ph.name); if (this._srcMsg.placeholders.hasOwnProperty(phName)) { return this._srcMsg.placeholders[phName]; } @@ -105,14 +111,26 @@ class I18nToHtmlVisitor implements i18n.Visitor { visitIcuPlaceholder(ph: i18n.IcuPlaceholder, context?: any): any { throw 'unreachable code'; } + /** + * Convert a source message to a translated text string: + * - text nodes are replaced with their translation, + * - placeholders are replaced with their content, + * - ICU nodes are converted to ICU expressions. + */ private _convertToText(srcMsg: i18n.Message): string { const digest = this._digest(srcMsg); + const mapper = this._mapperFactory ? this._mapperFactory(srcMsg) : null; + if (this._i18nNodesByMsgId.hasOwnProperty(digest)) { - this._srcMsgStack.push(this._srcMsg); + this._contextStack.push({msg: this._srcMsg, mapper: this._mapper}); this._srcMsg = srcMsg; + this._mapper = (name: string) => mapper ? mapper.toInternalName(name) : name; + const nodes = this._i18nNodesByMsgId[digest]; const text = nodes.map(node => node.visit(this)).join(''); - this._srcMsg = this._srcMsgStack.pop(); + const context = this._contextStack.pop(); + this._srcMsg = context.msg; + this._mapper = context.mapper; return text; } diff --git a/modules/@angular/compiler/test/i18n/integration_spec.ts b/modules/@angular/compiler/test/i18n/integration_spec.ts index 88078cf4c1..b7a36dd371 100644 --- a/modules/@angular/compiler/test/i18n/integration_spec.ts +++ b/modules/@angular/compiler/test/i18n/integration_spec.ts @@ -165,6 +165,7 @@ const XTB = ` name="START_BOLD_TEXT"><b>beaucoup</b>} } {VAR_PLURAL, plural, =0 {Pas de réponse} =1 {une réponse} other {INTERPOLATION réponse} } FOO<a>BAR</a> + MAP_NAME `; const XMB = ` i18n attribute on tags @@ -191,7 +192,8 @@ const XMB = ` i18n attribute on tags with an explicit ID {VAR_PLURAL, plural, =0 {zero} =1 {one} =2 {two} other {<b>many</b>} } {VAR_PLURAL, plural, =0 {Found no results} =1 {Found one result} other {Found INTERPOLATION results} } - foo<a>bar</a>`; + foo<a>bar</a> + MAP_NAME`; const HTML = `
@@ -246,4 +248,6 @@ const HTML = ` }
foobar
+ +
{{ 'test' //i18n(ph="map name") }}
`; diff --git a/modules/@angular/compiler/test/i18n/message_bundle_spec.ts b/modules/@angular/compiler/test/i18n/message_bundle_spec.ts index b82a30322b..5bd1a1a15a 100644 --- a/modules/@angular/compiler/test/i18n/message_bundle_spec.ts +++ b/modules/@angular/compiler/test/i18n/message_bundle_spec.ts @@ -42,7 +42,7 @@ export function main(): void { }); } -class _TestSerializer implements Serializer { +class _TestSerializer extends Serializer { write(messages: i18n.Message[]): string { return messages.map(msg => `${serializeNodes(msg.nodes)} (${msg.meaning}|${msg.description})`) .join('//');