PageTemplate macro expansion check cannot handle recursive macros

Bug #732972 reported by Marius Gedminas
6
This bug affects 1 person
Affects Status Importance Assigned to Milestone
zope.pagetemplate
Invalid
Undecided
Unassigned

Bug Description

Consider this page template, based on a real example (which is z3c.formui's div-form.pt):

  <ul metal:define-macro="tree">
    <li tal:content="context/name" />
    <li tal:repeat="context context/children">
      <ul metal:use-macro="tree" />
    </li>
  </ul>

Note that the macro is recursive, but it is not infinitely recursive, since it redefines 'context' and the metal:use-macro call is conditioned on a TALES expression "context/children" returning a non-empty list.

If you attempt to call pt_render(namespace, source=1) in order to check macro expansion, the TAL interpreter disables TAL interpretation, which makes that macro invocation unconditionally recursive. You'll get a RuntimeError: maximum recursion limit exceeded, after a while.

This is painful because PageTemplateTracebackSupplement calls pt_errors(namespace), which calls pt_render(namespace, source=1), which means you end up triggering recursion errors whenever you're trying to format a traceback that involves a page template using a recursive macro invocation.

Revision history for this message
Marius Gedminas (mgedmin) wrote :

I think the best fix would be to make TALInterpreter (from zope.tal) perform a recursion check in do_useMacro: if the same macro already appears in the macro stack, *and* self.tal is False, then the recursive macro expansion should silently stop.

In the shorter term I'm considering a workaround that disables macro expansion checks just for PageTemplateTracebackSupplement.

Revision history for this message
Marius Gedminas (mgedmin) wrote :

I've committed the short workaround version (with a test) in r128621.

Changed in zope.pagetemplate:
status: New → In Progress
Revision history for this message
Marius Gedminas (mgedmin) wrote :

(This means zope.pagetemplate 4.0.0 will have the workaround, when it comes out.)

Revision history for this message
Maurits van Rees (maurits-vanrees) wrote :

4.0.0 has been released on PyPI.

That gives problems when I use it on Plone 4.2 when using Chameleon/five.pt (so not standard Plone out of the box). Some tests go wrong or at least give extra error output. It also happens when as anonymous user you try to view a document that is private. You are redirected to the login page just fine, but in the logs you get an error:

Traceback (most recent call last):
  File "/Users/mauritsvanrees/shared-eggs/zExceptions-2.13.0-py2.7.egg/zExceptions/ExceptionFormatter.py", line 153, in formatLine
    supp = factory(*args)
  File "/Users/mauritsvanrees/community/plone-coredev/4.2/src/zope.pagetemplate/src/zope/pagetemplate/pagetemplate.py", line 265, in __init__
    e = pt.pt_errors(namespace, check_macro_expansion=False)
TypeError: pt_errors() got an unexpected keyword argument 'check_macro_expansion'

You get the same error twice, actually.

The problem is that not all page templates that are passed to the PageTemplateTracebackSupplement are from zope.pagetemplate. In the above case the class of the template is Products.CMFCore.FSPageTemplate.FSPageTemplate.

This simple change in pagetemplate.py would fix it for me:

- e = pt.pt_errors(namespace, check_macro_expansion=False)
+ try:
+ e = pt.pt_errors(namespace, check_macro_expansion=False)
+ except TypeError:
+ # Old page template.
+ e = pt.pt_errors(namespace)

I am not sure if that is the best way to go.
Alternatively, the pt_errors in Zope2/src/Products/PageTemplates/PageTemplate.py could be changed to accept check_macro_expansion as argument (and probably ignore it).

Revision history for this message
Maurits van Rees (maurits-vanrees) wrote :

I have committed the fix in zope.pagetemplate that I proposed in the previous comment.

I also did the alternative change in Products.PageTemplates in Zope2 trunk and branch 2.13, accepting and ignoring the check_macro_expansion argument.

Changed in zope.pagetemplate:
status: In Progress → Fix Committed
Revision history for this message
Marius Gedminas (mgedmin) wrote :

For the record: that would be r129009 in zope.pagetemplate, r129010 in Zope/branches/2.13, and r129011 in Zope/trunk.

I apologize for the inconvenience. The 'try/except TypeError' makes me cringe, but the only alternative I can come up with --

    if 'check_macro_expansion' in inspect.getargspec(pt.pt_errors)[0]:
        kw = {'check_macro_expansion': False}
    else:
        kw = {}
    e = pt.pt_errors(namespace, **kw)

-- is also full of hack.

Revision history for this message
Colin Watson (cjwatson) wrote :

The zope.pagetemplate project on Launchpad has been archived at the request of the Zope developers (see https://answers.launchpad.net/launchpad/+question/683589 and https://answers.launchpad.net/launchpad/+question/685285). If this bug is still relevant, please refile it at https://github.com/zopefoundation/zope.pagetemplate.

Changed in zope.pagetemplate:
status: Fix Committed → Invalid
To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.