Issue 2112 - Support fixed text in strings.

cms/converters.py

 # coding: utf-8
 
 # This file is part of the Adblock Plus web scripts,
 # Copyright (C) 2006-2015 Eyeo GmbH
 #
 # Adblock Plus is free software: you can redistribute it and/or modify
 # it under the terms of the GNU General Public License version 3 as
 # published by the Free Software Foundation.
 #
 # Adblock Plus is distributed in the hope that it will be useful,
@@ skipping 43 matching lines @@
   def parse(self, text, pagename):
     self.reset()
     self._string = []
     self._fixed_strings = []
     self._inside_fixed = False
     self._attrs = {}
     self._pagename = pagename
 
     try:
       self.feed(text)
-      return "".join(self._string), self._attrs, map(lambda s: "".join(s), self._fixed_strings)

[Wladimir Palant, 2015/03/26 20:48:56]

Nit: ["".join(s) for s in self._fixed_strings] is shorter and easier to read
than map().

[kzar, 2015/03/26 22:11:32]

Done.

+      return "".join(self._string), self._attrs, ["".join(s) for s in self._fixed_strings]
     finally:
       self._string = None
       self._attrs = None
       self._pagename = None
       self._inside_fixed = False
       self._fixed_strings = None
 
   def handle_starttag(self, tag, attrs):
-    if tag not in self._whitelist:

[Wladimir Palant, 2015/03/26 20:48:56]

No tags should be allowed inside <fix> - meaning |if self._inside_fixed or tag
not in self._whitelist|

[kzar, 2015/03/26 22:11:32]

Done.

-      raise Exception("Unexpected HTML tag '%s' in localizable string on page %s" % (tag, self._pagename))
-    if tag == "fix":
+    if self._inside_fixed:
+      raise Exception("Unexpected HTML tag '%s' inside a fixed string on page %s" % (tag, self._pagename))
+    elif tag == "fix":
       self._inside_fixed = True
       self._fixed_strings.append([])
-    else:
+    elif tag in self._whitelist:
       self._attrs.setdefault(tag, []).append(attrs)
       self._string.append("<%s>" % tag)
+    else:
+      raise Exception("Unexpected HTML tag '%s' in localizable string on page %s" % (tag, self._pagename))
 
   def handle_endtag(self, tag):
     if tag == "fix":
       self._string.append("{%d}" % len(self._fixed_strings))
       self._inside_fixed = False
     else:
       self._string.append("</%s>" % tag)
 
-  def _append_string(self, s):

[Wladimir Palant, 2015/03/26 20:48:56]

Nit: we are adding to self._string a lot, this helper only applies to text data.
Call it _append_text()?

[kzar, 2015/03/26 22:11:32]

Done.

+  def _append_text(self, s):
     if self._inside_fixed:
       self._fixed_strings[-1].append(s)
     else:
       self._string.append(s)
 
   def handle_data(self, data):
     # Note: lack of escaping here is intentional. The result is a locale string,
     # HTML escaping is applied when this string is inserted into the document.
-    self._append_string(data)
+    self._append_text(data)
 
   def handle_entityref(self, name):
-    self._append_string(self.unescape("&%s;" % name))
+    self._append_text(self.unescape("&%s;" % name))
 
   def handle_charref(self, name):
-    self._append_string(self.unescape("&#%s;" % name))
+    self._append_text(self.unescape("&#%s;" % name))
 
 class Converter:
-  whitelist = set(["a", "em", "fix", "strong"])

[Wladimir Palant, 2015/03/26 20:48:56]

It makes no sense to change this set - <fix> isn't actually whitelisted, it gets
special treatment inside AttributeParser. Note that this set is also used below
when deciding which attributes to restore.

So I'd suggest passing in the original set to AttributeParser. handle_starttag()
could then go like this:

  if self._inside_fixed:
    # Unexpected tag inside a fixed string
  elif tag == "fix":
    ...
  elif tag in self.whitelist:
    ...
  else:
    # Unexpected tag in localizable string

[kzar, 2015/03/26 22:11:32]

Done.

+  whitelist = set(["a", "em", "strong"])
 
   def __init__(self, params, key="pagedata"):
     self._params = params
     self._key = key
     self._attribute_parser = AttributeParser(self.whitelist)
 
     # Read in any parameters specified at the beginning of the file
     lines = params[key].splitlines(True)
     while lines and re.search(r"^\s*[\w\-]+\s*=", lines[0]):
       name, value = lines.pop(0).split("=", 1)
@@ skipping 12 matching lines @@
     default, saved_attributes, fixed_strings = self._attribute_parser.parse(default, self._params["page"])
 
     # Get translation
     if self._params["locale"] != self._params["defaultlocale"] and name in localedata:
       result = localedata[name].strip()
     else:
       result = default
 
     # Insert fixed strings
     for i in range(len(fixed_strings)):
-      result = re.sub(re_escape("{%d}" % (i + 1)), fixed_strings[i], result, 1)

[Wladimir Palant, 2015/03/26 20:48:56]

Escape the string being inserted? It looks like currently < will be turned
into <.

[kzar, 2015/03/26 22:11:32]

I originally did escape fixed_strings[i] here but it seems unnecessary. I'm not
entirely sure why, perhaps because of the `result = escape(result)` below?

[Wladimir Palant, 2015/03/27 07:29:48]

I see, you are inserting them before the escaping happens. Then there is another
issue however...

+      result = re.sub(r"\{%d\}" % (i + 1), fixed_strings[i], result, 1)
 
     # Insert attributes
     result = escape(result)
     for tag in self.whitelist:
       saved = saved_attributes.get(tag, [])
       for attrs in saved:
         attrs = map(lambda (name, value): '%s="%s"' % (escape(name), escape(value)), attrs)
         result = re.sub(
           r"%s([^<>]*?)%s" % (re_escape("<%s>" % tag), re_escape("</%s>" % tag)),
           r'<%s %s>\1</%s>' % (tag, " ".join(attrs), tag),
@@ skipping 218 matching lines @@
         stack.pop()
       stack[-1]["subitems"].append(item)
       stack.append(item)
     return structured
 
 converters = {
   "html": RawConverter,
   "md": MarkdownConverter,
   "tmpl": TemplateConverter,
 }