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
angular · Jan 24, 2017 · d02eab4 · d02eab4
1 parent fc55018
commit d02eab4
Show file tree

Hide file tree

Showing 10 changed files with 175 additions and 43 deletions.
diff --git 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
@@ -52,7 +52,7 @@ export class Extractor {
 
   extract(rootFiles: string[]): Promise<MessageBundle> {
     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
@@ -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
@@ -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;
 }
diff --git 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
@@ -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 = `<!ELEMENT messagebundle (msg)*>
 
 <!ELEMENT ex (#PCDATA)>`;
 
-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,53 +85,64 @@ 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(`}`));
 
     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(`</${ph.tag}>`)]);
-    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
@@ -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
@@ -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;
     }