Issue 3084 - [cms] Show full tracebacks for exceptions passing template code

cms/converters.py

Index: cms/converters.py
===================================================================
--- a/cms/converters.py
+++ b/cms/converters.py
@@ -112,6 +112,7 @@
   whitelist = {"a", "em", "strong", "code", "span"}
   missing_translations = 0
   total_translations = 0
+  removed_line = ""
 
   def __init__(self, params, key="pagedata"):
     self._params = params
@@ -120,11 +121,15 @@
     self._seen_defaults  = {}
 
     # 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)
+    data, filename = params[key]
+    lines = data.splitlines(True)
+    for i, line in enumerate(lines):
+      if not re.search(r"^\s*[\w\-]+\s*=", line):
+        break
+      name, value = line.split("=", 1)
       params[name.strip()] = value.strip()
-    params[key] = "".join(lines)
+      lines[i] = self.removed_line

[Wladimir Palant, 2015/09/16 17:35:52]

Why not just assign "\n" here? I don't think that the few extra newlines are a
big deal, and you could drop the removed_line attribute then.

[Sebastian Noack, 2015/09/16 19:04:04]

Done.

+    params[key] = ("".join(lines), filename)
 
   def localize_string(self, page, name, default, comment, localedata, escapes):
     def escape(s):
@@ -277,7 +282,7 @@
       return result
 
 class RawConverter(Converter):
-  def get_html(self, source):
+  def get_html(self, (source, filename)):
     result = self.insert_localized_strings(source, html_escapes)
     result = self.process_links(result)
     return result
@@ -292,7 +297,7 @@
     re.escape(jinja2.escape(Converter.include_end_regex))
   )
 
-  def get_html(self, source):
+  def get_html(self, (source, filename)):
     def remove_unnecessary_entities(match):
       char = unichr(int(match.group(1)))
       if char in html_escapes:
@@ -318,16 +323,19 @@
     result = self.process_links(result)
     return result
 
+class SourceTemplateLoader(jinja2.BaseLoader):
+  def __init__(self, source):
+    self.source = source
+
+  def get_source(self, environment, template):
+    try:
+      result = self.source.read_file(template + ".tmpl")
+    except Exception:
+      raise jinja2.TemplateNotFound(template)
+    return result + (None,)

[Wladimir Palant, 2015/09/16 17:35:53]

I'm not really happy with unrelated changes being mixed in here. And in this
particular case, I really don't think that this should be a public class - it's
an implementation detail of the TemplateConverter class.

[Sebastian Noack, 2015/09/16 19:04:04]

The unrelated change wasn't intentional. As you can see in the previous patch
set, I removed the template loader here, and added it back again with this patch
set. However, frankly, I don't like these nested classes. IMO they make the code
harder to read. And quoting the Zen of Python: "flat is better than nested". So
I'm inclined to leave it like that, unless you insist. But note that the
AttributeParser class for example is also defined at the top-level.

+
 class TemplateConverter(Converter):
-  class _SourceLoader(jinja2.BaseLoader):
-    def __init__(self, source):
-      self.source = source
-
-    def get_source(self, environment, template):
-      try:
-        return self.source.read_file(template + ".tmpl"), None, None
-      except Exception:
-        raise jinja2.TemplateNotFound(template)
+  removed_line = "{#\n#}"
 
   def __init__(self, *args, **kwargs):
     Converter.__init__(self, *args, **kwargs)
@@ -350,16 +358,28 @@
           continue
 
         path = "%s/%s" % (dirname, filename)
+        namespace = self._params["source"].exec_file(path)
+
         name = os.path.basename(root)
-        dictionary[name] = self._params["source"].import_symbol(path, name)
+        try:
+          dictionary[name] = namespace[name]
+        except KeyError:
+          raise Exception("Expected symbol %r not found in %r" % (name, path))
 
-    self._env = jinja2.Environment(loader=self._SourceLoader(self._params["source"]), autoescape=True)
+    self._env = jinja2.Environment(loader=SourceTemplateLoader(self._params["source"]), autoescape=True)
     self._env.filters.update(filters)
     self._env.globals.update(globals)
 
-  def get_html(self, source):
-    template = self._env.from_string(source)
-    module = template.make_module(self._params)
+  def get_html(self, (source, filename)):
+    env = self._env
+    code = env.compile(source, None, filename)
+    template = env.template_class.from_code(env, code, env.globals)

[Wladimir Palant, 2015/09/16 17:35:52]

From what I can tell, Environment.template_class is undocumented - and it is
merely a constant anyway. So I'd suggest using jinja2.Template.from_code()
directly here.

[Sebastian Noack, 2015/09/16 19:04:04]

Done.

+
+    try:
+      module = template.make_module(self._params)
+    except Exception:
+      env.handle_exception()
+
     for key, value in module.__dict__.iteritems():
       if not key.startswith("_"):
         self._params[key] = value